Re: [PATCH 5/5] t5543-atomic-push.sh: add basic tests for atomic pushes

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

Stefan Beller <sbeller@google.com> writes:

> This adds tests for the atomic push option.
> The first four tests check if the atomic option works in
> good conditions and the last three patches check if the atomic
> option prevents any change to be pushed if just one ref cannot
> be updated.
>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>
> Notes:
>     Originally Ronnie had a similar patch prepared. But as I added
>     more tests and cleaned up the existing tests (e.g. use test_commit
>     instead of "echo one >file && gitadd file && git commit -a -m 'one'",
>     removal of dead code), the file has changed so much that I'd rather
>     take ownership.
>
>  t/t5543-atomic-push.sh | 185 +++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 185 insertions(+)
>  create mode 100755 t/t5543-atomic-push.sh
>
> diff --git a/t/t5543-atomic-push.sh b/t/t5543-atomic-push.sh
> new file mode 100755
> index 0000000..6cbedc5
> --- /dev/null
> +++ b/t/t5543-atomic-push.sh
> @@ -0,0 +1,185 @@
> +#!/bin/sh
> +
> +test_description='pushing to a repository using the atomic push option'
> +
> +. ./test-lib.sh
> +
> +D=`pwd`

$(pwd)???

> +mk_repo_pair () {
> +	rm -rf workbench upstream thirdparty &&
> +	mkdir upstream &&
> +	(
> +		cd upstream &&
> +		git init --bare #&&
> +		#git config receive.denyCurrentBranch warn

Please drop unused comments; they are distracting.

> +	) &&
> +	mkdir workbench &&
> +	(
> +		cd workbench &&
> +		git init &&
> +		git remote add up ../upstream
> +	)
> +}

Hmph.  Shouldn't you be using test_create_repo to create these, so
that templates are used from the build (not install) location, and
their hooks are disabled?

> +test_push_failed () {

Does that mean "test_push_must_fail"?

> +	workbench_master=$(cd workbench && git show-ref -s --verify refs/heads/master) &&
> +	upstream_master=$(cd upstream && git show-ref -s --verify refs/heads/master) &&
> +	test "$workbench_master" != "$upstream_master" &&
> +
> +	workbench_second=$(cd workbench && git show-ref -s --verify refs/heads/second) &&
> +	upstream_second=$(cd upstream && git show-ref -s --verify refs/heads/second) &&
> +	test "$workbench_second" != "$upstream_second"

So the tests that use this helper always try to update master and
second?  Is "they are different" the only thing that matters?  Or
should you be verifying "They are left as the same value they used
to have before the attempted push that must have failed"?

It might be a good time to use "-C" option, e.g. "git -C workbench show-ref ...",
a bit more in our tests.

> +}
> +
> +test_push_succeeded () {
> +	workbench_master=$(cd workbench && git show-ref -s --verify refs/heads/master) &&
> +	upstream_master=$(cd upstream && git show-ref -s --verify refs/heads/master) &&
> +	test "$workbench_master" = "$upstream_master"

Broken &&-chain?

> +
> +	workbench_second=$(cd workbench && git show-ref -s --verify refs/heads/second) &&
> +	upstream_second=$(cd upstream && git show-ref -s --verify refs/heads/second) &&
> +	test "$workbench_second" = "$upstream_second"
> +}
> +
> +test_expect_success 'atomic push works for a single branch' '
> +	mk_repo_pair &&
> +	(
> +		cd workbench &&
> +		test_commit one &&
> +		git push --mirror up &&
> +		test_commit two &&
> +		git push --atomic-push --mirror up
> +	) &&
> +	workbench_master=$(cd workbench && git show-ref -s --verify refs/heads/master) &&
> +	upstream_master=$(cd upstream && git show-ref -s --verify refs/heads/master) &&
> +	test "$workbench_master" = "$upstream_master"

Hmph.  It is a shame that the nice helper you created cannot be used
here.

> +'
> +
> +test_expect_success 'atomic push works for two branches' '
> +	mk_repo_pair &&
> +	(
> +		cd workbench &&
> +		test_commit one &&
> +		git branch second &&
> +		git push --mirror up &&
> +		test_commit two &&
> +		git checkout second &&
> +		test_commit three &&
> +		git push --atomic-push --mirror up
> +	) &&
> +	test_push_succeeded
> +'

OK.

> +test_expect_success 'atomic push works in combination with --mirror' '
> +	mk_repo_pair &&
> +	ls workbench &&

Debugging?

