Subject: Re: [PATCH 2/7] stash: integrate with sparse index

From: Derrick Stolee <[email-masked]>
Date: Tue, 26 Apr 2022 08:53:52 -0400

On 4/25/2022 1:49 PM, Victoria Dye via GitGitGadget wrote:
> From: Victoria Dye <[email-masked]>
> 
> Enable sparse index in 'git stash' by disabling
> 'command_requires_full_index'.

>  ensure_not_expanded () {
>  	rm -f trace2.txt &&
> -	echo >>sparse-index/untracked.txt &&
> +	if test -z $WITHOUT_UNTRACKED_TXT

Do we need quotes around "$WITHOUT_UNTRACKED_TXT"?

I mean, I suppose we don't since the tests pass when this variable
is unset, but I think it is a good practice to be careful about
empty variables. Or am I wrong?

> +	then
> +		echo >>sparse-index/untracked.txt
> +	fi &&

> -	ensure_not_expanded checkout-index -f a &&
> -	ensure_not_expanded checkout-index -f --all &&
> -	for ref in update-deep update-folder1 update-folder2 update-deep
> -	do
> -		echo >>sparse-index/README.md &&
> -		ensure_not_expanded reset --hard $ref || return 1
> -	done &&
> -

Moving these to be within the stash tests is interesting.

> +test_expect_success 'sparse-index is not expanded: stash' '
> +	init_repos &&
> +
> +	echo >>sparse-index/a &&
> +	ensure_not_expanded stash &&
> +	ensure_not_expanded stash list &&
> +	ensure_not_expanded stash show stash@{0} &&
> +	! ensure_not_expanded stash apply stash@{0} &&
> +	ensure_not_expanded stash drop stash@{0} &&
> +
> +	echo >>sparse-index/deep/new &&
> +	! ensure_not_expanded stash -u &&
> +	(
> +		WITHOUT_UNTRACKED_TXT=1 &&
> +		! ensure_not_expanded stash pop
> +	) &&
> +
> +	ensure_not_expanded stash create &&
> +	oid=$(git -C sparse-index stash create) &&
> +	ensure_not_expanded stash store -m "test" $oid &&
> +	ensure_not_expanded reset --hard &&
> +	! ensure_not_expanded stash pop &&
> +
> +	ensure_not_expanded checkout-index -f a &&
> +	ensure_not_expanded checkout-index -f --all &&
> +	for ref in update-deep update-folder1 update-folder2 update-deep
> +	do
> +		echo >>sparse-index/README.md &&
> +		ensure_not_expanded reset --hard $ref || return 1
> +	done

It is not obvious why that's necessary here. Perhaps a later
change will build upon these checkout-index commands within
this test?

Thanks,
-Stolee