Re: [PATCH 1/3] send-pack: fix push.negotiate with remote helper

From: Jonathan Tan <jonathantanmy@google.com>
Date: Jul 14, 2021, 19:25 UTC
Message-ID: <20210714192509.4082107-1-jonathantanmy@google.com>
In-Reply-To: <YO4SZs40xzHEb6W7@google.com>

Show 17 quoted lines

> >  
> >  	if (from_stdin) {
> >  		if (args.stateless_rpc) {
> > diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
> > index 0916f76302..5ce32e531a 100755
> > --- a/t/t5516-fetch-push.sh
> > +++ b/t/t5516-fetch-push.sh
> > @@ -1768,4 +1768,53 @@ test_expect_success 'denyCurrentBranch and worktrees' '
> >  	test_must_fail git -C cloned push --delete origin new-wt
> >  '
> >  
> > +. "$TEST_DIRECTORY"/lib-httpd.sh
> > +start_httpd
> 
> Ah, so fetch-push test wasn't doing any HTTP testing whatsoever. Does
> that mean there is a better place for these to go? Or does it mean that
> fetch-push test was under-testing?

These are closely related to the non-HTTP push negotiation tests that I previously added to this file, so I don't think there is a better place. As for whether this test is under-testing, I also don't think so - most fetch/push processes would be the same regardless of protocol (but not push negotiation, apparently).

Show 21 quoted lines

> 
> > +
> > +test_expect_success 'http push with negotiation' '
> > +	SERVER="$HTTPD_DOCUMENT_ROOT_PATH/server" &&
> > +	URI="$HTTPD_URL/smart/server" &&
> > +
> > +	rm -rf client &&
> > +	git init client &&
> > +	test_commit -C client first_commit &&
> > +	test_commit -C client second_commit &&
> > +
> > +	# Without negotiation
> > +	test_create_repo "$SERVER" &&
> > +	test_config -C "$SERVER" http.receivepack true &&
> > +	git -C client push "$URI" first_commit:refs/remotes/origin/first_commit &&
> Pushing a branch with just the first commit...
> > +	git -C "$SERVER" config receive.hideRefs refs/remotes/origin/first_commit &&
> transfer.hideRefs (referenced by receive.hideRefs) says this ref will be
> omitted from advertisement, so we are forcing either an inefficient push
> or a negotiation to occur, by having the server initially claim not to
> know about it.

Currently, the client attempts the negotiation before the push ref advertisement, so the ref advertisement does not influence whether the negotiation happens or not.

(One might ask, should the negotiation happen after the push ref advertisement then? This is an interesting idea, because the push ref advertisement could have information that shows us that negotiation is unnecessary, but I think that eventually it's best if we suppress the push ref advertisement.)

> But it's only omitted from the *initial* advertisement,
> so it will be advertised in later rounds of negotiation, right?

There is no ref advertisement in any round of negotiation (there is no ls-refs call). It will be acknowledged if the client queries for it, though. (In any case, this config is not meant to affect the negotiation part at all, because the negotiation part is using the "fetch" protocol and the "receive" in "receive.hideRefs" means that it only affects the "push" protocol.)

> > +	GIT_TRACE2_EVENT="$(pwd)/event" git -C client -c protocol.version=2 \
> > +		push "$URI" refs/heads/main:refs/remotes/origin/main &&
> And then from 'main' we push first_commit and second_commit?

Depending on the result of negotiation. Here, we push both commits (as you can see from the next line you quoted below).

> > +	grep_wrote 6 event && # 2 commits, 2 trees, 2 blobs
> Nice, I like the comment - this helps.

Thanks.

Show 19 quoted lines

> > +
> > +	# Same commands, but with negotiation
> > +	rm event &&
> > +	rm -rf "$SERVER" &&
> Ok, clean up the trace and the server so we can start over, but we don't
> need to recreate the client commits because the server doesn't know
> about them anyway. Fine.
> > +	test_create_repo "$SERVER" &&
> > +	test_config -C "$SERVER" http.receivepack true &&
> > +	git -C client push "$URI" first_commit:refs/remotes/origin/first_commit &&
> > +	git -C "$SERVER" config receive.hideRefs refs/remotes/origin/first_commit &&
> > +	GIT_TRACE2_EVENT="$(pwd)/event" git -C client -c protocol.version=2 -c push.negotiate=1 \
> > +		push "$URI" refs/heads/main:refs/remotes/origin/main &&
> And then here's the same set of commands with push negotiation, ok.
> > +	grep_wrote 3 event # 1 commit, 1 tree, 1 blob
> 
> Is there any reason the event counts would change or be
> non-deterministic outside of negotiation? Or, in other words, is this
> potentially flaky?

In general, no - the information that the server conveys to the client in the ref advertisement (which commits it has) is quite well-defined, so it can be precisely computed which objects the server has and hasn't. One potential point of flakiness is if we change the client to push extra objects (say, if the client decided to store one packfile per commit and all its trees and blobs, and then push the entire packfile whenever it needs to push that commit), but I don't think that we're heading in that direction.

Show 12 quoted lines

> 
> > +'
> > +
> > +test_expect_success 'http push with negotiation proceeds anyway even if negotiation fails' '
> > +	rm event &&
> > +	rm -rf "$SERVER" &&
> > +	test_create_repo "$SERVER" &&
> > +	test_config -C "$SERVER" http.receivepack true &&
> > +	git -C client push "$URI" first_commit:refs/remotes/origin/first_commit &&
> Hmm, this relies on 'client' being in the same state the above test left
> it. Probably better to recreate it or at least leave a loud warning
> about it in a comment above this test definition...

I ended up recreating it.

Show 7 quoted lines

> 
> > +	git -C "$SERVER" config receive.hideRefs refs/remotes/origin/first_commit &&
> > +	GIT_TEST_PROTOCOL_VERSION=0 GIT_TRACE2_EVENT="$(pwd)/event" git -C client -c push.negotiate=1 \
> > +		push "$URI" refs/heads/main:refs/remotes/origin/main 2>err &&
> 
> And we're pushing with protocol v0 so no negotiation can occur here,
> right?

Yes, but this is worth adding a comment for, so I added it.

Show 14 quoted lines

> 
> > +	grep_wrote 6 event && # 2 commits, 2 trees, 2 blobs
> > +	test_i18ngrep "push negotiation failed" err
> > +'
> > +
> > +# DO NOT add non-httpd-specific tests here, because the last part of this
> > +# test script is only executed when httpd is available and enabled.
> > +
> >  test_done
> > -- 
> > 2.32.0.288.g62a8d224e6-goog
> > 
> 
> Thanks for answering novice questions :)

Thanks for taking a look.