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

From: Quint Guvernator <quintus.public@gmail.com>
Date: Mar 18, 2014, 02:33 UTC
Message-ID: <CALs4jVFVO8Jf-nn0PMtOYeMMKsB869KWQv8vV5kAg1itC1isiQ@mail.gmail.com>
In-Reply-To: <CAPig+cRGwBV5CE3X9yGZGRNtKojCagz_F_f4Kd+S+D_qX+ZwJw@mail.gmail.com>

2014-03-17 21:59 GMT-04:00 Eric Sunshine <sunshine@sunshineco.com>:

Show 6 quoted lines

> 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?

[1]: https://github.com/git/git/blob/fca26a/Documentation/SubmittingPatches#L87-L116

Show 8 quoted lines

> 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?

Thank you for the feedback, Quint