Re: [PATCH 3/7] t5528-push-default.sh: add helper functions

[Junio C Hamano <gitster@pobox.com>]

Matthieu Moy <Matthieu.Moy@imag.fr> writes:

> Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
> ---
>  t/t5528-push-default.sh |   45 ++++++++++++++++++++++++++++++++++++++-------
>  1 file changed, 38 insertions(+), 7 deletions(-)

The use of helpers makes the body of the test easier to follow.

> diff --git a/t/t5528-push-default.sh b/t/t5528-push-default.sh
> index c334c51..da7d3d8 100755
> --- a/t/t5528-push-default.sh
> +++ b/t/t5528-push-default.sh
> @@ -13,16 +13,47 @@ test_expect_success 'setup bare remotes' '
>  	git push parent2 HEAD
>  '
>  
> +# $1 = local revision
> +# $2 = remote revision (tested to be equal to the local one)
> +check_pushed_commit () {
> +	git log -1 --format='%h %s' >expect &&
> +	git --git-dir=repo1 log -1 --format='%h %s' "$2" >actual &&
> +	test_cmp expect actual
> +}

Hmph.  How is $1 used in the above to make it compare between local and
remote?  Does the first one need to have "$1" before " >expect"?

> +# $1 = push.default value
> +# $2 = expected target branch for the push
> +test_push_success () {
> +	git -c push.default="$1" push &&
> +	check_pushed_commit HEAD "$2"
> +}
> +
> +# $1 = push.default value
> +# other arguments = target branches that should not be touched
> +test_push_failure () {
> +	push_default=$1 &&
> +	shift &&
> +	if test $# -gt 0
> +	then
> +		# branch may not exist
> +		test_might_fail git --git-dir=repo1 \
> +			log --no-walk --format='%h %s' "$@" >expect
> +	fi &&
> +	test_must_fail git -c push.default="$1" &&

What subcommand does this run with one-shot override configuration?  Do
we need " push" before " &&"?

> +	if test $# -gt 0
> +	then
> +		test_might_fail git --git-dir=repo1 \
> +			log -1 --format='%h %s' "$@" >actual
> +	fi &&
> +	test_cmp expect actual
> +}

This allows us to look at not just the failure of the push operation,
but also inspects the resulting repository to make sure things that must
stay intact do, which is very nice.  The callers can even pass "--all"
to it, instead of enumerating the branches, which would be very useful
when testing a single-branch modes like `current` and `upstream`.

>  test_expect_success '"upstream" pushes to configured upstream' '
>  	git checkout master &&
>  	test_config branch.master.remote parent1 &&
>  	test_config branch.master.merge refs/heads/foo &&
> -	test_config push.default upstream &&
>  	test_commit two &&
> -	git push &&
> -	echo two >expect &&
> -	git --git-dir=repo1 log -1 --format=%s foo >actual &&
> -	test_cmp expect actual
> +	test_push_success upstream foo
>  '
>
>  test_expect_success '"upstream" does not push on unconfigured remote' '
> @@ -30,7 +61,7 @@ test_expect_success '"upstream" does not push on unconfigured remote' '
>  	test_unconfig branch.master.remote &&
>  	test_config push.default upstream &&
>  	test_commit three &&
> -	test_must_fail git push
> +	test_push_failure upstream master
>  '

... and we can use --all not master here, right?

>  test_expect_success '"upstream" does not push on unconfigured branch' '
> @@ -39,7 +70,7 @@ test_expect_success '"upstream" does not push on unconfigured branch' '
>  	test_unconfig branch.master.merge &&
>  	test_config push.default upstream
>  	test_commit four &&
> -	test_must_fail git push
> +	test_push_failure upstream master
>  '

Same here.

Thanks.