Re: [PATCH 02/17] revert: Inline add_message_to_msg function

[Jonathan Nieder <jrnieder@gmail.com>]

Hi,

Ramkumar Ramachandra wrote:

> Fixed all issues.  The commit message now reads
>
> revert: Inline add_message_to_msg function
>
> The add_message_to_msg function is poorly implemented, has an unclear
> API, and only one callsite.  Replace the callsite with a cleaner
> implementation.  Additionally, fix a bug introduced in 9509af6 (Make
> git-revert & git-cherry-pick a builtin, 2007-03-01) -- a NULL pointer
> was being dereferenced when "\n\n" was not found in "message".

That's basically the same as before, with "dereferenced" in place of
"incremented".  An improvement, sure, but it still doesn't answer the
basic questions like "how can I reproduce the bug?".

So no, in the commit message at least I don't think all issues are
fixed.  The subject line says the idea is to inline this function, but
it's actually a behavior change.  The description of the behavior
change makes reference to a variable "message" without saying what the
content of that variable is --- e.g., will trying to cherry-pick a
commit with a one-line commit message trigger this bug?  (No, it
won't.)

And honestly, making that small change and calling it fixed just feels
half-hearted and rushed.  After changing it, did you look back over
the message, read it as though unknowledgeable about the patch, and
decide whether it conveyed the right impression, which is the
equivalent of testing when writing documentation?