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 208D51F405 for ; Thu, 9 Aug 2018 09:22:55 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1730202AbeHILqv (ORCPT ); Thu, 9 Aug 2018 07:46:51 -0400 Received: from mout.gmx.net ([212.227.17.21]:48279 "EHLO mout.gmx.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1727579AbeHILqv (ORCPT ); Thu, 9 Aug 2018 07:46:51 -0400 Received: from [192.168.0.129] ([37.201.193.145]) by mail.gmx.com (mrgmx101 [212.227.17.168]) with ESMTPSA (Nemesis) id 0M2XkX-1g4fXa2Iy5-00sLFx; Thu, 09 Aug 2018 11:22:43 +0200 Date: Thu, 9 Aug 2018 11:22:44 +0200 (DST) From: Johannes Schindelin X-X-Sender: virtualbox@gitforwindows.org To: phillip.wood@dunelm.org.uk cc: Johannes Schindelin via GitGitGadget , git@vger.kernel.org, Junio C Hamano Subject: Re: [PATCH v2 2/2] rebase --exec: make it work with --rebase-merges In-Reply-To: Message-ID: References: <7ca441a89674ee77cbbb3ec17f931aecba7bfa0d.1533549169.git.gitgitgadget@gmail.com> User-Agent: Alpine 2.21.1 (DEB 209 2017-03-23) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII X-Provags-ID: V03:K1:gIrKHVXkPGtqe/dczYNDx9lXdLKkvXnXvk6FsD+luZQCudp53v0 bOIUByhN5fTYmH/sTwi04yHwQNt7QbwmIZswvGFYmvCzjiIcSYqAVMSpqHZ3+0JGPGD0hxk EN45fF+uzIXxZV9AlzGNnIH/dOyjMx438DQj8MP0qEIc2yYGMSF6VyaXO2XuYqqXn7TgnFm hOHrui8V9b2hNszXDRTZA== X-UI-Out-Filterresults: notjunk:1;V01:K0:6/n+YCDHc0g=:p53q0QAxUhEWx9cODdUQUF Zyl6mQSX5FT+85k4n3kkTu5VQsKUXBwYmTOXgBR4maM2l69lTaBLXRLIx558qK3zZ1G5qa+uc 3kEvItjeRVIhtclWfxDbFuZDZzoH2fg6GkJTth3aq49/njjtwjaU0jAVeU8WnHOqNNRzzn6dM 1LQt+AJKj90cT55DkAESu/0IeFPHwS+g122MEiCEzqo6ZFhB3PmctbDsfwbsmOW44vxjCcp7F OW4TJf2iyPXS1deiVtxx2HThyEsom1EKLW3xny1nFESqWPGWb/qt3Wbp3cv8uQhMBne4E3ybu pNHdDQ+Ad4hfNzOd5/D+a9HTMe9cj7NljeKBAOIRuZxV86FjgANcYHsoxHnI9qOvxNq/26NRf OqglGF0kfEEnFtHoWA11kPQGacZfjKMXlWreN6p3UvCMB8yGD5tfwd8jYAFsRalUkilZczpNy uGhHEUIgT+0+hjiusNjFHjWdVxgAAIpWwcPlR6avmKQgwm26AodPqGwMvxcJdQP3gMWrL5A+C 7HSZiJfYSyJsEbDr7DFX1JDKhE0cblwcRJ+IrCKaQn0YYm9F86NLxKSGMxEIZIPwiOSlIx//N fTYDhxxsTjQs3DC9QDLzv5/BxXa42saWdbVUAJh2l/d6g6NR15/fmj3hw1HZIV+jeMgWJ+BQI 0guOH2z/C0KJxcPW/KVeu8nkJgCG2kfBcJ+9oyCCQ2FoePnlezn7wni7L/DQmpclMT0YgjNKU /QC+qF82DCOmfORFsIs1HaREDJYGj3mC/7RUxLB7fdYXcRP9JHwI0YmRjkt+SwuCVUkyssXcX h2s8oHg Sender: git-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: git@vger.kernel.org Hi Phillip, On Mon, 6 Aug 2018, Phillip Wood wrote: > 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. Yes, this is very much on purpose. Take this todo list, for example: pick 123456 this patch # pick 987654 this was an empty commit You definitely do not want the `exec` to appear after that commented-out empty commit. > > + 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); That does not really work, as `insert` can point *after* the last line, in which case `todo_list.items[insert]` is undefined (and in the worst case, causes a segmentation fault). > I'm not sure it matters that much though Well, it does matter to me. After having this in the back of my head, and after your comment, I think it *is* worth the additional complexity after all. Will come up with a new iteration. Ciao, Dscho