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

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

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

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

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

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