Re: [patch] selected/context colors

[Julian Foad]

Charles Levert wrote:
> * On Monday 2005-11-14 at 12:58:20 +0000, Julian Foad wrote:
>
> > I can understand your point of view too,
>
> It's not so much my point of view as it's what
> 2.5.1a used to do.  See below.

(That is, for one thing, highlighting matches within context lines with "-v".)

Oops.  I stand corrected.  I'm sorry for not checking this before.  This will 
change my opinion.  I will re-read the previous message and what I wrote, and 
see what I think now.


> (Make sure to try 2.5.1a with different combinations
> of options, to see what its behavior actually was.)

Testing v2.5.1 now, I find:

* The combination "-v -C" indeed highlighted matches within context lines.

* The combination "-o -v" didn't produce any output.  I consider that an 
unsupported combination, and likewise "-o -C", because the output from that 
appears to be totally spurious (undesigned, accidental).  Therefore I think it 
is our job to forbid or define the meaning of these combinations.

Are there any other combinations I should try?



> > (A detail: the old "GREP_COLOR" option should override 
>
> The old GREP_COLOR does _not_ override
> GREP_COLORS, to be explicit.  It can change
> the default, but is itself overriden by the new
> GREP_COLORS, which has priority.

Sorry - I knew that, but wrote it badly.  I meant "should affect", not "should 
override".

> > "matched-text-in-selected-line" only, since no highlighting used to be done 
> > on context lines.)

And I was wrong there.

> But I see what you mean.  Check it out for
> yourself, the last official release did color
> matched text, whether in selected lines or in
> context lines.  To be compatible in behavior
> in 2.5.1a, GREP_COLOR should provisionally
> set _both_ matched-text-in-selected-line and
> matched-text-in-context-line, before GREP_COLORS
> gets a chance to set them individually.

Yes, that would be sensible.


> > Secondly, the log entry indicates that this patch also changes the 
> > behaviour of "-o".  We're discussing that in another thread and it 
> > shouldn't be done until we've decided.
>
> This is a bug I introduced and that wasn't
> there before.  It's about not processing things
> in prline() when it's asked to.

Er... I'm not sure what bug this refers to and when "before" refers to, so no 
comment yet.

> The other discussion is about prline() being
> called at all for some lines, e.g., with -v
> and -o.
>
> I would like to fix what I did to prline() and
> restore its correct functionality.  prline()
> is mechanism, not policy.  The other thread [...]

Yes, prline is mechanism.  Therefore the definition of "its correct 
functionality" is for us to decide, and change whenever we like (since it is a 
private function).  The purpose of the function should be documented in a 
documentation comment at the top of the function (or, theoretically, it could 
be documented somewhere else), but there is no such comment.  That's a real 
problem: it means that anybody working in the file has to make assumptions 
(sometimes verifiable by examining the code, sometimes not) that may disagree 
with how the author or other callers of the function interpreted it.

Without a written spec. for the function, it is meaningless to ask whether a 
bug (in Grep's behaviour) is due to the function's implementation or the way 
the function is called.  The bug can be due to a mis-match of expectations 
between the caller and the implementation.



> It's interesting, though.  In both cases
> (discussion threads), the things I have advocated
> are what used to be in 2.5.1a (except for a bug
> with needless line prefixes being printed without
> a newline and with no content).  The things you
> are advocating are a departure from what 2.5.1a
> used to do.

That's partly because I assumed things about the way they behaved, without 
checking.  It is also partly because I took the view that things that have 
never worked properly should be designed properly by us now: we don't need to 
maintain compatibility with behaviours that were very broken.

> More ironically, the things you are advocating in
> the other thread are a consequence of something
> I proposed in patch #3768 (but where I didn't
> fully envision the consequences for -v).

Oh dear.  I'm sorry for the confusion I've added.  I hope some parts of the 
discussion have been useful nevertheless.

Please give me a few days to re-check the historical facts and re-consider my 
views.

- Julian