> +	(
> +		cd workbench &&
> +		test_commit one &&
> +		git checkout -b second &&
> +		test_commit two &&
> +		git push --atomic-push --mirror up

It is not wrong per-se, but haven't you already tested in
combination with --mirror in the previous test?

> +	) &&
> +	test_push_succeeded
> +'
> +
> +test_expect_success 'atomic push works in combination with --force' '
> +	mk_repo_pair &&
> +	(
> +		cd workbench &&
> +		test_commit one &&
> +		git checkout -b second &&
> +		test_commit two &&
> +		test_commit three &&
> +		test_commit four &&
> +		git push --mirror up &&
> +		git reset --hard HEAD^ &&
> +		git push --force --atomic-push up master second
> +	) &&
> +	test_push_succeeded
> +'

OK.

> +# set up two branches where master can be pushed but second can not
> +# (non-fast-forward). Since second can not be pushed the whole operation
> +# will fail and leave master untouched.
> +test_expect_success 'atomic push fails if one branch fails locally' '
> +	mk_repo_pair &&
> +	(
> +		cd workbench &&
> +		test_commit one &&
> +		git checkout -b second master &&
> +		test_commit two &&
> +		test_commit three &&
> +		test_commit four &&
> +		git push --mirror up

Broken &&-chain.

> +		git reset --hard HEAD~2 &&
> +		git checkout master &&
> +		test_commit five &&
> +		! git push --atomic-push --all up

test_must_fail?

> +	) &&
> +	test_push_failed

You not only want to see master and second in upstream different from 
those in workbench, but they point at specific commits that the previous
mirror push updated to.

Instead of test_push_failed / test_push_succeeded, how about a
single helper that takes the two commit object names you expect
these two branches point at?  E.g.

	check_branches upstream master HEAD@{2} second HEAD~

(I am probably miscounting HEAD@{$offset} here; this is for
illustration only) that roughly does

	check_branches () {
		target=$1
                shift
                while test $# -ne 0
                do
			git -C "$target" rev-parse --verify "refs/heads/$1" >actual &&
			git rev-parse "$2" >expect &&
			test_cmp expect actual || return 1
			shift
                        shift
		done
	}

or something like that?

> +test_expect_success 'atomic push fails if one branch fails remotely' '
> +	# prepare the repo
> +	mk_repo_pair &&
> +	(
> +		cd workbench &&
> +		test_commit one &&
> +		git checkout -b second master &&
> +		test_commit two &&
> +		git push --mirror up
> +	) &&
> +	# a third party modifies the server side:
> +	(
> +		git clone upstream thirdparty &&
> +		cd thirdparty
> +		git checkout second
> +		test_commit unrelated_changes &&
> +		git push origin second
> +	) &&
> +	# see if we can now push both branches.
> +	(
> +		cd workbench &&
> +		git checkout master &&
> +		test_commit three &&
> +		git checkout second &&
> +		test_commit four &&
> +		! git push --atomic-push up master second
> +	) &&
> +	test_push_failed
> +'

What's the value of this test?  Isn't it a non-fast-forward check
you already tested in the previous one?

> +test_expect_success 'atomic push fails if one tag fails remotely' '
> +	# prepare the repo
> +	mk_repo_pair &&
> +	(
> +		cd workbench &&
> +		test_commit one &&
> +		git checkout -b second master &&
> +		test_commit two &&
> +		git push --mirror up
> +	) &&
> +	# a third party modifies the server side:
> +	(
> +		git clone upstream thirdparty &&
> +		cd thirdparty
> +		git checkout second
> +		git tag test_tag &&
> +		git push --tags origin second
> +	) &&

Broken &&-chain aside, you do not need thirdparty.  Doing the "git
tag" inside upstream itself should suffice.

> +	# see if we can now push both branches.
> +	(
> +		cd workbench &&
> +		git checkout master &&
> +		test_commit three &&
> +		git checkout second &&
> +		test_commit four &&
> +		git tag test_tag &&
> +		! git push --tags --atomic-push up master second

test_must_fail?

> +	) &&
> +	test_push_failed
> +'

One more failure mode you would want to check is what if "update"
hook in the receiving repository rejected one update (but not the
other).  Something along the lines of:

	... setup ...
	git -C workbench push up &&
        write_script upstream/hooks/update <<-\EOF &&
	# only allow update to master from now on
        test "$1" = "refs/heads/master"
        EOF
        ... further update to master and second ...
        test_must_fail git -C workbench push up
	check_branches upstream master ... second ...


perhaps?