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

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

 



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?
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]