Page MenuHomeFreeBSD

GNU grep: use --color=auto by default
Needs RevisionPublic

Authored by arrowd on Apr 12 2019, 8:28 AM.

Details

Summary

GNU grep defaults to --color=auto when --color is passed, but if no flag is given, no coloring applied.
Recent GNU grep in Ubuntu colorises its output by default, so we are following the behaviour there.

Test Plan

grep foo file_with_foo now highlights found substrings.

Diff Detail

Repository
rS FreeBSD src repository
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 23642
Build 22624: arc lint + arc unit

Event Timeline

arrowd created this revision.Apr 12 2019, 8:28 AM
avg resigned from this revision.Apr 25 2019, 8:07 PM
0mp requested changes to this revision.May 6 2019, 7:47 AM
0mp added a subscriber: 0mp.
0mp added inline comments.
gnu/usr.bin/grep/grep.c
1721

You've got style(9) issues.

This revision now requires changes to proceed.May 6 2019, 7:47 AM
0mp added inline comments.May 6 2019, 7:53 AM
gnu/usr.bin/grep/grep.c
1721

Actually, this is not really style(9)-compliant file as I look at it now...

Why this if statement had to be moved to a different place?

arrowd added a comment.May 6 2019, 8:19 AM

That if(color_option == 2) block checks terminal features and enables/disables output coloring based on that.

The problem is that this check is currently located inside command-line handling loop while ((opt = get_nondigit_option (argc, argv, &default_context)) != -1). Thus, it only gets executed if you actually pass --color=something. So, I just moved it out of the loop, so it would be always be executed.

0mp added a comment.May 8 2019, 9:20 PM

LGTM.

It would be great to update the manual page at the same time.

In D19888#435371, @0mp wrote:

LGTM.

I don't have src commit bit.

It would be great to update the manual page at the same time.

What should be updated, exactly? The --color option is already documented.

0mp added a comment.May 13 2019, 2:18 PM
In D19888#435371, @0mp wrote:

It would be great to update the manual page at the same time.

What should be updated, exactly? The --color option is already documented.

The default behavior of --color flag. I am not sure if that's necessary at this point.

Also, I forgot that we are talking about the GNU grep here.

Then your D20253 was perfectly sensible, why did you discarded it?

What needs to be done to get this in?

Another bump.

lwhsu added a subscriber: lwhsu.Aug 6 2019, 3:55 PM

I think this is good, as we also have --color=auto default in ls(1)

Does that mean I can commit this myself, despite having commit bit for ports only?

lwhsu added a comment.Aug 12 2019, 9:20 AM

Does that mean I can commit this myself, despite having commit bit for ports only?

Sorry for being misunderstanding. I don't things I have enough experience with these codes and I'd like to have more comments. How about asking on -arch or -current list?

@kevans I know it's gnugrep not bsdgrep, but could you provide some comments?

kevans added a comment.Sep 3 2019, 6:47 PM

@kevans I know it's gnugrep not bsdgrep, but could you provide some comments?

Hi,

Sorry... admittedly, I've avoided this because color default is a really sensitive topic and I'm still recovering from enabling them by default in ls(1), which isn't fair to @arrowd. I'm again making progress on replacing gnugrep with bsdgrep, and I'd be inclined to ask that we not do this with gnugrep in base and instead flip this switch in bsdgrep. My reasoning here is that I don't trust that the color behavior in base gnugrep (in general buggy as all get-out) is compatible with either ports gnugrep or base bsdgrep, where-as I've put plenty of work into making sure that bsdgrep behavior matches ports gnugrep behavior here. Since I can't enable bsdgrep-as-default in earlier branches due to the regex nightmare, I'd strongly prefer not to go "uncolored in earlier versions of earlier branches" -> "potentially buggy color output in earlier branches" -> "closer color output in newer branches" -> "ports gnugrep color output"