Alex Henrie <alexhenrie24@xxxxxxxxx> writes:
> 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@xxxxxxxxx>
> ---
> 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.
> 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.
> + # 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.
> + 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 &&
