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

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Jul 13, 2021, 23:11 UTC
Message-ID: <87bl753i2p.fsf@evledraar.gmail.com>
In-Reply-To: <042612733181ef348a67edc736637c7cd13c7a6d.1624486920.git.jonathantanmy@google.com>

On Wed, Jun 23 2021, Jonathan Tan wrote:

Show 45 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;
>  
>  	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
> +
> +test_expect_success 'http push with negotiation' '
> +	SERVER="$HTTPD_DOCUMENT_ROOT_PATH/server" &&
> +	URI="$HTTPD_URL/smart/server" &&
> +
> +	rm -rf client &&

Nothing in this test file has created a "client" directory at this point, so we shouldn't have this to remove it.

I think the real reason for this is that you're just copy/pasting this from elsewhere. I've got an unsubmitted series where I fixed various callsites in these t/t*http* tests to use test_when_finished instead.

This pattern of having the next test clean up after you leads to bad inter-dependencies, there were a few things broken e.g. if earlier tests were skipped. Let's not continue that pattern.

Show 6 quoted lines

> +	git init client &&
> +	test_commit -C client first_commit &&
> +	test_commit -C client second_commit &&
> +
> +	# Without negotiation
> +	test_create_repo "$SERVER" &&

s/test_create_repo/git init/g

Show 10 quoted lines

> +	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 \
> +		push "$URI" refs/heads/main:refs/remotes/origin/main &&
> +	grep_wrote 6 event && # 2 commits, 2 trees, 2 blobs
> +
> +	# Same commands, but with negotiation
> +	rm event &&
> +	rm -rf "$SERVER" &&

Ditto test_when_finished, or actually:

> +	test_create_repo "$SERVER" &&
> +	test_config -C "$SERVER" http.receivepack true &&

If we're entirely setting up the server again shouldn't this just be another test_expect_success block?

Show 18 quoted lines

> +	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 &&
> +	grep_wrote 3 event # 1 commit, 1 tree, 1 blob
> +'
> +
> +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 &&
> +	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 &&
> +	grep_wrote 6 event && # 2 commits, 2 trees, 2 blobs
> +	test_i18ngrep "push negotiation failed" err

s/test_i18ngrep/grep/, or actually better yet is there a reason not to do test_cmp here? I'm pretty sure there's not.

The reason I've got that unsubmitted series is because I found a bug that was cleverly hidden away by an earlier such 'grep the output' code/test you'd added recently.

There *are* cases where we e.g. get STDERR output from the OS itself about bad connections, or races, but we should at least:

    grep error: <stderr >error-lines.actual &&
    test_cmp error-lines.expect error-lines.actual

To check that we get the errors we expected, and *only* those.

Show 6 quoted lines

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

Also, instead of this comment let's just create another t*-fetch-push-http.sh test. Because:

 * This test is already really slow, it takes me 13s to run it now. I
   haven't tested, but setting up apache will make it even slower.

 * It's also an anti-pattern to switch midway to needing an external
   daemon v.s. doing it in a dedicated test.

   E.g. if you have the relevant GIT_* settings to run http:// tests,
   but not git:// tests we'll skip the former entirely in
   t5700-protocol-v1.sh, because we'll "skip all" as soon as we see we
   can't start the git-daemon.

   That specifically isn't an issue here, but better to avoid setting up
   the pattern.

 * What *is* an issue with it here is that the "skip all" in TAP is only
   handled before you run any tests, e.g. compare:
       
       $ prove ./t5700-protocol-v1.sh
       ./t5700-protocol-v1.sh .. ok    
       All tests successful.
       Files=1, Tests=21,  2 wallclock secs ( 0.02 usr  0.00 sys +  0.80 cusr  0.80 csys =  1.62 CPU)
       Result: PASS
       
       $ GIT_TEST_GIT_DAEMON=false GIT_TEST_HTTPD=false prove ./t5700-protocol-v1.sh
       ./t5700-protocol-v1.sh .. skipped: git-daemon testing disabled (unset GIT_TEST_GIT_DAEMON to enable)
       Files=1, Tests=0,  0 wallclock secs ( 0.01 usr  0.00 sys +  0.02 cusr  0.00 csys =  0.03 CPU)
       Result: NOTESTS
       
       $ GIT_TEST_GIT_DAEMON=true GIT_TEST_HTTPD=false prove ./t5700-protocol-v1.sh
       ./t5700-protocol-v1.sh .. ok    
       All tests successful.
       Files=1, Tests=16,  1 wallclock secs ( 0.01 usr  0.00 sys +  0.63 cusr  0.59 csys =  1.23 CPU)
       Result: PASS
   
   I.e. if you stick an inclusion of lib-httpd.sh this late in a test
   file we won't get a prominent notice saying we're categorically
   skipping certain types of tests based on an external dependency.

   If you had your own dedicated test instead you'd get:
       
       $ GIT_TEST_HTTPD=false  prove ./t5705-protocol-v2-http.sh 
       ./t5705-protocol-v2-http.sh .. skipped: Network testing disabled (unset GIT_TEST_HTTPD to enable)
       Files=1, Tests=0,  0 wallclock secs ( 0.01 usr  0.01 sys +  0.02 cusr  0.01 csys =  0.05 CPU)
       Result: NOTESTS