Re: [PATCH] sequencer: finish parsing the todo list despite an invalid first line
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Jul 19, 2023, 21:32 UTC
- Message-ID
- <xmqq351ja8p8.fsf@gitster.g>
- In-Reply-To
- <20230719144339.447852-1-alexhenrie24@gmail.com>
Alex Henrie <alexhenrie24@gmail.com> writes:
Show 31 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.
>
> 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;Well spotted.
Show 9 quoted lines
> 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' '
A very good test to the point.
Show 11 quoted lines
> + # 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 && > + test_i18ngrep "cannot .fixup. without a previous commit" \ > + actual && > + test_i18ngrep "You can fix this with .git rebase --edit-todo.." \ > + actual &&
These days, we do not add new uses of test_i18n_grep; just replacing it with "grep" would be good enough, so I'll touch them up locally.
> + 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 &&
Makes me wonder if "grep -v" is too loose (i.e. are there good reasons to expect/allow that comments would be different and will not compare well if they are left in?) and is too tight (i.e. can the rebase machinery when rewriting the todo file reformat the contents on the non-comment lines in such a way that they do not compare byte-for-byte identical?). But we'll find out if its the latter (and we do not care too much if future changes to the command will start clobbering the comment lines).
Will queue with minimum fixups.
Thanks.
Show 7 quoted lines
> + test_cmp orig actual > + ) > +' > + > test_expect_success 'tabs and spaces are accepted in the todolist' ' > rebase_setup_and_clean indented-comment && > write_script add-indent.sh <<-\EOF &&