Show 108 quoted lines
>
> 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);
> elseShow 26 quoted lines
> 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 &&
>>
>