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

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

 



On Mon, Mar 17, 2014 at 10:33 PM, Quint Guvernator
<quintus.public@xxxxxxxxx> wrote:
> 2014-03-17 21:59 GMT-04:00 Eric Sunshine <sunshine@xxxxxxxxxxxxxx>:
>> 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.

> [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.
--
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]