RFC patch, 2 partssequencer: allow origin line below commit title

This is somewhat of a follow-up to my previous e-mail with subject "[PATCH] sequencer: support folding in rfc2822 footer" [1], in which I proposed relaxing the definition of a commit message footer to allow multiple-line field bodies (as described in RFC2822), but its strictness was deemed deliberate.

Below is a patch set that allows placing the "cherry picked from" line without taking into account the definition of a commit message footer. For example, "git cherry-pick -x" (with the appropriate configuration variable or argument) would, to this commit message:

  commit title

  This is an explanatory paragraph.

  Footer: foo

place the "(cherry picked from ...)" line below "commit title".

Would this be better?

[1] <1472846322-5592-1-git-send-email-jonathantanmy@google.com>

Jonathan Tan (2):
  sequencer: refactor message and origin appending
  sequencer: allow origin line below commit title

 Documentation/config.txt          |  4 +++
 Documentation/git-cherry-pick.txt | 15 ++++++++-
 builtin/revert.c                  | 38 ++++++++++++++++++++-
 sequencer.c                       | 69 +++++++++++++++++++++++++++++----------
 sequencer.h                       |  7 ++++
 t/t3511-cherry-pick-x.sh          | 59 +++++++++++++++++++++++++++++++++
 6 files changed, 172 insertions(+), 20 deletions(-)

When git cherry-pick -x is invoked, a "(cherry picked from commit ...)" line is appended to the footer of a commit message that Git interprets to contain a footer; otherwise it is appended at the end as a new paragraph, preceded by a blank line. This behavior may appear inconsistent, especially to users who differ from Git in their interpretation of what constitutes a footer.

Provide the user, through a configuration option and command-line flag, the option of placing the "cherry picked" line below the commit title instead of the current behavior. This allows the "cherry picked" line to be placed in a consistent manner, independent of the nature of the footer of the existing commit message.

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
 Documentation/config.txt          |  4 +++
 Documentation/git-cherry-pick.txt | 15 +++++++++-
 builtin/revert.c                  | 38 ++++++++++++++++++++++++-
 sequencer.c                       | 39 ++++++++++++++++++++------
 sequencer.h                       |  7 +++++
 t/t3511-cherry-pick-x.sh          | 59 +++++++++++++++++++++++++++++++++++++++
 6 files changed, 152 insertions(+), 10 deletions(-)

Move the appending of the commit message and origin information into its own function, in preparation for a subsequent patch.

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
 sequencer.c | 46 ++++++++++++++++++++++++++++------------------
 1 file changed, 28 insertions(+), 18 deletions(-)

Jonathan Tan <jonathantanmy@google.com> writes:

It does not necessarily mean we can never change it when we did something deliberately, though. With a good enough justification, and with a transitition plan if the backward incompatibility is severe enough to warrant one, we can change things.

I vaguely recall that there were some discussion on the definition of "what's a trailer line" with folks from the kernel land, perhaps while discussing the interpret-trailers topic. IIRC, when somebody passes an improved version along, the resulting message's trailer block may look like this:

    Signed-off-by: Original Author <original@author.xz>
    [fixed typo in the variable names]
    Signed-off-by: Somebhody Else <somebody@else.xz>

and an obvious "wish" of theirs was to treat not just RFC2822-like "a line that begins with token followed by a colon" but also these short comments as part of the trailer block. Your original wish in [*1*] is to also treat "a line that begin with a whitespace that follows a line that begins with token followed by a colon" as part of the trailer block and I personally think that is a reasonable thing to wish for, too.

I recall that I was somewhat surprised and dissapointed to see no change to interpret-trailers when you tried [*1*], which was really about improving the definition of what the trailer block is, by the way.

In any case, if we want to improve what the trailer block is, we would certainly need to make sure what is inserted by "cherry-pick -x" is also considered as part of the trailer block, so it may be necessary to change it to "Cherry-picked-from: ..." while doing so. I dunno.

It is not immediately obvious what such a change buys us. Wouldn't the current code place that line below "Footer: foo"? I cannot think of any reason why anybody would want to place "cherry-picked from" immediately below the title and before the first line of the body.

[Footnotes]

*1* http://public-inbox.org/git/1472846322-5592-1-git-send-email-jonathantanmy@google.com/

