| Message ID | c8f641132160d6bbd72a5e4921f1c9f0b3d40242.1633082702.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | rebase: reset_head() related fixes and improvements | expand |
"Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes: > From: Phillip Wood <phillip.wood@dunelm.org.uk> > > The reset bit should only be set if flags contains RESET_HEAD_HARD. > The test for `!deatch_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 > ("builtin rebase: support `git rebase <upstream> <switch-to>`", > 2018-09-04) started using reset_head() to checkout <switch-to> when > fast-forwarding. Sorry, but it is not quite clear what exactly is "fix checkout" in the context of this change, even with the above paragraph that describes the internals but not any end-user visible effect. Can this step come with its own addition to t/ to demonstrate the breakage that is fixed? > Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> > --- > reset.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/reset.c b/reset.c > index 79310ae071b..fc4dae3fd2d 100644 > --- a/reset.c > +++ b/reset.c > @@ -57,7 +57,7 @@ int reset_head(struct repository *r, struct object_id *oid, const char *action, > unpack_tree_opts.update = 1; > unpack_tree_opts.merge = 1; > init_checkout_metadata(&unpack_tree_opts.meta, switch_to_branch, oid, NULL); > - if (!detach_head) > + if (reset_hard) > unpack_tree_opts.reset = 1; > > if (repo_read_index_unmerged(r) < 0) {
On Fri, Oct 1, 2021 at 6:06 AM Phillip Wood via GitGitGadget <gitgitgadget@gmail.com> wrote: > The reset bit should only be set if flags contains RESET_HEAD_HARD. > The test for `!deatch_head` dates back to the original implementation s/deatch_head/detach_head/ > of reset_head() in ac7f467fef ("builtin/rebase: support running "git > rebase <upstream>"", 2018-08-07) and was correct until e65123a71d > ("builtin rebase: support `git rebase <upstream> <switch-to>`", > 2018-09-04) started using reset_head() to checkout <switch-to> when > fast-forwarding. > > Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
Hi Junio On 01/10/2021 21:26, Junio C Hamano wrote: > "Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes: > >> From: Phillip Wood <phillip.wood@dunelm.org.uk> >> >> The reset bit should only be set if flags contains RESET_HEAD_HARD. >> The test for `!deatch_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 >> ("builtin rebase: support `git rebase <upstream> <switch-to>`", >> 2018-09-04) started using reset_head() to checkout <switch-to> when >> fast-forwarding. > > Sorry, but it is not quite clear what exactly is "fix checkout" in > the context of this change, even with the above paragraph that > describes the internals but not any end-user visible effect. "git checkout" refuses to overwrite untracked files but reset_head() does when checking out a branch. > Can this step come with its own addition to t/ to demonstrate the > breakage that is fixed? I can add a test to check that a checkout does not remove untracked files. However such a test would pass on top of en/remaving-untracked-fixes without the fix in this patch. I cannot think of a way to specifically test that unpack_tree_opts.reset == 0 unless RESET_HEAD_HARD is given after en/removing-untracked-fixes is merged. Elijah's fixes will stop the "reset" mode of reset_head() from wiping untracked files which is good. The reset flag to unpack trees also affects unmerged entries but rebase does not try to checkout anything if the index contains unmerged entries. Best Wishes Phillip >> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> >> --- >> reset.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/reset.c b/reset.c >> index 79310ae071b..fc4dae3fd2d 100644 >> --- a/reset.c >> +++ b/reset.c >> @@ -57,7 +57,7 @@ int reset_head(struct repository *r, struct object_id *oid, const char *action, >> unpack_tree_opts.update = 1; >> unpack_tree_opts.merge = 1; >> init_checkout_metadata(&unpack_tree_opts.meta, switch_to_branch, oid, NULL); >> - if (!detach_head) >> + if (reset_hard) >> unpack_tree_opts.reset = 1; >> >> if (repo_read_index_unmerged(r) < 0) {
Phillip Wood <phillip.wood123@gmail.com> writes: >> Sorry, but it is not quite clear what exactly is "fix checkout" in >> the context of this change, even with the above paragraph that >> describes the internals but not any end-user visible effect. > > "git checkout" refuses to overwrite untracked files but reset_head() > does when checking out a branch. By the name of the function, I would have expected that would be the intended behaviour, though. Isn't it emulating "reset --hard" that was used when the calling commands were implemented as scripts? What I am wondering is if the overwriting behaviour is annoying for some callers but is essential for other callers, and unless we tell readers that we vetted all the callers and everybody wants to keep the paths overwritten from the tree-ish in the working tree that are not in the index, I feel uneasy to add a label "fix" to such a change to a callee used from multiple code paths. > files. However such a test would pass on top of > en/remaving-untracked-fixes without the fix in this patch. I cannot > think of a way to specifically test that unpack_tree_opts.reset == 0 > unless RESET_HEAD_HARD is given after en/removing-untracked-fixes is > merged. Meaning that this fix is redundant, as the other topic has already been cooking and well in the process of graduating? Thanks.
diff --git a/reset.c b/reset.c index 79310ae071b..fc4dae3fd2d 100644 --- a/reset.c +++ b/reset.c @@ -57,7 +57,7 @@ int reset_head(struct repository *r, struct object_id *oid, const char *action, unpack_tree_opts.update = 1; unpack_tree_opts.merge = 1; init_checkout_metadata(&unpack_tree_opts.meta, switch_to_branch, oid, NULL); - if (!detach_head) + if (reset_hard) unpack_tree_opts.reset = 1; if (repo_read_index_unmerged(r) < 0) {