| Message ID | 20220125233612.46831-1-chooglen@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | sequencer.c: set GIT_WORK_TREE in run_git_checkout | expand |
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.
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.
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.
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
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
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(+) base-commit: bc3ae46b420f58dfe2bfd87714dca096a043d554