[PATCH] sequencer: finish parsing the todo list despite an invalid first line

[PATCH] sequencer: finish parsing the todo list despite an invalid first line

[Alex Henrie]

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;
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 &&
+		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
+	)
+'
+
 test_expect_success 'tabs and spaces are accepted in the todolist' '
 	rebase_setup_and_clean indented-comment &&
 	write_script add-indent.sh <<-\EOF &&
-- 
2.41.0

Re: [PATCH] sequencer: finish parsing the todo list despite an invalid first line

[Junio C Hamano]

Alex Henrie <alexhenrie24@gmail.com> 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@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.

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

Re: [PATCH] sequencer: finish parsing the todo list despite an invalid first line

[Phillip Wood]

Hi Alex

Thanks for working on this.

On 19/07/2023 15:43, Alex Henrie wrote:
> 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.


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

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

> +	)
> +'
> +
>   test_expect_success 'tabs and spaces are accepted in the todolist' '
>   	rebase_setup_and_clean indented-comment &&
>   	write_script add-indent.sh <<-\EOF &&

Re: [PATCH] sequencer: finish parsing the todo list despite an invalid first line

[Alex Henrie]

On Thu, Jul 20, 2023 at 3:42 AM Phillip Wood <phillip.wood123@gmail.com> wrote:

> On 19/07/2023 15:43, Alex Henrie wrote:
> > 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.

When the user first runs `git rebase`, the commit template contains
the following message:

# However, if you remove everything, the rebase will be aborted.

When running `git rebase --edit-todo`, that message is replaced with:

# You are editing the todo file of an ongoing interactive rebase.
# To continue rebase after editing, run:
#     git rebase --continue

The second message is indeed more accurate after the rebase has
started: Deleting all of the lines in `git rebase --edit-todo` drops
all of the commits; it does not abort the rebase.

It would be nice to preserve as much of the user's original input as
possible, but that's not a project that I'm going to tackle. As far as
a minimal fix for the regression, we can either leave the todo file
untouched and display inaccurate advice during `git rebase
--edit-todo`, or we can lose any long commit IDs that the user entered
and display equivalent short hex IDs instead. I would prefer the
latter.

> > +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.

Good point. I used --root because I copied and pasted from the
preceding test, but HEAD~4 would make the intent of the test more
clear. That change and the grep change that Junio suggested are
probably worth a v2.

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

To me it seems pretty far-fetched that a future bug would cause the
_initial_ commit template to be missing anything. But if you're
concerned about it, would you like to send a follow-up patch to revise
the test as you see fit?

> We could perhaps check the error message from "git rebase --edit-todo"
> as well.

That sounds like another good change for v2.

Thanks for the feedback,

-Alex

[PATCH v2 0/1] sequencer: finish parsing the todo list despite an invalid first line

[Alex Henrie]

Changes from v1:
- Use `grep` instead of `test_i18ngrep`
- Check for an error message from --edit-todo
- Move and rephrase the comment in the test

Thanks to Junio and Phillip for your feedback.

Alex Henrie (1):
  sequencer: finish parsing the todo list despite an invalid first line

 sequencer.c                   |  2 +-
 t/t3404-rebase-interactive.sh | 18 ++++++++++++++++++
 2 files changed, 19 insertions(+), 1 deletion(-)

Range-diff against v1:
1:  ceb53efb79 ! 1:  8005d81440 sequencer: finish parsing the todo list despite an invalid first line
    @@ t/t3404-rebase-interactive.sh: test_expect_success 'static check of bad command'
      '
      
     +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 &&
    -+		test_i18ngrep "cannot .fixup. without a previous commit" \
    -+				actual &&
    -+		test_i18ngrep "You can fix this with .git rebase --edit-todo.." \
    -+				actual &&
    ++		grep "cannot .fixup. without a previous commit" actual &&
    ++		grep "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 "cannot .fixup. without a previous commit" actual &&
    ++		grep "You can fix this with .git rebase --edit-todo.." actual &&
     +		grep -v "^#" .git/rebase-merge/git-rebase-todo >actual &&
    ++		# check that --edit-todo did not lose any of the todo list
     +		test_cmp orig actual
     +	)
     +'
-- 
2.41.0

[PATCH v2 1/1] sequencer: finish parsing the todo list despite an invalid first line

[Alex Henrie]

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 | 18 ++++++++++++++++++
 2 files changed, 19 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..d133cbae32 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -1596,6 +1596,24 @@ 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' '
+	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 &&
+		grep "cannot .fixup. without a previous commit" actual &&
+		grep "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 "cannot .fixup. without a previous commit" actual &&
+		grep "You can fix this with .git rebase --edit-todo.." actual &&
+		grep -v "^#" .git/rebase-merge/git-rebase-todo >actual &&
+		# check that --edit-todo did not lose any of the todo list
+		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 &&
-- 
2.41.0

