| Message ID | pull.1492.v2.git.1682089074.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | rebase -i: impove handling of failed commands | expand |
"Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes: > This series fixes several bugs in the way we handle a commit cannot be > picked because it would overwrite an untracked file. > > * after a failed pick "git rebase --continue" will happily commit any > staged changes even though no commit was picked. > > * the commit of the failed pick is recorded as rewritten even though no > commit was picked. > > * the "done" file used by "git status" to show the recently executed > commands contains an incorrect entry. > > Thanks for the comments on V1, this series has now grown somewhat. > Previously I was worried that refactoring would change the behavior, but > having thought about it the current behavior is wrong and should be changed. So much has changed since the original one, I did not even recognise this was a redoing of the "rescheduled step" topic (which I do not think I have kept in 'seen'). Building on top of the more recent 'master' like you did here is very much appreciated.
Hi Phillip! We picked up this series during Review Club. You can browse the notes at https://docs.google.com/document/d/14L8BAumGTpsXpjDY8VzZ4rRtpAjuGrFSRqn3stCuS_w/edit?pli=1 but we'll post any substantial feedback back to the mailing list anyway. Firstly, I have to acknowledge that this series appears to be geared towards to reviewers who are already familiar with the rebase machinery and don't require a lot of context on the changes. None of the Review Club attendees were familiar with it, so we had trouble following along certain patches, but we might not have been the intended audience anyway. I can leave comments from that perspective, i.e. what would have been useful from _if_ this were written for folks who weren't already familiar with the rebase code, which could be useful if we were trying to train people to reviewer sequencer.c. However, I don't think this warrants substantial changes if the series is already clear to the intended audience. "Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes: > This series fixes several bugs in the way we handle a commit cannot be > picked because it would overwrite an untracked file. > > * after a failed pick "git rebase --continue" will happily commit any > staged changes even though no commit was picked. This sounds like a safer default for most users, but I worry that there are seasoned users relying on the old behavior. > * the commit of the failed pick is recorded as rewritten even though no > commit was picked. Sounds like a reasonable fix. > * the "done" file used by "git status" to show the recently executed > commands contains an incorrect entry. This also sounds reasonable, but from Johannes' upthread response, this sounds like this isn't universally agreed upon. Perhaps the underlying issue is that the behavior of "git rebase --continue", and the todo/done lists is underspecified (in public and internal documentation), so we end up having to reestablish what the 'correct' behavior is (which will probably end up being just a matter of personal taste). This isn't strictly necessary for the series, but it would be nice for us to establish what the correct behavior _should be_ (even if we aren't there yet) and use that to guide future fixes.
This series fixes several bugs in the way we handle a commit cannot be picked because it would overwrite an untracked file. * after a failed pick "git rebase --continue" will happily commit any staged changes even though no commit was picked. * the commit of the failed pick is recorded as rewritten even though no commit was picked. * the "done" file used by "git status" to show the recently executed commands contains an incorrect entry. Thanks for the comments on V1, this series has now grown somewhat. Previously I was worried that refactoring would change the behavior, but having thought about it the current behavior is wrong and should be changed. Changes since V1: Rebased onto master to avoid a conflict with ab/remove-implicit-use-of-the-repository * Patches 1-3 are new preparatory changes * Patches 4 & 5 are new and fix the first two issues listed above. * Patch 6 is the old patch 1 which has been rebased and the commit message reworded. It fixes the last issues listed above. Phillip Wood (6): rebase -i: move unlink() calls rebase -i: remove patch file after conflict resolution sequencer: factor out part of pick_commits() rebase --continue: refuse to commit after failed command rebase: fix rewritten list for failed pick rebase -i: fix adding failed command to the todo list sequencer.c | 170 ++++++++++++++++++---------------- t/t3404-rebase-interactive.sh | 49 +++++++--- t/t3430-rebase-merges.sh | 35 +++++-- t/t5407-post-rewrite-hook.sh | 11 +++ 4 files changed, 165 insertions(+), 100 deletions(-) base-commit: 9c6990cca24301ae8f82bf6291049667a0aef14b Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1492%2Fphillipwood%2Frebase-dont-write-done-when-rescheduling-v2 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1492/phillipwood/rebase-dont-write-done-when-rescheduling-v2 Pull-Request: https://github.com/gitgitgadget/git/pull/1492 Range-diff vs v1: -: ----------- > 1: 3dfb2c6903b rebase -i: move unlink() calls -: ----------- > 2: 227aea031b5 rebase -i: remove patch file after conflict resolution -: ----------- > 3: 31bb644e769 sequencer: factor out part of pick_commits() -: ----------- > 4: 9356d14b09a rebase --continue: refuse to commit after failed command -: ----------- > 5: f8e64c1b631 rebase: fix rewritten list for failed pick 1: dc51a7499bc ! 6: a836b049b90 rebase -i: do not update "done" when rescheduling command @@ Metadata Author: Phillip Wood <phillip.wood@dunelm.org.uk> ## Commit message ## - rebase -i: do not update "done" when rescheduling command + rebase -i: fix adding failed command to the todo list - As the sequencer executes todo commands it appends them to - .git/rebase-merge/done. This file is used by "git status" to show the - recently executed commands. Unfortunately when a command is rescheduled + When rebasing commands are moved from the todo list in "git-rebase-todo" + to the "done" file (which is used by "git status" to show the recently + executed commands) just before they are executed. This means that if a + command fails because it would overwrite an untracked file it has to be + added back into the todo list before the rebase stops for the user to + fix the problem. + + Unfortunately when a failed command is added back into the todo list the command preceding it is erroneously appended to the "done" file. - This means that when rebase stops after rescheduling "pick B" the "done" + This means that when rebase stops after "pick B" fails the "done" file contains pick A @@ Commit message pick A pick B - Fix this by not updating the "done" file when adding a rescheduled - command back into the "todo" file. A couple of the existing tests are + Fix this by not updating the "done" file when adding a failed command + back into the "git-rebase-todo" file. A couple of the existing tests are modified to improve their coverage as none of them trigger this bug or check the "done" file. - Note that the rescheduled command will still be appended to the "done" - file again when it is successfully executed. Arguably it would be better - not to do that but fixing it would be more involved. - Reported-by: Stefan Haller <lists@haller-berlin.de> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> @@ sequencer.c: static int pick_commits(struct repository *r, int check_todo = 0; - if (save_todo(todo_list, opts)) -+ if (save_todo(todo_list, opts, 0)) ++ if (save_todo(todo_list, opts, reschedule)) return -1; if (is_rebase_i(opts)) { if (item->command != TODO_COMMENT) { -@@ sequencer.c: static int pick_commits(struct repository *r, - todo_list->current), - get_item_line(todo_list, - todo_list->current)); -- todo_list->current--; -- if (save_todo(todo_list, opts)) -+ if (save_todo(todo_list, opts, 1)) - return -1; - } - if (item->command == TODO_EDIT) { @@ sequencer.c: static int pick_commits(struct repository *r, 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)) -+ if (save_todo(todo_list, opts, 1)) ++ if (save_todo(todo_list, opts, reschedule)) return -1; if (item->commit) - return error_with_patch(r, + write_rebase_head(&item->commit->object.oid); ## t/t3404-rebase-interactive.sh ## @@ t/t3404-rebase-interactive.sh: test_expect_success 'todo count' ' @@ t/t3404-rebase-interactive.sh: test_expect_success 'todo count' ' + head -n3 todo >expect && + test_cmp expect .git/rebase-merge/done && + rm file2 && + test_path_is_missing .git/rebase-merge/author-script && + test_path_is_missing .git/rebase-merge/patch && + test_path_is_missing .git/MERGE_MSG && +@@ t/t3404-rebase-interactive.sh: test_expect_success 'rebase -i commits that overwrite untracked files (pick)' ' + grep "error: you have staged changes in your working tree" err && + git reset --hard HEAD && git rebase --continue && - test_cmp_rev HEAD I + test_cmp_rev HEAD D &&