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
