Re: [PATCH 1/1] general style: replaces memcmp() with starts_with()

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Mar 18, 2014, 04:07 UTC
Message-ID: <CAPig+cSu=zgXdd55dj-u6mOzHqpUme7r-7OH+jjnOEt2P6C0vw@mail.gmail.com>
In-Reply-To: <CALs4jVFVO8Jf-nn0PMtOYeMMKsB869KWQv8vV5kAg1itC1isiQ@mail.gmail.com>

On Mon, Mar 17, 2014 at 10:33 PM, Quint Guvernator <quintus.public@gmail.com> wrote:

Show 16 quoted lines

> 2014-03-17 21:59 GMT-04:00 Eric Sunshine <sunshine@sunshineco.com>:
>> I can't speak for Junio, but the description could be made more
>> concise and to-the-point. Aside from using imperative voice, you can
>> eliminate redundancy, some of which comes from repeating in prose what
>> the patch itself already states more concisely and precisely, and some
>> from repeating what is implied by the fact that you're making such a
>> change in the first place.
>
> Wow, thanks for the detailed review. This mail will be something to
> which I can refer when making future changes.
>
>> In the subject, "general style" is a bit unusual. This isn't just a
>> stylistic change; it's intended to improve code clarity.
>
> It felt a little awkward, but I was trying to follow our guide [1] for
> commit messages. It is all right to omit the leading identifier?

Since this particular patch touches so many different files and functions, it is difficult to craft a suitable leading identifier. The alternative would be to split the patch into smaller pieces. Ultimately, as the project maintainer, Junio must be the one to decide if the monolithic patch lacking leading identifier is sufficient or if smaller patches would be preferred.

Show 15 quoted lines

> [1]: https://github.com/git/git/blob/fca26a/Documentation/SubmittingPatches#L87-L116
>
>> A patch of this nature doesn't require much more description than
>> stating what it does ("replace memcmp() with starts_with()") and why
>> ("improve code clarity"). The following rewrite might be sufficient:
>>
>>     Subject: replace memcmp() with starts_with()
>>
>>     starts_with() indicates the intention of the check more clearly
>>     than memcmp().
>
> This is more concise; thank you. I will adapt this as the message for
> the next version of this patch. Would it be wise to mention magic
> numbers, as the discussion surrounding the rationale of this patch,
> especially with Junio and Michael, has centered around that?

I was thinking of mentioning magic numbers in the example, but decided that their removal was a natural and obvious consequence of the improvement to code clarity, so it wasn't strictly necessary to talk about it. On the other hand, it is a good secondary justification, thus it should be perfectly acceptable to mention it. If I was writing the commit message, I might start by saying "As an additional benefit..." and then talk a bit about magic number retirement.