| Message ID | a1808f0b10cfb519613bc292e30b884962a83275.1673037405.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 (!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; > + } We tend to avoid sscanf() to parse out integral values, as it is a bit too permissive to our liking (especially while parsing the object header), but here it probably is OK, I guess. > + /** > + * If the bundle is part of a list with the creationToken > + * heuristic, then we use this member for sorting the bundles. > + */ > + uint64_t creationToken; > }; Is the idea behind the type is that creationTokens, while we leave up to the bundle providers what the actual values (other than zero) mean, must be comparable to give them a total order, and uint64 would be a usable type for bundle providers to come up with such values easily? Thanks.
On 1/8/2023 10:08 PM, Junio C Hamano wrote: > "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes: > >> + 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; >> + } > > We tend to avoid sscanf() to parse out integral values, as it is a > bit too permissive to our liking (especially while parsing the > object header), but here it probably is OK, I guess. I tried to find another way to parse uint64ts, but could not find another example in the codebase. I'd be happy to change it if we have a preferred method. >> + /** >> + * If the bundle is part of a list with the creationToken >> + * heuristic, then we use this member for sorting the bundles. >> + */ >> + uint64_t creationToken; >> }; > > Is the idea behind the type is that creationTokens, while we leave > up to the bundle providers what the actual values (other than zero) > mean, must be comparable to give them a total order, and uint64 > would be a usable type for bundle providers to come up with such > values easily? One easy way to create the total order is to use Unix epoch timestamps (on the same machine, to avoid clock skew). We cannot use the machine-local width of the timestamp types, though. And there is no need to use timestamp-like values. The bundle provider could keep an integer count. I did add a test that ensures we test a creationToken that would not fit within 32 bits. Thanks, -Stolee
Derrick Stolee via GitGitGadget wrote: > From: Derrick Stolee <derrickstolee@github.com> > > The previous change taught Git to parse the bundle.heuristic value, > especially when its value is "creationToken". Now, teach Git to parse > the bundle.<id>.creationToken values on each bundle in a bundle list. > > Before implementing any logic based on creationToken values for the > creationToken heuristic, parse and print these values for testing > purposes. > > Signed-off-by: Derrick Stolee <derrickstolee@github.com> > --- > bundle-uri.c | 10 ++++++++++ > bundle-uri.h | 6 ++++++ > t/t5750-bundle-uri-parse.sh | 18 ++++++++++++++++++ > 3 files changed, 34 insertions(+) > > diff --git a/bundle-uri.c b/bundle-uri.c > index 56c94595c2a..63e2cc21057 100644 > --- a/bundle-uri.c > +++ b/bundle-uri.c > @@ -80,6 +80,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; > } > > @@ -190,6 +193,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; > + } Like the 'heuristic' key in the last patch, the parsing of 'creationToken' is pretty straightforward. > 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 > +' And the tests cover both valid and invalid cases nicely. > + > test_done
diff --git a/bundle-uri.c b/bundle-uri.c index 56c94595c2a..63e2cc21057 100644 --- a/bundle-uri.c +++ b/bundle-uri.c @@ -80,6 +80,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; } @@ -190,6 +193,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 ad82174112d..1cae418211b 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