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

From: Emily Shaffer <emilyshaffer@google.com>
Date: Jul 11, 2019, 20:57 UTC
Message-ID: <20190711205706.GD113966@google.com>
In-Reply-To: <xmqqpnmhu6mh.fsf@gitster-ct.c.googlers.com>

On Wed, Jul 10, 2019 at 10:44:22AM -0700, Junio C Hamano wrote:

Show 12 quoted lines

> Emily Shaffer <emilyshaffer@google.com> writes:
> 
> > +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.

s/up/"$&"/ - thanks.

[snip]

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

This is nice to hear, thanks! :)

Show 6 quoted lines

> > +	# 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.

Hmm. I'd like to argue that part of the requirement is to show the user what happened; for example, in an earlier iteration I was not successfully reporting the "collateral damage" refs to the user, even though they were not being pushed. To that end, I'd rather check both.

Show 5 quoted lines

> 
> 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.

Sure, I agree.

Show 14 quoted lines

> 
> > +	# 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).

Will swap, wasn't sure on preference between regex or process count. Thanks.

 - Emily