quoted

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

OK - I have changed it so that each test is independent and cleans up after
itself.

quoted

+	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

Done.

quoted

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

quoted

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

In my current version, I have changed everything to "git init". Should I
use "test_create_repo" instead?

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

I only made one block at first because the test without negotiation is
there just for comparing object counts, but OK, I have split it up into
2 blocks.

quoted

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

I didn't want to make the test prescribe unnecessary details, but good
point about hiding bugs. I have switched it to what you describe.

quoted

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

I think this is a pattern that we need, though. Sometimes there are
closely related fetch/push tests (as in here, and as in t5700) that want
to test similar things across different protocols.

 * 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

Is it useful to know the count of tests that are skipped? (The user
presumably already knows that some are skipped because they set the
environment variable in the first place.)