[PATCH v3 0/1] sequencer: finish parsing the todo list despite an invalid first line

[Alex Henrie]

Changes from v2:
- Include accidentally omitted file redirect so that the output of
  --edit-todo is actually tested

Alex Henrie (1):
  sequencer: finish parsing the todo list despite an invalid first line

 sequencer.c                   |  2 +-
 t/t3404-rebase-interactive.sh | 18 ++++++++++++++++++
 2 files changed, 19 insertions(+), 1 deletion(-)

Range-diff against v2:
1:  8005d81440 ! 1:  b1af2df3f5 sequencer: finish parsing the todo list despite an invalid first line
    @@ t/t3404-rebase-interactive.sh: test_expect_success 'static check of bad command'
     +		grep "cannot .fixup. without a previous commit" actual &&
     +		grep "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 &&
    ++		test_must_fail git rebase --edit-todo 2>actual &&
     +		grep "cannot .fixup. without a previous commit" actual &&
     +		grep "You can fix this with .git rebase --edit-todo.." actual &&
     +		grep -v "^#" .git/rebase-merge/git-rebase-todo >actual &&
-- 
2.41.0

[PATCH v3 1/1] sequencer: finish parsing the todo list despite an invalid first line

[Alex Henrie]

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 | 18 ++++++++++++++++++
 2 files changed, 19 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..fba89146cf 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -1596,6 +1596,24 @@ 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' '
+	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 &&
+		grep "cannot .fixup. without a previous commit" actual &&
+		grep "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 2>actual &&
+		grep "cannot .fixup. without a previous commit" actual &&
+		grep "You can fix this with .git rebase --edit-todo.." actual &&
+		grep -v "^#" .git/rebase-merge/git-rebase-todo >actual &&
+		# check that --edit-todo did not lose any of the todo list
+		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 &&
-- 
2.41.0

[PATCH v4 0/1] sequencer: finish parsing the todo list despite an invalid first line

[Alex Henrie]

