git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Junio C Hamano <gitster@pobox.com>
Date
Dec 15, 2014, 22:29 UTC
Message-ID
<xmqq4mswczs4.fsf@gitster.dls.corp.google.com>
In-Reply-To
<1418673368-20785-6-git-send-email-sbeller@google.com>
Stefan Beller <sbeller@google.com> writes:
Show 33 quoted lines
> 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)???
Show 7 quoted lines
> +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.
Show 8 quoted lines
> +	) &&
> +	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"?
Show 7 quoted lines
> +	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.

Show 6 quoted lines
> +}
> +
> +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?
Show 18 quoted lines
> +
> +	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.

Show 16 quoted lines
> +'
> +
> +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?
Show 6 quoted lines
> +	(
> +		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?

Show 19 quoted lines
> +	) &&
> +	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.
Show 13 quoted lines
> +# 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?
Show 29 quoted lines
> +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?

Show 18 quoted lines
> +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.

Show 9 quoted lines
> +	# 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?
Previous: Stefan BellerNext: Junio C Hamano
Message 12 of 56 in “Add a flag to push atomically”
  1. 0/5 Add a flag to push atomicallyStefan Beller, Dec 15, 2014
  2. 1/5 receive-pack.c: add protocol support to negotiate atomic-pushStefan Beller, Dec 15, 2014
  3. Junio C HamanoDec 15, 2014
  4. Stefan BellerDec 15, 2014
  5. 2/5 send-pack.c: add an --atomic-push command line argumentStefan Beller, Dec 15, 2014
  6. Junio C HamanoDec 15, 2014
  7. 3/5 receive-pack.c: use a single ref_transaction for atomic pushesStefan Beller, Dec 15, 2014
  8. Junio C HamanoDec 15, 2014
  9. 4/5 push.c: add an --atomic-push argumentStefan Beller, Dec 15, 2014
  10. Junio C HamanoDec 15, 2014
  11. 5/5 t5543-atomic-push.sh: add basic tests for atomic pushesStefan Beller, Dec 15, 2014
  12. Junio C HamanoDec 15, 2014
  13. Junio C HamanoDec 15, 2014
  14. 1/6 receive-pack.c: add protocol support to negotiate atomic-pushStefan Beller, Dec 16, 2014
  15. 2/6 send-pack: Invert the return value of ref_update_to_be_sentStefan Beller, Dec 16, 2014
  16. Junio C HamanoDec 16, 2014
  17. 3/6 send-pack.c: add --atomic command line argumentStefan Beller, Dec 16, 2014
  18. Junio C HamanoDec 16, 2014
  19. 4/6 receive-pack.c: use a single ref_transaction for atomic pushesStefan Beller, Dec 16, 2014
  20. Eric SunshineDec 16, 2014
  21. Eric SunshineDec 16, 2014
  22. Junio C HamanoDec 16, 2014
  23. 5/6 push.c: add an --atomic-push argumentStefan Beller, Dec 16, 2014
  24. Eric SunshineDec 16, 2014
  25. Junio C HamanoDec 16, 2014
  26. Junio C HamanoDec 16, 2014
  27. 6/6 t5543-atomic-push.sh: add basic tests for atomic pushesStefan Beller, Dec 16, 2014
  28. receive-pack: refuse all commands if one fails in atomic modeStefan Beller, Dec 16, 2014
  29. Junio C HamanoDec 16, 2014
  30. Eric SunshineDec 16, 2014
  31. Junio C HamanoDec 16, 2014
  32. Stefan BellerDec 16, 2014
  33. Junio C HamanoDec 16, 2014
  34. Stefan BellerDec 16, 2014
  35. Junio C HamanoDec 16, 2014
  36. Stefan BellerDec 16, 2014
  37. Junio C HamanoDec 16, 2014
  38. 0/6 atomic pushesStefan Beller, Dec 17, 2014
  39. 1/6 receive-pack.c: add protocol support to negotiate atomicStefan Beller, Dec 17, 2014
  40. Eric SunshineDec 19, 2014
  41. 2/6 send-pack: Rename ref_update_to_be_sent to check_to_send_updateStefan Beller, Dec 17, 2014
  42. Junio C HamanoDec 17, 2014
  43. 3/6 send-pack.c: add --atomic command line argumentStefan Beller, Dec 17, 2014
  44. Junio C HamanoDec 17, 2014
  45. Eric SunshineDec 19, 2014
  46. 4/6 receive-pack.c: use a single ref_transaction for atomic pushesStefan Beller, Dec 17, 2014
  47. Junio C HamanoDec 17, 2014
  48. Stefan BellerDec 17, 2014
  49. Junio C HamanoDec 18, 2014
  50. 4/6 receive-pack.c: use a single ref_transaction for atomic pushesStefan Beller, Dec 18, 2014
  51. Eric SunshineDec 18, 2014
  52. 4/6 receive-pack.c: use a single ref_transaction for atomic pushesStefan Beller, Dec 19, 2014
  53. Eric SunshineDec 19, 2014
  54. 5/6 push.c: add an --atomic argumentStefan Beller, Dec 17, 2014
  55. Eric SunshineDec 19, 2014
  56. 6/6 t5543-atomic-push.sh: add basic tests for atomic pushesStefan Beller, Dec 17, 2014

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.