Re: [PATCH] sequencer: finish parsing the todo list despite an invalid first line
Hi Alex
Thanks for working on this.
On 19/07/2023 15:43, Alex Henrie wrote:
Show 8 quoted lines
> ddb81e5072 (rebase-interactive: use todo_list_write_to_file() in
> edit_todo_list(), 2019-03-05) made edit_todo_list more efficient by
> replacing transform_todo_file with todo_list_parse_insn_buffer.
> Unfortunately, that innocuous change caused a regression because
> todo_list_parse_insn_buffer would stop parsing after encountering an
> invalid 'fixup' line. If the user accidentally made the first line a
> 'fixup' and tried to recover from their mistake with `git rebase
> --edit-todo`, all of the commands after the first would be lost.
I found this description a little confusing as transform_todo_file() also called todo_list_parse_insn_buffer(). transform_todo_file() does not exist anymore but it looked like
static int transform_todo_file(unsigned flags)
{
const char *todo_file = rebase_path_todo();
struct todo_list todo_list = TODO_LIST_INIT;
int res; if (strbuf_read_file(&todo_list.buf, todo_file, 0) < 0)
return error_errno(_("could not read '%s'."), todo_file); if (todo_list_parse_insn_buffer(the_repository, todo_list.buf.buf,
&todo_list)) {
todo_list_release(&todo_list);
return error(_("unusable todo list: '%s'"), todo_file);
} res = todo_list_write_to_file(the_repository, &todo_list,
todo_file,
NULL, NULL, -1, flags);
todo_list_release(&todo_list); if (res)
return error_errno(_("could not write '%s'."), todo_file);
return 0;
}If it could not parse the todo list it did not try and write it to disc. After ddb81e5072 this changed as edit_todo_list() tries to shorten the OIDs in the todo list before it is edited even if it cannot be parsed. The fix below works around that by making sure we always try and parse the whole todo list even if the the first line is a fixup command. That is a worthwhile improvement because it means we notify the user of all the errors we find rather than just the first one and is in keeping with the way we handle other invalid lines. It does not however fix the root cause of this regression which is the change in behavior in edit_todo_list().
After the user edits the todo file we do not try to transform the OIDs if it cannot be parsed or has missing commits. Therefore it still contains the shortened OIDs and editing hints and there is no need for edit_todo_list() to call write_todo_list() when "incorrect" is true.
Show 38 quoted lines
> To avoid throwing away important parts of the todo list, change
> todo_list_parse_insn_buffer to keep going and not return early on error.
>
> Signed-off-by: Alex Henrie <alexhenrie24@gmail.com>
> ---
> sequencer.c | 2 +-
> t/t3404-rebase-interactive.sh | 19 +++++++++++++++++++
> 2 files changed, 20 insertions(+), 1 deletion(-)
>
> diff --git a/sequencer.c b/sequencer.c
> index cc9821ece2..adc9cfb4df 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -2702,7 +2702,7 @@ int todo_list_parse_insn_buffer(struct repository *r, char *buf,
> if (fixup_okay)
> ; /* do nothing */
> else if (is_fixup(item->command))
> - return error(_("cannot '%s' without a previous commit"),
> + res = error(_("cannot '%s' without a previous commit"),
> command_to_string(item->command));
> else if (!is_noop(item->command))
> fixup_okay = 1;
> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
> index ff0afad63e..d2801ffee4 100755
> --- a/t/t3404-rebase-interactive.sh
> +++ b/t/t3404-rebase-interactive.sh
> @@ -1596,6 +1596,25 @@ test_expect_success 'static check of bad command' '
> test C = $(git cat-file commit HEAD^ | sed -ne \$p)
> '
>
> +test_expect_success 'the first command cannot be a fixup' '
> + # When using `git rebase --edit-todo` to recover from this error, ensure
> + # that none of the original todo list is lost
> + rebase_setup_and_clean fixup-first &&
> + (
> + set_fake_editor &&
> + test_must_fail env FAKE_LINES="fixup 1 2 3 4 5" \
> + git rebase -i --root 2>actual &&Thanks for taking the time to add a test. It is not worth a re-roll on its own, but there is no need to use "--root" here. It is confusing as it is not clear if we're refusing "fixup" as the first command because we're rewriting the root commit or if we always refuse to have "fixup" as the first command.
As an aside this restriction is pretty easy to defeat. In fact I think we probably allow a todo list that starts with
[new root] fixup <commit>
which is a bug. We certainly allow todo lists starting with
exec true / label <label> / reset <commit> fixup <commit>
but that is not the concern of this patch.
Show 8 quoted lines
> + test_i18ngrep "cannot .fixup. without a previous commit" \
> + actual &&
> + test_i18ngrep "You can fix this with .git rebase --edit-todo.." \
> + actual &&
> + grep -v "^#" .git/rebase-merge/git-rebase-todo >orig &&
> + test_must_fail git rebase --edit-todo &&
> + grep -v "^#" .git/rebase-merge/git-rebase-todo >actual &&
> + test_cmp orig actual
We check that the uncommitted lines after running "git rebase --edit-todo" match the uncommitted lines after the initial edit. That's fine to detect if the second edit truncates the file but it will still pass if the initial edit starts truncating the todo list as well. As we expect that git should not change an incorrect todo list we do not need to filter out the lines beginning with "#".
To ensure we detect a regression where the first edit truncates the todo list we could do something like
test_when_finished "git rebase --abort" &&
cat >todo <<-EOF &&
fixup $(git log -1 --format="%h %s" B)
pick $(git log -1 --format="%h %s" C)
EOF
(
set_replace_editor todo &&
test_must_fail git rebase -i A 2>actual
) &&
test_i18ngrep "cannot .fixup. without a previous commit" actual &&
test_i18ngrep "You can fix this with .git rebase --edit-todo.." actual &&
# check initial edit has not truncated todo list
grep -v "^#" .git/rebase-merge/git-rebase-todo >actual &&
test_cmp todo actual &&
cat .git/rebase-merge/git-rebase-todo >expect &&
test_must_fail git rebase --edit-todo &&
# check the list is unchanged by --edit-todo
test_cmp expect .git/rebase-merge/git-rebase-todo
We could perhaps check the error message from "git rebase --edit-todo" as well.
Thanks for finding this and working on a fix
Phillip
Show 6 quoted lines
> + )
> +'
> +
> test_expect_success 'tabs and spaces are accepted in the todolist' '
> rebase_setup_and_clean indented-comment &&
> write_script add-indent.sh <<-\EOF &&
Alex Henrie