Re: [PATCH v4] Documentation fix: git log -p does not imply -c.

From: Adam Monsen <haircut@gmail.com>
Date: Mar 9, 2011, 21:25 UTC
Message-ID: <4D77F03B.4050605@gmail.com>
In-Reply-To: <7vtyfdaz4k.fsf@alter.siamese.dyndns.org>

Junio C Hamano wrote:

>> - Read previous commit messages. Emulate the best ones.
> 
> I thought you were trying to teach new people how to gauge the "goodness"
> with this list.  How would they know which ones are "best"? ;-)

Heh, glad you asked. I was being intentionally vague. The list just provides some ideas. "Best" is an ideal. It's subjective. I don't want people to just do what I say, I want to inspire them to do something better.

Maybe:

  Read previous commit messages. Strive to make yours better.

Again, we're talking about commit messages, not raising children or anything. But commit messages help others, so why not try to make them the "best" they can be?

> I try to give this list to new contributors early in their initiation
> process (ideally before their patches hit the codebase).  That is probably
> why many of the existing commits you saw in "git log" more or less
> conformed to the recommendation.

Cool. That's well-written and helpful.

>> - Be verbose!
> 
> Please don't.  We want sufficiently detailed description, but we don't
> want verbosity.

:) fair enough. I wrote this because, in the Mifos community, I struggle getting people to write enough of *anything* in commit messages. Good to know that's not a problem in git land.

> How about this as a not-too-verbose compromise?

I like it. Some specific comments below.

> -		- includes motivation for the change, and contrasts
> -		  its implementation with previous behaviour
> +	  . explains the problem the change tries to solve, iow, what
> +	    is wrong with the current code without the change.

Good, but spell out "in other words". I had to look up that acronym.

> +	  . justifies the way the change solves the problem, iow, why
> +	    the result with the change is better.

Ditto.

> +	  . alternate solutions considered but discarded, if any.

Prepend with "mentions" or something.

> +	- try to make sure your explanation can be understood without
> +	  external resources. Instead of giving a URL to a mailing list
> +	  archive, summarize the relevant points of the discussion.

+1, but I prefer both the summary *and* the link.

So maybe:

  Instead of providing only a URL to a mailing list archive, summarize
  the relevant points of the discussion.

Show 5 quoted lines

> -Describe the technical detail of the change(s).
> +Give an explanation for the change(s) that is detailed enough so
> +that people can judge if it is good thing to do, without reading
> +the actual patch text to determine how well the code does what
> +the explanation promises to do.

+1!

I noticed a few grammar errors in the doc, too. I'll reply with another patch.

Sorry if this continued work on the SubmittingPatches documentation is getting annoying. It's been useful for me to learn how things are done around here. And it's important that the document be well written, so people actually use it. I just thought we might as well improve it a little more since we've already started.