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

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

[-- Attachment #1: Type: text/plain, Size: 3923 bytes --]

Hi,

On Sun, 4 Nov 2018, Duy Nguyen wrote:

> On Sun, Nov 4, 2018 at 5:45 PM Phillip Wood <phillip.wood@talktalk.net> wrote:
> >
> > On 04/11/2018 07:22, Nguyễn Thái Ngọc Duy wrote:
> > > 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.

Since this is a wilfull backwards-incompatible patch, it needs to be done
carefully.

I am not enthused by the idea of breaking power users' setups, and I could
imagine that a much better way to do it would be to introduce a config
setting that guards the new behavior, then add a deprecation notice when
-x is used without that config setting, and then let that simmer for a
couple of versions.

Ciao,
Johannes

> 
> [1] https://public-inbox.org/git/20181017143921.GR270328@devbig004.ftw2.facebook.com/
> 
> > > @@ -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: ?
> 
> > > -             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.
> 
> > > @@ -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
>