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

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 4, 2012, 18:43 UTC
Message-ID: <7vsjaxaasv.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1244407605.1231.1346779006295.JavaMail.root@bazinga.schuettel.ch>

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.

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

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