Re: svn commit: r316477 - head/usr.bin/grep

[Ngie Cooper (yaneurabeya)]

> On Apr 4, 2017, at 17:54, Kyle Evans  wrote:
> 
> On Tue, Apr 4, 2017 at 7:51 PM, Rodney W. Grimes 
>  wrote:
> Also it might be worth making NetBSD aware of the bad output, and fix
> it there, and the regression test, and everyone well be better off.

The color tests are new to FreeBSD/NetBSD. They don’t exist in NetBSD yet.

> FWIW- I've been in contact with ngie@ w.r.t regression testing and the color 
> tests, and it appears that he's willing to help with the upstream aspect.

*they/she ;).

Thank you!
-Ngie


signature.asc
Description: Message signed with OpenPGP using GPGMail

Re: svn commit: r316477 - head/usr.bin/grep

[Kyle Evans]

On Tue, Apr 4, 2017 at 7:51 PM, Rodney W. Grimes <
free...@pdx.rh.cn85.dnsmgr.net> wrote:
>
> Also it might be worth making NetBSD aware of the bad output, and fix
> it there, and the regression test, and everyone well be better off.
>

FWIW- I've been in contact with ngie@ w.r.t regression testing and the
color tests, and it appears that he's willing to help with the upstream
aspect.
___
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"

Re: svn commit: r316477 - head/usr.bin/grep

[Rodney W. Grimes]

> On 04.04.2017 15:24, Kyle Evans wrote:
> > On Tue, Apr 4, 2017 at 7:07 AM, Andrey Chernov  > > wrote:
> > 
> > On 04.04.2017 2:16, Ed Maste wrote:
> > >   if (color)
> > > - fprintf(stdout, "\33[m\33[K");
> > > + fprintf(stdout, "\33[00m\33[K");
> > 
> > Please back that one out. We don't need to handle internally or print
> > remotely excessive 00.
> > At least according to
> > https://en.wikipedia.org/wiki/ANSI_escape_code
> > 
> > "With no parameters, CSI m is treated as CSI 0 m (reset / normal), which
> > is typical of most of the ANSI escape sequences."
> > 
> > 
> > Hi ache@,
> > 
> > This specific change was made in the name of explicitly matching colored
> > output of GNU grep for simplification of regression test purposes,
> > rather than for good form. Is it still unacceptable to do so?
> 
> IMHO everyday usage by everyone weights much more than occasional
> regression tests run which can be fixed instead of this place. F.e. we
> already do a lot of local fixes in the NetBSD regression tests instead
> of pretending to mimic NetBSD in 100% in the system itself.


Also it might be worth making NetBSD aware of the bad output, and fix
it there, and the regression test, and everyone well be better off.

-- 
Rod Grimes rgri...@freebsd.org
___
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"

Re: svn commit: r316477 - head/usr.bin/grep

[Andrey Chernov]

