Re: More builtin git-am issues..

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

Note that I am not saying that you personally did any bad job while working on the interpret-trailers topic. We collectively designed its feature based on a much narrower definition of what the trailer block is than what is used in the real world in practice, so we do not have a good way to locate an existing entry that is not a (key, value), no matter what the syntax to denote which is key and which is value is, insert a new entry that is not a (key, value), or remove an existing entry that is not a (key, value), all of which will be necessary to mutate trailer blocks people use in the real life.

I think a good way forward would be to go like this:

 * a helper function that starts from a flat <buf, len> (or a
   strbuf) and identifies the end of the body of the message,
   i.e. find "^---$", "^Conflicts:$", etc. and skip blank lines
   backwards.  That is what ignore_non_trailer() in commit.c does,
   and that can be shared across everybody that mutates a log
   message.

 * a helper function that takes the result of the above as a flat
   <buf, len> (or a strbuf) and identifies the beginning of a
   trailer block.  That may be just the matter of scanning backwards
   from the end of the trailer block ignore_non_trailer() identified
   for the first blank line, as I agree with Linus's "So quite
   frankly, at that point if git doesn't recognize it as a sign-off
   block, I don't think it's a big deal" in the thread.

   Not having that and not calling that function can reintroduce the
   recent "interpret-trailers corner case" bug Matthieu brought up.

With these, everybody except interpret-trailers that mutates a log message can add a new signoff consistently. And then, building on these, "interpret-trailers" can be written like this:

 (1) starting from a flat <buf, len> (or a strbuf), using the above
     helpers, identify the parts of the log message that is the
     trailer block (and you will know what is before and what is
     after the trailer block).

 (2) keep the part before the trailer block and the part after the
     trailer block (this could be empty) in one strbuf each; we do
     not want to mutate these parts, and it is pointless to split
     them further into individual lines.

 (3) express the lines in the trailer block in a richer data
     structure that is easier to manipulate (i.e. reorder the lines,
     insert, delete, etc.) and work on it.

 (4) when manipulation of the trailer block is finished, reconstruct
     the resulting message by concatenating the "before trailer"
     part, "trailer" part, and "after trailer" part.

As to the exact design of "a richer data structure" and the manipulation we may want on the trailer, I currently do not have a strong "it should be this way" opinion yet, but after looking at various examples Linus gave us in the discussion, my gut feelig is that it would be best to keep the operation simple and generic, e.g. "find a line that matches this regexp and replace it with this line", "insert this line at the end", "delete all lines that match this regexp", etc.