| Message ID | df4019458665eccf2b16cdf1d6c1061186a62711.1690903412.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | e032abd5a0bec40a700527f71073f3ba21fce7d6 |
| Headers | show |
| Series | rebase -i: impove handling of failed commands | expand |
Hi Phillip, On Tue, 1 Aug 2023, Phillip Wood via GitGitGadget wrote: > From: Phillip Wood <phillip.wood@dunelm.org.uk> > > git rebase keeps a list that maps the OID of each commit before it was > rebased to the OID of the equivalent commit after the rebase. This list > is used to drive the "post-rewrite" hook that is called at the end of a > successful rebase. When a rebase stops for the user to resolve merge > conflicts the OID of the commit being picked is written to > ".git/rebase-merge/stopped-sha". Then when the rebase is continued that > OID is added to the list of rewritten commits. Unfortunately if a commit > cannot be picked because it would overwrite an untracked file we still > write the "stopped-sha1" file. This means that when the rebase is > continued the commit is added into the list of rewritten commits even > though it has not been picked yet. > > Fix this by not calling error_with_patch() for failed commands. The pick > has failed so there is nothing to commit and therefore we do not want to > set up the state files for committing staged changes when the rebase > continues. This change means we no-longer write a patch for the failed > command or display the error message printed by error_with_patch(). As > the command has failed the patch isn't really useful and in any case the > user can inspect the commit associated with the failed command by > inspecting REBASE_HEAD. Unless the user has disabled it we already print > an advice message that is more helpful than the message from > error_with_patch() which the user will still see. Even if the advice is > disabled the user will see the messages from the merge machinery > detailing the problem. > > To simplify writing REBASE_HEAD in this case pick_one_commit() is > modified to avoid duplicating the code that adds the failed command > back into the todo list. This motivates the change well, and answered all but one of the questions I had about it, being: > diff --git a/sequencer.c b/sequencer.c > index 62277e7bcc1..e25abfd2fb4 100644 > --- a/sequencer.c > +++ b/sequencer.c > [...] > @@ -4658,12 +4659,8 @@ static int pick_one_commit(struct repository *r, > check_todo); > if (is_rebase_i(opts) && res < 0) { > /* Reschedule */ > - advise(_(rescheduled_advice), > - get_item_line_length(todo_list, todo_list->current), > - get_item_line(todo_list, todo_list->current)); > - todo_list->current--; Why is it okay to remove this decrement? Here is why: The code that calls `save_todo()` in the `if (reschedule)` block of the loop of `pick_commits()` _duplicates_ the logic that is removed here, including the advice and the decrementing of `current`. I had to instrument the code before and after this patch to figure this out, as I had missed the fact that the now-remaining code also decremented the `current` attribute. So: All is good with this patch. If you'd like to amend the commit message accordingly, I would not be opposed, but I could now live equally as easily without it. > - if (save_todo(todo_list, opts)) > - return -1; > + *reschedule = 1; > + return -1; > } > if (item->command == TODO_EDIT) { > struct commit *commit = item->commit; I'd like to point out how delighted I am about this careful test case: > diff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh > index 96ae0edf1e1..4938ebb1c17 100755 > --- a/t/t3430-rebase-merges.sh > +++ b/t/t3430-rebase-merges.sh > @@ -165,12 +165,12 @@ test_expect_success 'failed `merge -C` writes patch (may be rescheduled, too)' ' > test_config sequence.editor \""$PWD"/replace-editor.sh\" && > test_tick && > test_must_fail git rebase -ir HEAD && > + test_cmp_rev REBASE_HEAD H^0 && > grep "^merge -C .* G$" .git/rebase-merge/done && > grep "^merge -C .* G$" .git/rebase-merge/git-rebase-todo && > - test_path_is_file .git/rebase-merge/patch && > + test_path_is_missing .git/rebase-merge/patch && > > : fail because of merge conflict && > - rm G.t .git/rebase-merge/patch && > git reset --hard conflicting-G && > test_must_fail git rebase --continue && > ! grep "^merge -C .* G$" .git/rebase-merge/git-rebase-todo && > diff --git a/t/t5407-post-rewrite-hook.sh b/t/t5407-post-rewrite-hook.sh > index 5f3ff051ca2..ad7f8c6f002 100755 > --- a/t/t5407-post-rewrite-hook.sh > +++ b/t/t5407-post-rewrite-hook.sh > @@ -17,6 +17,12 @@ test_expect_success 'setup' ' > git checkout A^0 && > test_commit E bar E && > test_commit F foo F && > + git checkout B && > + git merge E && > + git tag merge-E && > + test_commit G G && > + test_commit H H && > + test_commit I I && > git checkout main && > > test_hook --setup post-rewrite <<-EOF > @@ -173,6 +179,48 @@ test_fail_interactive_rebase () { > ) > } > > +test_expect_success 'git rebase with failed pick' ' > + clear_hook_input && > + cat >todo <<-\EOF && > + exec >bar > + merge -C merge-E E > + exec >G > + pick G > + exec >H 2>I > + pick H > + fixup I > + EOF > + > + ( > + set_replace_editor todo && > + test_must_fail git rebase -i D D 2>err > + ) && > + grep "would be overwritten" err && > + rm bar && > + > + test_must_fail git rebase --continue 2>err && > + grep "would be overwritten" err && > + rm G && > + > + test_must_fail git rebase --continue 2>err && > + grep "would be overwritten" err && > + rm H && > + > + test_must_fail git rebase --continue 2>err && > + grep "would be overwritten" err && > + rm I && > + > + git rebase --continue && > + echo rebase >expected.args && > + cat >expected.data <<-EOF && > + $(git rev-parse merge-E) $(git rev-parse HEAD~2) > + $(git rev-parse G) $(git rev-parse HEAD~1) > + $(git rev-parse H) $(git rev-parse HEAD) > + $(git rev-parse I) $(git rev-parse HEAD) > + EOF > + verify_hook_input > +' > + > test_expect_success 'git rebase -i (unchanged)' ' > git reset --hard D && > clear_hook_input && Here is my ACK. Thank you, Johannes
Hi Dscho On 23/08/2023 09:55, Johannes Schindelin wrote: > Hi Phillip, > > On Tue, 1 Aug 2023, Phillip Wood via GitGitGadget wrote: > >> From: Phillip Wood <phillip.wood@dunelm.org.uk> >> >> git rebase keeps a list that maps the OID of each commit before it was >> rebased to the OID of the equivalent commit after the rebase. This list >> is used to drive the "post-rewrite" hook that is called at the end of a >> successful rebase. When a rebase stops for the user to resolve merge >> conflicts the OID of the commit being picked is written to >> ".git/rebase-merge/stopped-sha". Then when the rebase is continued that >> OID is added to the list of rewritten commits. Unfortunately if a commit >> cannot be picked because it would overwrite an untracked file we still >> write the "stopped-sha1" file. This means that when the rebase is >> continued the commit is added into the list of rewritten commits even >> though it has not been picked yet. >> >> Fix this by not calling error_with_patch() for failed commands. The pick >> has failed so there is nothing to commit and therefore we do not want to >> set up the state files for committing staged changes when the rebase >> continues. This change means we no-longer write a patch for the failed >> command or display the error message printed by error_with_patch(). As >> the command has failed the patch isn't really useful and in any case the >> user can inspect the commit associated with the failed command by >> inspecting REBASE_HEAD. Unless the user has disabled it we already print >> an advice message that is more helpful than the message from >> error_with_patch() which the user will still see. Even if the advice is >> disabled the user will see the messages from the merge machinery >> detailing the problem. >> >> To simplify writing REBASE_HEAD in this case pick_one_commit() is >> modified to avoid duplicating the code that adds the failed command >> back into the todo list. > > This motivates the change well, and answered all but one of the questions > I had about it, being: > >> diff --git a/sequencer.c b/sequencer.c >> index 62277e7bcc1..e25abfd2fb4 100644 >> --- a/sequencer.c >> +++ b/sequencer.c >> [...] >> @@ -4658,12 +4659,8 @@ static int pick_one_commit(struct repository *r, >> check_todo); >> if (is_rebase_i(opts) && res < 0) { >> /* Reschedule */ >> - advise(_(rescheduled_advice), >> - get_item_line_length(todo_list, todo_list->current), >> - get_item_line(todo_list, todo_list->current)); >> - todo_list->current--; > > Why is it okay to remove this decrement? > > Here is why: The code that calls `save_todo()` in the `if (reschedule)` > block of the loop of `pick_commits()` _duplicates_ the logic that is > removed here, including the advice and the decrementing of `current`. > > I had to instrument the code before and after this patch to figure this > out, as I had missed the fact that the now-remaining code also decremented > the `current` attribute. > > So: All is good with this patch. If you'd like to amend the commit message > accordingly, I would not be opposed, but I could now live equally as > easily without it. I'll try and add something to the commit message when I re-roll Thanks Phillip >> - if (save_todo(todo_list, opts)) >> - return -1; >> + *reschedule = 1; >> + return -1; >> } >> if (item->command == TODO_EDIT) { >> struct commit *commit = item->commit; > > I'd like to point out how delighted I am about this careful test case: > >> diff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh >> index 96ae0edf1e1..4938ebb1c17 100755 >> --- a/t/t3430-rebase-merges.sh >> +++ b/t/t3430-rebase-merges.sh >> @@ -165,12 +165,12 @@ test_expect_success 'failed `merge -C` writes patch (may be rescheduled, too)' ' >> test_config sequence.editor \""$PWD"/replace-editor.sh\" && >> test_tick && >> test_must_fail git rebase -ir HEAD && >> + test_cmp_rev REBASE_HEAD H^0 && >> grep "^merge -C .* G$" .git/rebase-merge/done && >> grep "^merge -C .* G$" .git/rebase-merge/git-rebase-todo && >> - test_path_is_file .git/rebase-merge/patch && >> + test_path_is_missing .git/rebase-merge/patch && >> >> : fail because of merge conflict && >> - rm G.t .git/rebase-merge/patch && >> git reset --hard conflicting-G && >> test_must_fail git rebase --continue && >> ! grep "^merge -C .* G$" .git/rebase-merge/git-rebase-todo && >> diff --git a/t/t5407-post-rewrite-hook.sh b/t/t5407-post-rewrite-hook.sh >> index 5f3ff051ca2..ad7f8c6f002 100755 >> --- a/t/t5407-post-rewrite-hook.sh >> +++ b/t/t5407-post-rewrite-hook.sh >> @@ -17,6 +17,12 @@ test_expect_success 'setup' ' >> git checkout A^0 && >> test_commit E bar E && >> test_commit F foo F && >> + git checkout B && >> + git merge E && >> + git tag merge-E && >> + test_commit G G && >> + test_commit H H && >> + test_commit I I && >> git checkout main && >> >> test_hook --setup post-rewrite <<-EOF >> @@ -173,6 +179,48 @@ test_fail_interactive_rebase () { >> ) >> } >> >> +test_expect_success 'git rebase with failed pick' ' >> + clear_hook_input && >> + cat >todo <<-\EOF && >> + exec >bar >> + merge -C merge-E E >> + exec >G >> + pick G >> + exec >H 2>I >> + pick H >> + fixup I >> + EOF >> + >> + ( >> + set_replace_editor todo && >> + test_must_fail git rebase -i D D 2>err >> + ) && >> + grep "would be overwritten" err && >> + rm bar && >> + >> + test_must_fail git rebase --continue 2>err && >> + grep "would be overwritten" err && >> + rm G && >> + >> + test_must_fail git rebase --continue 2>err && >> + grep "would be overwritten" err && >> + rm H && >> + >> + test_must_fail git rebase --continue 2>err && >> + grep "would be overwritten" err && >> + rm I && >> + >> + git rebase --continue && >> + echo rebase >expected.args && >> + cat >expected.data <<-EOF && >> + $(git rev-parse merge-E) $(git rev-parse HEAD~2) >> + $(git rev-parse G) $(git rev-parse HEAD~1) >> + $(git rev-parse H) $(git rev-parse HEAD) >> + $(git rev-parse I) $(git rev-parse HEAD) >> + EOF >> + verify_hook_input >> +' >> + >> test_expect_success 'git rebase -i (unchanged)' ' >> git reset --hard D && >> clear_hook_input && > > Here is my ACK. > > Thank you, > Johannes
diff --git a/sequencer.c b/sequencer.c index 62277e7bcc1..e25abfd2fb4 100644 --- a/sequencer.c +++ b/sequencer.c @@ -4155,6 +4155,7 @@ static int do_merge(struct repository *r, if (ret < 0) { error(_("could not even attempt to merge '%.*s'"), merge_arg_len, arg); + unlink(git_path_merge_msg(r)); goto leave_merge; } /* @@ -4645,7 +4646,7 @@ N_("Could not execute the todo command\n" static int pick_one_commit(struct repository *r, struct todo_list *todo_list, struct replay_opts *opts, - int *check_todo) + int *check_todo, int* reschedule) { int res; struct todo_item *item = todo_list->items + todo_list->current; @@ -4658,12 +4659,8 @@ static int pick_one_commit(struct repository *r, check_todo); if (is_rebase_i(opts) && res < 0) { /* Reschedule */ - advise(_(rescheduled_advice), - get_item_line_length(todo_list, todo_list->current), - get_item_line(todo_list, todo_list->current)); - todo_list->current--; - if (save_todo(todo_list, opts)) - return -1; + *reschedule = 1; + return -1; } if (item->command == TODO_EDIT) { struct commit *commit = item->commit; @@ -4763,7 +4760,8 @@ static int pick_commits(struct repository *r, } } if (item->command <= TODO_SQUASH) { - res = pick_one_commit(r, todo_list, opts, &check_todo); + res = pick_one_commit(r, todo_list, opts, &check_todo, + &reschedule); if (!res && item->command == TODO_EDIT) return 0; } else if (item->command == TODO_EXEC) { @@ -4817,10 +4815,7 @@ static int pick_commits(struct repository *r, if (save_todo(todo_list, opts)) return -1; if (item->commit) - return error_with_patch(r, - item->commit, - arg, item->arg_len, - opts, res, 0); + write_rebase_head(&item->commit->object.oid); } else if (is_rebase_i(opts) && check_todo && !res && reread_todo_if_changed(r, todo_list, opts)) { return -1; diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh index ff0afad63e2..6d3788c588b 100755 --- a/t/t3404-rebase-interactive.sh +++ b/t/t3404-rebase-interactive.sh @@ -1287,7 +1287,9 @@ test_expect_success 'rebase -i commits that overwrite untracked files (pick)' ' >file6 && test_must_fail git rebase --continue && test_cmp_rev HEAD F && + test_cmp_rev REBASE_HEAD I && rm file6 && + test_path_is_missing .git/rebase-merge/patch && git rebase --continue && test_cmp_rev HEAD I ' @@ -1305,7 +1307,9 @@ test_expect_success 'rebase -i commits that overwrite untracked files (squash)' >file6 && test_must_fail git rebase --continue && test_cmp_rev HEAD F && + test_cmp_rev REBASE_HEAD I && rm file6 && + test_path_is_missing .git/rebase-merge/patch && git rebase --continue && test $(git cat-file commit HEAD | sed -ne \$p) = I && git reset --hard original-branch2 @@ -1323,7 +1327,9 @@ test_expect_success 'rebase -i commits that overwrite untracked files (no ff)' ' >file6 && test_must_fail git rebase --continue && test $(git cat-file commit HEAD | sed -ne \$p) = F && + test_cmp_rev REBASE_HEAD I && rm file6 && + test_path_is_missing .git/rebase-merge/patch && git rebase --continue && test $(git cat-file commit HEAD | sed -ne \$p) = I ' diff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh index 96ae0edf1e1..4938ebb1c17 100755 --- a/t/t3430-rebase-merges.sh +++ b/t/t3430-rebase-merges.sh @@ -165,12 +165,12 @@ test_expect_success 'failed `merge -C` writes patch (may be rescheduled, too)' ' test_config sequence.editor \""$PWD"/replace-editor.sh\" && test_tick && test_must_fail git rebase -ir HEAD && + test_cmp_rev REBASE_HEAD H^0 && grep "^merge -C .* G$" .git/rebase-merge/done && grep "^merge -C .* G$" .git/rebase-merge/git-rebase-todo && - test_path_is_file .git/rebase-merge/patch && + test_path_is_missing .git/rebase-merge/patch && : fail because of merge conflict && - rm G.t .git/rebase-merge/patch && git reset --hard conflicting-G && test_must_fail git rebase --continue && ! grep "^merge -C .* G$" .git/rebase-merge/git-rebase-todo && diff --git a/t/t5407-post-rewrite-hook.sh b/t/t5407-post-rewrite-hook.sh index 5f3ff051ca2..ad7f8c6f002 100755 --- a/t/t5407-post-rewrite-hook.sh +++ b/t/t5407-post-rewrite-hook.sh @@ -17,6 +17,12 @@ test_expect_success 'setup' ' git checkout A^0 && test_commit E bar E && test_commit F foo F && + git checkout B && + git merge E && + git tag merge-E && + test_commit G G && + test_commit H H && + test_commit I I && git checkout main && test_hook --setup post-rewrite <<-EOF @@ -173,6 +179,48 @@ test_fail_interactive_rebase () { ) } +test_expect_success 'git rebase with failed pick' ' + clear_hook_input && + cat >todo <<-\EOF && + exec >bar + merge -C merge-E E + exec >G + pick G + exec >H 2>I + pick H + fixup I + EOF + + ( + set_replace_editor todo && + test_must_fail git rebase -i D D 2>err + ) && + grep "would be overwritten" err && + rm bar && + + test_must_fail git rebase --continue 2>err && + grep "would be overwritten" err && + rm G && + + test_must_fail git rebase --continue 2>err && + grep "would be overwritten" err && + rm H && + + test_must_fail git rebase --continue 2>err && + grep "would be overwritten" err && + rm I && + + git rebase --continue && + echo rebase >expected.args && + cat >expected.data <<-EOF && + $(git rev-parse merge-E) $(git rev-parse HEAD~2) + $(git rev-parse G) $(git rev-parse HEAD~1) + $(git rev-parse H) $(git rev-parse HEAD) + $(git rev-parse I) $(git rev-parse HEAD) + EOF + verify_hook_input +' + test_expect_success 'git rebase -i (unchanged)' ' git reset --hard D && clear_hook_input &&