Re: [PATCH v2 2/2] rebase --exec: make it work with --rebase-merges
- From
- Phillip Wood <phillip.wood@talktalk.net>
- Date
- Aug 6, 2018, 15:23 UTC
- Message-ID
- <f7da64c2-0477-07dd-35ef-7cfd16447bd5@talktalk.net>
- In-Reply-To
- <7ca441a89674ee77cbbb3ec17f931aecba7bfa0d.1533549169.git.gitgitgadget@gmail.com>
Hi Johannes On 06/08/18 10:52, Johannes Schindelin via GitGitGadget wrote:
Show 71 quoted lines
>
> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> 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 <johannes.schindelin@gmx.de>
> ---
> 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 <commands> 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 <commands> 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.
Show 20 quoted lines
> + 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 <commands> */
> - 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
Show 17 quoted lines
> > 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 && >