Re: [PATCH v2 10/10] bundle-uri: test missing bundles with heuristic

[Victoria Dye <vdye@github.com>]

Derrick Stolee via GitGitGadget wrote:
> From: Derrick Stolee <derrickstolee@github.com>
> 
> The creationToken heuristic uses a different mechanism for downloading
> bundles from the "standard" approach. Specifically: it uses a concrete
> order based on the creationToken values and attempts to download as few
> bundles as possible. It also modifies local config to store a value for
> future fetches to avoid downloading bundles, if possible.
> 
> However, if any of the individual bundles has a failed download, then
> the logic for the ordering comes into question. It is important to avoid
> infinite loops, assigning invalid creation token values in config, but
> also to be opportunistic as possible when downloading as many bundles as
> seem appropriate.
> 
> These tests were used to inform the implementation of
> fetch_bundles_by_token() in bundle-uri.c, but are being added
> independently here to allow focusing on faulty downloads. There may be
> more cases that could be added that result in modifications to
> fetch_bundles_by_token() as interesting data shapes reveal themselves in
> real scenarios.
> 

The expanded testing is great, thanks for adding it!

> +	# Case 2: middle bundle does not exist, only two bundles can unbundle
> +	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 = fake.bundle
> +		creationToken = 2
> +
> +	[bundle "bundle-3"]
> +		uri = bundle-3.bundle
> +		creationToken = 3
> +
> +	[bundle "bundle-4"]
> +		uri = bundle-4.bundle
> +		creationToken = 4
> +	EOF
> +
> +	GIT_TRACE2_EVENT="$(pwd)/trace-clone-2.txt" \
> +	git clone --single-branch --branch=base \
> +		--bundle-uri="$HTTPD_URL/bundle-list" \
> +		"$HTTPD_URL/smart/fetch.git" download-2 &&
> +
> +	# Bundle failure does not set these configs.
> +	test_must_fail git -C download-2 config fetch.bundleuri &&
> +	test_must_fail git -C download-2 config fetch.bundlecreationtoken &&
> +
> +	cat >expect <<-EOF &&
> +	$HTTPD_URL/bundle-list
> +	$HTTPD_URL/bundle-4.bundle
> +	$HTTPD_URL/bundle-3.bundle
> +	$HTTPD_URL/fake.bundle
> +	$HTTPD_URL/bundle-1.bundle
> +	EOF
> +	test_remote_https_urls <trace-clone-2.txt >actual &&
> +	test_cmp expect actual &&
> +
> +	# Only base bundle unbundled.
> +	git -C download-2 for-each-ref --format="%(refname)" "refs/bundles/*" >refs &&
> +	cat >expect <<-EOF &&
> +	refs/bundles/base
> +	refs/bundles/right
> +	EOF
> +	test_cmp expect refs &&

Maybe I'm misreading, but I don't think the comment ("Only base bundle
unbundled") lines up with the expected bundle refs (both bundle-1
('refs/bundles/base') and bundle-3 ('refs/bundles/right') seem to be
unbundled). 

> +
> +	# Case 3: top bundle does not exist, rest unbundle fine.
> +	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 = fake.bundle
> +		creationToken = 4
> +	EOF
> +
> +	GIT_TRACE2_EVENT="$(pwd)/trace-clone-3.txt" \
> +	git clone --single-branch --branch=base \
> +		--bundle-uri="$HTTPD_URL/bundle-list" \
> +		"$HTTPD_URL/smart/fetch.git" download-3 &&
> +
> +	# As long as we have continguous successful downloads,
> +	# we _do_ set these configs.
> +	test_cmp_config -C download-3 "$HTTPD_URL/bundle-list" fetch.bundleuri &&
> +	test_cmp_config -C download-3 3 fetch.bundlecreationtoken &&
> +
> +	cat >expect <<-EOF &&
> +	$HTTPD_URL/bundle-list
> +	$HTTPD_URL/fake.bundle
> +	$HTTPD_URL/bundle-3.bundle
> +	$HTTPD_URL/bundle-2.bundle
> +	$HTTPD_URL/bundle-1.bundle
> +	EOF
> +	test_remote_https_urls <trace-clone-3.txt >actual &&
> +	test_cmp expect actual &&
> +
> +	# All bundles failed to unbundle
> +	git -C download-3 for-each-ref --format="%(refname)" "refs/bundles/*" >refs &&
> +	cat >expect <<-EOF &&
> +	refs/bundles/base
> +	refs/bundles/left
> +	refs/bundles/right
> +	EOF
> +	test_cmp expect refs

Similar issue with the comment here - it says that all bundles *failed* to
unbundle, but the test case description ("Case 3: top bundle does not exist,
rest unbundle fine.") and the result show bundle-1, bundle-2, and bundle-3
all unbundling successfully.

> +'
> +
> +# Expand the bundle list to include other interesting shapes, specifically
> +# interesting for use when fetching from a previous state.
> +#
> +# ---------------- bundle-7
> +#       7
> +#     _/|\_
> +# ---/--|--\------ bundle-6
> +#   5   |   6
> +# --|---|---|----- bundle-4
> +#   |   4   |
> +#   |  / \  /
> +# --|-|---|/------ bundle-3 (the client will be caught up to this point.)
> +#   \ |   3
> +# ---\|---|------- bundle-2
> +#     2   |
> +# ----|---|------- bundle-1
> +#      \ /
> +#       1
> +#       |
> +# (previous commits)

...

> +	# Case 1: all bundles exist: successful unbundling of all bundles

...

> +	# Case 2: middle bundle does not exist, only bundle-4 can unbundle

...

> +	# Case 3: top bundle does not exist, rest unbundle fine.

The rest of these cases look okay and, at a high-level, it's helpful to have
these additional tests covering a different topology.