* [PATCH] sequencer.c: set GIT_WORK_TREE in run_git_checkout @ 2022-01-25 23:36 Glen Choo 2022-01-25 23:39 ` Glen Choo 2022-01-26 0:12 ` Elijah Newren 0 siblings, 2 replies; 5+ messages in thread From: Glen Choo @ 2022-01-25 23:36 UTC (permalink / raw) To: git; +Cc: Glen Choo, Elijah Newren, Junio C Hamano When rebasing in a subdirectory of a worktree, Git fails due to a dirty worktree: error: The following untracked working tree files would be overwritten by merge: a/b/c Please move or remove them before you merge. This occurs because "git rebase" invokes a "git checkout" without propagating the GIT_WORK_TREE environment variable, causing the worktree to be assumed to be ".". This was not an issue until bc3ae46b42 (rebase: do not attempt to remove startup_info->original_cwd, 2021-12-09), when the .dir of the "git checkout" child process was changed such that it no longer runs at the root of the worktree. Propagate GIT_WORK_TREE to the "git checkout" child process and test that rebase in a subdirectory of a worktree succeeds. Signed-off-by: Glen Choo <chooglen@google.com> --- Here is a fix for the bug I found in [1]. I didn't look through the rest of the "preserve cwd" thread for other possible bugs pertaining to worktrees, but I didn't find any during a cursory glance at sequencer.c. I'm also not sure if this is the idiomatic way to do it since, I assume, we'd always want to propagate GIT_WORK_TREE, but this is identical to how do_exec() sets GIT_WORK_TREE in the same file (perhaps this is something that should be refactored). [1] https://lore.kernel.org/git/kl6lilu71rzl.fsf@chooglen-macbookpro.roam.corp.google.com sequencer.c | 2 ++ t/t3400-rebase.sh | 29 +++++++++++++++++++++++++++++ 2 files changed, 31 insertions(+) diff --git a/sequencer.c b/sequencer.c index 83f257e7fa..e193dcf846 100644 --- a/sequencer.c +++ b/sequencer.c @@ -4233,6 +4233,8 @@ static int run_git_checkout(struct repository *r, struct replay_opts *opts, strvec_push(&cmd.args, "checkout"); strvec_push(&cmd.args, commit); strvec_pushf(&cmd.env_array, GIT_REFLOG_ACTION "=%s", action); + strvec_pushf(&cmd.env_array, "GIT_WORK_TREE=%s", + absolute_path(get_git_work_tree())); if (opts->verbose) ret = run_command(&cmd); diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh index 23dbd3c82e..8b8b66538b 100755 --- a/t/t3400-rebase.sh +++ b/t/t3400-rebase.sh @@ -416,4 +416,33 @@ test_expect_success MINGW,SYMLINKS_WINDOWS 'rebase when .git/logs is a symlink' mv actual_logs .git/logs ' +test_expect_success 'rebase when inside worktree subdirectory' ' + git init main-wt && + ( + cd main-wt && + git commit --allow-empty -m "initial" && + # create commit with foo/bar/baz + mkdir -p foo/bar && + touch foo/bar/baz && + git add foo/bar/baz && + git commit -m "add foo/bar/baz" && + # create commit with a/b/c + mkdir -p a/b && + touch a/b/c && + git add a/b/c && + git commit -m "add a/b/c" && + # create another branch for our other worktree + git branch other && + git worktree add ../other-wt other && + ( + cd ../other-wt && + mkdir -p random/dir && + ( + cd random/dir && + git rebase --onto HEAD^^ HEAD^ # drops the HEAD^ commit + ) + ) + ) +' + test_done base-commit: bc3ae46b420f58dfe2bfd87714dca096a043d554 -- 2.33.GIT ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] sequencer.c: set GIT_WORK_TREE in run_git_checkout 2022-01-25 23:36 [PATCH] sequencer.c: set GIT_WORK_TREE in run_git_checkout Glen Choo @ 2022-01-25 23:39 ` Glen Choo 2022-01-26 0:13 ` Elijah Newren 2022-01-26 0:12 ` Elijah Newren 1 sibling, 1 reply; 5+ messages in thread From: Glen Choo @ 2022-01-25 23:39 UTC (permalink / raw) To: git; +Cc: Elijah Newren, Junio C Hamano Glen Choo <chooglen@google.com> writes: > When rebasing in a subdirectory of a worktree, Git fails due to a dirty > worktree: > > error: The following untracked working tree files would be overwritten > by merge: > a/b/c > Please move or remove them before you merge. > > This occurs because "git rebase" invokes a "git checkout" without > propagating the GIT_WORK_TREE environment variable, causing the worktree > to be assumed to be ".". This was not an issue until bc3ae46b42 (rebase: > do not attempt to remove startup_info->original_cwd, 2021-12-09), when > the .dir of the "git checkout" child process was changed such that it no > longer runs at the root of the worktree. > > Propagate GIT_WORK_TREE to the "git checkout" child process and test > that rebase in a subdirectory of a worktree succeeds. > > Signed-off-by: Glen Choo <chooglen@google.com> > --- > Here is a fix for the bug I found in [1]. I didn't look through the rest > of the "preserve cwd" thread for other possible bugs pertaining to > worktrees, but I didn't find any during a cursory glance at sequencer.c. > > I'm also not sure if this is the idiomatic way to do it since, I assume, > we'd always want to propagate GIT_WORK_TREE, but this is identical to > how do_exec() sets GIT_WORK_TREE in the same file (perhaps this is > something that should be refactored). > > [1] https://lore.kernel.org/git/kl6lilu71rzl.fsf@chooglen-macbookpro.roam.corp.google.com I forgot to add this to the previous email: cc-ing Junio because this bug is in master. ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] sequencer.c: set GIT_WORK_TREE in run_git_checkout 2022-01-25 23:39 ` Glen Choo @ 2022-01-26 0:13 ` Elijah Newren 0 siblings, 0 replies; 5+ messages in thread From: Elijah Newren @ 2022-01-26 0:13 UTC (permalink / raw) To: Glen Choo; +Cc: Git Mailing List, Junio C Hamano On Tue, Jan 25, 2022 at 3:39 PM Glen Choo <chooglen@google.com> wrote: > ... > I forgot to add this to the previous email: > > cc-ing Junio because this bug is in master. ...and is a new regression in 2.35. :-( Sorry about that. ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] sequencer.c: set GIT_WORK_TREE in run_git_checkout 2022-01-25 23:36 [PATCH] sequencer.c: set GIT_WORK_TREE in run_git_checkout Glen Choo 2022-01-25 23:39 ` Glen Choo @ 2022-01-26 0:12 ` Elijah Newren 2022-01-26 0:22 ` Glen Choo 1 sibling, 1 reply; 5+ messages in thread From: Elijah Newren @ 2022-01-26 0:12 UTC (permalink / raw) To: Glen Choo; +Cc: Git Mailing List, Junio C Hamano Looks like you beat me to sending out a patch. On Tue, Jan 25, 2022 at 3:36 PM Glen Choo <chooglen@google.com> wrote: > > When rebasing in a subdirectory of a worktree, Git fails due to a dirty > worktree: > > error: The following untracked working tree files would be overwritten > by merge: > a/b/c > Please move or remove them before you merge. > > This occurs because "git rebase" invokes a "git checkout" without > propagating the GIT_WORK_TREE environment variable, causing the worktree > to be assumed to be ".". This was not an issue until bc3ae46b42 (rebase: > do not attempt to remove startup_info->original_cwd, 2021-12-09), when > the .dir of the "git checkout" child process was changed such that it no > longer runs at the root of the worktree. > > Propagate GIT_WORK_TREE to the "git checkout" child process and test > that rebase in a subdirectory of a worktree succeeds. This is correct, but leaves the reader wondering why the failure to set GIT_WORK_TREE fails when in a non-primary worktree, but does not fail in the primary working tree. Here's how I described it in the patch I was about to send to the list: """ In commit bc3ae46 ("rebase: do not attempt to remove startup_info->original_cwd", 2021-12-09), we wanted to allow the checkout subprocess to know which directory the parent process was running from, so that the subprocess could protect it. However... When run from a non-main worktree, setup_git_directory() will note that the discovered git directory (/PATH/TO/.git/worktree/non-main-worktree) does not match DEFAULT_GIT_DIR_ENVIRONMENT (see setup_discovered_git_dir()), and decide to set GIT_DIR in the environment. This matters because... Whenever git is run with the GIT_DIR environment variable set, and GIT_WORK_TREE not set, it presumes that '.' is the working tree. So... This combination results in rebase failing when run from a subdirectory of a non-main worktree. Fix it by also setting the GIT_WORK_TREE environment variable along with setting cmd.dir. A possibly more involved fix we could consider for later would be to make setup.c set GIT_WORK_TREE whenever (a) it discovers both the git directory and the working tree and (b) it decides to set GIT_DIR in the environment. I did not attempt that here as such would be too big of a change for a 2.35.1 release. """ (Another fix would also be making sequencer stop forking a "git checkout" subprocess; it should be able to call a library function, and then quit dropping and re-reading the index. But that's also a much bigger change...) > Signed-off-by: Glen Choo <chooglen@google.com> > --- > Here is a fix for the bug I found in [1]. I didn't look through the rest > of the "preserve cwd" thread for other possible bugs pertaining to > worktrees, but I didn't find any during a cursory glance at sequencer.c. We'll need the same fix for stash due to 0fce211ccc ("stash: do not attempt to remove startup_info->original_cwd", 2021-12-09) -- in that case, though, it's calling git-clean rather than git-checkout. But the same idea. > I'm also not sure if this is the idiomatic way to do it since, I assume, > we'd always want to propagate GIT_WORK_TREE, but this is identical to > how do_exec() sets GIT_WORK_TREE in the same file (perhaps this is > something that should be refactored). I'd rather only set GIT_WORK_TREE if something is also setting GIT_DIR, but I don't think it hurts to set it in all cases. > [1] https://lore.kernel.org/git/kl6lilu71rzl.fsf@chooglen-macbookpro.roam.corp.google.com > > sequencer.c | 2 ++ > t/t3400-rebase.sh | 29 +++++++++++++++++++++++++++++ > 2 files changed, 31 insertions(+) > > diff --git a/sequencer.c b/sequencer.c > index 83f257e7fa..e193dcf846 100644 > --- a/sequencer.c > +++ b/sequencer.c > @@ -4233,6 +4233,8 @@ static int run_git_checkout(struct repository *r, struct replay_opts *opts, > strvec_push(&cmd.args, "checkout"); > strvec_push(&cmd.args, commit); > strvec_pushf(&cmd.env_array, GIT_REFLOG_ACTION "=%s", action); > + strvec_pushf(&cmd.env_array, "GIT_WORK_TREE=%s", > + absolute_path(get_git_work_tree())); struct repository *r already contains the path to the worktree in r->worktree; I think it'd be better to use it. > if (opts->verbose) > ret = run_command(&cmd); > diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh > index 23dbd3c82e..8b8b66538b 100755 > --- a/t/t3400-rebase.sh > +++ b/t/t3400-rebase.sh > @@ -416,4 +416,33 @@ test_expect_success MINGW,SYMLINKS_WINDOWS 'rebase when .git/logs is a symlink' > mv actual_logs .git/logs > ' > > +test_expect_success 'rebase when inside worktree subdirectory' ' > + git init main-wt && > + ( > + cd main-wt && > + git commit --allow-empty -m "initial" && > + # create commit with foo/bar/baz > + mkdir -p foo/bar && > + touch foo/bar/baz && > + git add foo/bar/baz && > + git commit -m "add foo/bar/baz" && > + # create commit with a/b/c > + mkdir -p a/b && > + touch a/b/c && > + git add a/b/c && > + git commit -m "add a/b/c" && > + # create another branch for our other worktree > + git branch other && > + git worktree add ../other-wt other && > + ( > + cd ../other-wt && > + mkdir -p random/dir && > + ( > + cd random/dir && > + git rebase --onto HEAD^^ HEAD^ # drops the HEAD^ commit > + ) > + ) > + ) > +' Nice testcase. You would have seen potentially even more confusing results had some of these commits merely modified files rather than introducing totally new files. ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] sequencer.c: set GIT_WORK_TREE in run_git_checkout 2022-01-26 0:12 ` Elijah Newren @ 2022-01-26 0:22 ` Glen Choo 0 siblings, 0 replies; 5+ messages in thread From: Glen Choo @ 2022-01-26 0:22 UTC (permalink / raw) To: Elijah Newren; +Cc: Git Mailing List, Junio C Hamano Elijah Newren <newren@gmail.com> writes: > On Tue, Jan 25, 2022 at 3:36 PM Glen Choo <chooglen@google.com> wrote: >> >> When rebasing in a subdirectory of a worktree, Git fails due to a dirty >> worktree: >> >> error: The following untracked working tree files would be overwritten >> by merge: >> a/b/c >> Please move or remove them before you merge. >> >> This occurs because "git rebase" invokes a "git checkout" without >> propagating the GIT_WORK_TREE environment variable, causing the worktree >> to be assumed to be ".". This was not an issue until bc3ae46b42 (rebase: >> do not attempt to remove startup_info->original_cwd, 2021-12-09), when >> the .dir of the "git checkout" child process was changed such that it no >> longer runs at the root of the worktree. >> >> Propagate GIT_WORK_TREE to the "git checkout" child process and test >> that rebase in a subdirectory of a worktree succeeds. > > This is correct, but leaves the reader wondering why the failure to > set GIT_WORK_TREE fails when in a non-primary worktree, but does not > fail in the primary working tree. Ah, that is true. Thanks for digging into it deeper. > Here's how I described it in the patch I was about to send to the list: > > """ > In commit bc3ae46 ("rebase: do not attempt to remove > startup_info->original_cwd", 2021-12-09), we wanted to allow the > checkout subprocess to know which directory the parent process was > running from, so that the subprocess could protect it. However... > > When run from a non-main worktree, setup_git_directory() will note > that the discovered git directory > (/PATH/TO/.git/worktree/non-main-worktree) does not match > DEFAULT_GIT_DIR_ENVIRONMENT (see setup_discovered_git_dir()), and > decide to set GIT_DIR in the environment. This matters because... > > Whenever git is run with the GIT_DIR environment variable set, and > GIT_WORK_TREE not set, it presumes that '.' is the working tree. So... > > This combination results in rebase failing when run from a subdirectory > of a non-main worktree. Fix it by also setting the GIT_WORK_TREE > environment variable along with setting cmd.dir. > > A possibly more involved fix we could consider for later would be to > make setup.c set GIT_WORK_TREE whenever (a) it discovers both the git > directory and the working tree and (b) it decides to set GIT_DIR in the > environment. I did not attempt that here as such would be too big of a > change for a 2.35.1 release. > """ > > (Another fix would also be making sequencer stop forking a "git > checkout" subprocess; it should be able to call a library function, > and then quit dropping and re-reading the index. But that's also a > much bigger change...) > >> Signed-off-by: Glen Choo <chooglen@google.com> >> --- >> Here is a fix for the bug I found in [1]. I didn't look through the rest >> of the "preserve cwd" thread for other possible bugs pertaining to >> worktrees, but I didn't find any during a cursory glance at sequencer.c. > > We'll need the same fix for stash due to 0fce211ccc ("stash: do not > attempt to remove startup_info->original_cwd", 2021-12-09) -- in that > case, though, it's calling git-clean rather than git-checkout. But > the same idea. Let's use your patch [1] instead of mine :) [1] https://github.com/git/git/pull/1205 ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2022-01-26 0:22 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-01-25 23:36 [PATCH] sequencer.c: set GIT_WORK_TREE in run_git_checkout Glen Choo 2022-01-25 23:39 ` Glen Choo 2022-01-26 0:13 ` Elijah Newren 2022-01-26 0:12 ` Elijah Newren 2022-01-26 0:22 ` Glen Choo
Code repositories for project(s) associated with this public inbox https://80x24.org/mirrors/git.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).