From: Phillip Wood <phillip.wood@talktalk.net>
Date: Mon, 06 Aug 2018 16:00:37 GMT
Subject: Re: [PATCH v2 2/2] rebase --exec: make it work with --rebase-merges
Message-ID: <f5378e50-bdd6-cca5-f535-1f07da7c19ba@talktalk.net>
In-Reply-To: <f7da64c2-0477-07dd-35ef-7cfd16447bd5@talktalk.net>

On 06/08/18 16:23, Phillip Wood wrote:
> 
> Hi Johannes
> On 06/08/18 10:52, Johannes Schindelin via GitGitGadget wrote:
>>
>> 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.
> 
>> +            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

Oops that should be 'else if (!offset)'

>          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 &&
>>
>