threads / patch / 57312

patchsequencer, stash: fix running from worktree subdir

Subject: [PATCH] sequencer, stash: fix running from worktree subdir

## tl;dr

3 messages between Jan 26, 2022 and Jan 26, 2022. Diffs are folded; open one to read it.

replies: 2people: 3as markdown or json

Elijah Newren via GitGitGadget· Jan 26, 2022, 01:43 UTC · lore

From: Elijah Newren <newren@gmail.com>

In commits bc3ae46b42 ("rebase: do not attempt to remove startup_info->original_cwd", 2021-12-09) and 0fce211ccc ("stash: do not attempt to remove startup_info->original_cwd", 2021-12-09), we wanted to allow the 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 the subcommand being very confused about the working tree. 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.

Test-case-by: Glen Choo <chooglen@google.com>
Signed-off-by: Elijah Newren <newren@gmail.com>
---
    [2.35 regression] sequencer, stash: fix running from worktree subdir
    
    Sorry for the regression in Git 2.35; thanks for the report Glen (and
    Eric for feedback on earlier patches we've been bouncing back and forth
    that I've incorporated here)
    
    Previous discussion thread starting over here:
    https://lore.kernel.org/git/kl6lilu71rzl.fsf@chooglen-macbookpro.roam.corp.google.com/

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1205%2Fnewren%2Ffix-rebase-subdir-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1205/newren/fix-rebase-subdir-v1
Pull-Request: https://github.com/git/git/pull/1205

 builtin/stash.c   |  6 +++++-
 sequencer.c       |  5 ++++-
 t/t3400-rebase.sh | 21 +++++++++++++++++++++
 3 files changed, 30 insertions(+), 2 deletions(-)

Show changes to 3 files +30 −2

builtin/stash.c, sequencer.c, t/t3400-rebase.sh

diff --git a/builtin/stash.c b/builtin/stash.c
index 1ef2017c595..86cd0b456e7 100644
--- a/builtin/stash.c
+++ b/builtin/stash.c
@@ -1539,8 +1539,12 @@ static int do_push_stash(const struct pathspec *ps, const char *stash_msg, int q
 			struct child_process cp = CHILD_PROCESS_INIT;
 
 			cp.git_cmd = 1;
-			if (startup_info->original_cwd)
+			if (startup_info->original_cwd) {
 				cp.dir = startup_info->original_cwd;
+				strvec_pushf(&cp.env_array, "%s=%s",
+					     GIT_WORK_TREE_ENVIRONMENT,
+					     the_repository->worktree);
+			}
 			strvec_pushl(&cp.args, "clean", "--force",
 				     "--quiet", "-d", ":/", NULL);
 			if (include_untracked == INCLUDE_ALL_FILES)
diff --git a/sequencer.c b/sequencer.c
index 6abd72160cc..5213d16e971 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -4223,8 +4223,11 @@ static int run_git_checkout(struct repository *r, struct replay_opts *opts,
 
 	cmd.git_cmd = 1;
 
-	if (startup_info->original_cwd)
+	if (startup_info->original_cwd) {
 		cmd.dir = startup_info->original_cwd;
+		strvec_pushf(&cmd.env_array, "%s=%s",
+			     GIT_WORK_TREE_ENVIRONMENT, r->worktree);
+	}
 	strvec_push(&cmd.args, "checkout");
 	strvec_push(&cmd.args, commit);
 	strvec_pushf(&cmd.env_array, GIT_REFLOG_ACTION "=%s", action);
diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh
index 23dbd3c82ed..71b1735e1dd 100755
--- a/t/t3400-rebase.sh
+++ b/t/t3400-rebase.sh
@@ -416,4 +416,25 @@ 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" &&
+		mkdir -p foo/bar &&
+		test_commit foo/bar/baz &&
+		mkdir -p a/b &&
+		test_commit a/b/c &&
+		# create another branch for our other worktree
+		git branch other &&
+		git worktree add ../other-wt other &&
+		cd ../other-wt &&
+		# create and cd into a subdirectory
+		mkdir -p random/dir &&
+		cd random/dir &&
+		# now do the rebase
+		git rebase --onto HEAD^^ HEAD^  # drops the HEAD^ commit
+	)
+'
+
 test_done

base-commit: 89bece5c8c96f0b962cfc89e63f82d603fd60bed

-- 
gitgitgadget

Glen Choo· Jan 26, 2022, 17:41 UTC · re: Elijah Newren via GitGitGadget · lore

Re: [PATCH] sequencer, stash: fix running from worktree subdir

"Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:

Show 26 quoted lines

> From: Elijah Newren <newren@gmail.com>
>
> In commits bc3ae46b42 ("rebase: do not attempt to remove
> startup_info->original_cwd", 2021-12-09) and 0fce211ccc ("stash: do not
> attempt to remove startup_info->original_cwd", 2021-12-09), we wanted to
> allow the 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 the subcommand being very confused about
> the working tree.  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.

As the commit message explains, GIT_DIR and GIT_WORK_TREE are closely linked, and this interaction is subtle enough that we'd want to guard against it instead of policing it manually. So, yes, setting them together makes a lot of sense.

Show 70 quoted lines

>  builtin/stash.c   |  6 +++++-
>  sequencer.c       |  5 ++++-
>  t/t3400-rebase.sh | 21 +++++++++++++++++++++
>  3 files changed, 30 insertions(+), 2 deletions(-)
>
> diff --git a/builtin/stash.c b/builtin/stash.c
> index 1ef2017c595..86cd0b456e7 100644
> --- a/builtin/stash.c
> +++ b/builtin/stash.c
> @@ -1539,8 +1539,12 @@ static int do_push_stash(const struct pathspec *ps, const char *stash_msg, int q
>  			struct child_process cp = CHILD_PROCESS_INIT;
>  
>  			cp.git_cmd = 1;
> -			if (startup_info->original_cwd)
> +			if (startup_info->original_cwd) {
>  				cp.dir = startup_info->original_cwd;
> +				strvec_pushf(&cp.env_array, "%s=%s",
> +					     GIT_WORK_TREE_ENVIRONMENT,
> +					     the_repository->worktree);
> +			}
>  			strvec_pushl(&cp.args, "clean", "--force",
>  				     "--quiet", "-d", ":/", NULL);
>  			if (include_untracked == INCLUDE_ALL_FILES)
> diff --git a/sequencer.c b/sequencer.c
> index 6abd72160cc..5213d16e971 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -4223,8 +4223,11 @@ static int run_git_checkout(struct repository *r, struct replay_opts *opts,
>  
>  	cmd.git_cmd = 1;
>  
> -	if (startup_info->original_cwd)
> +	if (startup_info->original_cwd) {
>  		cmd.dir = startup_info->original_cwd;
> +		strvec_pushf(&cmd.env_array, "%s=%s",
> +			     GIT_WORK_TREE_ENVIRONMENT, r->worktree);
> +	}
>  	strvec_push(&cmd.args, "checkout");
>  	strvec_push(&cmd.args, commit);
>  	strvec_pushf(&cmd.env_array, GIT_REFLOG_ACTION "=%s", action);
> diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh
> index 23dbd3c82ed..71b1735e1dd 100755
> --- a/t/t3400-rebase.sh
> +++ b/t/t3400-rebase.sh
> @@ -416,4 +416,25 @@ 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" &&
> +		mkdir -p foo/bar &&
> +		test_commit foo/bar/baz &&
> +		mkdir -p a/b &&
> +		test_commit a/b/c &&
> +		# create another branch for our other worktree
> +		git branch other &&
> +		git worktree add ../other-wt other &&
> +		cd ../other-wt &&
> +		# create and cd into a subdirectory
> +		mkdir -p random/dir &&
> +		cd random/dir &&
> +		# now do the rebase
> +		git rebase --onto HEAD^^ HEAD^  # drops the HEAD^ commit
> +	)
> +'
> +
>  test_done

Looks good :)

Reviewed-by: Glen Choo <chooglen@google.com>

Junio C Hamano· Jan 26, 2022, 20:01 UTC · re: Elijah Newren via GitGitGadget · lore

Re: [PATCH] sequencer, stash: fix running from worktree subdir

"Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:

Show 31 quoted lines

> From: Elijah Newren <newren@gmail.com>
>
> In commits bc3ae46b42 ("rebase: do not attempt to remove
> startup_info->original_cwd", 2021-12-09) and 0fce211ccc ("stash: do not
> attempt to remove startup_info->original_cwd", 2021-12-09), we wanted to
> allow the 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 the subcommand being very confused about
> the working tree.  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.
>
> Test-case-by: Glen Choo <chooglen@google.com>
> Signed-off-by: Elijah Newren <newren@gmail.com>
> ---
>     [2.35 regression] sequencer, stash: fix running from worktree subdir

Thanks. This is the kind of fix we all should be concentrating on in the first week after the release.

Very much appreciated.

Will queue.

Show 67 quoted lines

> diff --git a/builtin/stash.c b/builtin/stash.c
> index 1ef2017c595..86cd0b456e7 100644
> --- a/builtin/stash.c
> +++ b/builtin/stash.c
> @@ -1539,8 +1539,12 @@ static int do_push_stash(const struct pathspec *ps, const char *stash_msg, int q
>  			struct child_process cp = CHILD_PROCESS_INIT;
>  
>  			cp.git_cmd = 1;
> -			if (startup_info->original_cwd)
> +			if (startup_info->original_cwd) {
>  				cp.dir = startup_info->original_cwd;
> +				strvec_pushf(&cp.env_array, "%s=%s",
> +					     GIT_WORK_TREE_ENVIRONMENT,
> +					     the_repository->worktree);
> +			}
>  			strvec_pushl(&cp.args, "clean", "--force",
>  				     "--quiet", "-d", ":/", NULL);
>  			if (include_untracked == INCLUDE_ALL_FILES)
> diff --git a/sequencer.c b/sequencer.c
> index 6abd72160cc..5213d16e971 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -4223,8 +4223,11 @@ static int run_git_checkout(struct repository *r, struct replay_opts *opts,
>  
>  	cmd.git_cmd = 1;
>  
> -	if (startup_info->original_cwd)
> +	if (startup_info->original_cwd) {
>  		cmd.dir = startup_info->original_cwd;
> +		strvec_pushf(&cmd.env_array, "%s=%s",
> +			     GIT_WORK_TREE_ENVIRONMENT, r->worktree);
> +	}
>  	strvec_push(&cmd.args, "checkout");
>  	strvec_push(&cmd.args, commit);
>  	strvec_pushf(&cmd.env_array, GIT_REFLOG_ACTION "=%s", action);
> diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh
> index 23dbd3c82ed..71b1735e1dd 100755
> --- a/t/t3400-rebase.sh
> +++ b/t/t3400-rebase.sh
> @@ -416,4 +416,25 @@ 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" &&
> +		mkdir -p foo/bar &&
> +		test_commit foo/bar/baz &&
> +		mkdir -p a/b &&
> +		test_commit a/b/c &&
> +		# create another branch for our other worktree
> +		git branch other &&
> +		git worktree add ../other-wt other &&
> +		cd ../other-wt &&
> +		# create and cd into a subdirectory
> +		mkdir -p random/dir &&
> +		cd random/dir &&
> +		# now do the rebase
> +		git rebase --onto HEAD^^ HEAD^  # drops the HEAD^ commit
> +	)
> +'
> +
>  test_done
>
> base-commit: 89bece5c8c96f0b962cfc89e63f82d603fd60bed