Re: [PATCH 1/8] t5558: add tests for creationToken heuristic

[Derrick Stolee <derrickstolee@github.com>]

On 1/17/2023 1:17 PM, Victoria Dye wrote:
> Derrick Stolee via GitGitGadget wrote:

>> +	for b in 1 2 3 4
>> +	do
>> +		test_bundle_downloaded bundle-$b.bundle trace-clone.txt ||
>> +			return 1
>> +	done
> 
> Because the current state of bundle list handling is equivalent to "no
> heuristic", this pre-existing test is just updated to verify all bundles are
> downloaded. This isn't new behavior, but it'll be relevant to compare with
> the behavior of the 'creationToken' heuristic. 
> 
> I was going to ask how the tests verify that *only* the expected bundles are
> downloaded, and it looks like later patches [1] handle that with
> '! test_bundle_downloaded' checks. That approach seems a bit fragile (if a
> bundle's name doesn't match the '! test_bundle_downloaded' check for some
> reason, the bundle can be either downloaded or not with no effect on the
> test result). Would something like a 'test_downloaded_bundle_count' work
> instead?

Or, perhaps we could check the exact list _and order_ using this slightly
more generic helper?

# Given a GIT_TRACE2_EVENT log over stdin, writes to stdout a list of URLs
# sent to git-remote-https child processes.
test_remote_https_urls() {
	grep -e '"event":"child_start".*"argv":\["git-remote-https",".*"\]' |
		sed -e 's/{"event":"child_start".*"argv":\["git-remote-https","//g' \
		    -e 's/"\]}//g'
}

With a test example looking a lot like this:

	cat >expect <<-EOF &&
	$HTTPD_URL/newest.bundle
	$HTTPD_URL/new.bundle
	$HTTPD_URL/everything.bundle
	EOF

	test_remote_https_urls <trace-clone.txt >actual &&
	test_cmp expect actual

Thanks for the inspiration.

>> +test_expect_success 'clone bundle list (http, creationToken)' '
>> +	cp clone-from/bundle-*.bundle "$HTTPD_DOCUMENT_ROOT_PATH/" &&
>> +	cat >"$HTTPD_DOCUMENT_ROOT_PATH/bundle-list" <<-EOF &&
>> +	[bundle]
>> +		version = 1
>> +		mode = all
>> +		heuristic = creationToken
>> +
>> +	[bundle "bundle-1"]
>> +		uri = bundle-1.bundle
>> +		creationToken = 1
>> +
>> +	[bundle "bundle-2"]
>> +		uri = bundle-2.bundle
>> +		creationToken = 2
>> +
>> +	[bundle "bundle-3"]
>> +		uri = bundle-3.bundle
>> +		creationToken = 3
>> +
>> +	[bundle "bundle-4"]
>> +		uri = bundle-4.bundle
>> +		creationToken = 4
>> +	EOF
>> +
>> +	git clone --bundle-uri="$HTTPD_URL/bundle-list" . clone-list-http-2 &&
>> +
>> +	git -C clone-from for-each-ref --format="%(objectname)" >oids &&
>> +	git -C clone-list-http-2 cat-file --batch-check <oids
> 
> This test looks like the one that was updated above, but adds the
> 'creationToken' heuristic key. However, the 'test_bundle_downloaded' check
> isn't included - if it were, it would need to verify that all bundles were
> downloaded, with the heuristic being ignored, all bundles will be downloaded
> (which isn't consistent with what the 'creationToken' heuristic will
> *eventually* do).
> 
> As a matter of personal preference (so no pressure to change if you
> disagree), I find this test in its current state a bit misleading; because
> it's a 'test_expect_success' and there's no "NEEDSWORK" or "TODO", I could
> easily assume that cloning from a bundle list with the 'creationToken'
> heuristic is working as-intended at this point (that is, there's no
> indication that it's not implemented). 

It's true that it's not implemented right now, and it is a bit misleading
because of that. At clone time, the only thing that will change with the
implementation is possibly the order of the files being downloaded (and
the order is not predictable before that implementation).

The restriction of _not_ downloading some files comes only for the 'git
fetch' implementation, so these test changes are only foundations for those
future tests.

The only benefit of having these tests right now is that we get some
demonstration that the existing implementation ignores unknown properties
in the bundle list.

> If you did want to change it, adding a 'NEEDSWORK' comment, changing to
> 'test_expect_failure' & including the appropriate 'test_bundle_downloaded'
> check, or moving this test to the patch where the heuristic is implemented
> would mitigate any confusion. That said, this "issue" is resolved by the
> end of the series anyway, so it's really a low priority fix.
I think if we wanted to go this route, we could do it with the "download
the bundles in this order" check, or possibly by adding the 'git fetch'
behavior into the test at this point. I'll consider these options for v2.

Thanks,
-Stolee