From: Johannes Sixt <j6t@kdbg.org>
Date: Tue, 15 Nov 2016 07:03:24 GMT
Subject: Re: [PATCH 1/2] push: --dry-run updates submodules when --recurse-submodules=on-demand
Message-ID: <4a72ad14-0a8f-ede9-9f54-601fcd37740b@kdbg.org>
In-Reply-To: <1479172735-698-2-git-send-email-bmwill@google.com>

Am 15.11.2016 um 02:18 schrieb Brandon Williams:
> diff --git a/t/t5531-deep-submodule-push.sh b/t/t5531-deep-submodule-push.sh
> index 198ce84..e6ccc30 100755
> --- a/t/t5531-deep-submodule-push.sh
> +++ b/t/t5531-deep-submodule-push.sh
> @@ -427,7 +427,31 @@ test_expect_success 'push unpushable submodule recursively fails' '
>  		cd submodule.git &&
>  		git rev-parse master >../actual
>  	) &&
> -	test_cmp expected actual
> +	test_cmp expected actual &&
> +	git -C work reset --hard master^

This line looks like a clean-up to be done after the test case. You 
should wrap it in test_when_finished, but outside of a sub-shell, which 
looks like it's just one line earlier, before the test_cmp.

> +'
> +
> +test_expect_failure 'push --dry-run does not recursively update submodules' '
> +	(
> +		cd work &&
> +		(
> +			cd gar/bage &&
> +			git checkout master &&
> +			git rev-parse master >../../../expected_submodule &&
> +			> junk9 &&
> +			git add junk9 &&
> +			git commit -m "Ninth junk"
> +		) &&

Could you please avoid this nested sub-shell? It is fine to cd around 
when you are in a sub-shell.

> +		git checkout master &&
> +		git rev-parse master >../expected_pub

Broken && chain.

> +		git add gar/bage &&
> +		git commit -m "Ninth commit for gar/bage" &&
> +		git push --dry-run --recurse-submodules=on-demand ../pub.git master
> +	) &&
> +	git -C submodule.git rev-parse master >actual_submodule &&
> +	git -C pub.git rev-parse master >actual_pub &&

All of the commands above are 'git something' that could become 'git -C 
work something' and then the sub-shell would be unnecessary. I'm not 
sure I would appreciate the verbosity of the result, though. (Perhaps 
aligning the git subcommands after -C foo would help.)

> +	test_cmp expected_pub actual_pub &&
> +	test_cmp expected_submodule actual_submodule
>  '
>
>  test_done
>