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

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Wed, Jul 14 2021, Jonathan Tan wrote:

>> > +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.
>
>> 
>> > +	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.
>
>> 
>> > +	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 &&
>
> In my current version, I have changed everything to "git init". Should I
> use "test_create_repo" instead?

Sorry I just copy/pasted that from your version, yes indeed
s/test_create_repo/git init/ (a trivial thing, but they're 1=1
equivalent these days after a recent change of mine in
test-lib-functions.sh).

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

Yeah, in my split-up WIP there's a bit of that, in my split-up of them I
didn't find the end result harder to read, e.g. in
t/t5702-protocol-v2.sh it's mostly in different sections of the file,
first git://, then file:// and then http://, with some misc in-between,
having a t/t57*-protocol-{git,file,http}.sh didn't make things harder to
read.

>> 
>>  * 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.)

A thing I do really often is to run some glob of tests, say I'm working
on git-fetch, and run *fetch*, I'd get this in the output:
    
    [23:58:15] t5527-fetch-odd-refs.sh ......................... ok      121 ms ( 0.01 usr  0.00 sys +  0.72 cusr  0.38 csys =  1.11 CPU)
    [23:58:15] t5535-fetch-push-symref.sh ...................... ok       90 ms ( 0.01 usr  0.00 sys +  0.14 cusr  0.02 csys =  0.17 CPU)
    [23:58:16] t5536-fetch-conflicts.sh ........................ ok      180 ms ( 0.01 usr  0.01 sys +  0.28 cusr  0.07 csys =  0.37 CPU)
    [23:58:16] t5539-fetch-http-shallow.sh ..................... skipped: no web server found at '/usr/sbin/apache'
    [23:58:16] t5550-http-fetch-dumb.sh ........................ skipped: no web server found at '/usr/sbin/apache'
    [23:58:16] t5551-http-fetch-smart.sh ....................... skipped: no web server found at '/usr/sbin/apache'
    [23:58:16] t5554-noop-fetch-negotiator.sh .................. ok        3 ms ( 0.00 usr  0.00 sys +  0.07 cusr  0.02 csys =  0.09 CPU)
    [23:58:16] t5537-fetch-shallow.sh .......................... ok      641 ms ( 0.03 usr  0.01 sys +  0.87 cusr  0.31 csys =  1.22 CPU)
    [23:58:16] t5582-fetch-negative-refspec.sh ................. ok      564 ms ( 0.02 usr  0.01 sys +  0.77 cusr  0.32 csys =  1.12 CPU)

It's useful to know that a portion of the tests was skipped entirely,
whereas if we do git:// tests first, and then later http:// the loss of
coverage is silent (unless you run it individually under --verbose) or
whatever.

This is really useful for the http tests iin particular, it's really
common not to have apache installed.