{"thread":{"id":"57312","subject":"[PATCH] sequencer, stash: fix running from worktree subdir","startedAt":"2022-01-26T01:43:50Z","lastAt":"2022-01-26T20:01:33Z","messageCount":3,"participants":["Elijah Newren via GitGitGadget","Glen Choo","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"446910","messageId":"pull.1205.git.git.1643161426138.gitgitgadget@gmail.com","threadId":"57312","inReplyTo":null,"subject":"[PATCH] sequencer, stash: fix running from worktree subdir","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-01-26T01:43:45Z","receivedAt":"2022-01-26T01:43:50Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren <newren@gmail.com>\n\nIn commits bc3ae46b42 (\"rebase: do not attempt to remove\nstartup_info->original_cwd\", 2021-12-09) and 0fce211ccc (\"stash: do not\nattempt to remove startup_info->original_cwd\", 2021-12-09), we wanted to\nallow the 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 the subcommand being very confused about\nthe working tree.  Fix it by also setting the GIT_WORK_TREE environment\nvariable 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\nTest-case-by: Glen Choo <chooglen@google.com>\nSigned-off-by: Elijah Newren <newren@gmail.com>\n---\n    [2.35 regression] sequencer, stash: fix running from worktree subdir\n    \n    Sorry for the regression in Git 2.35; thanks for the report Glen (and\n    Eric for feedback on earlier patches we've been bouncing back and forth\n    that I've incorporated here)\n    \n    Previous discussion thread starting over here:\n    https://lore.kernel.org/git/kl6lilu71rzl.fsf@chooglen-macbookpro.roam.corp.google.com/\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1205%2Fnewren%2Ffix-rebase-subdir-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1205/newren/fix-rebase-subdir-v1\nPull-Request: https://github.com/git/git/pull/1205\n\n builtin/stash.c   |  6 +++++-\n sequencer.c       |  5 ++++-\n t/t3400-rebase.sh | 21 +++++++++++++++++++++\n 3 files changed, 30 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/stash.c b/builtin/stash.c\nindex 1ef2017c595..86cd0b456e7 100644\n--- a/builtin/stash.c\n+++ b/builtin/stash.c\n@@ -1539,8 +1539,12 @@ static int do_push_stash(const struct pathspec *ps, const char *stash_msg, int q\n \t\t\tstruct child_process cp = CHILD_PROCESS_INIT;\n \n \t\t\tcp.git_cmd = 1;\n-\t\t\tif (startup_info->original_cwd)\n+\t\t\tif (startup_info->original_cwd) {\n \t\t\t\tcp.dir = startup_info->original_cwd;\n+\t\t\t\tstrvec_pushf(&cp.env_array, \"%s=%s\",\n+\t\t\t\t\t     GIT_WORK_TREE_ENVIRONMENT,\n+\t\t\t\t\t     the_repository->worktree);\n+\t\t\t}\n \t\t\tstrvec_pushl(&cp.args, \"clean\", \"--force\",\n \t\t\t\t     \"--quiet\", \"-d\", \":/\", NULL);\n \t\t\tif (include_untracked == INCLUDE_ALL_FILES)\ndiff --git a/sequencer.c b/sequencer.c\nindex 6abd72160cc..5213d16e971 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -4223,8 +4223,11 @@ static int run_git_checkout(struct repository *r, struct replay_opts *opts,\n \n \tcmd.git_cmd = 1;\n \n-\tif (startup_info->original_cwd)\n+\tif (startup_info->original_cwd) {\n \t\tcmd.dir = startup_info->original_cwd;\n+\t\tstrvec_pushf(&cmd.env_array, \"%s=%s\",\n+\t\t\t     GIT_WORK_TREE_ENVIRONMENT, r->worktree);\n+\t}\n \tstrvec_push(&cmd.args, \"checkout\");\n \tstrvec_push(&cmd.args, commit);\n \tstrvec_pushf(&cmd.env_array, GIT_REFLOG_ACTION \"=%s\", action);\ndiff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh\nindex 23dbd3c82ed..71b1735e1dd 100755\n--- a/t/t3400-rebase.sh\n+++ b/t/t3400-rebase.sh\n@@ -416,4 +416,25 @@ 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\tmkdir -p foo/bar &&\n+\t\ttest_commit foo/bar/baz &&\n+\t\tmkdir -p a/b &&\n+\t\ttest_commit 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\tcd ../other-wt &&\n+\t\t# create and cd into a subdirectory\n+\t\tmkdir -p random/dir &&\n+\t\tcd random/dir &&\n+\t\t# now do the rebase\n+\t\tgit rebase --onto HEAD^^ HEAD^  # drops the HEAD^ commit\n+\t)\n+'\n+\n test_done\n\nbase-commit: 89bece5c8c96f0b962cfc89e63f82d603fd60bed\n-- \ngitgitgadget\n"},{"id":"446976","messageId":"kl6l35la1jka.fsf@chooglen-macbookpro.roam.corp.google.com","threadId":"57312","inReplyTo":"pull.1205.git.git.1643161426138.gitgitgadget@gmail.com","subject":"Re: [PATCH] sequencer, stash: fix running from worktree subdir","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2022-01-26T17:41:09Z","receivedAt":"2022-01-26T17:41:25Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"\"Elijah Newren via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> From: Elijah Newren <newren@gmail.com>\n>\n> In commits bc3ae46b42 (\"rebase: do not attempt to remove\n> startup_info->original_cwd\", 2021-12-09) and 0fce211ccc (\"stash: do not\n> attempt to remove startup_info->original_cwd\", 2021-12-09), we wanted to\n> allow the 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 the subcommand being very confused about\n> the working tree.  Fix it by also setting the GIT_WORK_TREE environment\n> 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\nAs the commit message explains, GIT_DIR and GIT_WORK_TREE are closely\nlinked, and this interaction is subtle enough that we'd want to guard\nagainst it instead of policing it manually. So, yes, setting them\ntogether makes a lot of sense.\n\n>  builtin/stash.c   |  6 +++++-\n>  sequencer.c       |  5 ++++-\n>  t/t3400-rebase.sh | 21 +++++++++++++++++++++\n>  3 files changed, 30 insertions(+), 2 deletions(-)\n>\n> diff --git a/builtin/stash.c b/builtin/stash.c\n> index 1ef2017c595..86cd0b456e7 100644\n> --- a/builtin/stash.c\n> +++ b/builtin/stash.c\n> @@ -1539,8 +1539,12 @@ static int do_push_stash(const struct pathspec *ps, const char *stash_msg, int q\n>  \t\t\tstruct child_process cp = CHILD_PROCESS_INIT;\n>  \n>  \t\t\tcp.git_cmd = 1;\n> -\t\t\tif (startup_info->original_cwd)\n> +\t\t\tif (startup_info->original_cwd) {\n>  \t\t\t\tcp.dir = startup_info->original_cwd;\n> +\t\t\t\tstrvec_pushf(&cp.env_array, \"%s=%s\",\n> +\t\t\t\t\t     GIT_WORK_TREE_ENVIRONMENT,\n> +\t\t\t\t\t     the_repository->worktree);\n> +\t\t\t}\n>  \t\t\tstrvec_pushl(&cp.args, \"clean\", \"--force\",\n>  \t\t\t\t     \"--quiet\", \"-d\", \":/\", NULL);\n>  \t\t\tif (include_untracked == INCLUDE_ALL_FILES)\n> diff --git a/sequencer.c b/sequencer.c\n> index 6abd72160cc..5213d16e971 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -4223,8 +4223,11 @@ static int run_git_checkout(struct repository *r, struct replay_opts *opts,\n>  \n>  \tcmd.git_cmd = 1;\n>  \n> -\tif (startup_info->original_cwd)\n> +\tif (startup_info->original_cwd) {\n>  \t\tcmd.dir = startup_info->original_cwd;\n> +\t\tstrvec_pushf(&cmd.env_array, \"%s=%s\",\n> +\t\t\t     GIT_WORK_TREE_ENVIRONMENT, r->worktree);\n> +\t}\n>  \tstrvec_push(&cmd.args, \"checkout\");\n>  \tstrvec_push(&cmd.args, commit);\n>  \tstrvec_pushf(&cmd.env_array, GIT_REFLOG_ACTION \"=%s\", action);\n> diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh\n> index 23dbd3c82ed..71b1735e1dd 100755\n> --- a/t/t3400-rebase.sh\n> +++ b/t/t3400-rebase.sh\n> @@ -416,4 +416,25 @@ 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\tmkdir -p foo/bar &&\n> +\t\ttest_commit foo/bar/baz &&\n> +\t\tmkdir -p a/b &&\n> +\t\ttest_commit 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\tcd ../other-wt &&\n> +\t\t# create and cd into a subdirectory\n> +\t\tmkdir -p random/dir &&\n> +\t\tcd random/dir &&\n> +\t\t# now do the rebase\n> +\t\tgit rebase --onto HEAD^^ HEAD^  # drops the HEAD^ commit\n> +\t)\n> +'\n> +\n>  test_done\n\nLooks good :)\n\nReviewed-by: Glen Choo <chooglen@google.com>\n"},{"id":"446988","messageId":"xmqqfspaqnaf.fsf@gitster.g","threadId":"57312","inReplyTo":"pull.1205.git.git.1643161426138.gitgitgadget@gmail.com","subject":"Re: [PATCH] sequencer, stash: fix running from worktree subdir","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-01-26T20:01:28Z","receivedAt":"2022-01-26T20:01:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Elijah Newren via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> From: Elijah Newren <newren@gmail.com>\n>\n> In commits bc3ae46b42 (\"rebase: do not attempt to remove\n> startup_info->original_cwd\", 2021-12-09) and 0fce211ccc (\"stash: do not\n> attempt to remove startup_info->original_cwd\", 2021-12-09), we wanted to\n> allow the 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 the subcommand being very confused about\n> the working tree.  Fix it by also setting the GIT_WORK_TREE environment\n> 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> Test-case-by: Glen Choo <chooglen@google.com>\n> Signed-off-by: Elijah Newren <newren@gmail.com>\n> ---\n>     [2.35 regression] sequencer, stash: fix running from worktree subdir\n\nThanks.  This is the kind of fix we all should be concentrating on\nin the first week after the release.\n\nVery much appreciated.\n\nWill queue.\n\n> diff --git a/builtin/stash.c b/builtin/stash.c\n> index 1ef2017c595..86cd0b456e7 100644\n> --- a/builtin/stash.c\n> +++ b/builtin/stash.c\n> @@ -1539,8 +1539,12 @@ static int do_push_stash(const struct pathspec *ps, const char *stash_msg, int q\n>  \t\t\tstruct child_process cp = CHILD_PROCESS_INIT;\n>  \n>  \t\t\tcp.git_cmd = 1;\n> -\t\t\tif (startup_info->original_cwd)\n> +\t\t\tif (startup_info->original_cwd) {\n>  \t\t\t\tcp.dir = startup_info->original_cwd;\n> +\t\t\t\tstrvec_pushf(&cp.env_array, \"%s=%s\",\n> +\t\t\t\t\t     GIT_WORK_TREE_ENVIRONMENT,\n> +\t\t\t\t\t     the_repository->worktree);\n> +\t\t\t}\n>  \t\t\tstrvec_pushl(&cp.args, \"clean\", \"--force\",\n>  \t\t\t\t     \"--quiet\", \"-d\", \":/\", NULL);\n>  \t\t\tif (include_untracked == INCLUDE_ALL_FILES)\n> diff --git a/sequencer.c b/sequencer.c\n> index 6abd72160cc..5213d16e971 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -4223,8 +4223,11 @@ static int run_git_checkout(struct repository *r, struct replay_opts *opts,\n>  \n>  \tcmd.git_cmd = 1;\n>  \n> -\tif (startup_info->original_cwd)\n> +\tif (startup_info->original_cwd) {\n>  \t\tcmd.dir = startup_info->original_cwd;\n> +\t\tstrvec_pushf(&cmd.env_array, \"%s=%s\",\n> +\t\t\t     GIT_WORK_TREE_ENVIRONMENT, r->worktree);\n> +\t}\n>  \tstrvec_push(&cmd.args, \"checkout\");\n>  \tstrvec_push(&cmd.args, commit);\n>  \tstrvec_pushf(&cmd.env_array, GIT_REFLOG_ACTION \"=%s\", action);\n> diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh\n> index 23dbd3c82ed..71b1735e1dd 100755\n> --- a/t/t3400-rebase.sh\n> +++ b/t/t3400-rebase.sh\n> @@ -416,4 +416,25 @@ 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\tmkdir -p foo/bar &&\n> +\t\ttest_commit foo/bar/baz &&\n> +\t\tmkdir -p a/b &&\n> +\t\ttest_commit 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\tcd ../other-wt &&\n> +\t\t# create and cd into a subdirectory\n> +\t\tmkdir -p random/dir &&\n> +\t\tcd random/dir &&\n> +\t\t# now do the rebase\n> +\t\tgit rebase --onto HEAD^^ HEAD^  # drops the HEAD^ commit\n> +\t)\n> +'\n> +\n>  test_done\n>\n> base-commit: 89bece5c8c96f0b962cfc89e63f82d603fd60bed\n"}]}