Re: Bug #11022: Line wrapping causes GREP_COLOR background color to "smear"

[Charles Levert]

* On Tuesday 2005-06-14 at 00:44:56 +0100, Julian Foad wrote:
> Charles Levert wrote:
> >There are also bugs such as
> >
> >   <https://savannah.gnu.org/bugs/?func=detailitem&item_id=11022>
> >
> >that could be closed right now as well because
> >a complete solution is available to them.
> 
> And, in that patch issue:
> >The framework introduced by the preprocessor macros is a good thing to 
> >have now as moves the actual SGR strings in one place and it will be 
> >re-used many times by a newer patch #3644.
> >
> >This includes a corresponding update to tests/foad1.sh as well.
> >
> >Only the ChangeLog entry is missing and will be added upon commit. 
> 
> It would really help me and others to review it if you would provide a log
> message that describes what the purpose of the patch is, and what changes 
> it makes.

For the patches I wrote, keep in mind that I can
do both at once myself: apply them and write a
complete ChangeLog entry.


> In this case the patch is simple enough that I can determine the two things
> that it does by inspection, and write a log message like so (in the style 
> used
> by Apache and Subversion):
> 
> [[[
> Fix bug #11022: Line wrapping causes GREP_COLOR background color to "smear",
> by outputting a "clear to end of line" control sequence after colored 
> output.
> Factor out the printing of colour control sequences into macros.
> 
> * src/grep.c
>   Add new macros for starting and ending colored output.
>   Make the "end of colour" string also clear to the end of the line.
>   (prline): Use the new macros.
> 
> * tests/foad1.sh
>   Adjust the regression tests to expect the new codes.
> ]]]
> 
> You could condense that a bit for ChangeLog, but don't leave out the "why" 
> parts.

Do we really need to have both a ChangeLog
entry and a long CVS commit message?  I would
rather cover everything that needs to be in the
ChangeLog (like was done before the GNU Project
really started using CVS widely) and make sure
to commit the ChangeLog file and the modified
files at the same time, with a shorter CVS commit
log message.  After all, the ChangeLog file is
the one to be part of the distributed software
packages, not the CVS logs.


> In your patch you say:
> >+/* Select Graphic Rendition (SGR) strings.  */
> >+#define SGR_ARG "\33[%sm"
> >+/* Also Erase in Line (EL) to Right by default.  */
> >+#define SGR_END "\33[m\33[K"
> 
> I didn't really understand that last comment - especially the "by default".
> Could you explain it or tell me if this would be a good replacement:

Keep in mind that this was cannibalized from
a bigger patch.  The "by default" refers
to the capability to change that with the
proposed GREP_COLORS.  I didn't feel it was
worth changing, especially since it's still
the default, there's just no way to override it
for now.


> >/* Select Graphic Rendition (SGR) strings.  */
> >/* Start colored output with a given color code. */
> >#define SGR_ARG "\33[%sm"
> >/* End colored output and erase to end of line.  */
> >#define SGR_END "\33[m\33[K"
> 
> I'm not totally confident that this clearing to EOL is the right approach 
> to the problem.  My concerns run along the lines of "Isn't this the 
> terminal's fault, somehow?" and "What if people are depending on Grep to 
> output just simple colour control sequences?"  What do you think?  I would 
> be happier if I could see a precedent for this behaviour, perhaps in 
> another GNU program.

I have explored this in so much details it's
not funny.  But that was a while ago, so please
search the archive and look at patch #3644.
I looked exhaustively at all terminals in
the terminfo database.  After a discussion
with Stepan, the default was changed from not
having the "\33[K" but being able to add it to
the opposite.


> Actually, I'm not sure I'm convinced by your "tabs" argument.

Already hashed to death as well.  Please
experiment with "sgreol.sh" from patch #3644.


> If, on a 
> particular terminal, tabs fail to fill a background that is meant to be 
> noraml colour (e.g. black), won't tabs also fail to fill parts of the 
> matched text that are meant to be highlighted (e.g. red)?

Yes!!  (Finally somebody who understands this!)
But unfortunately, that just most terminals (all
the usual VT ones) are designed, and grep is
only one colorizing program that has to suffer
with this.  It would be totally inappropriate
for grep to do tab expansion as it must strive
to report the matching text as is.

Keep in mind that the default for highlighting
matched text, bold red over default background,
only sets the foreground and thus does not suffer
from this problem

The preferred workaround is to simply to pipe the
output to "less -R", which does tab expansion
but still allow the user to highlight them
by searching (/^I).  (Shameless plug:  use my
version of less to get better results with
"less -R", highlighting, horizontal scrolling,
etc.)


> Maybe the latter 
> is not so ugly, but it's still wrong.

The proper design for terminals would have been
to have the HT (tab) character fill the cells
that are moved over with colored spaces and
to reserve the currently implemented behavior
for CHT ("\33[I").  As it stands, there are two
ways to get the current behavior and it's just
redundant.  Also keep in mind that the currently
implemented HT behavior will also skip over and
leave any characters that are already there if
they are not spaces.

> Should we also clear to EOL after 
> changing colour at the start of a match, for symmetry?

Nothing grep does necessitates that.  Otherwise,
it not even our job to make sure the whole
terminal is in a sane state when we start.


> Sorry I've so many questions for such an apparently simple patch.

It is a complex issue, but most of it is not
grep's problem.  I have explored it in great
details, not just for grep, so ask away if you
still feel like it's necessary.