Re: [PATCH] cherry-pick: Append -x line on separate paragraph

From: Robin Stocker <robin@nibor.org>
Date: Sep 5, 2012, 20:36 UTC
Message-ID: <1410595949.1269.1346877367487.JavaMail.root@bazinga.schuettel.ch>
In-Reply-To: <7vsjaxaasv.fsf@alter.siamese.dyndns.org>

Junio C Hamano writes:

Show 16 quoted lines

> Robin Stocker <robin@nibor.org> writes:
> 
> >  		if (opts->record_origin) {
> > + /* Some implementations don't terminate message with final \n, so
> > add it */
> > + if (msg.message[strlen(msg.message)-1] != '\n')
> > + strbuf_addch(&msgbuf, '\n');
> 
> I can agree that this is a good change.
> 
> > + strbuf_addch(&msgbuf, '\n');
> 
> But this is somewhat dubious. Even if what we are adding is merely
> an extra LF, that changes the mechanically generated output format
> and can break existing hooks that read from these generated commit
> log template.

Hm, for a script to break because of an extra LF it would have to be very badly written. If it looks for "\n(cherry picked ...", it would still work. But I see the point.

> Is there a reason better than "having an empty line there look
> better to _me_" to justify this change?

Yes:

* If the original commit message consisted just of a summary line,
  the commit message after -x would then not have a blank second
  line, which is bad style, e.g.:

The title of the original commit (cherry picked ...)

* If the original message did not have any trailers, the appended
  text would stick to the last paragraph, even though it is a
  separate thing.

These don't apply to the git project itself, as its commit message always have at least a Signed-off-by. But there are projects where this is not the case and the above reasons apply.

Maybe the solution is to detect if the original commit message ends with a trailer and in that case keep the existing behavior of not inserting a blank line?

Show 36 quoted lines

> >  			strbuf_addstr(&msgbuf, "(cherry picked from commit ");
> >  			strbuf_addstr(&msgbuf, sha1_to_hex(commit->object.sha1));
> >  			strbuf_addstr(&msgbuf, ")\n");
> 
> Having said that, I've seen proposals to update this message to
> format more like the other trailers, so that we would see this:
> 
> The title of the original commit
> 
> The log message taken from the original
> commit comes here.
> 
> Signed-off-by: First person who signed off the original
> Signed-off-by: Another person who signed off the original
> Cherry-picked-from: a9bbc121ea850e49d52ba3cb5a6b7f8077d195d2
> 
> in the editor, to allow you to add your own Sign-off at the end to
> make it look like this:
> 
> The title of the original commit
> 
> The log message taken from the original
> commit comes here.
> 
> Signed-off-by: First person who signed off the original
> Signed-off-by: Another person who signed off the original
> Cherry-picked-from: a9bbc121ea850e49d52ba3cb5a6b7f8077d195d2
> Signed-off-by: Me who did the cherry-pick
> 
> I think that might be a worthwhile thing to do perhaps as an
> optional behaviour (e.g. perhaps triggered with a new option
> "--trailer", or with the same "-x" but only when "cherry-pick.origin
> = trailer" configuration is set, or something). At that point, the
> output will look vastly different to existing hooks and those who
> care how this field looks like are forced to be updated, but as long
> as it is an opt-in feature, it may be worth it.

Oh, I like that proposal. I'd lean towards a new --trailer option I think.

It would have the same problem of having to append it on a separate paragraph if the original commit message does not already have a trailer though.

But I still think that adding the "(cherry picked ..." on a separate paragraph would be a good thing until "Cherry-picked-from" can be used.

Regards,
  Robin Stocker