On 09/29/2016 02:56 PM, Junio C Hamano wrote:

If we allowed arbitrary lines in the trailer block, this would solve my original problem, yes.

Sorry, I had missed that.

Looking at that, it seems that sequencer.c started interpreting the last paragraph of the commit message as a footer and adding an exception for "cherry picked from" in commit b971e04 ("sequencer.c: always separate "(cherry picked from" from commit body", 2013-02-12). So the interpretations of sequencer.c and interpret-trailers were already divergent, but I should have probably at least discussed that.

Yes, the current code would place it below "Footer: foo" without a blank line before it, but if it thinks that the so-called footer is not actually a footer, it would insert a blank line before that line.

As for a reason:

1) I do not have a specific reason for placing it in that exact 
position, but I would like to be able to place the "cherry picked from" 
line without affecting the last paragraph (specifically, without making 
the "cherry picked from" line the only line in the last paragraph). It 
seems to me that placing it below the title was the most straightforward 
way to do that - this way, Git can have its own idea of what a footer 
constitutes, and the user can treat it as completely separate from the 
"cherry picked from" line mechanism.

1a) (Avoiding the footer might also be a good way of more clearly defining what the footer is. For example, currently, "cherry picked from" is treated as a special case in sequencer.c but not in trailer.c, as far as I can tell. If we consistently avoided the footer, we wouldn't need such a special case anywhere.)

2) The Linux kernel's repository has some "commit ... upstream." lines 
in this position (below the commit title) - for example, in commit 
dacc0987fd2e.

[1] https://kernel.googlesource.com/pub/scm/linux/kernel/git/stable/linux-stable/+/dacc0987fd2e25a8b4b8c19778862ba12ce76d0a

Jonathan Tan <jonathantanmy@google.com> writes:

OK.

It is not too late to discuss it. I still think it is a good longer term plan to try to unify the definition of what a trailer block is and the implementation of the code that determines the boundary between the log message proper and the trailer block and that allows us to manipulate the trailer block, that currently is scattered across multiple places into one. Historically, "commit -s" had one (because it needed to decide if it needs to see if the last sign-off is already the one it is adding, and to decide if a blank line is needed before the sing-off being added), "am -s" had another, and "cherry-pick" probably had one, too. "interpret-trailers" was, at least originally, envisioned as an effort to develop a unified machinery that can be called from these codepaths, and to aid the development and encourage its use, it also had its own end-user facing command. Your interest in the "trailer" topic may be a good trigger for us to further that original vision.

That is one of the numerous shortcomings of the "interpret-trailers" that is still not finished, I would say.

"A group of people seem to prefer it there" does not lead to "therefore let's move it there for everybody". It does open a possibility that we may want to add a new option to put it there, but does not justify changing what existing "-x" option does.

On 09/30/2016 12:34 PM, Junio C Hamano wrote:

To clarify, my patch adds the new option you described (to place it below the title instead of at the bottom of the commit message). The default is still the current behavior.

Jonathan Tan <jonathantanmy@google.com> writes:

Ah, sorry, I missed that.  No objection from me on this point then.

Thanks.

Junio C Hamano <gitster@pobox.com> writes:

Here is an experiment I ran during my lunch break. The script (attached) is meant to run in the kernel repository and for each log messages of each non-merge commit:

 * find its last paragraph, where the definition of paragraph is
   simply "a blank/empty line";

 * inspect if there is at least one RFC2822-header-looking line, or
   a line that begins with "(cherry picked from";

 * dump the ones that do not pass the above criteria.

My cursory look of the output did not spot a legitimate trailer block that we should have identified. The output lines shown were ones that are not signed off at all (e.g. af8c34ce6ae32add that says "Linux 4.7-rc2"), ones that has three-dash line "---" in them (e.g. 133d558216d9), ones that has diffstat that should have been after "---" (e.g. 259307074bfcf1f).

