Bug 1034631

I wouldn't like to over-engineer this feature. I don't see the benefit of grep output not being colored and e.g. zgrep output being colored. Thus I think one control (colorgrep) for all the coloring should be enough.

But I think we need four bugzillas. The grep coloring script needs to be patched, because the guard for non-interactive shells blocks any sane detection of the state of this feature. I think there is no benefit of this guard, thus I am removing it. Patch attached.

Next we need patches for the respective scripts. I am attaching example patch for the zgrep and cloning the bug. Feel free to improve it. I think patches for the other scripts should be similar.

Created attachment 829314 [details]
Proposed fix for grep

Created attachment 829316 [details]
Proposed fix for zgrep

Grep part of the fix pushed.

I disagree a little with that solution.  At least for the zgrep patch from
comment #3.  Relying on alias and parsing its content from non-interactive
script which zgrep basically is a kludge.  And imo very hard to get upstream.

That means I would rather create another alias for zgrep like
'zgrep --color=auto' (Jardo, do you see any problem there?) - that is imo
clean downstream solution.  CCing also Michal as a gzip maintainer.

(Even grep itself now relies on alias expansion turned ON.  Which is not the
case you proposed - the output of zgrep is colored always).

----
I tried to think about dropping the need to parse /etc/GREP_COLORS in each
wrapper.  That is unfortunately not an easy task (only one way which comes to
my mind is some kind of parsing script, eg.
`grepconf.sh --interactive-color`).

As for the grep fix from comment #2, checking for PS1 could speed up things a
little.  But basically it does not matter IMO.

Reopening for the parsing script idea, but feel free to close - I may parse
/etc/GREP_COLORS by hand in xz (same way we can do in gzip, etc.).

(In reply to Pavel Raiskup from comment #5)
> I disagree a little with that solution.  At least for the zgrep patch from
> comment #3.  Relying on alias and parsing its content from non-interactive
> script which zgrep basically is a kludge.  And imo very hard to get upstream.
> 
I agree with that, the attached patch was only proof of concept / quick hack.

> That means I would rather create another alias for zgrep like
> 'zgrep --color=auto' (Jardo, do you see any problem there?) - that is imo
> clean downstream solution.  CCing also Michal as a gzip maintainer.
> 
I don't like the idea to have/install such aliases with the grep package. But I could add some variable to the colorgrep.sh, e.g. GREP_COLOR=0/1, would it be helpful for you?

> > That means I would rather create another alias for zgrep like
> > 'zgrep --color=auto' (Jardo, do you see any problem there?) - that is imo
> > clean downstream solution.  CCing also Michal as a gzip maintainer.
> > 
> I don't like the idea to have/install such aliases with the grep package.

Yes, it should be installed on per-package basis.

> But I could add some variable to the colorgrep.sh, e.g. GREP_COLOR=0/1,
> would it be helpful for you?

That is not as easy as it looks — you need to guarantee that the particular
profile.d script of grep wrapper (e.g. xz.sh, bzip.sh) is sourced *after* the
colorgrep.sh to reuse such GREP_COLOR variable.  Note that the idea is to not
touch upstream sources (/usr/bin/xzgrep, /usr/bin/zgrep, etc.).

At least regarding bash, /etc/profile uses the following bash code:
 for i in /etc/profile.d/*.sh ; do ...

And bash do alphabetically sorted expansion (according its man), so it should work. But AFAIK there is no Fedora doc that guarantee such behaviour during /etc/profile.d expansion, thus this behaviour may change in the future (although unlikely).

(In reply to Jaroslav Škarvada from comment #8)
> At least regarding bash, /etc/profile uses the following bash code:
>  for i in /etc/profile.d/*.sh ; do ...

Yes, changing of filename should not change behavior..  Consider bzgrep.

> And bash do alphabetically sorted expansion (according its man), so it
> should work. But AFAIK there is no Fedora doc that guarantee such behaviour
> during /etc/profile.d expansion, thus this behaviour may change in the
> future (although unlikely).

Yes, for that reason - I think the special binary (it can be calmly in
%{_libexecdir}) is the only way (or to have sourceable script for each shell).

Created attachment 849803 [details]
Possible approach with dedicated script.

Jardo, could you consider attached approach?  I could reuse the binary
/usr/libexec/grepconf.sh in xz (similarly gzip could, etc..).

Pavel

Created attachment 849812 [details]
Possible approach with dedicated script.

Fix for:
-if test x"$1" == x"--interactive-color"; then
+if test x"$1" = x"--interactive-color"; then

Ping?

(In reply to Pavel Raiskup from comment #12)
> Ping?

Sorry for the delay. To be honest, I forgot about it :)

Created attachment 928783 [details]
Proposed fix

Slightly modified / simplified patch. Are you OK with it?

(In reply to Jaroslav Škarvada from comment #14)
> Slightly modified / simplified patch. Are you OK with it?

Better one, thanks!

Cloning to bzip2, gzip.

The following example scripts can be used to make the *grep output colored:

/etc/profile.d/colorgrep.csh:
/usr/libexec/grepconf.sh -c
if ( $status == 1 ) then
    exit
endif
alias grep 'grep --color=auto'


/etc/profile.d/colorgrep.sh:
/usr/libexec/grepconf.sh -c || return
alias grep='grep --color=auto' 2>/dev/null