Applying outstanding patches [was: Release what we've got?]

[Julian Foad]

Charles Levert wrote:
> Either way, I'd like to see the Red Hat/Fedora
> patches applied soon (either before or just after
> a release).
>
> Recall that we first had agreed to start applying
> them, but Claudio asked to be given a while
> for him to come up with something even better.
> Unfortunately, he's gone from this project,
> but this means that our best avenue is now to
> apply these patches as is at first (as opposed
> to reworking them).  They have been tested by
> quite a user community.

I have applied those that I felt happy with.  I have looked at each remaining 
one and felt that it was not right or that I could not understand it or that I 
could not reproduce the problem (as with bug #13161 - missing check on memchr() 
return val in EGexecute - 
<https://savannah.gnu.org/bugs/?func=detailitem&item_id=13161> - where the 
"complete test case" succeeds for me even with unpatched grep-2.5.1).

I ought to pick one (at a time), analyse it as best I can, post my analysis and 
questions here on this list, and see what other people can do to help.

If you have some time, please could you pick one of them and do that?

The first thing that would help the review in nearly all cases is a good log 
message for the change that explains what behaviour was changed, why (including 
pointer to bug report if applicable), whether and where there is a regression 
test, and finally what source code was changed to fix the problem.

The next thing that most of the patches need is a regression test.

Even though those patches have had real-world testing, that does not in itself 
mean that they are good.  Some of them have definitely been poor, fixing an 
obvious case and missing the less obvious cases, or fixing something by 
duplicating a lot of code.  So analyse them critically and do not accept code 
that you do not understand or of poor quality.


Here are some initial comments on the RedHat patches specifically:

"oi" patch:
  Is it safe to be changing our copy of "regex.h" like this?  Why the change 
from #include "regex.h" to <regex.h> ?  Do these changes to search.c make some 
of our existing case-folding logic redundant?

"w" patch:
  I'm a bit uncomfortable with the amount of code added by this patch, but if 
it really does fix the corresponding tests (which are already in CVS) then 
probably OK.

"icolor" patch:
  This just removes some code, saying that it is "redundant and incorrect".  I 
suspect that it depends on some other patch to have been applied first - and I 
think maybe I asked and was told - but the patch issue doesn't say so, and it 
should.  If I'm wrong, and all the tests that currently pass still pass after 
applying this, that would be great, but I think I tried that before.  I think 
this is superceded by patch #3767: Remove two match_icase code paths from 
prline() in src/grep.c.

"dfa-optional" patch:
  This makes Grep's operation depend on an environment variable 
"GREP_USE_DFA".  We shouldn't do that, at least not without providing some 
documentation.

"egf-speedup" patch:
  We need to read the mailing list to see what was said about this.  It may be 
OK, but it may well have been superceded, and it looks to me like it adds far 
too much code.

- Julian