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.9 required=3.0 tests=AWL,BAYES_00,DKIMWL_WL_MED, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,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 47B581F453 for ; Sun, 4 Nov 2018 21:12:58 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1729147AbeKEG3R (ORCPT ); Mon, 5 Nov 2018 01:29:17 -0500 Received: from smtp-out-5.talktalk.net ([62.24.135.69]:52444 "EHLO smtp-out-5.talktalk.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1728634AbeKEG3R (ORCPT ); Mon, 5 Nov 2018 01:29:17 -0500 Received: from [192.168.2.240] ([92.22.32.73]) by smtp.talktalk.net with SMTP id JPhVgDQn0dJAeJPhWgU5jk; Sun, 04 Nov 2018 21:12:54 +0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=talktalk.net; s=cmr1711; t=1541365974; bh=nVY7tsasjYKlgl5HItvzg3q+hevC/XA2ZIvUdnnAXpU=; h=Reply-To:Subject:To:Cc:References:From:Date:In-Reply-To; b=NfoPgy7FH9KnldfsbmN50hxz4TaOw7DZoIxHiVyPifDhZ4kTbXkXJNYMFojYsS5qL BQ50URZQ73ML6M6YtZtHMpzrUUdeXoorZ+gX7w9WXn+HYdw5DDg3ALxOLxLgldzQ50 n1q2jnE5gjA7aB1p9g/0YYkOwSEFSKY5UWQD5WWI= X-Originating-IP: [92.22.32.73] X-Spam: 0 X-OAuthority: v=2.3 cv=V8BTL9vi c=1 sm=1 tr=0 a=w3K0eKD2tyZHkEydg3BQCA==:117 a=w3K0eKD2tyZHkEydg3BQCA==:17 a=IkcTkHD0fZMA:10 a=nN7BH9HXAAAA:8 a=5rxgeBVgAAAA:8 a=3j4BkbkPAAAA:8 a=g-MFkQ3IFOGqQ53_wAEA:9 a=QEXdDO2ut3YA:10 a=PwKx63F5tFurRwaNxrlG:22 Reply-To: phillip.wood@dunelm.org.uk Subject: Re: [PATCH/RFC] sequencer.c: record revert/cherry-pick commit with trailer lines To: Duy Nguyen , Phillip Wood Cc: Git Mailing List References: <20181104072253.12357-1-pclouds@gmail.com> <27bcf7a6-8590-fa21-8381-697e1b030182@talktalk.net> From: Phillip Wood Message-ID: <1a667e9c-3944-4bc3-fb60-68e79dff0c34@talktalk.net> Date: Sun, 4 Nov 2018 21:12:53 +0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.2.1 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-GB-large Content-Transfer-Encoding: 8bit X-CMAE-Envelope: MS4wfN9dvU6KbikgqsDVQXhikhkjwx3wsxt5/5/FT9NdaJ3MbOz3nr/Q8gKdlfKyFL5neLKS9CrtZmCyD4/g//RskDcdsdalnFseQPXdoUzDNayk3EmG2jUt RiiqE3w0yjc/T4q4jU0t70NiflXc7fJoKPGy36qm80nGCGPBSyflZBcJ1KQQpxL1PNvcEbuuCxgM74h0ceny2D07B6qIgZmTGAa6Wm6WdveapWiF93Ki6rwV PDpgE+H+F7rALvijsj4NOw== Sender: git-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: git@vger.kernel.org Hi Duy On 04/11/2018 17:41, 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. > > [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: ? I think Revert-parent: is good, though you seem to have gone for including it the Revert: trailer in v2. >>> - 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. Ah you're right, I had forgotten that the revert message body is empty, unlike cherry-pick where the message is copied (and that does the right thing already when there are existing trailers). Best wishes Phillip >>> @@ -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. >