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

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Mar 18, 2014, 01:59 UTC
Message-ID: <CAPig+cRGwBV5CE3X9yGZGRNtKojCagz_F_f4Kd+S+D_qX+ZwJw@mail.gmail.com>
In-Reply-To: <CALs4jVGxmirDdO03kaKco7=NrTaXBinY=1U71=fYwX1mbqNDOA@mail.gmail.com>

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

Show 14 quoted lines

> 2014-03-17 18:52 GMT-04:00 Junio C Hamano <gitster@pobox.com>:
>> Thanks.  This probably needs retitled, though (hint: "replaces"?
>> who does so?) and the message rewritten (see numerous reviews on
>> other GSoC micros from Eric).
>
> I found some messages [1] by Eric concerning imperative voice ("simplify"
> rather than "simplifies/ed").
>
> Other than the change of verb, what sort of changes are you looking for in
> the description? It doesn't look much different than, for instance, this
> [2] commit in the log.
>
> [1]: http://article.gmane.org/gmane.comp.version-control.git/243848
> [2]: https://github.com/git/git/commit/0eea5a6e91d3da6932c13f16fdf4b4e5ed91b93c

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. Here's your original:

    Subject: general style: replaces memcmp() with starts_with()

    memcmp() is replaced with negated starts_with() when comparing
    strings from the beginning and when it is logical to do
    so. starts_with() looks nicer and it saves the extra argument of
    the length of the comparing string.

In the subject, "general style" is a bit unusual. This isn't just a stylistic change; it's intended to improve code clarity.

Examples of redundancy:

"memcmp() is replaced with ...": The subject already says this.

"negated starts_with()": Having to negate the value is a necessary artifact of switching to starts_with(), thus it's a mere implementation detail of the change. There is no mystery here. Anyone familiar with memcmp() and starts_with() will understand implicitly why the value is negated.

"when comparing strings from the beginning": That's effectively implied by the name starts_with(). (And, if you did happen use starts_with() at a location other than the start-of-string, a reviewer would likely point out that doing so makes the code less readable.)

"when it is logical to do so": The scope of the patch already implies that the changes are restricted to cases when it is logical to do so (and if it's not, a reviewer will question the illogical changes).

"starts_with() looks nicer": Subjective, as written. Reworded to be more forceful, it might make a decent justification for the patch (see below).

"saves the extra argument": This is incidental to the real change, which is to make the code read more clearly, and is an obvious artifact of switching from memcmp() to starts_with().

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().