From: Phillip Wood <phillip.wood123@gmail.com>
Date: Tue, 16 Sep 2025 09:18:31 GMT
Subject: Re: [PATCH v2 4/4] stash: honor stash.index in apply, pop modes
Message-ID: <25836bc2-db3a-4761-b13d-c587728f4c3c@gmail.com>
In-Reply-To: <585e124467dcb1ec1de71fa72e749140d44dc367.1757982870.git.ben.knoble+github@gmail.com>

Hi Ben

On 16/09/2025 01:37, D. Ben Knoble wrote:
> With stash.index=true, git-stash(1) command now tries to reinstate the
> index by default in the "apply" and "pop" modes. Not doing so creates a
> common trap [1], [2]: "git stash apply" is not the reverse of "git stash
> push" because carefully staged indices are lost and have to be manually
> recreated. OTOH, this mode is not always desirable and may create more
> conflicts when applying stashes. As usual, "--no-index" will disable
> this behavior if you set "stash.index".

I don't have a strong opinion either way on the new config setting but I 
do think we should rationalize the new tests. Assuming we already have 
good coverage for "git stash pop --index" then all we need to do is 
check that "git -c stash.index=true stash pop", "git -c stash.index=true 
stash pop --no-index" and "git -c stash.index=false stash pop --index". 
We don't need an exhaustive list of tests that check the config setting 
in scenarios like "create twos stashes, drop the second one and apply 
the first". Tests like that add no new coverage for the changes in this 
patch and slow the test suite down.

Thanks