On 04.04.2017 16:41, Kyle Evans wrote:
> Upon further review, newer versions of GNU grep(1) drop the explicit
> '00' and add the \33[K dropped in this commit a few lines above the
> mentioned one. In that case, we might as well revert those two lines and
> consider this an improvement. I guess it depends on the overall outlook
> of the tests, though -- should we introduce more tests that fail by
> default as they go in, or adapt similar (not strictly buggy, but not
> ideal) behavior to start with and then improve after we can grant
> bsdgrep(1) replacement status?

It seems new GNU grep takes right course, so I vote to make our thing
like there and not as in its obsoleted version.

\33[K clears from cursor to the rest of the line (with background color,
in case terminal have "ut" termcap capability). Consider how multi-line
pattern will look with it and without.

___
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"

Re: svn commit: r316477 - head/usr.bin/grep

[Kyle Evans]

On Tue, Apr 4, 2017 at 8:53 AM, Ed Maste  wrote:

> In this case I'd rather we remove the 00 and have the test verify
> that. I think it's fine if there are a few tests that fail when run
> with GNU grep where it has the undesired, differing behaviour.
>

Excellent- can you please revert this entire bit then:

@@ -474,13 +509,13 @@ printline(struct str *line, int sep, reg
fwrite(line->dat + a, matches[i].rm_so - a,
1,
stdout);
if (color)
-   fprintf(stdout, "\33[%sm\33[K", color);
+   fprintf(stdout, "\33[%sm", color);

fwrite(line->dat + matches[i].rm_so,
matches[i].rm_eo - matches[i].rm_so, 1,
stdout);
if (color)
-   fprintf(stdout, "\33[m\33[K");
+   fprintf(stdout, "\33[00m\33[K");
a = matches[i].rm_eo;
if (oflag)
putchar('\n');

I'll address this in the unit tests review shortly.
___
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"

Re: svn commit: r316477 - head/usr.bin/grep

[Ed Maste]

On 4 April 2017 at 09:09, Kyle Evans  wrote:
>
> This is where it gets kind of finnicky, and I'll step out and let others
> weigh in -- the test I refer to was specifically written by me
> (https://reviews.freebsd.org/D10112), and it's entirely to make it easier
> for me to check and quantify to others where we're actually regressing or
> improving as I continue fixing things in bsdgrep(1).

In this case I'd rather we remove the 00 and have the test verify
that. I think it's fine if there are a few tests that fail when run
with GNU grep where it has the undesired, differing behaviour.
___
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"

Re: svn commit: r316477 - head/usr.bin/grep

[Kyle Evans]

On Tue, Apr 4, 2017 at 8:19 AM, Andrey Chernov  wrote:

> On 04.04.2017 16:09, Kyle Evans wrote:
> >
> > First, apologies, I must rewind: what did you  mean initially by "We
> > don't need to handle internally [...]"
> >  -- the second half I understand, but it occurs to me that we should
> > already be internally handling \33[m = \33[00m as defined by ANSI.
>
> Of course we must handle (and already handle) both according to ANSI,
> but few CPU cycles here, then in another place, yet another, etc. making
> big time in sum when CPU does nothing useful but resource eating.


I'm omitting a response to this due to the previously mentioned "letting
others weigh in," but I do see where you're coming from. =)

I see another problem, but it sounds a bit artificially. What if someone
> decides to parse grep-colored output and assumes gnu grep only (which is
> usual in Linux). I can't determine probability of such situation.
> Usually people use generic ANSI parser which understand both cases (f.e.
> to convert it to HTML).


I agree that this sounds like an artificially conceived situation. On the
other hand, we've seen a lot of people make *a lot* of bad assumptions
biased towards Linux over in ports. I would hope that the probability of
one relying on this exact output to be really really really really close to
0%.

Upon further review, newer versions of GNU grep(1) drop the explicit '00'
and add the \33[K dropped in this commit a few lines above the mentioned
one. In that case, we might as well revert those two lines and consider
this an improvement. I guess it depends on the overall outlook of the
tests, though -- should we introduce more tests that fail by default as
they go in, or adapt similar (not strictly buggy, but not ideal) behavior
to start with and then improve after we can grant bsdgrep(1) replacement
status?
___
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"

Re: svn commit: r316477 - head/usr.bin/grep

[Andrey Chernov]

On 04.04.2017 16:05, Bruce Evans wrote:
> This might be to work around a bug in a terminal driver.  I needed the
> 0 (but just one) to reset the attributes in my attribute tests, on at
> least 1 of the tested terminals (current sc, current vt, old sc, old
> xterm, old cygwin xterm).  CSI m seems to be working for sc now, and the
> teken code looks OK.

I never meet such one. Even most earlier specs tells about CSI m and it
works in sc from its first day. You can check how often CSI m used by
inspecting /etc/termcap changes, there is no 00 too.

___
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"

Re: svn commit: r316477 - head/usr.bin/grep

[Andrey Chernov]

On 04.04.2017 16:09, Kyle Evans wrote:
> 
> First, apologies, I must rewind: what did you  mean initially by "We
> don't need to handle internally [...]"
>  -- the second half I understand, but it occurs to me that we should
> already be internally handling \33[m = \33[00m as defined by ANSI.

Of course we must handle (and already handle) both according to ANSI,
but few CPU cycles here, then in another place, yet another, etc. making
big time in sum when CPU does nothing useful but resource eating.

> On Tue, Apr 4, 2017 at 7:37 AM, Andrey Chernov  > wrote:
> 
> IMHO everyday usage by everyone weights much more than occasional
> regression tests run which can be fixed instead of this place. F.e. we
> already do a lot of local fixes in the NetBSD regression tests instead
> of pretending to mimic NetBSD in 100% in the system itself.
> 
> 
> This is where it gets kind of finnicky, and I'll step out and let others
> weigh in -- the test I refer to was specifically written by me
> (https://reviews.freebsd.org/D10112), and it's entirely to make it
> easier for me to check and quantify to others where we're actually
> regressing or improving as I continue fixing things in bsdgrep(1). This
> is easier to do so when we don't have trivial failures due to minor
> formatting differences when they should ultimately be interpreted
> equivalently either way, such as in this case. I expect to be find other
> quirky bugs in gnugrep(1) while reviewing the remaining PRs in Bugzilla,
> and thus expect this to be more important in the months to come.

I see another problem, but it sounds a bit artificially. What if someone
decides to parse grep-colored output and assumes gnu grep only (which is
usual in Linux). I can't determine probability of such situation.
Usually people use generic ANSI parser which understand both cases (f.e.
to convert it to HTML).

___
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"

Re: svn commit: r316477 - head/usr.bin/grep

[Kyle Evans]

First, apologies, I must rewind: what did you  mean initially by "We don't
need to handle internally [...]"
 -- the second half I understand, but it occurs to me that we should
already be internally handling \33[m = \33[00m as defined by ANSI.

On Tue, Apr 4, 2017 at 7:37 AM, Andrey Chernov  wrote:
>
> IMHO everyday usage by everyone weights much more than occasional
> regression tests run which can be fixed instead of this place. F.e. we
> already do a lot of local fixes in the NetBSD regression tests instead
> of pretending to mimic NetBSD in 100% in the system itself.


This is where it gets kind of finnicky, and I'll step out and let others
weigh in -- the test I refer to was specifically written by me (
https://reviews.freebsd.org/D10112), and it's entirely to make it easier
for me to check and quantify to others where we're actually regressing or
improving as I continue fixing things in bsdgrep(1). This is easier to do
so when we don't have trivial failures due to minor formatting differences
when they should ultimately be interpreted equivalently either way, such as
in this case. I expect to be find other quirky bugs in gnugrep(1) while
reviewing the remaining PRs in Bugzilla, and thus expect this to be more
important in the months to come.
___
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"

Re: svn commit: r316477 - head/usr.bin/grep

[Bruce Evans]

On Tue, 4 Apr 2017, Andrey Chernov wrote:


On 04.04.2017 2:16, Ed Maste wrote:

if (color)
-   fprintf(stdout, "\33[m\33[K");
+   fprintf(stdout, "\33[00m\33[K");


Please back that one out. We don't need to handle internally or print
remotely excessive 00.
At least according to
https://en.wikipedia.org/wiki/ANSI_escape_code
"With no parameters, CSI m is treated as CSI 0 m (reset / normal), which
is typical of most of the ANSI escape sequences."


This might be to work around a bug in a terminal driver.  I needed the
0 (but just one) to reset the attributes in my attribute tests, on at
least 1 of the tested terminals (current sc, current vt, old sc, old
xterm, old cygwin xterm).  CSI m seems to be working for sc now, and the
teken code looks OK.

I first thought that this was a misplaced 0 for \033.  2 digits for
octal looks strange.  I would use hex in C anyway.  In my user attribute
tests, I have to use printf(1), and noticed that printf(1) is missing too
many features in printf(3), or worse, is incompatible, especially for hex
escape sequences and %c format.

Bruce
___
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"

Re: svn commit: r316477 - head/usr.bin/grep

[Andrey Chernov]

On 04.04.2017 15:24, Kyle Evans wrote:
> On Tue, Apr 4, 2017 at 7:07 AM, Andrey Chernov  > wrote:
> 
> On 04.04.2017 2:16, Ed Maste wrote:
> >   if (color)
> > - fprintf(stdout, "\33[m\33[K");
> > + fprintf(stdout, "\33[00m\33[K");
> 
> Please back that one out. We don't need to handle internally or print
> remotely excessive 00.
> At least according to
> https://en.wikipedia.org/wiki/ANSI_escape_code
> 
> "With no parameters, CSI m is treated as CSI 0 m (reset / normal), which
> is typical of most of the ANSI escape sequences."
> 
> 
> Hi ache@,
> 
> This specific change was made in the name of explicitly matching colored
> output of GNU grep for simplification of regression test purposes,
> rather than for good form. Is it still unacceptable to do so?

IMHO everyday usage by everyone weights much more than occasional
regression tests run which can be fixed instead of this place. F.e. we
already do a lot of local fixes in the NetBSD regression tests instead
of pretending to mimic NetBSD in 100% in the system itself.

___
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"

Re: svn commit: r316477 - head/usr.bin/grep

[Kyle Evans]

On Tue, Apr 4, 2017 at 7:07 AM, Andrey Chernov  wrote:

> On 04.04.2017 2:16, Ed Maste wrote:
> >   if (color)
> > - fprintf(stdout, "\33[m\33[K");
> > + fprintf(stdout, "\33[00m\33[K");
>
> Please back that one out. We don't need to handle internally or print
> remotely excessive 00.
> At least according to
> https://en.wikipedia.org/wiki/ANSI_escape_code
> "With no parameters, CSI m is treated as CSI 0 m (reset / normal), which
> is typical of most of the ANSI escape sequences."


Hi ache@,

This specific change was made in the name of explicitly matching colored
output of GNU grep for simplification of regression test purposes, rather
than for good form. Is it still unacceptable to do so?

Thanks,

Kyle Evans
___
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"

Re: svn commit: r316477 - head/usr.bin/grep

[Andrey Chernov]

On 04.04.2017 2:16, Ed Maste wrote:
>   if (color) 
> - fprintf(stdout, "\33[m\33[K");
> + fprintf(stdout, "\33[00m\33[K");

Please back that one out. We don't need to handle internally or print
remotely excessive 00.
At least according to
https://en.wikipedia.org/wiki/ANSI_escape_code
"With no parameters, CSI m is treated as CSI 0 m (reset / normal), which
is typical of most of the ANSI escape sequences."

___
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"