| Message ID | 9356d14b09a468d8ef2884cd7d76e59ec5c16691.1682089075.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | rebase -i: impove handling of failed commands | expand |
On Fri, Apr 21, 2023 at 11:12 AM Phillip Wood via GitGitGadget <gitgitgadget@gmail.com> wrote: > If a commit cannot be picked because it would overwrite an untracked > file then "git rebase --continue" should refuse to commit any staged > changes as the commit was not picked. Do this by using the existing > check for a missing author script in run_git_commit() which prevents > "rebase --continue" from committing staged changes after failed exec > commands. > > When fast-forwarding it is not necessary to write the author script as > we're reusing an existing commit, not creating a new one. If a > fast-forwarded commit is modified by an "edit" or "reword" command then > the modification is committed with "git commit --amend" which reuses the > author of the commit being amended so the author script is not needed. > baf8ec8d3a (rebase -r: don't write .git/MERGE_MSG when fast-forwarding, > 2021-08-20) changed run_git_commit() to allow a missing author script > when rewording a commit. This changes extends that to allow a missing s/changes/change/ > author script whenever the commit is being amended. > > If we're not fast-forwarding then we must remove the author script if > the pick fails. > > Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
"Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes: > From: Phillip Wood <phillip.wood@dunelm.org.uk> > > If a commit cannot be picked because it would overwrite an untracked > file then "git rebase --continue" should refuse to commit any staged > changes as the commit was not picked. It makes perfect sense to refuse blindly committing. But this makes me wonder what the procedure for the user to recover. In the simplest case, the untracked file may be expendable and the user would wish to easily redo the step after removing it? Like $ git rebase ... stops with "merging will overwrite untracked 'foo'" ... $ git rebase --continue ... refuses thanks to the fix in this step ... $ rm foo ... now what? "git rebase --redo"? "git rebase --continue"? > Do this by using the existing > check for a missing author script in run_git_commit() which prevents > "rebase --continue" from committing staged changes after failed exec > commands. Depending on the recovery procedure, this may or may not be a wise design decision, even though it may be the quickest way to implement it. If the recovery procedure involves redoing the failed step from scratch (i.e. "rm foo && git reset --hard && git rebase" would try to restart by replaying the failed step anew), then loss of author script file has no downside. If we want to salvage as much as what was done in the initial attempt, maybe not. > When fast-forwarding it is not necessary to write the author script as I'd prefer a comma before "it is not" here. > we're reusing an existing commit, not creating a new one. If a > fast-forwarded commit is modified by an "edit" or "reword" command then > the modification is committed with "git commit --amend" which reuses the > author of the commit being amended so the author script is not needed. OK. > baf8ec8d3a (rebase -r: don't write .git/MERGE_MSG when fast-forwarding, > 2021-08-20) changed run_git_commit() to allow a missing author script > when rewording a commit. This changes extends that to allow a missing It is unclear to me what "This changes extends" refers to. Could you rephrase? > author script whenever the commit is being amended. > > If we're not fast-forwarding then we must remove the author script if > the pick fails. The changes described in these three paragraphs are about efforts that were made needed only because the approach chosen to stop "continue" from continuing in this situation happens to be to remove the author script. If it were done differently (perhaps by adding another flag file that "continue" pays attention to), none of them may be necessary? > @@ -4141,6 +4140,7 @@ static int do_merge(struct repository *r, > if (ret < 0) { > error(_("could not even attempt to merge '%.*s'"), > merge_arg_len, arg); > + unlink(rebase_path_author_script()); > goto leave_merge; > } I agree that this is the right location to add new code to stop "continue" from moving forward. I do not know if the "unlink the author script" is the right choice of such a new code, though. Coming back to my first point, perhaps adding an advice() after this error() would help end users who see this error to learn what to do to move forward? > diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh > index ff0afad63e2..c1fe55dc2c1 100755 > --- a/t/t3404-rebase-interactive.sh > +++ b/t/t3404-rebase-interactive.sh > @@ -1288,6 +1288,12 @@ test_expect_success 'rebase -i commits that overwrite untracked files (pick)' ' > test_must_fail git rebase --continue && > test_cmp_rev HEAD F && > rm file6 && Before the pre-context is an attempt to run "git rebase -i" to replay a commit that adds file6, stop the sequence by marking a step as "edit" to take control back. Then we create file6 manually in the working tree and make sure that "git rebase --continue" fails in the pre-context we can see here. The recovery I asked about earlier is done with "rm file6" in this case. > + test_path_is_missing .git/rebase-merge/author-script && This is testing the implementation. The failed "continue" would have removed the file thanks to the change we saw earlier. > + echo changed >file1 && > + git add file1 && > + test_must_fail git rebase --continue 2>err && Then we make some edit, and try to "--continue". Why should this fail? Is it because the earlier "rebase --continue" that failed did not replay the original commit due to untracked file and the user needs to redo the step in its entirety before the working tree becomes ready to take any further changes? > + grep "error: you have staged changes in your working tree" err && > + git reset --hard HEAD && And this "reset --hard" is another thing in the recovery procedure the user needs to take (the other one being the removal of file6 we have seen earlier). After that, "rebase --continue" will replay the step that was interrupted by the untracked file6 that was in the working tree. OK. > git rebase --continue && > test_cmp_rev HEAD I > ' We of course do not want to become overly "helpful" and run "reset --hard" ourselves when we issue the "could not even attempt to merge" message, but when we step back and see what the user wanted to do, this is still not entirely satisfactory, is it? My understanding of what the user wanted to do (and let's pretend that creation of file6 in the middle was merely for test writer's convenience and in the real scenario of what the user wanted to do, there was the file left untracked from the beginning before the rebase started) is to redo the A---F---I chain, making a bit of change to F when it is recreated, and then replay I on top of the result of tweaked F. But instead of allowing them to edit F by doing some modification to file1, we ended up forcing the user to discard the edit made to file1 with "reset --hard", and "continue" replayed I on top of the replayed F that "edit" did not have a chance to modify. Of course, the user can now go back to A and replay F and I on top, essentially redoing the "rebase -i" with FAKE_LINES="edit 1 2" and this time, because untracked file6 is gone, it may work better and F may allow to be tweaked. But then it does not look any better than saying "git rebase --abort" before doing the final "continue", which will also allow the user to redo the whole thing from scratch again. So, I dunno.
"Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes: > If a commit cannot be picked because it would overwrite an untracked > file then "git rebase --continue" should refuse to commit any staged > changes as the commit was not picked. Do this by using the existing > check for a missing author script in run_git_commit() which prevents > "rebase --continue" from committing staged changes after failed exec > commands. For someone unfamiliar with "git rebase" code, I think it is easy enough to gather that "rebase --continue" will refuse to accept staged changes if the author script is missing, so we are reusing that mechanism to achieve our desired effect. It's not obvious whether this might have unintended consequences (Are we reusing something unrelated for an unintended purpose?) or what alternatives exist (Is sequencer.c so complex that there isn't another way to do this?). It would have been helpful for me to see how these considerations factored into your decision. > When fast-forwarding it is not necessary to write the author script as > we're reusing an existing commit, not creating a new one. If a > fast-forwarded commit is modified by an "edit" or "reword" command then > the modification is committed with "git commit --amend" which reuses the > author of the commit being amended so the author script is not needed. > baf8ec8d3a (rebase -r: don't write .git/MERGE_MSG when fast-forwarding, > 2021-08-20) changed run_git_commit() to allow a missing author script > when rewording a commit. This changes extends that to allow a missing > author script whenever the commit is being amended. As I understand it, the author script can now be missing in other circumstances, so we have to adjust the rest of the machinery to handle that case? If so, this seems to suggest that there are some unintended consequences. > diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh > index ff0afad63e2..c1fe55dc2c1 100755 > --- a/t/t3404-rebase-interactive.sh > +++ b/t/t3404-rebase-interactive.sh > @@ -1288,6 +1288,12 @@ test_expect_success 'rebase -i commits that overwrite untracked files (pick)' ' > test_must_fail git rebase --continue && > test_cmp_rev HEAD F && > rm file6 && > + test_path_is_missing .git/rebase-merge/author-script && Checking that the path is missing seems like testing implementation details. If so, I would prefer to remove this assertion here and elsewhere. > + echo changed >file1 && > + git add file1 && > + test_must_fail git rebase --continue 2>err && > + grep "error: you have staged changes in your working tree" err && > + git reset --hard HEAD && This seems reasonable.
diff --git a/sequencer.c b/sequencer.c index 2d463818dd1..55bf0a72c3a 100644 --- a/sequencer.c +++ b/sequencer.c @@ -1055,7 +1055,7 @@ static int run_git_commit(const char *defmsg, if (is_rebase_i(opts) && ((opts->committer_date_is_author_date && !opts->ignore_date) || - !(!defmsg && (flags & AMEND_MSG))) && + !(flags & AMEND_MSG)) && read_env_script(&cmd.env)) { const char *gpg_opt = gpg_sign_opt_quoted(opts); @@ -2216,8 +2216,6 @@ static int do_pick_commit(struct repository *r, if (opts->allow_ff && !is_fixup(command) && ((parent && oideq(&parent->object.oid, &head)) || (!parent && unborn))) { - if (is_rebase_i(opts)) - write_author_script(msg.message); res = fast_forward_to(r, &commit->object.oid, &head, unborn, opts); if (res || command != TODO_REWORD) @@ -2324,9 +2322,10 @@ static int do_pick_commit(struct repository *r, command == TODO_REVERT) { res = do_recursive_merge(r, base, next, base_label, next_label, &head, &msgbuf, opts); - if (res < 0) + if (res < 0) { + unlink(rebase_path_author_script()); goto leave; - + } res |= write_message(msgbuf.buf, msgbuf.len, git_path_merge_msg(r), 0); } else { @@ -4141,6 +4140,7 @@ static int do_merge(struct repository *r, if (ret < 0) { error(_("could not even attempt to merge '%.*s'"), merge_arg_len, arg); + unlink(rebase_path_author_script()); goto leave_merge; } /* diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh index ff0afad63e2..c1fe55dc2c1 100755 --- a/t/t3404-rebase-interactive.sh +++ b/t/t3404-rebase-interactive.sh @@ -1288,6 +1288,12 @@ test_expect_success 'rebase -i commits that overwrite untracked files (pick)' ' test_must_fail git rebase --continue && test_cmp_rev HEAD F && rm file6 && + test_path_is_missing .git/rebase-merge/author-script && + echo changed >file1 && + git add file1 && + test_must_fail git rebase --continue 2>err && + grep "error: you have staged changes in your working tree" err && + git reset --hard HEAD && git rebase --continue && test_cmp_rev HEAD I ' @@ -1306,6 +1312,7 @@ test_expect_success 'rebase -i commits that overwrite untracked files (squash)' test_must_fail git rebase --continue && test_cmp_rev HEAD F && rm file6 && + test_path_is_missing .git/rebase-merge/author-script && git rebase --continue && test $(git cat-file commit HEAD | sed -ne \$p) = I && git reset --hard original-branch2 @@ -1324,6 +1331,7 @@ test_expect_success 'rebase -i commits that overwrite untracked files (no ff)' ' test_must_fail git rebase --continue && test $(git cat-file commit HEAD | sed -ne \$p) = F && rm file6 && + test_path_is_missing .git/rebase-merge/author-script && 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 f03599c63b9..360ec787ffd 100755 --- a/t/t3430-rebase-merges.sh +++ b/t/t3430-rebase-merges.sh @@ -168,13 +168,15 @@ test_expect_success 'failed `merge -C` writes patch (may be rescheduled, too)' ' 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/author-script && : 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 && - test_path_is_file .git/rebase-merge/patch + test_path_is_file .git/rebase-merge/patch && + test_path_is_file .git/rebase-merge/author-script ' test_expect_success 'failed `merge <branch>` does not crash' '