Re: [PATCH/RFC] sequencer.c: record revert/cherry-pick commit with trailer lines

On Sun, Nov 4, 2018 at 5:45 PM Phillip Wood [off-list ref] wrote:

On 04/11/2018 07:22, Nguyễn Thái Ngọc Duy wrote:

quoted

When a commit is reverted (or cherry-picked with -x) we add an English
sentence recording that commit id in the new commit message. Make
these real trailer lines instead so that they are more friendly to
parsers (especially "git interpret-trailers").

A reverted commit will have a new trailer

     Revert: <commit-id>

Similarly a cherry-picked commit with -x will have

     Cherry-Pick: <commit-id>

I think this is a good idea though I wonder if it will break someones
script that is looking for the messages generated by -x at the moment.

It will [1] but I still think it's worth the trouble. The script will
be less likely to break after, and you can use git-interpret-trailers
instead of plain grep.

[1] https://public-inbox.org/git/20181017143921.GR270328@devbig004.ftw2.facebook.com/

quoted

@@ -1758,16 +1757,10 @@ static int do_pick_commit(enum todo_command command, struct commit *commit,
              base_label = msg.label;
              next = parent;
              next_label = msg.parent_label;
-             strbuf_addstr(&msgbuf, "Revert \"");
-             strbuf_addstr(&msgbuf, msg.subject);
-             strbuf_addstr(&msgbuf, "\"\n\nThis reverts commit ");
-             strbuf_addstr(&msgbuf, oid_to_hex(&commit->object.oid));
-
-             if (commit->parents && commit->parents->next) {
-                     strbuf_addstr(&msgbuf, ", reversing\nchanges made to ");
-                     strbuf_addstr(&msgbuf, oid_to_hex(&parent->object.oid));
-             }

As revert currently records the parent given on the command line when
reverting a merge commit it would probably be a good idea to add that
either as a separate trailer or to the Revert: trailer and possibly also
generate it for cherry picks.

My mistake. I didn't read carefully and thought it was logging
commit->parents, which is pointless.

So what should be the trailer for this (I don't think putting it in
Revert: is a good idea, too much to parse)? Revert-parent: ?
Revert-merge: ?

quoted

-             strbuf_addstr(&msgbuf, ".\n");
+             strbuf_addf(&msgbuf, "Revert \"%s\"\n\n", msg.subject);

If the message already contains trailers then should we just append the
Revert trailer those rather than inserting "\n\n"?

Umm.. but this \n\n is for separating the subject and the body. I
think we need it anyway, trailer or not.

quoted

@@ -1784,9 +1777,8 @@ static int do_pick_commit(enum todo_command command, struct commit *commit,
                      strbuf_complete_line(&msgbuf);
                      if (!has_conforming_footer(&msgbuf, NULL, 0))
                              strbuf_addch(&msgbuf, '\n');
-                     strbuf_addstr(&msgbuf, cherry_picked_prefix);
-                     strbuf_addstr(&msgbuf, oid_to_hex(&commit->object.oid));
-                     strbuf_addstr(&msgbuf, ")\n");
+                     strbuf_addf(&msgbuf, "Cherry-Pick: %s\n",
+                                 oid_to_hex(&commit->object.oid));

I will probably make this "Cherry-picked-from:" to match our S-o-b style.
-- 
Duy