Re: [PATCH 2/8] bundle-uri: parse bundle.heuristic=creationToken
- From
Victoria Dye <vdye@github.com>
- Date
- Jan 17, 2023, 19:13 UTC
- Message-ID
- <5fb1385a-3821-8e1f-b31c-2c38f87dc965@github.com>
- In-Reply-To
- <9007249b9488c23f00c2d498ffd520e4af8b37a4.1673037405.git.gitgitgadget@gmail.com>
Derrick Stolee via GitGitGadget wrote:
Show 37 quoted lines
> 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.
Show 26 quoted lines
> +
> 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...
Show 16 quoted lines
> +
> 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.
Show 27 quoted lines
> 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