Junio C Hamano writes:
> Robin Stocker <robin@xxxxxxxxx> 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?
> > 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
--
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