| Message ID | ff629bc119b466dcd827f758b3d892fefd6a9703.1675171760.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 512fccf8a503bd8617fe46cb62c77480b83fbaea |
| Headers | show |
| Series | Bundle URIs V: creationToken heuristic for incremental fetches | expand |
"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes: > + if (info->creationToken) > + fprintf(fp, "\tcreationToken = %"PRIu64"\n", info->creationToken); So, a bundle info with .creationToken set to 0 signals that the token is missing for that bundle. REMOTE_BUNDLE_INFO_INIT clears all members, so unless we find a concrete value and assign to the member, we'll have 0 in the member (and never a random value). Very good. We used to avoid camelCased variable names and member names and instead prefer snake_case; I wonder if it is still the case. Looking at bundle.h it still seems to be the case, and we may want to match that. > @@ -203,6 +206,13 @@ static int bundle_list_update(const char *key, const char *value, > return 0; > } > > + if (!strcmp(subkey, "creationtoken")) { > + if (sscanf(value, "%"PRIu64, &bundle->creationToken) != 1) > + warning(_("could not parse bundle list key %s with value '%s'"), > + "creationToken", value); > + return 0; > + } Should this be a warning, or a hard error? I _think_ it depends on how much we trust creationToken supplied by the bundle providers for correctness. If we only use the values as hint to gain performance, and downloading the bundles in a wrong order (due to bogus creationToken values assigned to them) does not lead to correctness error, then warning is fine. Looking good.
diff --git a/bundle-uri.c b/bundle-uri.c index 36ec542718d..d4277b2e3a7 100644 --- a/bundle-uri.c +++ b/bundle-uri.c @@ -83,6 +83,9 @@ static int summarize_bundle(struct remote_bundle_info *info, void *data) FILE *fp = data; fprintf(fp, "[bundle \"%s\"]\n", info->id); fprintf(fp, "\turi = %s\n", info->uri); + + if (info->creationToken) + fprintf(fp, "\tcreationToken = %"PRIu64"\n", info->creationToken); return 0; } @@ -203,6 +206,13 @@ static int bundle_list_update(const char *key, const char *value, return 0; } + if (!strcmp(subkey, "creationtoken")) { + if (sscanf(value, "%"PRIu64, &bundle->creationToken) != 1) + warning(_("could not parse bundle list key %s with value '%s'"), + "creationToken", value); + return 0; + } + /* * At this point, we ignore any information that we don't * understand, assuming it to be hints for a heuristic the client diff --git a/bundle-uri.h b/bundle-uri.h index 2e44a50a90b..ef32840bfa6 100644 --- a/bundle-uri.h +++ b/bundle-uri.h @@ -42,6 +42,12 @@ struct remote_bundle_info { * this boolean is true. */ unsigned unbundled:1; + + /** + * If the bundle is part of a list with the creationToken + * heuristic, then we use this member for sorting the bundles. + */ + uint64_t creationToken; }; #define REMOTE_BUNDLE_INFO_INIT { 0 } diff --git a/t/t5750-bundle-uri-parse.sh b/t/t5750-bundle-uri-parse.sh index 6fc92a9c0d4..81bdf58b944 100755 --- a/t/t5750-bundle-uri-parse.sh +++ b/t/t5750-bundle-uri-parse.sh @@ -258,10 +258,13 @@ test_expect_success 'parse config format: creationToken heuristic' ' heuristic = creationToken [bundle "one"] uri = http://example.com/bundle.bdl + creationToken = 123456 [bundle "two"] uri = https://example.com/bundle.bdl + creationToken = 12345678901234567890 [bundle "three"] uri = file:///usr/share/git/bundle.bdl + creationToken = 1 EOF test-tool bundle-uri parse-config expect >actual 2>err && @@ -269,4 +272,19 @@ test_expect_success 'parse config format: creationToken heuristic' ' test_cmp_config_output expect actual ' +test_expect_success 'parse config format edge cases: creationToken heuristic' ' + cat >expect <<-\EOF && + [bundle] + version = 1 + mode = all + heuristic = creationToken + [bundle "one"] + uri = http://example.com/bundle.bdl + creationToken = bogus + EOF + + test-tool bundle-uri parse-config expect >actual 2>err && + grep "could not parse bundle list key creationToken with value '\''bogus'\''" err +' + test_done