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.8 required=3.0 tests=AWL,BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI, RCVD_IN_DNSWL_HI,T_DKIMWL_WL_MED 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 9F28B208EB for ; Mon, 6 Aug 2018 15:23:46 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1732715AbeHFRdU (ORCPT ); Mon, 6 Aug 2018 13:33:20 -0400 Received: from smtp-out-3.talktalk.net ([62.24.135.67]:43271 "EHLO smtp-out-3.talktalk.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1732430AbeHFRdU (ORCPT ); Mon, 6 Aug 2018 13:33:20 -0400 Received: from [192.168.2.240] ([92.22.26.195]) by smtp.talktalk.net with SMTP id mhMDfx1cGbZX5mhMDf3bxm; Mon, 06 Aug 2018 16:23:42 +0100 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=talktalk.net; s=cmr1711; t=1533569022; bh=FEXkrtw2zUejxljLRWa8hpOH+o5YwrQNKpQ+ZCLvhPA=; h=Reply-To:Subject:To:Cc:References:From:Date:In-Reply-To; b=JazL1+1lHvk++ot4TOY4KrnZTmn/EU0T2l8CCzvQ0uYy/iWN8S29moAK6GFwClMzH dY+jLUCxasTEs93Xa1XJJAkpMoFWwZl7yM5Zl8bNR8qnecaJ2WHsKe6cchxGutQqx7 ZgKr8xOxpfUHf8bBVYoLYbsExao0iccMsrqbYn80= X-Originating-IP: [92.22.26.195] X-Spam: 0 X-OAuthority: v=2.3 cv=Poq9kTE3 c=1 sm=1 tr=0 a=8bf3kEuDtVJeVZALKX4IsA==:117 a=8bf3kEuDtVJeVZALKX4IsA==:17 a=IkcTkHD0fZMA:10 a=RslZw344RIqlFeLviDgA:9 a=QEXdDO2ut3YA:10 Reply-To: phillip.wood@dunelm.org.uk Subject: Re: [PATCH v2 2/2] rebase --exec: make it work with --rebase-merges To: Johannes Schindelin via GitGitGadget , git@vger.kernel.org Cc: Junio C Hamano , Johannes Schindelin References: <7ca441a89674ee77cbbb3ec17f931aecba7bfa0d.1533549169.git.gitgitgadget@gmail.com> From: Phillip Wood Message-ID: Date: Mon, 6 Aug 2018 16:23:41 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <7ca441a89674ee77cbbb3ec17f931aecba7bfa0d.1533549169.git.gitgitgadget@gmail.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-GB-large Content-Transfer-Encoding: 7bit X-CMAE-Envelope: MS4wfPHHAHWPSkhEaEjR+ELymLRlxuuRh6SjZ5oVq2WSw8NE2ogCVHX4AvT/XAX/vfnu8MEuTpUGw4wBv+rBnpskhZ9cTgtFjsrUo07Mv1rwlnwUlJRBKAsJ pvvRqwgCqAb3rbbeAKWM4SnR1hx2As0QqbN+vqsqRYSW/9r1IXiZ/nPXLgXex7/dZyntUgEt6TY+OZ6eJFbQpJvlmh2aadgN33QzQOJ2En2K7ZiPOpuNM1q5 pRE3Ub5hmYAhoomCedpxnDSvSGUSS5/BLXgUBb9kSbk= Sender: git-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: git@vger.kernel.org Hi Johannes On 06/08/18 10:52, Johannes Schindelin via GitGitGadget wrote: > > From: Johannes Schindelin > > The idea of `--exec` is to append an `exec` call after each `pick`. > > Since the introduction of fixup!/squash! commits, this idea was extended > to apply to "pick, possibly followed by a fixup/squash chain", i.e. an > exec would not be inserted between a `pick` and any of its corresponding > `fixup` or `squash` lines. > > The current implementation uses a dirty trick to achieve that: it > assumes that there are only pick/fixup/squash commands, and then > *inserts* the `exec` lines before any `pick` but the first, and appends > a final one. > > With the todo lists generated by `git rebase --rebase-merges`, this > simple implementation shows its problems: it produces the exact wrong > thing when there are `label`, `reset` and `merge` commands. > > Let's change the implementation to do exactly what we want: look for > `pick` lines, skip any fixup/squash chains, and then insert the `exec` > line. Lather, rinse, repeat. > > While at it, also add `exec` lines after `merge` commands, because they > are similar in spirit to `pick` commands: they add new commits. > > Signed-off-by: Johannes Schindelin > --- > sequencer.c | 37 +++++++++++++++++++++++++++---------- > t/t3430-rebase-merges.sh | 2 +- > 2 files changed, 28 insertions(+), 11 deletions(-) > > diff --git a/sequencer.c b/sequencer.c > index 31038472f..ed2e694ff 100644 > --- a/sequencer.c > +++ b/sequencer.c > @@ -4244,10 +4244,9 @@ int sequencer_add_exec_commands(const char *commands) > { > const char *todo_file = rebase_path_todo(); > struct todo_list todo_list = TODO_LIST_INIT; > - struct todo_item *item; > struct strbuf *buf = &todo_list.buf; > size_t offset = 0, commands_len = strlen(commands); > - int i, first; > + int i, insert; > > if (strbuf_read_file(&todo_list.buf, todo_file, 0) < 0) > return error(_("could not read '%s'."), todo_file); > @@ -4257,19 +4256,37 @@ int sequencer_add_exec_commands(const char *commands) > return error(_("unusable todo list: '%s'"), todo_file); > } > > - first = 1; > - /* insert before every pick except the first one */ > - for (item = todo_list.items, i = 0; i < todo_list.nr; i++, item++) { > - if (item->command == TODO_PICK && !first) { > - strbuf_insert(buf, item->offset_in_buf + offset, > - commands, commands_len); > + /* > + * Insert after every pick. Here, fixup/squash chains > + * are considered part of the pick, so we insert the commands *after* > + * those chains if there are any. > + */ > + insert = -1; > + for (i = 0; i < todo_list.nr; i++) { > + enum todo_command command = todo_list.items[i].command; > + > + if (insert >= 0) { > + /* skip fixup/squash chains */ > + if (command == TODO_COMMENT) > + continue; insert is not updated so if the next command is not a fixup the exec line will be inserted before the comment. > + else if (is_fixup(command)) { > + insert = i + 1; > + continue; > + } > + strbuf_insert(buf, > + todo_list.items[insert].offset_in_buf + > + offset, commands, commands_len); > offset += commands_len; > + insert = -1; > } > - first = 0; > + > + if (command == TODO_PICK || command == TODO_MERGE) > + insert = i + 1; > } > > /* append final */ > - strbuf_add(buf, commands, commands_len); > + if (insert >= 0 || !offset) > + strbuf_add(buf, commands, commands_len); Having read your other message about this patch I think if you wanted to fix the position of the final exec in the case where the todo list ends with a comment you could do something like if (insert >= 0) strbuf_insert(buf, todo_list.items[insert].offset_in_buf + offset, commands, commands_len); else strbuf_add(buf, commands, commands_len); I'm not sure it matters that much though The rest of this patch looks fine to me Best Wishes Phillip > > i = write_message(buf->buf, buf->len, todo_file, 0); > todo_list_release(&todo_list); > diff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh > index 0bf5eaa37..90ae613e2 100755 > --- a/t/t3430-rebase-merges.sh > +++ b/t/t3430-rebase-merges.sh > @@ -363,7 +363,7 @@ test_expect_success 'octopus merges' ' > EOF > ' > > -test_expect_failure 'with --autosquash and --exec' ' > +test_expect_success 'with --autosquash and --exec' ' > git checkout -b with-exec H && > echo Booh >B.t && > test_tick && >