Hi, On Sun, 4 Nov 2018, Duy Nguyen wrote: > On Sun, Nov 4, 2018 at 5:45 PM Phillip Wood <phillip.wood@xxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxxxxxxxxxx/ > > > > @@ -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 >
