Re: [RFC PATCH v2 2/2] push: support pushing to a remote group

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

Usman Akinyemi <usmanakinyemi202@gmail.com> writes:

> diff --git a/t/t5566-push-group.sh b/t/t5566-push-group.sh
> new file mode 100755
> index 0000000000..9e0d378f2a
> --- /dev/null
> +++ b/t/t5566-push-group.sh
> @@ -0,0 +1,95 @@
> +#!/bin/sh
> +
> +test_description='push to remote group'
> +
> +. ./test-lib.sh
> +
> +test_expect_success 'setup' '
> +	for i in 1 2 3
> +	do
> +		git init --bare dest-$i.git &&
> +		git -C dest-$i.git symbolic-ref HEAD refs/heads/not-a-branch ||
> +		return 1
> +	done &&
> +	test_tick &&
> +	git commit --allow-empty -m "initial" &&
> +	git config set remote.remote-1.url "file://$(pwd)/dest-1.git" &&
> +	git config set remote.remote-1.fetch "+refs/heads/*:refs/remotes/remote-1/*" &&
> +	git config set remote.remote-2.url "file://$(pwd)/dest-2.git" &&
> +	git config set remote.remote-2.fetch "+refs/heads/*:refs/remotes/remote-2/*" &&
> +	git config set remote.remote-3.url "file://$(pwd)/dest-3.git" &&
> +	git config set remote.remote-3.fetch "+refs/heads/*:refs/remotes/remote-3/*" &&
> +	git config set remotes.all-remotes "remote-1 remote-2 remote-3"
> +'

So we have three remotes, dest-{1,2,3}.git/ that are all bare, and a
remote group "all-remotes" that name them.  Is there a reason why
you want to use an unborn HEAD?

> +test_expect_success 'push to remote group pushes to all members' '
> +	git push all-remotes HEAD:refs/heads/main &&

Our "push" exits with 0 status.  How would we make sure we pushed
correctly?

> +	j= &&
> +	for i in 1 2 3
> +	do
> +		git -C dest-$i.git for-each-ref >actual-$i &&

We grab dest-$i's refs to actual-$i

> +		if test -n "$j"
> +		then
> +			test_cmp actual-$j actual-$i

and make sure if refs in dest-N differ from dest-(N-1)'s refs.

> +		else
> +			cat actual-$i

of course, the first one has nothing to compare against, so we get a
debugging "cat" for it.

> +		fi &&
> +		j=$i ||
> +		return 1

But does this loop test what we really want to make sure?  You could
write your "group push" to push one commit less than what was asked
to push out to all remotes, and they will match with each other to
pass the above test, but it would be different from our original.

Don't we know the exact state of refs in these dest-$i.git
repositories?  If we do, then

    printf "%s commit\trefs/heads/main\n" >expect &&
    for i in 1 2 3
    do
	git -C dest-$i.git for-each-ref >actual &&
	test_cmp expect actual || return 1
    done

perhaps?

> +test_expect_success 'push second commit to group updates all members' '
> +	test_tick &&
> +	git commit --allow-empty -m "second" &&
> +	git push all-remotes HEAD:refs/heads/main &&
> +	for i in 1 2 3
> +	do
> +		git -C dest-$i.git rev-parse refs/heads/main >hash-$i ||
> +		return 1
> +	done &&
> +	test_cmp hash-1 hash-2 &&
> +	test_cmp hash-2 hash-3
> +'

Again, the primary thing we are interested in is that dest-*.git
has a copy of what we pushed.  They may be identical to each other
among themselves but they still could be different from what we
pushed, and that is something we want to catch, no?

    git rev-parse refs/heads/main >expect &&
    for i in 1 2 3
    do
	git -C dest-$i.git rev-parse refs/heads/main >actual &&
	test_cmp expect actual || return 1
    done

> +
> +test_expect_success 'push to single remote in group does not affect others' '
> +	test_tick &&
> +	git commit --allow-empty -m "third" &&
> +	git push remote-1 HEAD:refs/heads/main &&
> +	git -C dest-1.git rev-parse refs/heads/main >hash-after-1 &&
> +	git -C dest-2.git rev-parse refs/heads/main >hash-after-2 &&
> +	! test_cmp hash-after-1 hash-after-2
> +'

Obviously correct.

> +test_expect_success 'push to nonexistent group fails with error' '
> +	test_must_fail git push no-such-group HEAD:refs/heads/main
> +'

Obviously correct---we probably should already have a test to see
that a push to nonexistent remote repository fails (missing one you
cannot even tell if it is a single remote or a group), in which case
this is not even needed.

> +test_expect_success 'push explicit refspec to group' '
> +	test_tick &&
> +	git commit --allow-empty -m "fourth" &&
> +	git push all-remotes HEAD:refs/heads/other &&

Didn't we do this already?  We did so with 'main' into dest-*.git
that did not know anything about 'main' (after its HEAD repointed
to a missing branch).

> +	for i in 1 2 3
> +	do
> +		git -C dest-$i.git rev-parse refs/heads/other >other-hash-$i ||
> +		return 1
> +	done &&
> +	test_cmp other-hash-1 other-hash-2 &&
> +	test_cmp other-hash-2 other-hash-3
> +'
> +
> +test_expect_success 'mirror remote in group with refspec fails' '
> +	git config set remote.remote-1.mirror true &&
> +	test_must_fail git push all-remotes HEAD:refs/heads/main 2>err &&
> +	grep "mirror" err &&

test_grep??

> +	git config unset remote.remote-1.mirror
> +'
> +test_expect_success 'push.default=current works with group push' '
> +	git config set push.default current &&
> +	test_tick &&
> +	git commit --allow-empty -m "fifth" &&
> +	git push all-remotes &&
> +	git config unset push.default
> +'
> +
> +test_done