{"thread":{"id":"57311","subject":"[PATCH] sequencer.c: set GIT_WORK_TREE in run_git_checkout","startedAt":"2022-01-25T23:36:30Z","lastAt":"2022-01-26T00:22:30Z","messageCount":5,"participants":["Glen Choo","Elijah Newren"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"446895","messageId":"20220125233612.46831-1-chooglen@google.com","threadId":"57311","inReplyTo":null,"subject":"[PATCH] sequencer.c: set GIT_WORK_TREE in run_git_checkout","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2022-01-25T23:36:12Z","receivedAt":"2022-01-25T23:36:30Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"When rebasing in a subdirectory of a worktree, Git fails due to a dirty\nworktree:\n\n  error: The following untracked working tree files would be overwritten\nby merge:\n          a/b/c\n  Please move or remove them before you merge.\n\nThis occurs because \"git rebase\" invokes a \"git checkout\" without\npropagating the GIT_WORK_TREE environment variable, causing the worktree\nto be assumed to be \".\". This was not an issue until bc3ae46b42 (rebase:\ndo not attempt to remove startup_info->original_cwd, 2021-12-09), when\nthe .dir of the \"git checkout\" child process was changed such that it no\nlonger runs at the root of the worktree.\n\nPropagate GIT_WORK_TREE to the \"git checkout\" child process and test\nthat rebase in a subdirectory of a worktree succeeds.\n\nSigned-off-by: Glen Choo <chooglen@google.com>\n---\nHere is a fix for the bug I found in [1]. I didn't look through the rest\nof the \"preserve cwd\" thread for other possible bugs pertaining to\nworktrees, but I didn't find any during a cursory glance at sequencer.c.\n\nI'm also not sure if this is the idiomatic way to do it since, I assume,\nwe'd always want to propagate GIT_WORK_TREE, but this is identical to\nhow do_exec() sets GIT_WORK_TREE in the same file (perhaps this is\nsomething that should be refactored).\n\n[1] https://lore.kernel.org/git/kl6lilu71rzl.fsf@chooglen-macbookpro.roam.corp.google.com\n\n sequencer.c       |  2 ++\n t/t3400-rebase.sh | 29 +++++++++++++++++++++++++++++\n 2 files changed, 31 insertions(+)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 83f257e7fa..e193dcf846 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -4233,6 +4233,8 @@ static int run_git_checkout(struct repository *r, struct replay_opts *opts,\n \tstrvec_push(&cmd.args, \"checkout\");\n \tstrvec_push(&cmd.args, commit);\n \tstrvec_pushf(&cmd.env_array, GIT_REFLOG_ACTION \"=%s\", action);\n+\tstrvec_pushf(&cmd.env_array, \"GIT_WORK_TREE=%s\",\n+\t\t     absolute_path(get_git_work_tree()));\n \n \tif (opts->verbose)\n \t\tret = run_command(&cmd);\ndiff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh\nindex 23dbd3c82e..8b8b66538b 100755\n--- a/t/t3400-rebase.sh\n+++ b/t/t3400-rebase.sh\n@@ -416,4 +416,33 @@ test_expect_success MINGW,SYMLINKS_WINDOWS 'rebase when .git/logs is a symlink'\n \tmv actual_logs .git/logs\n '\n \n+test_expect_success 'rebase when inside worktree subdirectory' '\n+\tgit init main-wt &&\n+\t(\n+\t\tcd main-wt &&\n+\t\tgit commit --allow-empty -m \"initial\" &&\n+\t\t# create commit with foo/bar/baz\n+\t\tmkdir -p foo/bar &&\n+\t\ttouch foo/bar/baz &&\n+\t\tgit add foo/bar/baz &&\n+\t\tgit commit -m \"add foo/bar/baz\" &&\n+\t\t# create commit with a/b/c\n+\t\tmkdir -p a/b &&\n+\t\ttouch a/b/c &&\n+\t\tgit add a/b/c &&\n+\t\tgit commit -m \"add a/b/c\" &&\n+\t\t# create another branch for our other worktree\n+\t\tgit branch other &&\n+\t\tgit worktree add ../other-wt other &&\n+\t\t(\n+\t\t\tcd ../other-wt &&\n+\t\t\tmkdir -p random/dir &&\n+\t\t\t(\n+\t\t\t\tcd random/dir &&\n+\t\t\t\tgit rebase --onto HEAD^^ HEAD^  # drops the HEAD^ commit\n+\t\t\t)\n+\t\t)\n+\t)\n+'\n+\n test_done\n\nbase-commit: bc3ae46b420f58dfe2bfd87714dca096a043d554\n-- \n2.33.GIT\n\n"},{"id":"446896","messageId":"kl6lfspb1j23.fsf@chooglen-macbookpro.roam.corp.google.com","threadId":"57311","inReplyTo":"20220125233612.46831-1-chooglen@google.com","subject":"Re: [PATCH] sequencer.c: set GIT_WORK_TREE in run_git_checkout","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2022-01-25T23:39:48Z","receivedAt":"2022-01-25T23:39:51Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"Glen Choo <chooglen@google.com> writes:\n\n> When rebasing in a subdirectory of a worktree, Git fails due to a dirty\n> worktree:\n>\n>   error: The following untracked working tree files would be overwritten\n> by merge:\n>           a/b/c\n>   Please move or remove them before you merge.\n>\n> This occurs because \"git rebase\" invokes a \"git checkout\" without\n> propagating the GIT_WORK_TREE environment variable, causing the worktree\n> to be assumed to be \".\". This was not an issue until bc3ae46b42 (rebase:\n> do not attempt to remove startup_info->original_cwd, 2021-12-09), when\n> the .dir of the \"git checkout\" child process was changed such that it no\n> longer runs at the root of the worktree.\n>\n> Propagate GIT_WORK_TREE to the \"git checkout\" child process and test\n> that rebase in a subdirectory of a worktree succeeds.\n>\n> Signed-off-by: Glen Choo <chooglen@google.com>\n> ---\n> Here is a fix for the bug I found in [1]. I didn't look through the rest\n> of the \"preserve cwd\" thread for other possible bugs pertaining to\n> worktrees, but I didn't find any during a cursory glance at sequencer.c.\n>\n> I'm also not sure if this is the idiomatic way to do it since, I assume,\n> we'd always want to propagate GIT_WORK_TREE, but this is identical to\n> how do_exec() sets GIT_WORK_TREE in the same file (perhaps this is\n> something that should be refactored).\n>\n> [1] https://lore.kernel.org/git/kl6lilu71rzl.fsf@chooglen-macbookpro.roam.corp.google.com\n\nI forgot to add this to the previous email:\n\ncc-ing Junio because this bug is in master.\n"},{"id":"446899","messageId":"CABPp-BGkpjD=WFSFSY9g85MAZ2JUh+UCfi6NThevZ4RxZUxtVw@mail.gmail.com","threadId":"57311","inReplyTo":"20220125233612.46831-1-chooglen@google.com","subject":"Re: [PATCH] sequencer.c: set GIT_WORK_TREE in run_git_checkout","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2022-01-26T00:12:39Z","receivedAt":"2022-01-26T00:13:06Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"Looks like you beat me to sending out a patch.\n\nOn Tue, Jan 25, 2022 at 3:36 PM Glen Choo <chooglen@google.com> wrote:\n>\n> When rebasing in a subdirectory of a worktree, Git fails due to a dirty\n> worktree:\n>\n>   error: The following untracked working tree files would be overwritten\n> by merge:\n>           a/b/c\n>   Please move or remove them before you merge.\n>\n> This occurs because \"git rebase\" invokes a \"git checkout\" without\n> propagating the GIT_WORK_TREE environment variable, causing the worktree\n> to be assumed to be \".\". This was not an issue until bc3ae46b42 (rebase:\n> do not attempt to remove startup_info->original_cwd, 2021-12-09), when\n> the .dir of the \"git checkout\" child process was changed such that it no\n> longer runs at the root of the worktree.\n>\n> Propagate GIT_WORK_TREE to the \"git checkout\" child process and test\n> that rebase in a subdirectory of a worktree succeeds.\n\nThis is correct, but leaves the reader wondering why the failure to\nset GIT_WORK_TREE fails when in a non-primary worktree, but does not\nfail in the primary working tree.  Here's how I described it in the\npatch I was about to send to the list:\n\n\"\"\"\nIn commit bc3ae46 (\"rebase: do not attempt to remove\nstartup_info->original_cwd\", 2021-12-09), we wanted to allow the\ncheckout subprocess to know which directory the parent process was\nrunning from, so that the subprocess could protect it.  However...\n\nWhen run from a non-main worktree, setup_git_directory() will note\nthat the discovered git directory\n(/PATH/TO/.git/worktree/non-main-worktree) does not match\nDEFAULT_GIT_DIR_ENVIRONMENT (see setup_discovered_git_dir()), and\ndecide to set GIT_DIR in the environment.  This matters because...\n\nWhenever git is run with the GIT_DIR environment variable set, and\nGIT_WORK_TREE not set, it presumes that '.' is the working tree.  So...\n\nThis combination results in rebase failing when run from a subdirectory\nof a non-main worktree.  Fix it by also setting the GIT_WORK_TREE\nenvironment variable along with setting cmd.dir.\n\nA possibly more involved fix we could consider for later would be to\nmake setup.c set GIT_WORK_TREE whenever (a) it discovers both the git\ndirectory and the working tree and (b) it decides to set GIT_DIR in the\nenvironment.  I did not attempt that here as such would be too big of a\nchange for a 2.35.1 release.\n\"\"\"\n\n(Another fix would also be making sequencer stop forking a \"git\ncheckout\" subprocess; it should be able to call a library function,\nand then quit dropping and re-reading the index.  But that's also a\nmuch bigger change...)\n\n> Signed-off-by: Glen Choo <chooglen@google.com>\n> ---\n> Here is a fix for the bug I found in [1]. I didn't look through the rest\n> of the \"preserve cwd\" thread for other possible bugs pertaining to\n> worktrees, but I didn't find any during a cursory glance at sequencer.c.\n\nWe'll need the same fix for stash due to 0fce211ccc (\"stash: do not\nattempt to remove startup_info->original_cwd\", 2021-12-09) -- in that\ncase, though, it's calling git-clean rather than git-checkout.  But\nthe same idea.\n\n> I'm also not sure if this is the idiomatic way to do it since, I assume,\n> we'd always want to propagate GIT_WORK_TREE, but this is identical to\n> how do_exec() sets GIT_WORK_TREE in the same file (perhaps this is\n> something that should be refactored).\n\nI'd rather only set GIT_WORK_TREE if something is also setting\nGIT_DIR, but I don't think it hurts to set it in all cases.\n\n> [1] https://lore.kernel.org/git/kl6lilu71rzl.fsf@chooglen-macbookpro.roam.corp.google.com\n>\n>  sequencer.c       |  2 ++\n>  t/t3400-rebase.sh | 29 +++++++++++++++++++++++++++++\n>  2 files changed, 31 insertions(+)\n>\n> diff --git a/sequencer.c b/sequencer.c\n> index 83f257e7fa..e193dcf846 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -4233,6 +4233,8 @@ static int run_git_checkout(struct repository *r, struct replay_opts *opts,\n>         strvec_push(&cmd.args, \"checkout\");\n>         strvec_push(&cmd.args, commit);\n>         strvec_pushf(&cmd.env_array, GIT_REFLOG_ACTION \"=%s\", action);\n> +       strvec_pushf(&cmd.env_array, \"GIT_WORK_TREE=%s\",\n> +                    absolute_path(get_git_work_tree()));\n\nstruct repository *r already contains the path to the worktree in\nr->worktree; I think it'd be better to use it.\n\n>         if (opts->verbose)\n>                 ret = run_command(&cmd);\n> diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh\n> index 23dbd3c82e..8b8b66538b 100755\n> --- a/t/t3400-rebase.sh\n> +++ b/t/t3400-rebase.sh\n> @@ -416,4 +416,33 @@ test_expect_success MINGW,SYMLINKS_WINDOWS 'rebase when .git/logs is a symlink'\n>         mv actual_logs .git/logs\n>  '\n>\n> +test_expect_success 'rebase when inside worktree subdirectory' '\n> +       git init main-wt &&\n> +       (\n> +               cd main-wt &&\n> +               git commit --allow-empty -m \"initial\" &&\n> +               # create commit with foo/bar/baz\n> +               mkdir -p foo/bar &&\n> +               touch foo/bar/baz &&\n> +               git add foo/bar/baz &&\n> +               git commit -m \"add foo/bar/baz\" &&\n> +               # create commit with a/b/c\n> +               mkdir -p a/b &&\n> +               touch a/b/c &&\n> +               git add a/b/c &&\n> +               git commit -m \"add a/b/c\" &&\n> +               # create another branch for our other worktree\n> +               git branch other &&\n> +               git worktree add ../other-wt other &&\n> +               (\n> +                       cd ../other-wt &&\n> +                       mkdir -p random/dir &&\n> +                       (\n> +                               cd random/dir &&\n> +                               git rebase --onto HEAD^^ HEAD^  # drops the HEAD^ commit\n> +                       )\n> +               )\n> +       )\n> +'\n\nNice testcase.  You would have seen potentially even more confusing\nresults had some of these commits merely modified files rather than\nintroducing totally new files.\n"},{"id":"446900","messageId":"CABPp-BEaAu0=EEjDrTqghGdaNOG=nKv+L_b8kP-7mY=KspJZ4Q@mail.gmail.com","threadId":"57311","inReplyTo":"kl6lfspb1j23.fsf@chooglen-macbookpro.roam.corp.google.com","subject":"Re: [PATCH] sequencer.c: set GIT_WORK_TREE in run_git_checkout","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2022-01-26T00:13:22Z","receivedAt":"2022-01-26T00:13:44Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Tue, Jan 25, 2022 at 3:39 PM Glen Choo <chooglen@google.com> wrote:\n>\n...\n> I forgot to add this to the previous email:\n>\n> cc-ing Junio because this bug is in master.\n\n...and is a new regression in 2.35.  :-(\n\nSorry about that.\n"},{"id":"446902","messageId":"kl6lczkf1h32.fsf@chooglen-macbookpro.roam.corp.google.com","threadId":"57311","inReplyTo":"CABPp-BGkpjD=WFSFSY9g85MAZ2JUh+UCfi6NThevZ4RxZUxtVw@mail.gmail.com","subject":"Re: [PATCH] sequencer.c: set GIT_WORK_TREE in run_git_checkout","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2022-01-26T00:22:25Z","receivedAt":"2022-01-26T00:22:30Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"Elijah Newren <newren@gmail.com> writes:\n\n> On Tue, Jan 25, 2022 at 3:36 PM Glen Choo <chooglen@google.com> wrote:\n>>\n>> When rebasing in a subdirectory of a worktree, Git fails due to a dirty\n>> worktree:\n>>\n>>   error: The following untracked working tree files would be overwritten\n>> by merge:\n>>           a/b/c\n>>   Please move or remove them before you merge.\n>>\n>> This occurs because \"git rebase\" invokes a \"git checkout\" without\n>> propagating the GIT_WORK_TREE environment variable, causing the worktree\n>> to be assumed to be \".\". This was not an issue until bc3ae46b42 (rebase:\n>> do not attempt to remove startup_info->original_cwd, 2021-12-09), when\n>> the .dir of the \"git checkout\" child process was changed such that it no\n>> longer runs at the root of the worktree.\n>>\n>> Propagate GIT_WORK_TREE to the \"git checkout\" child process and test\n>> that rebase in a subdirectory of a worktree succeeds.\n>\n> This is correct, but leaves the reader wondering why the failure to\n> set GIT_WORK_TREE fails when in a non-primary worktree, but does not\n> fail in the primary working tree.\n\nAh, that is true. Thanks for digging into it deeper.\n\n> Here's how I described it in the patch I was about to send to the list:\n>\n> \"\"\"\n> In commit bc3ae46 (\"rebase: do not attempt to remove\n> startup_info->original_cwd\", 2021-12-09), we wanted to allow the\n> checkout subprocess to know which directory the parent process was\n> running from, so that the subprocess could protect it.  However...\n>\n> When run from a non-main worktree, setup_git_directory() will note\n> that the discovered git directory\n> (/PATH/TO/.git/worktree/non-main-worktree) does not match\n> DEFAULT_GIT_DIR_ENVIRONMENT (see setup_discovered_git_dir()), and\n> decide to set GIT_DIR in the environment.  This matters because...\n>\n> Whenever git is run with the GIT_DIR environment variable set, and\n> GIT_WORK_TREE not set, it presumes that '.' is the working tree.  So...\n>\n> This combination results in rebase failing when run from a subdirectory\n> of a non-main worktree.  Fix it by also setting the GIT_WORK_TREE\n> environment variable along with setting cmd.dir.\n>\n> A possibly more involved fix we could consider for later would be to\n> make setup.c set GIT_WORK_TREE whenever (a) it discovers both the git\n> directory and the working tree and (b) it decides to set GIT_DIR in the\n> environment.  I did not attempt that here as such would be too big of a\n> change for a 2.35.1 release.\n> \"\"\"\n>\n> (Another fix would also be making sequencer stop forking a \"git\n> checkout\" subprocess; it should be able to call a library function,\n> and then quit dropping and re-reading the index.  But that's also a\n> much bigger change...)\n>\n>> Signed-off-by: Glen Choo <chooglen@google.com>\n>> ---\n>> Here is a fix for the bug I found in [1]. I didn't look through the rest\n>> of the \"preserve cwd\" thread for other possible bugs pertaining to\n>> worktrees, but I didn't find any during a cursory glance at sequencer.c.\n>\n> We'll need the same fix for stash due to 0fce211ccc (\"stash: do not\n> attempt to remove startup_info->original_cwd\", 2021-12-09) -- in that\n> case, though, it's calling git-clean rather than git-checkout.  But\n> the same idea.\n\nLet's use your patch [1] instead of mine :)\n\n[1] https://github.com/git/git/pull/1205\n"}]}