Robin Stocker <robin@xxxxxxxxx> writes:
> 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.
If you approach this change from the "information left by -x is
somehow useful" point of view, it wouldn't make any difference.
Scripts can match "(cherry picked from ..." and glean useful
information out of it, with or without an empty line around it.
But if you look at it from the other perspective [*1*], it makes a
big difference. A script that wants to excise these lines used to
be able to just delete such a line. With the proposed change, it
also has to be aware of an empty line next to it.
>> Is there a reason better than "having an empty line there look
>> better to _me_" to justify this change?
>
> Yes:
Then please have them in the proposed commit log message to justify
the change. I think the following analysis I quoted from your
message summarizes the motivation well.
> * 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 ...)
This is very true. So we at least want an empty line added when the
original is one-liner.
> * 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.
The other side of this argument is if there are trailers, we would
not want an extra blank line. We need to look at the last paragraph
of the log message and if it does not end with a trailer, we want an
additional empty line.
> 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?
Yeah, that sounds like a good change from "this makes the result
look better" point of view.
> 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.
Yes. The logic would be the same. First terminate the incomplete
last line, if any, and then look at the last paragraph of the commit
log message (one liner is a natural degenerate case of this; its
single-line title is the last paragraph) and if and only if it does
not end with a trailer, add a blank line before adding the marker.
The only difference between the two would be how the "cherry-picked
from" line is formatted.
[Footnote]
*1* Originally, we added "(cherry picked from ..." by default, and
had a switch to disable it; later we made it off by default and made
it optional (and discouraged) with "-x" and this was for a reason.
Unless the original commit object is also available to the reader of
the history, the line is a useless noise, and many people are found
to cherry-pick from their private branches; by definition, the line
is useless in the resulting commit of such a cherry-pick.
--
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