| Message ID | 20230722212830.132135-2-alexhenrie24@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | sequencer: finish parsing the todo list despite an invalid first line | expand |
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 &&
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
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
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 &&
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
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.
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
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 &&
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(-)