Re: [PATCH 6/6] t/t5528-push-default: test pushdefault workflows

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

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> Introduce test_pushdefault_workflows(), and test that all push.default
> modes work with central and triangular workflows as expected.
>
> Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
> ---
>  t/t5528-push-default.sh | 36 ++++++++++++++++++++++++++++++++++++
>  1 file changed, 36 insertions(+)
>
> diff --git a/t/t5528-push-default.sh b/t/t5528-push-default.sh
> index b599186..eabc09d 100755
> --- a/t/t5528-push-default.sh
> +++ b/t/t5528-push-default.sh
> @@ -39,6 +39,26 @@ test_push_failure () {
>  	test_cmp expect actual
>  }
>  
> +# $1 = success or failure
> +# $2 = push.default value
> +# $3 = branch to check for actual output (master or foo)
> +# $4 = [optional] switch to triangular workflow
> +test_pushdefault_workflow () {
> +	workflow=central
> +	pushdefault=parent1
> +	if test -n "${4-}"; then
> +		workflow=triangular
> +		pushdefault=parent2
> +	fi
> +test_expect_success "push.default = $2 $1 in $workflow workflows" "
> +	test_config branch.master.remote parent1 &&
> +	test_config branch.master.merge refs/heads/foo &&
> +	test_config remote.pushdefault $pushdefault &&
> +	test_commit commit-for-$2${4+-triangular} &&
> +	test_push_$1 $2 $3 ${4+repo2}
> +"

Please indent the above; it is hard to spot where the actual test
begins and ends otherwise.

> +}
> +
>  test_expect_success '"upstream" pushes to configured upstream' '
>  	git checkout master &&
>  	test_config branch.master.remote parent1 &&
> @@ -115,4 +135,20 @@ test_expect_success 'push to existing branch, upstream configured with different
>  	test_cmp expect-other-name actual-other-name
>  '
>  
> +## test_pushdefault_workflow() arguments:
> +# $1 = success or failure
> +# $2 = push.default value
> +# $3 = branch to check for actual output (master or foo)
> +# $4 = [optional] switch to triangular workflow

What happens in current/upstream/simple modes is affected by the
current branch, but it is unclear on what branch do these all run at
the first glance.  It would be helpful to add a comment here that
says what the starting condition is, something like:

    # We are on 'master', which integrates with 'foo' from parent1
    # Both parent1 and parent2 repositories have 'master' and 'foo'
    # branches.

here.  It took me a while to tell what some of these comments wanted
to say without such a comment.

> +test_pushdefault_workflow success current master  # breaks push/pull symmetry

On 'master', push.default = current and we are in central workflow.
Because we do not use upstream but current, that deliberately breaks
the symmetry.  We make sure that 'master' is updated.

Looks correct.

> +test_pushdefault_workflow success upstream foo    # preserves push/pull symmetry

We use 'upstream'; current branch is set to integrate with 'foo' and
that is what is updated.  Looks good.

> +test_pushdefault_workflow failure simple master   # errors out on asymmetry

Simple is a safer form of 'current' in the central workflow.
The current branch is set to integrate with 'foo', which is
different name from the current branch'es name 'master', and the
push fails.

Looks correct, but do we want to make sure 'foo' is not updated
here as well?

> +test_pushdefault_workflow success matching master # always works

This also should update 'foo'; do we want to make sure that happens
too?

Otherwise we won't be able to tell between matching and current.

> +test_pushdefault_workflow success current master triangular  # always works

OK.

> +test_pushdefault_workflow failure upstream foo triangular    # always errors out

OK.

> +test_pushdefault_workflow success simple master triangular   # works like current

OK, because in triangular it is like current.

> +test_pushdefault_workflow success matching master triangular # always works

OK.

As this step adds a useful helper function for testing, it appears
to me that the helper can and should check the postcondition more
carefully, e.g. not just making sure what should be updated is
updated, but what should not be updated is not touched.

Other than that, looks fine to me.

> +
>  test_done