[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: Patch: non-empty matches following empty ones: ChangeLog entry

From: Julian Foad
Subject: Re: Patch: non-empty matches following empty ones: ChangeLog entry
Date: Fri, 02 Sep 2005 22:49:05 +0100
User-agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7.8) Gecko/20050511

Sorry, Charles - it was unfriendly of me to criticise your log message without praising the work itself, especially as you are the one doing all the work and I am doing none.

I do value this change.

My point was that I would appreciate some _extra_ information in the change description: specifically, a high-level description of the change's purpose and overall effect. This need only be a few words in many cases. This will make the change log useful to reviewers, people updating the documentation, programmers who think the code is doing the wrong thing and who therefore need to know why it was changed to do what it does, people writing a list of fixed bugs for the release notes, etc.

I believe it is traditional in GNU ChangeLogs _not_ to state the reason for a change, but, if that reason is not captured elsewhere (e.g. in a bug tracker), there is a serious loss of information. I hope we can do better than that tradition.

Charles Levert wrote:
Given the single way that print_line_middle()
is called in the source code, it's implicit that
this is about --only-matching and --color, and

Yes, but we don't want readers of the change log to have to study how that function is/was used in that version of the project in order to understand what the change was for.

* On Friday 2005-08-26 at 11:33:34 +0100, Julian Foad wrote:
2. With "--only-matching" or "--color", don't display or color empty
  matches.  (Behaviour change.)

They were not displayed or colorized before,
so there is no change in behavior there.

Oh, you're right. I had noticed that with v2.5.1 "-o -n" an empty match was printed (at least the line head was printed), and didn't realise that it had been fixed already, and your patch to the manuals implied that you were changing this behaviour and documenting the result of the change.

+    * src/grep.c (print_line_middle): In case of an empty match,
+      make minimal progress and continue instead of aborting process
+      of the remainder of the line, in case there's still an upcoming
+      non-empty match.
+    * tests/foad1.sh: Add two tests for this.

Here, "this" is the main functional fix: that non-empty matches are displayed or coloured even after an empty match. (To over-emphasise a point: It is not specifically testing for any implementation details, and doesn't care whether a print_line_middle() function even exists.)

+    * doc/grep.texi, doc/grep.1: Document this behavior, since

Here, "this" is not that. Instead, what you documented is that only non-empty matches are displayed or coloured.

+      --only-matching and --color are GNU extensions which are
+      otherwise unspecified by POSIX or other standards.

(That's good, indicating why it should be necessary to document it.)

So this patch was (a) fixing the bug that non-empty matches were missed after an empty match, and (b) documenting the existing intended behaviour w.r.t. empty matches in the manuals. (Yes?) Excellent. Knowing that, I can test it and be happy with it. Now, after all this, you see why it would be useful to have been told that explicitly... :-)

If you would update the ChangeLog entry (correcting the description of what was documented, and adding an overall summary) I'd very much appreciate that. Or, if you'd like me to do so, I can.

And thanks again for continuing to make these fixes to GNU Grep.

- Julian

reply via email to

[Prev in Thread] Current Thread [Next in Thread]