The story is the same if you run it in git.git; the "do we have at least one rfc2822-header-looking line or '(cherry picked from' line in the last paragraph? if so, then that is an existing trailer block" seems to be a good heuristics to cover many cases like these:

    d0196c8d5d3057c5c21a82f3d0113ca8e501033b
    Signed-off-by: Arnd Bergmann <arnd@arndb.de>
    [tomi.valkeinen@ti.com: resolved conflicts]
    Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>

    59f0aa9480cfef9173a648cec4537addc5f3ad94
    Link 1: https://bugzilla.kernel.org/show_bug.cgi?id=9916
            http://bugzilla.kernel.org/show_bug.cgi?id=10100
            https://lkml.org/lkml/2008/2/25/282
    Link 2: https://bugzilla.kernel.org/show_bug.cgi?id=9399
            https://bugzilla.kernel.org/show_bug.cgi?id=12461
            https://bugzilla.kernel.org/show_bug.cgi?id=11880
    Link 3: https://bugzilla.kernel.org/show_bug.cgi?id=11884
            https://bugzilla.kernel.org/show_bug.cgi?id=14081
            https://bugzilla.kernel.org/show_bug.cgi?id=14086
            https://bugzilla.kernel.org/show_bug.cgi?id=14446
    Link 4: https://bugzilla.kernel.org/show_bug.cgi?id=112911
    Signed-off-by: Lv Zheng <lv.zheng@intel.com>
    Tested-by: Chris Bainbridge <chris.bainbridge@gmail.com>
    Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

-- >8 -- #!/bin/sh

git log --no-merges |
perl -e '
	sub flush {
		my ($commit, @lines) = @_;
		my $seen_good = 0;
		for (@lines) {
			if (/^[-A-Za-z0-9]+: / ||
			    /^\(cherry picked from/) {
				$seen_good = 1;
				last;
			}
		}
		if (!$seen_good) {
			print "\n$commit\n";
			for (@lines) {
				print;
			}
		}
	}

	my (@lines, $this);
	while (<>) {
		if (/^commit (.*)$/) {
			my $next = $1;
			flush($this, @lines);
			@lines = ();
			$this = $next;
		}
		if (s/^    //) {
			if (/^\s*$/) {
				@lines = ();
			} else {
				push @lines, $_;
			}
		}
	}
	if (@lines && $this) {
		flush($this, @lines);
	}
'

On 09/30/2016 01:49 PM, Junio C Hamano wrote:

That sounds reasonable to me. Would a patch set to implement this new trailer block heuristic (in both sequencer and trailer) be reasonable? And if yes, should trailer know about the "(cherry picked from" prefix? (I can see it both ways - knowing about the "(cherry picked from" prefix would make it consistent with sequencer, but it seems like a detail that it shouldn't know about. Writing "Cherry-picked-from:" instead probably wouldn't solve our problem because, for backwards compatibility, we would still need to support reading the old format.)

Jonathan Tan <jonathantanmy@google.com> writes:

If we were to go that route, I'd suggest keeping the historical practice supported, exactly because you would need to be prepared to cherry-pick an old commit.

It may be necessary for the code to analize the lines in a block identified as "likely to be a trailing block" more carefully, though. The example 59f0aa94 in the message you are responding to has "Link 1:" that consists of 3 physical lines. An instruction to interpret-trailers to add a new one _after_ "Link-$n:" may have to treat these as a single logical line and do the addition after them, i.e. before "Link 2:" line, for example.

I also saw

	Signed-off-by: Some body <some@body.xz> (some comment
        that extends to the next line without being indented)
	Sined-off-by: Some body Else <some.body@else.xz>

where the only clue that the second line is logically a part of the first one was the balancing of parentheses (or [brakets]). To accomodate real-world use cases, you may have to take into account a lot more than the strict rfc-822 style "line folding".

On 10/03/2016 12:17 PM, Junio C Hamano wrote:

If we define a logical trailer line as the set of physical lines until the next trailer line...it is true that this has some precedence in that such lines can be written (although this might not be intentional):

   $ git interpret-trailers --trailer "a=foo
   bar" </dev/null

   a: foo
   bar

This solves the "Line 1:" case above, but raises other issues:

1. Checking for existence (trailer.ifexists) is now conceptually more 
difficult. For example, handling of inner whitespace in between lines 
might be confusing (currently, there is only one line, and whitespace 
handling is clearly described).
2. Replacement (trailer.ifexists=replace) might replace more than expected.
3. There is a corner case - the first line might not be a trailer line. 
Even if interpret-trailers knew about "(cherry picked from", a user 
might enter something else there. (We could just declare such blocks as 
non-trailers, but if we are already loosening the definition of a 
trailer, this might be something that we should allow.)

My initial thought was to think of a trailer as a block of trailer lines possibly interspersed with other lines. This leads to interpret-trailers placing the trailer in the wrong place if trailer.where=after, as you describe, but this might not be a big problem if trailer.where=after is not widely used (and it is not the default). (The other options behave as expected.)

There are other options like checking for indentation or checking for balanced parentheses/brackets, but I think that these would lead to surprising behavior for the user (this would mean that whitespace or certain characters could turn a valid trailer into an invalid one or vice versa, or change the behavior of trailer.ifexists, especially "replace").

Jonathan Tan <jonathantanmy@google.com> writes:

Yes, that is exactly why I said that it may be necessary for the code to analize the lines in a block identified as "likely to be a trailing block" more carefully. We can afford to be loose as long as the only allowed operation is to append one at the end, but once we start removing/replacing an existing entry, etc., the definition of what an entry is becomes very much relevant.

On 10/03/2016 03:13 PM, Junio C Hamano wrote:

I agree, and I was trying to discuss the possible alternatives for the definition of what an entry is in my previous e-mail.

If you think that the alternatives are still too loose, I'm not sure if we can make it any tighter. As far as I know, we're dealing with trailers like the following:

   Signed-off-by: A <author@example.com>
   [This has nothing to do with the above line]
   Signed-off-by: B <buthor@example.com>

and:

   Link 1: a link
     a continuation of the above

and:

   Signed-off-by: Some body <some@body.xz> (comment
   on two lines)

As I stated in the quoted paragraph, one possibility is to use indentation and/or balanced parentheses/brackets to determine if a trailer line continues onto the next line, and this would handle all the above cases, but I still think that these would lead to surprising behavior. Hence my suggestion to just simply define it as a single physical line. But if you think that the pros (of the more complicated approach) outweigh the cons, I'm OK with that.

One alternative is to postpone this decision by changing sequencer only (and not trailer) to tolerate other lines in the trailer. This would make them even more divergent (sequencer supports arbitrary lines while trailer doesn't), but they were divergent already (sequencer supports "(cherry picked by" but trailer doesn't).

Jonathan Tan <jonathantanmy@google.com> writes:

Given that we internally do not use the "trailers" for anything real, anything you decide to do here would be an improvement ;-) Before, users couldn't even get any of the examples (below, from your message) recognized as trailer blocks.

