On Mon, Mar 17, 2014 at 7:46 PM, Quint Guvernator
<quintus.public@xxxxxxxxx> wrote:
> 2014-03-17 18:52 GMT-04:00 Junio C Hamano <gitster@xxxxxxxxx>:
>> 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().
--
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