Re: [PATCH v3] stash: avoid recursive hard reset on submodules

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 12, 2019, 01:46 UTC
Message-ID: <xmqqsgnyenwj.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <20191011222448.GA650182@OJAN-PAAVO.localdomain>

Jakob Jarmar <jakob.j@gmail.com> writes:

Show 9 quoted lines

> diff --git a/t/t3906-stash-submodule.sh b/t/t3906-stash-submodule.sh
> index d7219d6f8f..83106fa958 100755
> --- a/t/t3906-stash-submodule.sh
> +++ b/t/t3906-stash-submodule.sh
> @@ -1,6 +1,6 @@
>  #!/bin/sh
>  
> -test_description='stash apply can handle submodules'
> +test_description='stash can handle submodules'

Good attention to the detail ;-)

> +setup_basic() {

Style. SP on both sides of () in our shell scripts (as seen in the existing shell function in the same file).

Show 12 quoted lines

> +	git init sub &&
> +	(
> +		cd sub &&
> +		test_commit sub_file
> +	) &&
> +	git init main &&
> +	(
> +		cd main &&
> +		git submodule add ../sub &&
> +		test_commit main_file
> +	) &&
> +	test_when_finished "rm -rf main sub"

Have test_when_finished that removes main and sub _before_ you start creating sub and main.

When the &&-cascade breaks anywhere, the control may not even reach your test_when_finished that registers the clean-up procedure. Imagine "git init sub" succeeds but "git init main" somehow fails---you still want to clean up "sub".

Other than that, looks reasonably well done.

Thanks for working on this.

Show 28 quoted lines

> +}
> +
> +test_expect_success 'stash push with submodule.recurse=true preserves dirty submodule worktree' '
> +	setup_basic &&
> +	(
> +		cd main &&
> +		git config submodule.recurse true &&
> +		echo "x" >main_file.t &&
> +		echo "y" >sub/sub_file.t &&
> +		git stash push &&
> +		test_must_fail git -C sub diff --quiet
> +	)
> +'
> +
> +test_expect_success 'stash push and pop with submodule.recurse=true preserves dirty submodule worktree' '
> +	setup_basic &&
> +	(
> +		cd main &&
> +		git config submodule.recurse true &&
> +		echo "x" >main_file.t &&
> +		echo "y" >sub/sub_file.t &&
> +		git stash push &&
> +		git stash pop &&
> +		test_must_fail git -C sub diff --quiet
> +	)
> +'
> +
>  test_done