Re: [PATCH 2/8] bundle-uri: parse bundle.heuristic=creationToken

From: Victoria Dye <vdye@github.com>
To: Derrick Stolee via GitGitGadget <gitgitgadget@gmail.com>,
	git@vger.kernel.org
Cc: gitster@pobox.com, me@ttaylorr.com, avarab@gmail.com,
	steadmon@google.com, chooglen@google.com,
	Derrick Stolee <derrickstolee@github.com>
Subject: Re: [PATCH 2/8] bundle-uri: parse bundle.heuristic=creationToken
Date: Tue, 17 Jan 2023 11:13:17 -0800	[thread overview]
Message-ID: <5fb1385a-3821-8e1f-b31c-2c38f87dc965@github.com> (raw)
In-Reply-To: <9007249b9488c23f00c2d498ffd520e4af8b37a4.1673037405.git.gitgitgadget@gmail.com>

Derrick Stolee via GitGitGadget wrote:
> From: Derrick Stolee <derrickstolee@github.com>
> 
> The bundle.heuristic value communicates that the bundle list is
> organized to make use of the bundle.<id>.creationToken values that may
> be provided in the bundle list. Those values will create a total order
> on the bundles, allowing the Git client to download them in a specific
> order and even remember previously-downloaded bundles by storing the
> maximum creation token value.
> 
> Before implementing any logic that parses or uses the
> bundle.<id>.creationToken values, teach Git to parse the
> bundle.heuristic value from a bundle list. We can use 'test-tool
> bundle-uri' to print the heuristic value and verify that the parsing
> works correctly.
> 
> Signed-off-by: Derrick Stolee <derrickstolee@github.com>
> ---
>  Documentation/config/bundle.txt |  7 +++++++
>  bundle-uri.c                    | 21 +++++++++++++++++++++
>  bundle-uri.h                    | 14 ++++++++++++++
>  t/t5750-bundle-uri-parse.sh     | 19 +++++++++++++++++++
>  4 files changed, 61 insertions(+)
> 
> diff --git a/Documentation/config/bundle.txt b/Documentation/config/bundle.txt
> index daa21eb674a..3faae386853 100644
> --- a/Documentation/config/bundle.txt
> +++ b/Documentation/config/bundle.txt
> @@ -15,6 +15,13 @@ bundle.mode::
>  	complete understanding of the bundled information (`all`) or if any one
>  	of the listed bundle URIs is sufficient (`any`).
>  
> +bundle.heuristic::
> +	If this string-valued key exists, then the bundle list is designed to
> +	work well with incremental `git fetch` commands. The heuristic signals
> +	that there are additional keys available for each bundle that help
> +	determine which subset of bundles the client should download. The
> +	only value currently understood is `creationToken`.

This description clearly describes the 'heuristic' key and what it does.

> +
>  bundle.<id>.*::
>  	The `bundle.<id>.*` keys are used to describe a single item in the
>  	bundle list, grouped under `<id>` for identification purposes.
> diff --git a/bundle-uri.c b/bundle-uri.c
> index 36268dda172..56c94595c2a 100644
> --- a/bundle-uri.c
> +++ b/bundle-uri.c
> @@ -9,6 +9,11 @@
>  #include "config.h"
>  #include "remote.h"
>  
> +static const char *heuristics[] = {
> +	[BUNDLE_HEURISTIC_NONE] = "",
> +	[BUNDLE_HEURISTIC_CREATIONTOKEN] = "creationToken",
> +};
> +
>  static int compare_bundles(const void *hashmap_cmp_fn_data,
>  			   const struct hashmap_entry *he1,
>  			   const struct hashmap_entry *he2,
> @@ -100,6 +105,9 @@ void print_bundle_list(FILE *fp, struct bundle_list *list)
>  	fprintf(fp, "\tversion = %d\n", list->version);
>  	fprintf(fp, "\tmode = %s\n", mode);
>  
> +	if (list->heuristic)
> +		printf("\theuristic = %s\n", heuristics[list->heuristic]);

Given this condition, the 'heuristic' key should not be sent if it's
'BUNDLE_HEURISTIC_NONE'. But, as a fallback...

> +
>  	for_all_bundles_in_list(list, summarize_bundle, fp);
>  }
>  
> @@ -142,6 +150,19 @@ static int bundle_list_update(const char *key, const char *value,
>  			return 0;
>  		}
>  
> +		if (!strcmp(subkey, "heuristic")) {
> +			int i;
> +			for (i = 0; i < BUNDLE_HEURISTIC__COUNT; i++) {
> +				if (!strcmp(value, heuristics[i])) {
> +					list->heuristic = i;
> +					return 0;
> +				}
> +			}

...this condition seems to handle 'BUNDLE_HEURISTIC_NONE' anyway. There's no
harm in this, since 'BUNDLE_HEURISTIC_NONE' is the default value of
'list->heuristic' anyway.

>  void init_bundle_list(struct bundle_list *list);
> diff --git a/t/t5750-bundle-uri-parse.sh b/t/t5750-bundle-uri-parse.sh
> index 7b4f930e532..6fc92a9c0d4 100755
> --- a/t/t5750-bundle-uri-parse.sh
> +++ b/t/t5750-bundle-uri-parse.sh
> @@ -250,4 +250,23 @@ test_expect_success 'parse config format edge cases: empty key or value' '
>  	test_cmp_config_output expect actual
>  '
>  
> +test_expect_success 'parse config format: creationToken heuristic' '
> +	cat >expect <<-\EOF &&
> +	[bundle]
> +		version = 1
> +		mode = all
> +		heuristic = creationToken
> +	[bundle "one"]
> +		uri = http://example.com/bundle.bdl
> +	[bundle "two"]
> +		uri = https://example.com/bundle.bdl
> +	[bundle "three"]
> +		uri = file:///usr/share/git/bundle.bdl
> +	EOF
> +
> +	test-tool bundle-uri parse-config expect >actual 2>err &&
> +	test_must_be_empty err &&
> +	test_cmp_config_output expect actual
> +'

And this test verifies that 'heuristic' is no longer being ignored. Looks
good!

> +
>  test_done