Changes from v3:
- Rebase onto HEAD~4 instead of --root (which was the original motivation
  for sending a new patch and I forgot to include that change; I probably
  shouldn't be doing Git development late at night...)

Alex Henrie (1):
  sequencer: finish parsing the todo list despite an invalid first line

 sequencer.c                   |  2 +-
 t/t3404-rebase-interactive.sh | 18 ++++++++++++++++++
 2 files changed, 19 insertions(+), 1 deletion(-)

Range-diff against v3:
1:  b1af2df3f5 ! 1:  f6fcdcd9a9 sequencer: finish parsing the todo list despite an invalid first line
    @@ t/t3404-rebase-interactive.sh: test_expect_success 'static check of bad command'
     +	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_must_fail env FAKE_LINES="fixup 1 2 3 4" \
    ++			       git rebase -i HEAD~4 2>actual &&
     +		grep "cannot .fixup. without a previous commit" actual &&
     +		grep "You can fix this with .git rebase --edit-todo.." actual &&
     +		grep -v "^#" .git/rebase-merge/git-rebase-todo >orig &&
-- 
2.41.0

[PATCH v4 1/1] sequencer: finish parsing the todo list despite an invalid first line

[Alex Henrie]

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 | 18 ++++++++++++++++++
 2 files changed, 19 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..8ffd2a7318 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -1596,6 +1596,24 @@ 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' '
+	rebase_setup_and_clean fixup-first &&
+	(
+		set_fake_editor &&
+		test_must_fail env FAKE_LINES="fixup 1 2 3 4" \
+			       git rebase -i HEAD~4 2>actual &&
+		grep "cannot .fixup. without a previous commit" actual &&
+		grep "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 2>actual &&
+		grep "cannot .fixup. without a previous commit" actual &&
+		grep "You can fix this with .git rebase --edit-todo.." actual &&
+		grep -v "^#" .git/rebase-merge/git-rebase-todo >actual &&
+		# check that --edit-todo did not lose any of the todo list
+		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 &&
-- 
2.41.0

Re: [PATCH] sequencer: finish parsing the todo list despite an invalid first line

[Phillip Wood]

Hi Alex

On 20/07/2023 23:37, Alex Henrie wrote:
> On Thu, Jul 20, 2023 at 3:42 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
> 
>> On 19/07/2023 15:43, Alex Henrie wrote:
>>> 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.
> 
> When the user first runs `git rebase`, the commit template contains
> the following message:
> 
> # However, if you remove everything, the rebase will be aborted.
> 
> When running `git rebase --edit-todo`, that message is replaced with:
> 
> # You are editing the todo file of an ongoing interactive rebase.
> # To continue rebase after editing, run:
> #     git rebase --continue
> 
> The second message is indeed more accurate after the rebase has
> started: Deleting all of the lines in `git rebase --edit-todo` drops
> all of the commits; it does not abort the rebase.

Oh, good point

> It would be nice to preserve as much of the user's original input as
> possible, but that's not a project that I'm going to tackle.

I think with your patch we do that anyway as other invalid lines are 
already passed through verbatim.

> As far as
> a minimal fix for the regression, we can either leave the todo file
> untouched and display inaccurate advice during `git rebase
> --edit-todo`, or we can lose any long commit IDs that the user entered
> and display equivalent short hex IDs instead. I would prefer the
> latter.

That's fine but the commit message should explain that decision and 
clarify why ddb81e5072 caused the regression as 
todo_list_parse_insn_buffer() is unchanged by that commit. The 
regression is caused by ddb81e5072 trying to shorten the OIDs and add 
the correct advice even when we cannot parse the todo list. That is a 
worthwhile change that we want to keep but means we need to tweak 
todo_list_parse_insn_buffer() in order to avoid truncating the todo list.

>>> +             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
> 
> To me it seems pretty far-fetched that a future bug would cause the
> _initial_ commit template to be missing anything.

Indeed but I'm talking about the initial todo list _after_ it has been 
edited by the user, not the initial template. If we started trying to 
lengthen the OIDs and remove the advice after the initial edit even when 
the list cannot be parsed then we'd have exactly this problem.

Best Wishes

Phillip

> But if you're
> concerned about it, would you like to send a follow-up patch to revise
> the test as you see fit?
> 
>> We could perhaps check the error message from "git rebase --edit-todo"
>> as well.
> 
> That sounds like another good change for v2.
> 
> Thanks for the feedback,
> 
> -Alex

Re: [PATCH] sequencer: finish parsing the todo list despite an invalid first line

[Phillip Wood]

On 21/07/2023 10:31, Phillip Wood wrote:
> That's fine but the commit message should explain that decision and 
> clarify why ddb81e5072 caused the regression

Maybe something like

Before the todo list is edited it is rewritten to shorten the OIDs of
the commits being picked and to append advice about editing the
list. The exact advice depends on whether the todo list is being
edited for the first time or not. After the todo list has been edited
it is rewritten to lengthen the OIDs of the commits being picked and to
remove the advice. If the edited list cannot be parsed then this last
step is skipped.

Prior to db81e50724 (rebase-interactive: use todo_list_write_to_file()
in edit_todo_list(), 2019-03-05) if the existing todo list could not
be parsed then the initial rewrite was skipped as well. This had the
unfortunate consequence that if the list could not be parsed after the
initial edit the advice given to the user was wrong when they
re-edited the list. This change relied on
todo_list_parse_insn_buffer() returning the whole todo list even when
it cannot be parsed. Unfortunately if the list starts with a "fixup"
command then it will be truncated and the remaining lines are
lost. Fix this by continuing to parse after an initial "fixup" commit
as we do when we see any other invalid line.

Best Wishes

Phillip

Re: [PATCH v4 0/1] sequencer: finish parsing the todo list despite an invalid first line

[Phillip Wood]

On 21/07/2023 07:07, Alex Henrie wrote:
> Changes from v3:
> - Rebase onto HEAD~4 instead of --root (which was the original motivation
>    for sending a new patch and I forgot to include that change; I probably
>    shouldn't be doing Git development late at night...)

Please don't feel that you have to re-roll straight away when someone 
reviews your patch - it's fine to wait a while if you're busy. Also if 
you disagree with some of the reviewer's comments it can be helpful to 
wait for them to respond before sending a new version in order to try 
and reach a consensus about the best way forward.

Best Wishes

Phillip

> Alex Henrie (1):
>    sequencer: finish parsing the todo list despite an invalid first line
> 
>   sequencer.c                   |  2 +-
>   t/t3404-rebase-interactive.sh | 18 ++++++++++++++++++
>   2 files changed, 19 insertions(+), 1 deletion(-)
> 
> Range-diff against v3:
> 1:  b1af2df3f5 ! 1:  f6fcdcd9a9 sequencer: finish parsing the todo list despite an invalid first line
>      @@ t/t3404-rebase-interactive.sh: test_expect_success 'static check of bad command'
>       +	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_must_fail env FAKE_LINES="fixup 1 2 3 4" \
>      ++			       git rebase -i HEAD~4 2>actual &&
>       +		grep "cannot .fixup. without a previous commit" actual &&
>       +		grep "You can fix this with .git rebase --edit-todo.." actual &&
>       +		grep -v "^#" .git/rebase-merge/git-rebase-todo >orig &&

Re: [PATCH] sequencer: finish parsing the todo list despite an invalid first line

[Junio C Hamano]

Phillip Wood <phillip.wood123@gmail.com> writes:

> On 21/07/2023 10:31, Phillip Wood wrote:
>> That's fine but the commit message should explain that decision and
>> clarify why ddb81e5072 caused the regression
>
> Maybe something like
>
> Before the todo list is edited it is rewritten to shorten the OIDs of
> the commits being picked and to append advice about editing the
> list. The exact advice depends on whether the todo list is being
> edited for the first time or not. After the todo list has been edited
> it is rewritten to lengthen the OIDs of the commits being picked and to
> remove the advice. If the edited list cannot be parsed then this last
> step is skipped.
>
> Prior to db81e50724 (rebase-interactive: use todo_list_write_to_file()
> in edit_todo_list(), 2019-03-05) if the existing todo list could not
> be parsed then the initial rewrite was skipped as well. This had the
> unfortunate consequence that if the list could not be parsed after the
> initial edit the advice given to the user was wrong when they
> re-edited the list. This change relied on
> todo_list_parse_insn_buffer() returning the whole todo list even when
> it cannot be parsed. Unfortunately if the list starts with a "fixup"
> command then it will be truncated and the remaining lines are
> lost. Fix this by continuing to parse after an initial "fixup" commit
> as we do when we see any other invalid line.
>
> Best Wishes
>
> Phillip

That does sounds a lot easier to understand as an explanation for
the reason why this change is necessary and sufficient.

Thanks for reviewing.

[PATCH v5 0/1] sequencer: finish parsing the todo list despite an invalid first line

[Alex Henrie]

Changes from v4:
- Put Phillip's suggested explanation in the commit message
- Check for truncation both after the first `git rebase` and after
  `git rebase --edit-todo`

Thanks to Phillip and Junio for your feedback.

Alex Henrie (1):
  sequencer: finish parsing the todo list despite an invalid first line

 sequencer.c                   |  2 +-
 t/t3404-rebase-interactive.sh | 27 +++++++++++++++++++++++++++
 2 files changed, 28 insertions(+), 1 deletion(-)

Range-diff against v4:
1:  f6fcdcd9a9 ! 1:  6fbe4fd3e6 sequencer: finish parsing the todo list despite an invalid first line
    @@ Metadata
      ## Commit message ##
         sequencer: finish parsing the todo list despite an invalid first line
     
    -    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.
    +    Before the todo list is edited it is rewritten to shorten the OIDs of
    +    the commits being picked and to append advice about editing the list.
    +    The exact advice depends on whether the todo list is being edited for
    +    the first time or not. After the todo list has been edited it is
    +    rewritten to lengthen the OIDs of the commits being picked and to remove
    +    the advice. If the edited list cannot be parsed then this last step is
    +    skipped.
     
    -    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.
    +    Prior to db81e50724 (rebase-interactive: use todo_list_write_to_file()
    +    in edit_todo_list(), 2019-03-05) if the existing todo list could not be
    +    parsed then the initial rewrite was skipped as well. This had the
    +    unfortunate consequence that if the list could not be parsed after the
    +    initial edit the advice given to the user was wrong when they re-edited
    +    the list. This change relied on todo_list_parse_insn_buffer() returning
    +    the whole todo list even when it cannot be parsed. Unfortunately if the
    +    list starts with a "fixup" command then it will be truncated and the
    +    remaining lines are lost. Fix this by continuing to parse after an
    +    initial "fixup" commit as we do when we see any other invalid line.
     
         Signed-off-by: Alex Henrie <alexhenrie24@gmail.com>
     
    @@ t/t3404-rebase-interactive.sh: test_expect_success 'static check of bad command'
     +test_expect_success 'the first command cannot be a fixup' '
     +	rebase_setup_and_clean fixup-first &&
     +	(
    -+		set_fake_editor &&
    -+		test_must_fail env FAKE_LINES="fixup 1 2 3 4" \
    -+			       git rebase -i HEAD~4 2>actual &&
    ++		cat >orig <<-EOF &&
    ++		fixup $(git log -1 --format="%h %s" B)
    ++		pick $(git log -1 --format="%h %s" C)
    ++		EOF
    ++
    ++		(
    ++			set_replace_editor orig &&
    ++			test_must_fail git rebase -i A 2>actual
    ++		) &&
     +		grep "cannot .fixup. without a previous commit" actual &&
     +		grep "You can fix this with .git rebase --edit-todo.." actual &&
    -+		grep -v "^#" .git/rebase-merge/git-rebase-todo >orig &&
    ++		# verify that the todo list has not been truncated
    ++		grep -v "^#" .git/rebase-merge/git-rebase-todo >actual &&
    ++		test_cmp orig actual &&
    ++
     +		test_must_fail git rebase --edit-todo 2>actual &&
     +		grep "cannot .fixup. without a previous commit" actual &&
     +		grep "You can fix this with .git rebase --edit-todo.." actual &&
    ++		# verify that the todo list has not been truncated
     +		grep -v "^#" .git/rebase-merge/git-rebase-todo >actual &&
    -+		# check that --edit-todo did not lose any of the todo list
     +		test_cmp orig actual
     +	)
     +'
-- 
2.41.0

[PATCH v5 1/1] sequencer: finish parsing the todo list despite an invalid first line

[Alex Henrie]

Before the todo list is edited it is rewritten to shorten the OIDs of
the commits being picked and to append advice about editing the list.
The exact advice depends on whether the todo list is being edited for
the first time or not. After the todo list has been edited it is
rewritten to lengthen the OIDs of the commits being picked and to remove
the advice. If the edited list cannot be parsed then this last step is
skipped.

Prior to db81e50724 (rebase-interactive: use todo_list_write_to_file()
in edit_todo_list(), 2019-03-05) if the existing todo list could not be
parsed then the initial rewrite was skipped as well. This had the
unfortunate consequence that if the list could not be parsed after the
initial edit the advice given to the user was wrong when they re-edited
the list. This change relied on todo_list_parse_insn_buffer() returning
the whole todo list even when it cannot be parsed. Unfortunately if the
list starts with a "fixup" command then it will be truncated and the
remaining lines are lost. Fix this by continuing to parse after an
initial "fixup" commit as we do when we see any other invalid line.

Signed-off-by: Alex Henrie <alexhenrie24@gmail.com>
---
 sequencer.c                   |  2 +-
 t/t3404-rebase-interactive.sh | 27 +++++++++++++++++++++++++++
 2 files changed, 28 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..c734983da0 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -1596,6 +1596,33 @@ 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' '
+	rebase_setup_and_clean fixup-first &&
+	(
+		cat >orig <<-EOF &&
+		fixup $(git log -1 --format="%h %s" B)
+		pick $(git log -1 --format="%h %s" C)
+		EOF
+
+		(
+			set_replace_editor orig &&
+			test_must_fail git rebase -i A 2>actual
+		) &&
+		grep "cannot .fixup. without a previous commit" actual &&
+		grep "You can fix this with .git rebase --edit-todo.." actual &&
+		# verify that the todo list has not been truncated
+		grep -v "^#" .git/rebase-merge/git-rebase-todo >actual &&
+		test_cmp orig actual &&
+
+		test_must_fail git rebase --edit-todo 2>actual &&
+		grep "cannot .fixup. without a previous commit" actual &&
+		grep "You can fix this with .git rebase --edit-todo.." actual &&
+		# verify that the todo list has not been truncated
+		grep -v "^#" .git/rebase-merge/git-rebase-todo >actual &&
+		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 &&
-- 
2.41.0

Re: [PATCH v5 1/1] sequencer: finish parsing the todo list despite an invalid first line

[Phillip Wood]

Hi Alex

On 22/07/2023 22:28, Alex Henrie wrote:
> Before the todo list is edited it is rewritten to shorten the OIDs of
> the commits being picked and to append advice about editing the list.
> The exact advice depends on whether the todo list is being edited for
> the first time or not. After the todo list has been edited it is
> rewritten to lengthen the OIDs of the commits being picked and to remove
> the advice. If the edited list cannot be parsed then this last step is
> skipped.
> 
> Prior to db81e50724 (rebase-interactive: use todo_list_write_to_file()
> in edit_todo_list(), 2019-03-05) if the existing todo list could not be
> parsed then the initial rewrite was skipped as well. This had the
> unfortunate consequence that if the list could not be parsed after the
> initial edit the advice given to the user was wrong when they re-edited
> the list. This change relied on todo_list_parse_insn_buffer() returning
> the whole todo list even when it cannot be parsed. Unfortunately if the
> list starts with a "fixup" command then it will be truncated and the
> remaining lines are lost. Fix this by continuing to parse after an
> initial "fixup" commit as we do when we see any other invalid line.

This version looks great apart from the test being run in an unnecessary 
subshell which looks like it got left in from the last version. Junio 
might be able to correct that when he applies the patch.

Thanks for updating the test and commit message

Best Wishes

Phillip

> Signed-off-by: Alex Henrie <alexhenrie24@gmail.com>
> ---
>   sequencer.c                   |  2 +-
>   t/t3404-rebase-interactive.sh | 27 +++++++++++++++++++++++++++
>   2 files changed, 28 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..c734983da0 100755
> --- a/t/t3404-rebase-interactive.sh
> +++ b/t/t3404-rebase-interactive.sh
> @@ -1596,6 +1596,33 @@ 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' '
> +	rebase_setup_and_clean fixup-first &&
> +	(
> +		cat >orig <<-EOF &&
> +		fixup $(git log -1 --format="%h %s" B)
> +		pick $(git log -1 --format="%h %s" C)
> +		EOF
> +
> +		(
> +			set_replace_editor orig &&
> +			test_must_fail git rebase -i A 2>actual
> +		) &&
> +		grep "cannot .fixup. without a previous commit" actual &&
> +		grep "You can fix this with .git rebase --edit-todo.." actual &&
> +		# verify that the todo list has not been truncated
> +		grep -v "^#" .git/rebase-merge/git-rebase-todo >actual &&
> +		test_cmp orig actual &&
> +
> +		test_must_fail git rebase --edit-todo 2>actual &&
> +		grep "cannot .fixup. without a previous commit" actual &&
> +		grep "You can fix this with .git rebase --edit-todo.." actual &&
> +		# verify that the todo list has not been truncated
> +		grep -v "^#" .git/rebase-merge/git-rebase-todo >actual &&
> +		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 &&

Re: [PATCH v5 1/1] sequencer: finish parsing the todo list despite an invalid first line

[Alex Henrie]

On Mon, Jul 24, 2023 at 4:02 AM Phillip Wood <phillip.wood123@gmail.com> wrote:

> On 22/07/2023 22:28, Alex Henrie wrote:
> > Before the todo list is edited it is rewritten to shorten the OIDs of
> > the commits being picked and to append advice about editing the list.
> > The exact advice depends on whether the todo list is being edited for
> > the first time or not. After the todo list has been edited it is
> > rewritten to lengthen the OIDs of the commits being picked and to remove
> > the advice. If the edited list cannot be parsed then this last step is
> > skipped.
> >
> > Prior to db81e50724 (rebase-interactive: use todo_list_write_to_file()
> > in edit_todo_list(), 2019-03-05) if the existing todo list could not be
> > parsed then the initial rewrite was skipped as well. This had the
> > unfortunate consequence that if the list could not be parsed after the
> > initial edit the advice given to the user was wrong when they re-edited
> > the list. This change relied on todo_list_parse_insn_buffer() returning
> > the whole todo list even when it cannot be parsed. Unfortunately if the
> > list starts with a "fixup" command then it will be truncated and the
> > remaining lines are lost. Fix this by continuing to parse after an
> > initial "fixup" commit as we do when we see any other invalid line.
>
> This version looks great apart from the test being run in an unnecessary
> subshell which looks like it got left in from the last version. Junio
> might be able to correct that when he applies the patch.

I think I see what you mean now: Because this test never performs a
successful rebase, rebase_setup_and_clean is overkill. I can send a v6
tonight that uses 'test_when_finished "git rebase --abort"' instead.

Thanks,

-Alex

Re: [PATCH v5 1/1] sequencer: finish parsing the todo list despite an invalid first line

[Phillip Wood]

Hi Alex

On 24/07/2023 16:26, Alex Henrie wrote:
> On Mon, Jul 24, 2023 at 4:02 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
> 
>> On 22/07/2023 22:28, Alex Henrie wrote:
>>> Before the todo list is edited it is rewritten to shorten the OIDs of
>>> the commits being picked and to append advice about editing the list.
>>> The exact advice depends on whether the todo list is being edited for
>>> the first time or not. After the todo list has been edited it is
>>> rewritten to lengthen the OIDs of the commits being picked and to remove
>>> the advice. If the edited list cannot be parsed then this last step is
>>> skipped.
>>>
>>> Prior to db81e50724 (rebase-interactive: use todo_list_write_to_file()
>>> in edit_todo_list(), 2019-03-05) if the existing todo list could not be
>>> parsed then the initial rewrite was skipped as well. This had the
>>> unfortunate consequence that if the list could not be parsed after the
>>> initial edit the advice given to the user was wrong when they re-edited
>>> the list. This change relied on todo_list_parse_insn_buffer() returning
>>> the whole todo list even when it cannot be parsed. Unfortunately if the
>>> list starts with a "fixup" command then it will be truncated and the
>>> remaining lines are lost. Fix this by continuing to parse after an
>>> initial "fixup" commit as we do when we see any other invalid line.
>>
>> This version looks great apart from the test being run in an unnecessary
>> subshell which looks like it got left in from the last version. Junio
>> might be able to correct that when he applies the patch.
> 
> I think I see what you mean now: Because this test never performs a
> successful rebase, rebase_setup_and_clean is overkill. I can send a v6
> tonight that uses 'test_when_finished "git rebase --abort"' instead.

I don't mind either way on that particular issue though I agree we could 
just use 'test_when_finished "git rebase --abort"' instead. What I was 
referring to was the subshell that comes after 'rebase_setup_and_clean'. 
The change I was looking for was removing the '(' after 
"rebase_setup_and_clean" at the beginning of the test, removing ')' at 
the end of the test and adjusting the indentation.

+test_expect_success 'the first command cannot be a fixup' '
+	rebase_setup_and_clean fixup-first &&
+	(

This subshell is unnecessary as we're not changing directory or 
exporting any environment variables

+		cat >orig <<-EOF &&
+		fixup $(git log -1 --format="%h %s" B)
+		pick $(git log -1 --format="%h %s" C)
+		EOF
+
+		(

This subshell is required as we're setting GIT_SEQUENCE_EDITOR in the 
environment. We only want that set for the initial rebase. In particular 
we do not want it set when we run "git rebase --edit-todo" below which 
is why we exit the subshell as soon as the initial rebase exits.


Best Wishes

Phillip

+			set_replace_editor orig &&
+			test_must_fail git rebase -i A 2>actual
+		) &&
+		grep "cannot .fixup. without a previous commit" actual &&
+		grep "You can fix this with .git rebase --edit-todo.." actual &&
+		# verify that the todo list has not been truncated
+		grep -v "^#" .git/rebase-merge/git-rebase-todo >actual &&
+		test_cmp orig actual &&
+
+		test_must_fail git rebase --edit-todo 2>actual &&
+		grep "cannot .fixup. without a previous commit" actual &&
+		grep "You can fix this with .git rebase --edit-todo.." actual &&
+		# verify that the todo list has not been truncated
+		grep -v "^#" .git/rebase-merge/git-rebase-todo >actual &&
+		test_cmp orig actual
+	)
+'


> Thanks,
> 
> -Alex

Re: [PATCH v5 1/1] sequencer: finish parsing the todo list despite an invalid first line

[Junio C Hamano]

Alex Henrie <alexhenrie24@gmail.com> writes:

> Before the todo list is edited it is rewritten to shorten the OIDs of
> the commits being picked and to append advice about editing the list.
> The exact advice depends on whether the todo list is being edited for
> the first time or not. After the todo list has been edited it is
> rewritten to lengthen the OIDs of the commits being picked and to remove
> the advice. If the edited list cannot be parsed then this last step is
> skipped.
>
> Prior to db81e50724 (rebase-interactive: use todo_list_write_to_file()
> in edit_todo_list(), 2019-03-05) if the existing todo list could not be
> parsed then the initial rewrite was skipped as well. This had the
> unfortunate consequence that if the list could not be parsed after the
> initial edit the advice given to the user was wrong when they re-edited
> the list. This change relied on todo_list_parse_insn_buffer() returning
> the whole todo list even when it cannot be parsed. Unfortunately if the
> list starts with a "fixup" command then it will be truncated and the
> remaining lines are lost. Fix this by continuing to parse after an
> initial "fixup" commit as we do when we see any other invalid line.
>
> Signed-off-by: Alex Henrie <alexhenrie24@gmail.com>
> ---
>  sequencer.c                   |  2 +-
>  t/t3404-rebase-interactive.sh | 27 +++++++++++++++++++++++++++
>  2 files changed, 28 insertions(+), 1 deletion(-)

Very cleanly explained.  Thanks for an update.

> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
> index ff0afad63e..c734983da0 100755
> --- a/t/t3404-rebase-interactive.sh
> +++ b/t/t3404-rebase-interactive.sh
> @@ -1596,6 +1596,33 @@ 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' '
> +	rebase_setup_and_clean fixup-first &&
> +	(
> +		cat >orig <<-EOF &&
> +		fixup $(git log -1 --format="%h %s" B)
> +		pick $(git log -1 --format="%h %s" C)
> +		EOF
> +
> +		(
> +			set_replace_editor orig &&
> +			test_must_fail git rebase -i A 2>actual
> +		) &&
> +		grep "cannot .fixup. without a previous commit" actual &&
> +		grep "You can fix this with .git rebase --edit-todo.." actual &&
> +		# verify that the todo list has not been truncated
> +		grep -v "^#" .git/rebase-merge/git-rebase-todo >actual &&
> +		test_cmp orig actual &&
> +
> +		test_must_fail git rebase --edit-todo 2>actual &&
> +		grep "cannot .fixup. without a previous commit" actual &&
> +		grep "You can fix this with .git rebase --edit-todo.." actual &&
> +		# verify that the todo list has not been truncated
> +		grep -v "^#" .git/rebase-merge/git-rebase-todo >actual &&
> +		test_cmp orig actual
> +	)
> +'

The structure of this new test piece, including the use of "log -1
--format", seems to follow existing tests, and very readable.  Why
do we have one extra level of subshell, though?  There is no "cd"
that may affect the later test pieces, and set_something_editor that
touches environment that may affect the later test pieces is called
in its own subshell already.

Other than that, looking good (there may be a valid reason why the
test piece needs the subshell around it, but it was just not apparent
to me).

>  test_expect_success 'tabs and spaces are accepted in the todolist' '
>  	rebase_setup_and_clean indented-comment &&
>  	write_script add-indent.sh <<-\EOF &&

Re: [PATCH v5 1/1] sequencer: finish parsing the todo list despite an invalid first line

[Alex Henrie]

On Mon, Jul 24, 2023 at 10:40 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> Alex Henrie <alexhenrie24@gmail.com> writes:

> > +test_expect_success 'the first command cannot be a fixup' '
> > +     rebase_setup_and_clean fixup-first &&
> > +     (
> > +             cat >orig <<-EOF &&
> > +             fixup $(git log -1 --format="%h %s" B)
> > +             pick $(git log -1 --format="%h %s" C)
> > +             EOF
> > +
> > +             (
> > +                     set_replace_editor orig &&
> > +                     test_must_fail git rebase -i A 2>actual
> > +             ) &&
> > +             grep "cannot .fixup. without a previous commit" actual &&
> > +             grep "You can fix this with .git rebase --edit-todo.." actual &&
> > +             # verify that the todo list has not been truncated
> > +             grep -v "^#" .git/rebase-merge/git-rebase-todo >actual &&
> > +             test_cmp orig actual &&
> > +
> > +             test_must_fail git rebase --edit-todo 2>actual &&
> > +             grep "cannot .fixup. without a previous commit" actual &&
> > +             grep "You can fix this with .git rebase --edit-todo.." actual &&
> > +             # verify that the todo list has not been truncated
> > +             grep -v "^#" .git/rebase-merge/git-rebase-todo >actual &&
> > +             test_cmp orig actual
> > +     )
> > +'
>
> The structure of this new test piece, including the use of "log -1
> --format", seems to follow existing tests, and very readable.  Why
> do we have one extra level of subshell, though?  There is no "cd"
> that may affect the later test pieces, and set_something_editor that
> touches environment that may affect the later test pieces is called
> in its own subshell already.
>
> Other than that, looking good (there may be a valid reason why the
> test piece needs the subshell around it, but it was just not apparent
> to me).

The only reason for the outer subshell is that I thought it was
required when using rebase_setup_and_clean, but I see now that
rebase_setup_and_clean is used in several tests without a subshell.
I'll drop it altogether in v6 and use `test_when_finished "git rebase
--abort"` instead.

Thanks,

-Alex

Re: [PATCH v5 1/1] sequencer: finish parsing the todo list despite an invalid first line

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> The structure of this new test piece, including the use of "log -1
> --format", seems to follow existing tests, and very readable.  Why
> do we have one extra level of subshell, though?  There is no "cd"
> that may affect the later test pieces, and set_something_editor that
> touches environment that may affect the later test pieces is called
> in its own subshell already.
>
> Other than that, looking good (there may be a valid reason why the
> test piece needs the subshell around it, but it was just not apparent
> to me).

Ah, now I notice that Phillip also noticed the same thing.

I just removed the outer subshell while queuing.  Thanks for working
on this, and thanks Phillip for excellent reviews.

Re: [PATCH v5 1/1] sequencer: finish parsing the todo list despite an invalid first line

[Alex Henrie]

On Mon, Jul 24, 2023 at 12:30 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Junio C Hamano <gitster@pobox.com> writes:
>
> > The structure of this new test piece, including the use of "log -1
> > --format", seems to follow existing tests, and very readable.  Why
> > do we have one extra level of subshell, though?  There is no "cd"
> > that may affect the later test pieces, and set_something_editor that
> > touches environment that may affect the later test pieces is called
> > in its own subshell already.
> >
> > Other than that, looking good (there may be a valid reason why the
> > test piece needs the subshell around it, but it was just not apparent
> > to me).
>
> Ah, now I notice that Phillip also noticed the same thing.
>
> I just removed the outer subshell while queuing.  Thanks for working
> on this, and thanks Phillip for excellent reviews.

That works. Thanks to both of you!

-Alex