From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.1 (2015-04-28) on dcvr.yhbt.net X-Spam-Level: X-Spam-ASN: AS31976 209.132.180.0/23 X-Spam-Status: No, score=-3.5 required=3.0 tests=AWL,BAYES_00, FREEMAIL_FORGED_FROMDOMAIN,FREEMAIL_FROM,HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,RCVD_IN_DNSWL_HI shortcircuit=no autolearn=ham autolearn_force=no version=3.4.1 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by dcvr.yhbt.net (Postfix) with ESMTP id 04AB31F453 for ; Mon, 5 Nov 2018 21:57:22 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S2388112AbeKFHTH (ORCPT ); Tue, 6 Nov 2018 02:19:07 -0500 Received: from mout.gmx.net ([212.227.17.22]:60165 "EHLO mout.gmx.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S2387580AbeKFHTH (ORCPT ); Tue, 6 Nov 2018 02:19:07 -0500 Received: from [192.168.0.129] ([37.201.193.149]) by mail.gmx.com (mrgmx103 [212.227.17.168]) with ESMTPSA (Nemesis) id 0Lkwpt-1flp6631u9-00akzr; Mon, 05 Nov 2018 22:57:15 +0100 Received: from [192.168.0.129] ([37.201.193.149]) by mail.gmx.com (mrgmx103 [212.227.17.168]) with ESMTPSA (Nemesis) id 0Lkwpt-1flp6631u9-00akzr; Mon, 05 Nov 2018 22:57:15 +0100 Date: Mon, 5 Nov 2018 22:57:14 +0100 (STD) From: Johannes Schindelin X-X-Sender: virtualbox@gitforwindows.org To: Duy Nguyen cc: Phillip Wood , Git Mailing List Subject: Re: [PATCH/RFC] sequencer.c: record revert/cherry-pick commit with trailer lines In-Reply-To: Message-ID: References: <20181104072253.12357-1-pclouds@gmail.com> <27bcf7a6-8590-fa21-8381-697e1b030182@talktalk.net> User-Agent: Alpine 2.21.1 (DEB 209 2017-03-23) MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="8323328-1042678719-1541455035=:86" X-Provags-ID: V03:K1:ejRqPvIJvsSW96Fonp0VmpKGKwqHpKxe4/0LNChcW4gUjqcdmtB leIRh9pYvoN+1L/jU1VAMLU9RpNZOoIpRCG72Ixv/5A8yh73fb5OBNQJUb0JS8iEbELrlxG jYhnZPYHwnZgy8HVp7jAR0HTV1mzfbT+HlonKlWz7TO48Bo3/zE8N4FfdVypjvbytkh+lWA pIuiawfehXjRUKavaxJJg== X-UI-Out-Filterresults: notjunk:1;V01:K0:n0HvmeZgrO4=:YKU62ROhItQkKfbdZNzfR0 dUrF4r7oInzzY2nuVA/fH28ZGrjqLVcR3EwrU8OzdqevQ5SipEoC/e8Tib1bc/jSkNbRDcYrM 8tn6TPGM6TupYrj1YOEZLuXP8X544xwxXc+y+7/GfmGyvKAf1BtwH96ktCy+7KIuDGhMckJvy E/LhciByIOvKh0pu7pDrFnhrpUL2ugyFmlaKjVVEKuhUToT2kHgIS5BcMs5hxJ45K06KxvCRa O/D4HNzHvbuCUHb1j6kWzBy93VIzHTRrxzetd0XmrX7Klb9iB2dioqZSYRJOS98Y2RzBipLfP RBrMsHjFgFRC/ZGHlPGHc/cOp7Grb6U3fcGpOnS1+qUjNmZUeq7DTfBvFund2dnIzMkm1iCsA M3EVHD3fQDm31YBdgNbkG4p0/dNPgwfvRpWHKqETJ1CeKfaDzuEeYy2D8hqf5b28u3OiVOTfO MJwJTVIUZbWSsxVR7rYT1LxfA7msiHQFv90vhUfLFlY089SfhxSEAmzHAQmu/iT12mJBnDDw4 7a53GUhwGpLNyxEnoGaXY2F1YN1FjlAKB3FRL2glmF6fISQ3iW/vk0YFesqFB/1ZbCyY8OM8a XJhVFLnGFLctTOtbVqa23dARGuqSEkwpNKDzcUVKLaWVmWkeKa0Npr4MbU3Wg/nhvHSekW9zF /ryDhz6ijKkw5g5VtwrVtNq8bLReDbzxj66t4VwKXHeHfW9Jj3BFxuVIjw6yVtPHlt8asfTgr FDyYbMQFPKqihDDYjQd5MOdwfrBzcGzY1EjtCOM54sG8pf0+R4pfZTp3dr34E6VBHMsnLgnic kD+YnCCbBX7qwZwKUHasJ+wT3Vt0jkzXBN7Trncjirw8tLVgMY= Sender: git-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: git@vger.kernel.org This message is in MIME format. The first part should be readable text, while the remaining parts are likely unreadable without MIME-aware tools. --8323328-1042678719-1541455035=:86 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8BIT Hi, On Sun, 4 Nov 2018, Duy Nguyen wrote: > On Sun, Nov 4, 2018 at 5:45 PM Phillip Wood 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: > > > > > > Similarly a cherry-picked commit with -x will have > > > > > > Cherry-Pick: > > > > 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 > --8323328-1042678719-1541455035=:86--