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

From: Emily Shaffer <emilyshaffer@google.com>
Date: Jul 13, 2021, 22:23 UTC
Message-ID: <YO4SZs40xzHEb6W7@google.com>
In-Reply-To: <042612733181ef348a67edc736637c7cd13c7a6d.1624486920.git.jonathantanmy@google.com>

On Wed, Jun 23, 2021 at 03:30:51PM -0700, Jonathan Tan wrote:

Show 27 quoted lines

> 
> Commit 477673d6f3 ("send-pack: support push negotiation", 2021-05-05)
> introduced the push.negotiate config variable and included a test. The
> test only covered pushing without a remote helper, so the fact that
> pushing with a remote helper doesn't work went unnoticed.
> 
> This is ultimately caused by the "url" field not being set in the args
> struct. This field being unset probably went unnoticed because besides
> push negotiation, this field is only used to generate a "pushee" line in
> a push cert (and if not given, no such line is generated). Therefore,
> set this field.
> 
> Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
> ---
>  builtin/send-pack.c   |  1 +
>  t/t5516-fetch-push.sh | 49 +++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 50 insertions(+)
> 
> diff --git a/builtin/send-pack.c b/builtin/send-pack.c
> index a7e01667b0..729dea1d25 100644
> --- a/builtin/send-pack.c
> +++ b/builtin/send-pack.c
> @@ -230,6 +230,7 @@ int cmd_send_pack(int argc, const char **argv, const char *prefix)
>  	args.atomic = atomic;
>  	args.stateless_rpc = stateless_rpc;
>  	args.push_options = push_options.nr ? &push_options : NULL;
> +	args.url = dest;

Sure, the fix itself is small and inoffensive. And the rest of the patch is regression testing.

Show 13 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?

Show 14 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. But it's only omitted from the *initial* advertisement, so it will be advertised in later rounds of negotiation, right?

> +	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?

> +	grep_wrote 6 event && # 2 commits, 2 trees, 2 blobs

Nice, I like the comment - this helps.

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

Show 6 quoted lines

> +	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?

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

> +	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?

Show 11 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 :)

 - Emily