Re: [PATCH 5/5] t5543-atomic-push.sh: add basic tests for atomic pushes
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`
Show 7 quoted lines
> +mk_repo_pair () {
> + rm -rf workbench upstream thirdparty &&
> + mkdir upstream &&
> + (
> + cd upstream &&
> + git init --bare #&&
> + #git config receive.denyCurrentBranch warnPlease 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"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
> +'
> +test_expect_success 'atomic push works in combination with --mirror' '
> + mk_repo_pair &&
> + ls workbench &&
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
> +'
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
> + git reset --hard HEAD~2 &&
> + git checkout master &&
> + test_commit five &&
> + ! git push --atomic-push --all up
> + ) &&
> + 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_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?
Stefan Beller
