Re: [RFC/PATCH 0/2] place cherry pick line below commit title

Jonathan Tan [off-list ref] writes:

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

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.

  Signed-off-by: A [off-list ref]
  [This has nothing to do with the above line]
  Signed-off-by: B [off-list ref]

and:

  Link 1: a link
    a continuation of the above

and:

  Signed-off-by: Some body [off-list ref] (comment
  on two lines)

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 [off-list ref]\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?

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

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.

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

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.

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

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 [off-list ref]\n",
    }

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

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.

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.