From: Phillip Wood Date: Mon, 06 Aug 2018 15:23:41 GMT Subject: Re: [PATCH v2 2/2] rebase --exec: make it work with --rebase-merges Message-ID: In-Reply-To: <7ca441a89674ee77cbbb3ec17f931aecba7bfa0d.1533549169.git.gitgitgadget@gmail.com> 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 && >