Bug 1034631 - [RFE] If grep is colored, *z*grep should be as well
Summary: [RFE] If grep is colored, *z*grep should be as well
Keywords:
Status: CLOSED RAWHIDE
Alias: None
Product: Fedora
Classification: Fedora
Component: grep
Version: rawhide
Hardware: Unspecified
OS: Linux
unspecified
low
Target Milestone: ---
Assignee: Jaroslav Škarvada
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks: 1034847 1034839 1034846
TreeView+ depends on / blocked
 
Reported: 2013-11-26 09:04 UTC by Cristian Ciupitu
Modified: 2014-08-20 13:25 UTC (History)
6 users (show)

Fixed In Version:
Doc Type: Enhancement
Doc Text:
Clone Of:
: 1034839 1034846 1034847 (view as bug list)
Environment:
Last Closed: 2014-08-20 13:25:16 UTC
Type: Bug
Embargoed:


Attachments (Terms of Use)
Proposed fix for grep (344 bytes, patch)
2013-11-26 14:59 UTC, Jaroslav Škarvada
no flags Details | Diff
Proposed fix for zgrep (480 bytes, patch)
2013-11-26 15:03 UTC, Jaroslav Škarvada
no flags Details | Diff
Possible approach with dedicated script. (2.41 KB, patch)
2014-01-14 08:46 UTC, Pavel Raiskup
no flags Details | Diff
Possible approach with dedicated script. (2.41 KB, patch)
2014-01-14 08:54 UTC, Pavel Raiskup
no flags Details | Diff
Proposed fix (2.52 KB, patch)
2014-08-20 12:18 UTC, Jaroslav Škarvada
no flags Details | Diff

Description Cristian Ciupitu 2013-11-26 09:04:29 UTC
Description of problem:
The output of regular grep is now colored by default, but output of grep
for compressed files is not.

The commands are:
 - for gzip: zgrep, zegrep and zfgrep
 - for bzip2: bz2grep, bz2egrep and bz2fgrep
 - for xz: xzgrep, xzegrep and xzfgrep

One option is to patch /etc/profile.d/colorgrep.sh from the grep package
and another one is to create /etc/profile.d/colorzgrep.sh for the gzip
package, /etc/profile.d/colorbz2grep.sh for the bzip2 package and so on
for all the compression packages. The latter would also mean splitting
this bug report into 3.

Version-Release number of selected component (if applicable):
grep-2.15-1.fc20.x86_64
gzip-1.6-2.fc20.x86_64
bzip2-1.0.6-9.fc20.x86_64
xz-5.1.2-6alpha.fc20.x86_64

How reproducible:
Every time

Steps to Reproduce:
1. Run interactively one of the commands on a compressed file, e.g.:
	zgrep system /usr/share/man/man8/systemd-activate.8.gz

Actual results:
No colored output.

Expected results:
Colored output.

Additional info:
This command:
	zgrep --color=auto system /usr/share/man/man8/systemd-activate.8.gz
has colored output.

Bug #697895 and bug #600832 are related.

Keywords: coloured.

Comment 1 Jaroslav Škarvada 2013-11-26 14:57:40 UTC
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.

Comment 2 Jaroslav Škarvada 2013-11-26 14:59:37 UTC
Created attachment 829314 [details]
Proposed fix for grep

Comment 3 Jaroslav Škarvada 2013-11-26 15:03:23 UTC
Created attachment 829316 [details]
Proposed fix for zgrep

Comment 4 Jaroslav Škarvada 2013-11-26 15:57:46 UTC
Grep part of the fix pushed.

Comment 5 Pavel Raiskup 2013-11-27 09:07:27 UTC
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.).

Comment 6 Jaroslav Škarvada 2013-11-27 09:29:18 UTC
(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?

Comment 7 Pavel Raiskup 2013-11-27 09:37:58 UTC
> > 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.).

Comment 8 Jaroslav Škarvada 2013-11-27 10:26:28 UTC
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).

Comment 9 Pavel Raiskup 2013-11-27 11:22:19 UTC
(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).

Comment 10 Pavel Raiskup 2014-01-14 08:46:40 UTC
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

Comment 11 Pavel Raiskup 2014-01-14 08:54:30 UTC
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

Comment 12 Pavel Raiskup 2014-08-19 07:23:36 UTC
Ping?

Comment 13 Jaroslav Škarvada 2014-08-20 12:17:05 UTC
(In reply to Pavel Raiskup from comment #12)
> Ping?

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

Comment 14 Jaroslav Škarvada 2014-08-20 12:18:55 UTC
Created attachment 928783 [details]
Proposed fix

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

Comment 15 Pavel Raiskup 2014-08-20 12:54:05 UTC
(In reply to Jaroslav Škarvada from comment #14)
> Slightly modified / simplified patch. Are you OK with it?

Better one, thanks!

Comment 16 Jaroslav Škarvada 2014-08-20 13:23:46 UTC
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


Note You need to log in before you can comment on or make changes to this bug.