So I would say it is perfectly OK if your update works only for cases we can clearly define the semantics for. For example, we can even start with something simple like:

 * A RFC822-header like line, together with any number of whitespace
   indented lines that immediately follow it, will be taken as a
   single logical trailer element (with embedded LF in it if it uses
   the "line folding").  For the purpose of "replace", the entire
   single logical trailer element is replaced.

 * A line that begins with "(cherry picked from" and "[" becomes a
   single logical trailer element.  No continuation of anything
   fancy.

 * A line with any other shape is a garbage line in a trailer
   block.  It is kept in its place, but because it does not even
   have <token> part, it will not participate in locating with
   "trailer.where", "trailer.ifexists", etc.

A block of lines that appear as the last paragraph in a commit message is a trailer block if and only if certain number or percentage of lines are non-garbage lines according to the above definition.

The operations done by the codepaths in the core part of the system are much simpler subset of what "interpret-trailers" wants to support, namely,

 - append "(cherry picked from X)" at the end.

 - append "S-o-b:" at the end.

 - append "S-o-b:" unless the same line appears as the last line in
   the existing trailer block.

and these are quite compatible with a simplified definition of what a logical line is illustrated in the above example, I would think.

I wonder if we can share a new helper function to do the detection (and classification) of a trailer block and parsing the logical lines out of a commit log message. The function signature could be as simple as taking a single <const char *> (or a strbuf) that holds a commit log message, and splitting it out into something like:

    struct {
	const char *whole;
	const char *end_of_message_proper;
	struct {
		const char *token;
		const char *contents;
	} *trailer;
	int alloc_trailers, nr_trailers;
    };

where 

 - whole points at the first byte of the input, i.e. the beginning
   of the commit message buffer.

 - end-of-message-proper points at the first byte of the trailer
   block into the buffer at "whole".

 - token is a canonical header name for easy comparison for
   interpret-trailers (you can use NULL for garbage lines, and made
   up token like "[bracket]" and "(cherrypick)" that would not clash
   with real tokens like "Signed-off-by").

 - contents is the bytes on the logical line, including the header
   part

