Re: [PATCH v2] transport-helper: enforce atomic in push_refs_with_push

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 10, 2019, 17:44 UTC
Message-ID: <xmqqpnmhu6mh.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <20190709211043.48597-1-emilyshaffer@google.com>

Emily Shaffer <emilyshaffer@google.com> writes:

Show 8 quoted lines

> +test_expect_success 'push --atomic also prevents branch creation, reports collateral' '
> +	# Setup upstream repo - empty for now
> +	d=$HTTPD_DOCUMENT_ROOT_PATH/atomic-branches.git &&
> +	git init --bare "$d" &&
> +	test_config -C "$d" http.receivepack true &&
> +	up="$HTTPD_URL"/smart/atomic-branches.git &&
> +
> +	# Tell up about two branches for now

-ECANTPARSE "Tell up" part.

> +	test_commit atomic1 &&
> +	test_commit atomic2 &&
> +	git branch collateral &&
> +	git push "$up" master collateral &&

OK, so an initially empty directory $d that appears to network clients as $up now has two branches, 'master' and 'collateral', both pointing at the same history that ends with two commits, atomic2 whose parent is atomic1.

Show 7 quoted lines

> +	# collateral is a valid push, but should be failed by atomic push
> +	git checkout collateral &&
> +	test_commit collateral1 &&
> +
> +	# Make master incompatible with upstream to provoke atomic
> +	git checkout master &&
> +	git reset --hard HEAD^ &&

collateral grows, master rewinds.

> +	# Add a new branch which should be failed by atomic push. This is a
> +	# regression case.
> +	git branch atomic &&

Another branch atomic is added

> +	# --atomic should cause entire push to be rejected
> +	test_must_fail git push --atomic "$up" master atomic collateral 2>output &&

Attempt to push all three: collateral alone would be OK, so is atomic, but because master rewinds, we expect none of the three to go through.

> +	# the new branch should not have been created upstream
> +	test_must_fail git -C "$d" rev-parse refs/heads/atomic &&

The new branch should not have been created; if this rev-parse succeeded, it would be a bug.

Up to point, I have no possible improvements to offer ;-) Very well done.

> +	# the failed refs should be indicated
> +	grep "master -> master" output | grep rejected &&

I'd rather see the effect, i.e. what the command did that can be observed externally, than the report, i.e. what the command claims to have done, if it is equally straight-forward to verify either.

That can be done by making sure that the output from "git -C "$d" rev-parse refs/heads/master" match output from "git rev-parse atomic2", no? That ensures 'master' in the receiving end stayed the same.

> +	# the collateral failure refs should be indicated
> +	grep "atomic -> atomic" output | grep "atomic push failed" &&
> +	grep "collateral -> collateral" output | grep "atomic push failed"

Likewise for the other two.

FWIW, these three can further lose a process each, i.e.

	grep "^ ! .*rejected.* master -> master" output

even if we for some reason do not want to check the effect and take the claim by the command being tested at the face value (which I do not think is a good idea).

Thanks.