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

Junio C Hamano writes:

Robin Stocker [off-list ref] writes:

quoted

 		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.

quoted

+ 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?

quoted

 			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