E.g. an element in trailer[] array may say

    {
	.token = "Signed-off-by",
        .contents = "Signed-Off-By: Some Body <some@body.xz>\n",
    }

With something like that, you can manipulate the "insert at ...", "replace", etc. in the trailer[] array and then produce an updated commit message fairly easily (i.e. copy out the bytes beginning at "whole" up to "end_of_message_proper", then iterate over trailer[] array and show their contents field). The codepaths in the core part only need to know

 - how to check the last item in trailer[] array to see if it ends
   with the same sign-off as they are trying to add.

 - how to append one new element to the trailer[] array.

 - reproduce an updated commit log message after the above.

Hmm?

Junio C Hamano <gitster@pobox.com> writes:

An addendum. We may also want to be prepared to accept an input that has some garbage lines _after_ the trailer block, if we can clearly identify them as such. For example, we could change the definition of "the last paragraph" as "the block of lines that do not have any empty (or blank) line, that appear either at the end of the input, or immediately before three-dash lines", to allow

    commit title
    
    explanation of the change

    Signed-off-by: Some Body <some@body.xz>
    [some other things]
    Acked-by: Some Other Person <some@other.xz>

    ---
     additional comment

which (unfortunately) is a rather common pattern for people who plan to send the commit over e-mail.

If we add a new field "const char *beginning_of_tail_garbage" next to "end_of_message_proper" that points at the blank line before the three-dash line in the above example, the parser should be able to break such an input into a parsed form, allow the trailer[] array to be manipulated and reproduce a commit log message.

On 10/04/2016 11:28 AM, Junio C Hamano wrote:

How important is this feature? It doesn't seem too difficult to add, although it does break compatibility (in particular, "--signoff" must now be documented as "after the last trailer" instead of "at the end of the commit message").

Jonathan Tan <jonathantanmy@google.com> writes:

The sign-off has always been meant to be appended at the end of the trailer block, so there is no "compatibility" issue. It's just how you phrase the explanation of the behaviour, I would think.

On 10/04/2016 10:25 AM, Junio C Hamano wrote:

Sounds reasonable to me. Would the "[" be a bit of overspecification, though, since Git doesn't produce it? Also, identifying it as a garbage line probably wouldn't change any behavior - in the Linux kernel examples, it is used to show what happened in between sign-offs, so there will always be one "Signed-off-by:" at the top. (But I do not feel strongly about this.)

I think the number should be 1 - that seems like the easiest to explain. But I'm OK with other suggestions.

I get the impression from the rest of your e-mail that no strings are meant to be copied - is that true? (That sounds like a good idea to me.) In which case this might be better:

   struct {
     const char *first_trailer; /* = end_of_message_proper */
     struct {
       const char *start;
       const char *value;
       const char *end;
     } *trailers;
     int trailers_nr, trailers_alloc;
   };

start = value for "[", "(cherry picked from" and garbage lines. We also need end because there is no \0 there (we didn't copy any strings).

The existing code (in trailer.c) uses a linked list to store trailers, but an array (as written in your e-mail) is probably better for us since clients would want to access the last element (as also written in your e-mail).

I don't think we need trailer block struct -> commit message conversion 
- when adding a new trailer or replacing an existing trailer, the client 
code can just remember the index and then modify its behavior 
accordingly when iterating through all trailers. But this conversion can 
be easily added if/when we need it.

 > Hmm?

Overall, this seems like a good idea - I'll go ahead and do this if there are no other objections.

It just occurred to me that there could be some corner cases when the trailer separator is configured to not include ":" - I'll make sure to include tests that check those corner cases.

Jonathan Tan <jonathantanmy@google.com> writes:

Good thinking. As "interpret trailers" cannot locate such a line to manipulate (as it lacks <token>), we can simply treat it as a garbage line.

I was envisioning that "whole", "end-of-message" can point into the input buffer, while trailer[].contents may have to be copied, if only to make it to a NUL-terminated string. But I am fine with <ptr,len> or <begin,end> pair to avoid copying .contents if that is desired. You'd need to worry about differentiating .contents that need to be freed after "interpret trailers" inserted a new entry or replaced the contents, though.