On Wed, Jan 26, 2022 at 5:05 AM Phillip Wood via GitGitGadget <gitgitgadget@xxxxxxxxx> wrote: > > Thanks to Junio and Elijah for their comments on V2. There aren't too many > changes this time. > > This series started out with the aim of converting 'rebase -i' to use > reset_head() instead of forking 'git checkout'. As 'rebase --apply' already > uses reset_head() I assumed this would be straight forward. However it has > morphed into a series of fixes for reset_head() followed by the intended > conversion of 'rebase -i'. > > reset.c::reset_head() started its life at ac7f467f (builtin/rebase: support > running "git rebase ", 2018-08-07) as a way to detach the HEAD to replay the > commits during "git rebase", but over time it learned to do many things, > like switching the tip of the branch to another commit, recording the old > value of HEAD in ORIG_HEAD while it does so, recording reflog entries for > both HEAD and for the branch. > > The API into the function got clunky and it is harder than necessary for the > callers to use the function correctly, which led to a handful of bugs that > are fixed by this series. The bugs include > > * passing the wrong oid to the post-checkout hook > * removing untracked files on checkout > * running the post-checkout hook if the checkout fails > * passing parameters to reset_head() that it does not use > * incorrect reflog messages for 'rebase --apply' > * sometimes setting ORIG_HEAD incorrectly at the start 'rebase --apply' > > Later steps of this series revamps the API so that it is harder to use it > incorrectly to prevent future bugs and finally convert 'rebase -i' to use > reset_head() > > Changes since V2: > > * Updated cover letter as suggested by Junio > * Patch 4 - fixed typos in the commit message spotted by Junio > * Patch 9 - Moved later in the series to simplify the autostash canges as > suggested by Junio. This used to be patch 7 > * Patch 10 - Added a comment to the commit message explaining why we cannot > BUG() on an invalid parameter until a change is made in a later commit > * Patch 13 - Reworded the first sentence commit message as suggest by > Elijah. This round looks good to me. Thanks! > Cover letter for V2: > > Thanks for the comments on V1. I have tried to improve the commit messages > to explain better the motivation and implications of the changes in this > series and I have added some more tests. I have rebased onto v2.34.0 to > avoid some merges conflicts. > > Changes since V1: > > * Patch 1 - unchanged. > * Patches 2, 3 - these are new and fix an bug I noticed while adding a test > to patch 4. > * Patches 4, 5 - improved commit messages and added tests. > * Patch 6 - reworded commit message. > * Patch 7 - split out some changes that used to be in patch 9. > * Patch 8 - in principle the same but the range-diff is noisy due to the > addition of patch 3. > * Patch 9 - reworded commit message. > * Patch 10 - unchanged. > * Patch 11 - reworded commit message and a couple of comments. > * Patch 12 - minor changes to comments. > * Patch 13 - cosmetic changes to commit message and tests. > * Patch 14 - cosmetic changes to commit message. > > Cover letter for V1: Fix some issues with the implementation and use of > reset_head(). The last patch was previously posted as [1], I have updated > the commit message and rebased it onto the fixes in this series. There are a > couple of small conflicts merging this into seen, I think they should be > easy to resolve (in rebase.c take both sides in reset.c take the changed > lines from each side). These patches are based on pw/rebase-of-a-tag-fix > > [1] > https://lore.kernel.org/git/39ad40c9297531a2d42b7263a1d41b1ecbc23c0a.1631108472.git.gitgitgadget@xxxxxxxxx/ > > Phillip Wood (14): > rebase: factor out checkout for up to date branch > t5403: refactor rebase post-checkout hook tests > rebase: pass correct arguments to post-checkout hook > rebase: do not remove untracked files on checkout > rebase --apply: don't run post-checkout hook if there is an error > reset_head(): remove action parameter > reset_head(): factor out ref updates > reset_head(): make default_reflog_action optional > create_autostash(): remove unneeded parameter > rebase: cleanup reset_head() calls > reset_head(): take struct rebase_head_opts > rebase --apply: fix reflog > rebase --apply: set ORIG_HEAD correctly > rebase -m: don't fork git checkout > > builtin/merge.c | 6 +- > builtin/rebase.c | 101 +++++++++++++---------- > reset.c | 149 ++++++++++++++++++++-------------- > reset.h | 48 ++++++++++- > sequencer.c | 47 ++++------- > sequencer.h | 3 +- > t/t3406-rebase-message.sh | 23 ++++++ > t/t3418-rebase-continue.sh | 26 ++++++ > t/t5403-post-checkout-hook.sh | 67 +++++++++++---- > 9 files changed, 312 insertions(+), 158 deletions(-) > > > base-commit: cd3e606211bb1cf8bc57f7d76bab98cc17a150bc > Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1049%2Fphillipwood%2Fwip%2Frebase-reset-head-fixes-v3 > Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1049/phillipwood/wip/rebase-reset-head-fixes-v3 > Pull-Request: https://github.com/gitgitgadget/git/pull/1049 > > Range-diff vs v2: > > 1: 0e84d00572e = 1: 0e84d00572e rebase: factor out checkout for up to date branch > 2: a67a5a03b94 = 2: a67a5a03b94 t5403: refactor rebase post-checkout hook tests > 3: 07867760e68 = 3: 07867760e68 rebase: pass correct arguments to post-checkout hook > 4: 2b499704c8f ! 4: f4b925508e7 rebase: do not remove untracked files on checkout > @@ Commit message > <upstream> is an ancestor of <branch> then it will fast-forward and > checkout <branch>. Normally a checkout or picking a commit during a > rebase will refuse to overwrite untracked files, however rebase does > - overwrite untracked files when checking <branch>. > + overwrite untracked files when checking out <branch>. > > The fix is to only set reset in `unpack_tree_opts` if flags contains > `RESET_HEAD_HARD`. t5403 may seem like an odd home for the new test > but it will be extended in the next commit to check that the > post-checkout hook is not run when the checkout fails. > > - The test for `!deatch_head` dates back to the > + The test for `!detach_head` dates back to the > original implementation of reset_head() in > ac7f467fef ("builtin/rebase: support running "git rebase <upstream>"", > 2018-08-07) and was correct until e65123a71d > 5: 04e7340a7e7 = 5: 4de5104d22d rebase --apply: don't run post-checkout hook if there is an error > 6: 32ffa98c1bc = 6: ff23498e93e reset_head(): remove action parameter > 8: 29e06e7d36d = 7: 688ebc45bf7 reset_head(): factor out ref updates > 9: 9d00a218daf ! 8: a5cc7eaa925 reset_head(): make default_reflog_action optional > @@ reset.c: int reset_head(struct repository *r, struct object_id *oid, > > leave_reset_head: > rollback_lock_file(&lock); > - > - ## sequencer.c ## > -@@ sequencer.c: void create_autostash(struct repository *r, const char *path) > - write_file(path, "%s", oid_to_hex(&oid)); > - printf(_("Created autostash: %s\n"), buf.buf); > - if (reset_head(r, NULL, NULL, RESET_HEAD_HARD, NULL, NULL, > -- "") < 0) > -+ NULL) < 0) > - die(_("could not reset --hard")); > - > - if (discard_index(r->index) < 0 || > 7: 341fe183c18 ! 9: dd3a22384d2 create_autostash(): remove unneeded parameter > @@ sequencer.c: void create_autostash(struct repository *r, const char *path, > printf(_("Created autostash: %s\n"), buf.buf); > if (reset_head(r, NULL, NULL, RESET_HEAD_HARD, NULL, NULL, > - default_reflog_action) < 0) > -+ "") < 0) > ++ NULL) < 0) > die(_("could not reset --hard")); > > if (discard_index(r->index) < 0 || > 10: 5ea636009e7 ! 10: ad7c6467987 rebase: cleanup reset_head() calls > @@ Commit message > currently possible so we delay fixing that caller until we can pass > the message as the branch reflog. > > + A later commit will make it a BUG() to pass reflog_orig_head without > + RESET_ORIG_HEAD, that changes cannot be done here as it needs to wait > + for move_to_original_branch() to be fixed first. > + > Signed-off-by: Phillip Wood <phillip.wood@xxxxxxxxxxxxx> > > ## builtin/rebase.c ## > 11: 24b0566aba5 = 11: d170703e833 reset_head(): take struct rebase_head_opts > 12: dc5d11291e7 = 12: 4973892561e rebase --apply: fix reflog > 13: 45a5b5e9818 ! 13: 0ef0e978112 rebase --apply: set ORIG_HEAD correctly > @@ Metadata > ## Commit message ## > rebase --apply: set ORIG_HEAD correctly > > - At the start of a rebase ORIG_HEAD is updated to tip of the branch > - being rebased. Unfortunately reset_head() always uses the current > - value of HEAD for this which is incorrect if the rebase is started > - with "git rebase <upstream> <branch>" as in that case ORIG_HEAD should > - be updated to <branch>. This only affects the "apply" backend as the > - "merge" backend does not yet use reset_head() for the initial > - checkout. Fix this by passing in orig_head when calling reset_head() > - and add some regression tests. > + At the start of a rebase, ORIG_HEAD is updated to the tip of the > + branch being rebased. Unfortunately reset_head() always uses the > + current value of HEAD for this which is incorrect if the rebase is > + started with "git rebase <upstream> <branch>" as in that case > + ORIG_HEAD should be updated to <branch>. This only affects the "apply" > + backend as the "merge" backend does not yet use reset_head() for the > + initial checkout. Fix this by passing in orig_head when calling > + reset_head() and add some regression tests. > > Signed-off-by: Phillip Wood <phillip.wood@xxxxxxxxxxxxx> > > 14: 3f64b9274b5 = 14: 9b9560ef676 rebase -m: don't fork git checkout > > -- > gitgitgadget
