| Message ID | pull.1492.v4.git.1694013771.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 to Eric, Glen and Junio for their comments on v2. Here are the > changes since v2: > > Patch 1 - Reworded the commit message. > > Patch 2 - Reworded the commit message, added a test and fixed error message > pointed out by Glen. > > Patch 3 - New cleanup. > > Patch 4 - Reworded the commit message, now only increments > todo_list->current if there is no error. > > Patch 5 - Swapped with next patch. Reworded the commit message, stopped > testing implementation (suggested by Glen). Expanded post-rewrite hook test. > > Patch 6 - Reworded the commit message, now uses the message file rather than > the author script to check if "rebase --continue" should commit staged > changes. Junio suggested using a separate file for this but I think that > would end up being more involved as we'd need to be careful about creating > and removing it. > > Patch 7 - Reworded the commit message. Thanks. This version looks good to me (although I am not as familiar with this part of the codebase as others on the Cc: line).
Hi Phillip, On Wed, 6 Sep 2023, Phillip Wood via GitGitGadget wrote: > Range-diff vs v3: > > 1: 1ab1ad2ef07 ! 1: ae4f873b3d0 rebase -i: move unlink() calls > @@ Metadata > ## Commit message ## > rebase -i: move unlink() calls > > - At the start of each iteration the loop that picks commits removes > - state files from the previous pick. However some of these are only > - written if there are conflicts and so we break out of the loop after > - writing them. Therefore they only need to be removed when the rebase > - continues, not in each iteration. > + At the start of each iteration the loop that picks commits removes the > + state files from the previous pick. However some of these files are only > + written if there are conflicts in which case we exit the loop before the > + end of the loop body. Therefore they only need to be removed when the > + rebase continues, not at the start of each iteration. > > Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> > > 2: e2a758eb4a5 ! 2: f540ed1d607 rebase -i: remove patch file after conflict resolution > @@ Commit message > now used in two different places rebase_path_patch() is added and used > to obtain the path for the patch. > > + To construct the path write_patch() previously used get_dir() which > + returns different paths depending on whether we're rebasing or > + cherry-picking/reverting. As this function is only called when > + rebasing it is safe to use a hard coded string for the directory > + instead. An assertion is added to make sure we don't starting calling > + this function when cherry-picking in the future. > + > Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> > > ## sequencer.c ## > @@ sequencer.c: static GIT_PATH_FUNC(rebase_path_amend, "rebase-merge/amend") > * For the post-rewrite hook, we make a list of rewritten commits and > * their new sha1s. The rewritten-pending list keeps the sha1s of > @@ sequencer.c: static int make_patch(struct repository *r, > + char hex[GIT_MAX_HEXSZ + 1]; > + int res = 0; > + > ++ if (!is_rebase_i(opts)) > ++ BUG("make_patch should only be called when rebasing"); > ++ > + oid_to_hex_r(hex, &commit->object.oid); > + if (write_message(hex, strlen(hex), rebase_path_stopped_sha(), 1) < 0) > return -1; > res |= write_rebase_head(&commit->object.oid); > > 3: 8f6c0e40567 ! 3: 818bdaf772d sequencer: use rebase_path_message() > @@ Commit message > made function to get the path name instead. This was the last > remaining use of the strbuf so remove it as well. > > + As with the previous patch we now use a hard coded string rather than > + git_dir() when constructing the path. This is safe for the same > + reason (make_patch() is only called when rebasing) and is protected by > + the assertion added in the previous patch. > + > Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> > > ## sequencer.c ## > 4: a1fad70f4b9 = 4: bd67765a864 sequencer: factor out part of pick_commits() > 5: df401945866 ! 5: f6f330f7063 rebase: fix rewritten list for failed pick > @@ Commit message > 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. > + The code to add a failed command back into the todo list is duplicated > + between pick_one_commit() and the loop in pick_commits(). Both sites > + print advice about the command being rescheduled, decrement the current > + item and save the todo list. To avoid duplicating this code > + pick_one_commit() is modified to set a flag to indicate that the command > + should be rescheduled in the main loop. This simplifies things as only > + the remaining copy of the code needs to be modified to set REBASE_HEAD > + rather than calling error_with_patch(). > > Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> > > 6: 2ed7cbe5fff = 6: 0ca5fccca17 rebase --continue: refuse to commit after failed command > 7: bbe0afde512 = 7: 8d5f6d51e19 rebase -i: fix adding failed command to the todo list Thank you for indulging me. This iteration looks good to me! Ciao, Johannes
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes: >> ... >> 6: 2ed7cbe5fff = 6: 0ca5fccca17 rebase --continue: refuse to commit after failed command >> 7: bbe0afde512 = 7: 8d5f6d51e19 rebase -i: fix adding failed command to the todo list > > Thank you for indulging me. This iteration looks good to me! Thanks, all. Let's merge it down to 'next'.
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 to Eric, Glen and Junio for their comments on v2. Here are the changes since v2: Patch 1 - Reworded the commit message. Patch 2 - Reworded the commit message, added a test and fixed error message pointed out by Glen. Patch 3 - New cleanup. Patch 4 - Reworded the commit message, now only increments todo_list->current if there is no error. Patch 5 - Swapped with next patch. Reworded the commit message, stopped testing implementation (suggested by Glen). Expanded post-rewrite hook test. Patch 6 - Reworded the commit message, now uses the message file rather than the author script to check if "rebase --continue" should commit staged changes. Junio suggested using a separate file for this but I think that would end up being more involved as we'd need to be careful about creating and removing it. Patch 7 - Reworded the commit message. 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 (7): rebase -i: move unlink() calls rebase -i: remove patch file after conflict resolution sequencer: use rebase_path_message() sequencer: factor out part of pick_commits() rebase: fix rewritten list for failed pick rebase --continue: refuse to commit after failed command rebase -i: fix adding failed command to the todo list sequencer.c | 182 ++++++++++++++++++---------------- t/t3404-rebase-interactive.sh | 53 +++++++--- t/t3418-rebase-continue.sh | 18 ++++ t/t3430-rebase-merges.sh | 30 ++++-- t/t5407-post-rewrite-hook.sh | 48 +++++++++ 5 files changed, 228 insertions(+), 103 deletions(-) base-commit: a80be152923a46f04a06bade7bcc72870e46ca09 Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1492%2Fphillipwood%2Frebase-dont-write-done-when-rescheduling-v4 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1492/phillipwood/rebase-dont-write-done-when-rescheduling-v4 Pull-Request: https://github.com/gitgitgadget/git/pull/1492 Range-diff vs v3: 1: 1ab1ad2ef07 ! 1: ae4f873b3d0 rebase -i: move unlink() calls @@ Metadata ## Commit message ## rebase -i: move unlink() calls - At the start of each iteration the loop that picks commits removes - state files from the previous pick. However some of these are only - written if there are conflicts and so we break out of the loop after - writing them. Therefore they only need to be removed when the rebase - continues, not in each iteration. + At the start of each iteration the loop that picks commits removes the + state files from the previous pick. However some of these files are only + written if there are conflicts in which case we exit the loop before the + end of the loop body. Therefore they only need to be removed when the + rebase continues, not at the start of each iteration. Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> 2: e2a758eb4a5 ! 2: f540ed1d607 rebase -i: remove patch file after conflict resolution @@ Commit message now used in two different places rebase_path_patch() is added and used to obtain the path for the patch. + To construct the path write_patch() previously used get_dir() which + returns different paths depending on whether we're rebasing or + cherry-picking/reverting. As this function is only called when + rebasing it is safe to use a hard coded string for the directory + instead. An assertion is added to make sure we don't starting calling + this function when cherry-picking in the future. + Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> ## sequencer.c ## @@ sequencer.c: static GIT_PATH_FUNC(rebase_path_amend, "rebase-merge/amend") * For the post-rewrite hook, we make a list of rewritten commits and * their new sha1s. The rewritten-pending list keeps the sha1s of @@ sequencer.c: static int make_patch(struct repository *r, + char hex[GIT_MAX_HEXSZ + 1]; + int res = 0; + ++ if (!is_rebase_i(opts)) ++ BUG("make_patch should only be called when rebasing"); ++ + oid_to_hex_r(hex, &commit->object.oid); + if (write_message(hex, strlen(hex), rebase_path_stopped_sha(), 1) < 0) return -1; res |= write_rebase_head(&commit->object.oid); 3: 8f6c0e40567 ! 3: 818bdaf772d sequencer: use rebase_path_message() @@ Commit message made function to get the path name instead. This was the last remaining use of the strbuf so remove it as well. + As with the previous patch we now use a hard coded string rather than + git_dir() when constructing the path. This is safe for the same + reason (make_patch() is only called when rebasing) and is protected by + the assertion added in the previous patch. + Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> ## sequencer.c ## 4: a1fad70f4b9 = 4: bd67765a864 sequencer: factor out part of pick_commits() 5: df401945866 ! 5: f6f330f7063 rebase: fix rewritten list for failed pick @@ Commit message 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. + The code to add a failed command back into the todo list is duplicated + between pick_one_commit() and the loop in pick_commits(). Both sites + print advice about the command being rescheduled, decrement the current + item and save the todo list. To avoid duplicating this code + pick_one_commit() is modified to set a flag to indicate that the command + should be rescheduled in the main loop. This simplifies things as only + the remaining copy of the code needs to be modified to set REBASE_HEAD + rather than calling error_with_patch(). Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> 6: 2ed7cbe5fff = 6: 0ca5fccca17 rebase --continue: refuse to commit after failed command 7: bbe0afde512 = 7: 8d5f6d51e19 rebase -i: fix adding failed command to the todo list