Phillip
> [1]: https://lore.kernel.org/git/CAPx1GvcxyDDQmCssMjEnt6JoV6qPc5ZUpgPLX3mpUC_4PNYA1w@mail.gmail.com/
> [2]: https://lore.kernel.org/git/c5a811ac-8cd3-c389-ac6d-29020a648c87@gmail.com/
> 
> Signed-off-by: D. Ben Knoble <ben.knoble+github@gmail.com>
> ---
>   Documentation/config/stash.adoc    |   5 +
>   builtin/stash.c                    |   9 +-
>   t/t3903-stash.sh                   | 155 +++++++++++++++++++++++++++++
>   t/t3904-stash-patch.sh             |  11 ++
>   t/t3905-stash-include-untracked.sh |  48 ++++++++-
>   5 files changed, 225 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/config/stash.adoc b/Documentation/config/stash.adoc
> index ec1edaeba6..e556105a15 100644
> --- a/Documentation/config/stash.adoc
> +++ b/Documentation/config/stash.adoc
> @@ -1,3 +1,8 @@
> +stash.index::
> +	If this is set to true, `git stash apply` and `git stash pop` will
> +	behave as if `--index` was supplied. Defaults to false. See the
> +	descriptions in linkgit:git-stash[1].
> +
>   stash.showIncludeUntracked::
>   	If this is set to true, the `git stash show` command will show
>   	the untracked files of a stash entry.  Defaults to false. See
> diff --git a/builtin/stash.c b/builtin/stash.c
> index d9b478d1d1..8a0eef3c70 100644
> --- a/builtin/stash.c
> +++ b/builtin/stash.c
> @@ -130,6 +130,7 @@ static struct strbuf stash_index_path = STRBUF_INIT;
>   static int show_stat = 1;
>   static int show_patch;
>   static int show_include_untracked;
> +static int use_index;
>   
>   /*
>    * w_commit is set to the commit containing the working tree
> @@ -662,7 +663,7 @@ static int apply_stash(int argc, const char **argv, const char *prefix,
>   {
>   	int ret = -1;
>   	int quiet = 0;
> -	int index = 0;
> +	int index = use_index;
>   	struct stash_info info = STASH_INFO_INIT;
>   	struct option options[] = {
>   		OPT__QUIET(&quiet, N_("be quiet, only report errors")),
> @@ -759,7 +760,7 @@ static int pop_stash(int argc, const char **argv, const char *prefix,
>   		     struct repository *repo UNUSED)
>   {
>   	int ret = -1;
> -	int index = 0;
> +	int index = use_index;
>   	int quiet = 0;
>   	struct stash_info info = STASH_INFO_INIT;
>   	struct option options[] = {
> @@ -864,6 +865,10 @@ static int git_stash_config(const char *var, const char *value,
>   		show_include_untracked = git_config_bool(var, value);
>   		return 0;
>   	}
> +	if (!strcmp(var, "stash.index")) {
> +		use_index = git_config_bool(var, value);
> +		return 0;
> +	}
>   	return git_diff_basic_config(var, value, ctx, cb);
>   }
>   
> diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
> index b8936a653b..1d53a94165 100755
> --- a/t/t3903-stash.sh
> +++ b/t/t3903-stash.sh
> @@ -111,6 +111,19 @@ setup_stash()
>   	test 1 = $(git show HEAD:file)
>   '
>   
> +test_expect_success 'apply stashed changes with stash.index' '
> +	test_config stash.index true &&
> +	git reset --hard HEAD^ &&
> +	echo 5 >other-file &&
> +	git add other-file &&
> +	test_tick &&
> +	git commit -m other-file &&
> +	git stash apply &&
> +	test 3 = $(cat file) &&
> +	test 2 = $(git show :file) &&
> +	test 1 = $(git show HEAD:file)
> +'
> +
>   test_expect_success 'apply stashed changes (including index)' '
>   	git reset --hard HEAD^ &&
>   	echo 6 >other-file &&
> @@ -150,6 +163,21 @@ setup_stash()
>   	test 1 = $(git show HEAD:file)
>   '
>   
> +test_expect_success 'drop top stash with stash.index' '
> +	test_config stash.index true &&
> +	git reset --hard &&
> +	git stash list >expected &&
> +	echo 7 >file &&
> +	git stash &&
> +	git stash drop &&
> +	git stash list >actual &&
> +	test_cmp expected actual &&
> +	git stash apply &&
> +	test 3 = $(cat file) &&
> +	test 2 = $(git show :file) &&
> +	test 1 = $(git show HEAD:file)
> +'
> +
>   test_expect_success 'drop middle stash' '
>   	git reset --hard &&
>   	echo 8 >file &&
> @@ -170,6 +198,27 @@ setup_stash()
>   	test 1 = $(git show HEAD:file)
>   '
>   
> +test_expect_success 'drop middle stash with stash.index' '
> +	test_config stash.index true &&
> +	git reset --hard &&
> +	echo 8 >file &&
> +	git stash &&
> +	echo 9 >file &&
> +	git stash &&
> +	git stash drop stash@{1} &&
> +	test 2 = $(git stash list | wc -l) &&
> +	git stash apply &&
> +	test 9 = $(cat file) &&
> +	test 1 = $(git show :file) &&
> +	test 1 = $(git show HEAD:file) &&
> +	git reset --hard &&
> +	git stash drop &&
> +	git stash apply &&
> +	test 3 = $(cat file) &&
> +	test 2 = $(git show :file) &&
> +	test 1 = $(git show HEAD:file)
> +'
> +
>   test_expect_success 'drop middle stash by index' '
>   	git reset --hard &&
>   	echo 8 >file &&
> @@ -236,6 +285,17 @@ setup_stash()
>   	test 0 = $(git stash list | wc -l)
>   '
>   
> +test_expect_success 'stash pop with stash.index' '
> +	test_config stash.index true &&
> +	git reset --hard &&
> +	setup_stash &&
> +	git stash pop &&
> +	test 3 = $(cat file) &&
> +	test 2 = $(git show :file) &&
> +	test 1 = $(git show HEAD:file) &&
> +	test 0 = $(git stash list | wc -l)
> +'
> +
>   cat >expect <<EOF
>   diff --git a/file2 b/file2
>   new file mode 100644
> @@ -328,6 +388,22 @@ setup_stash()
>   	test_must_be_empty output.out
>   '
>   
> +test_expect_success 'pop -q works and is quiet with stash.index' '
> +	# Added file, deleted file, modified file all staged for commit
> +	echo foo >new-file &&
> +	echo test >file &&
> +	git add new-file file &&
> +	git rm other-file &&
> +	git stash &&
> +
> +	test_config stash.index true &&
> +	git stash pop -q >output.out 2>&1 &&
> +	echo test >expect &&
> +	git show :file >actual &&
> +	test_cmp expect actual &&
> +	test_must_be_empty output.out
> +'
> +
>   test_expect_success 'pop -q --index works and is quiet' '
>   	echo foo >file &&
>   	git add file &&
> @@ -1178,6 +1254,19 @@ setup_stash()
>   	test_path_is_file bar
>   '
>   
> +test_expect_success 'stash -- <pathspec> stashes and restores the file with stash.index' '
> +	test_config stash.index true &&
> +	>foo &&
> +	>bar &&
> +	git add foo bar &&
> +	git stash push -- foo &&
> +	test_path_is_file bar &&
> +	test_path_is_missing foo &&
> +	git stash pop --no-index &&
> +	test_path_is_file foo &&
> +	test_path_is_file bar
> +'
> +
>   test_expect_success 'stash -- <pathspec> stashes in subdirectory' '
>   	mkdir sub &&
>   	>foo &&
> @@ -1194,6 +1283,24 @@ setup_stash()
>   	test_path_is_file bar
>   '
>   
> +test_expect_success 'stash -- <pathspec> stashes in subdirectory with stash.index' '
> +	test_config stash.index true &&
> +	rm -r sub &&
> +	mkdir sub &&
> +	>foo &&
> +	>bar &&
> +	git add foo bar &&
> +	(
> +		cd sub &&
> +		git stash push -- ../foo
> +	) &&
> +	test_path_is_file bar &&
> +	test_path_is_missing foo &&
> +	git stash pop --no-index &&
> +	test_path_is_file foo &&
> +	test_path_is_file bar
> +'
> +
>   test_expect_success 'stash with multiple pathspec arguments' '
>   	>foo &&
>   	>bar &&
> @@ -1209,6 +1316,22 @@ setup_stash()
>   	test_path_is_file extra
>   '
>   
> +test_expect_success 'stash with multiple pathspec arguments with stash.index' '
> +	test_config stash.index true &&
> +	>foo &&
> +	>bar &&
> +	>extra &&
> +	git add foo bar extra &&
> +	git stash push -- foo bar &&
> +	test_path_is_missing bar &&
> +	test_path_is_missing foo &&
> +	test_path_is_file extra &&
> +	git stash pop --no-index &&
> +	test_path_is_file foo &&
> +	test_path_is_file bar &&
> +	test_path_is_file extra
> +'
> +
>   test_expect_success 'stash with file including $IFS character' '
>   	>"foo bar" &&
>   	>foo &&
> @@ -1224,6 +1347,22 @@ setup_stash()
>   	test_path_is_file bar
>   '
>   
> +test_expect_success 'stash with file including $IFS character with stash.index' '
> +	test_config stash.index true &&
> +	>"foo bar" &&
> +	>foo &&
> +	>bar &&
> +	git add foo* &&
> +	git stash push -- "foo b*" &&
> +	test_path_is_missing "foo bar" &&
> +	test_path_is_file foo &&
> +	test_path_is_file bar &&
> +	git stash pop --no-index &&
> +	test_path_is_file "foo bar" &&
> +	test_path_is_file foo &&
> +	test_path_is_file bar
> +'
> +
>   test_expect_success 'stash with pathspec matching multiple paths' '
>   	echo original >file &&
>   	echo original >other-file &&
> @@ -1312,6 +1451,22 @@ setup_stash()
>   	test_path_is_file bar
>   '
>   
> +test_expect_success 'stash without verb with pathspec with stash.index' '
> +	test_config stash.index true &&
> +	>"foo bar" &&
> +	>foo &&
> +	>bar &&
> +	git add foo* &&
> +	git stash -- "foo b*" &&
> +	test_path_is_missing "foo bar" &&
> +	test_path_is_file foo &&
> +	test_path_is_file bar &&
> +	git stash pop --no-index &&
> +	test_path_is_file "foo bar" &&
> +	test_path_is_file foo &&
> +	test_path_is_file bar
> +'
> +
>   test_expect_success 'stash -k -- <pathspec> leaves unstaged files intact' '
>   	git reset &&
>   	>foo &&
> diff --git a/t/t3904-stash-patch.sh b/t/t3904-stash-patch.sh
> index ae313e3c70..fe402f6ab5 100755
> --- a/t/t3904-stash-patch.sh
> +++ b/t/t3904-stash-patch.sh
> @@ -42,6 +42,17 @@
>   	verify_state dir/foo work head
>   '
>   
> +test_expect_success 'git stash -p with stash.index' '
> +	test_config stash.index true &&
> +	set_state HEAD HEADfile_work HEADfile_index &&
> +	set_state dir/foo work index &&
> +	test_write_lines y n y | git stash save -p &&
> +	git reset --hard &&
> +	git stash apply &&
> +	verify_state HEAD HEADfile_work HEADfile_index &&
> +	verify_state dir/foo head index
> +'
> +
>   test_expect_success 'git stash -p --no-keep-index' '
>   	set_state HEAD HEADfile_work HEADfile_index &&
>   	set_state bar bar_work bar_index &&
> diff --git a/t/t3905-stash-include-untracked.sh b/t/t3905-stash-include-untracked.sh
> index 7704709054..5407f11030 100755
> --- a/t/t3905-stash-include-untracked.sh
> +++ b/t/t3905-stash-include-untracked.sh
> @@ -7,7 +7,7 @@
>   
>   . ./test-lib.sh
>   
> -test_expect_success 'stash save --include-untracked some dirty working directory' '
> +setup() {
>   	echo 1 >file &&
>   	git add file &&
>   	test_tick &&
> @@ -23,6 +23,10 @@
>   	git stash --include-untracked &&
>   	git diff-files --quiet &&
>   	git diff-index --cached --quiet HEAD
> +}
> +
> +test_expect_success 'stash save --include-untracked some dirty working directory' '
> +	setup
>   '
>   
>   test_expect_success 'stash save --include-untracked cleaned the untracked files' '
> @@ -108,6 +112,32 @@
>   	test_cmp untracked_expect untracked/untracked
>   '
>   
> +test_expect_success 'stash pop after save --include-untracked leaves files untracked again with stash.index' '
> +	git init repo &&
> +	test_when_finished rm -r repo &&
> +	(
> +		cd repo &&
> +		git config stash.index true &&
> +		setup &&
> +		cat >expect <<-EOF &&
> +		MM file
> +		?? HEAD
> +		?? actual
> +		?? expect
> +		?? file2
> +		?? untracked/
> +		EOF
> +
> +		git stash pop &&
> +		git status --porcelain >actual &&
> +		test_cmp expect actual &&
> +		echo 1 >expect_file2 &&
> +		test_cmp expect_file2 file2 &&
> +		echo untracked >untracked_expect &&
> +		test_cmp untracked_expect untracked/untracked
> +	)
> +'
> +
>   test_expect_success 'clean up untracked/ directory to prepare for next tests' '
>   	git clean --force --quiet -d
>   '
> @@ -221,6 +251,22 @@
>   	test_path_is_file bar
>   '
>   
> +test_expect_success 'stash push with $IFS character with stash.index' '
> +	test_config stash.index true &&
> +	>"foo bar" &&
> +	>foo &&
> +	>bar &&
> +	git add foo* &&
> +	git stash push --include-untracked -- "foo b*" &&
> +	test_path_is_missing "foo bar" &&
> +	test_path_is_file foo &&
> +	test_path_is_file bar &&
> +	git stash pop --no-index &&
> +	test_path_is_file "foo bar" &&
> +	test_path_is_file foo &&
> +	test_path_is_file bar
> +'
> +
>   test_expect_success 'stash previously ignored file' '
>   	cat >.gitignore <<-EOF &&
>   	ignored