On 1/19/2023 5:24 PM, Victoria Dye wrote:
> Derrick Stolee via GitGitGadget wrote:
>> + /*
>> + * If fetch.bundleCreationToken exists, parses to a uint64t, and
>> + * is not strictly smaller than the maximum creation token in the
>> + * bundle list, then do not download any bundles.
>> + */
>> + if (!repo_config_get_value(r,
>> + "fetch.bundlecreationtoken",
>> + &creationTokenStr) &&
>> + sscanf(creationTokenStr, "%"PRIu64, &maxCreationToken) == 1 &&
>> + sorted.items[0]->creationToken <= maxCreationToken) {
>> + free(sorted.items);
>> + return 0;
>> + }
>
> And here, we exit if the cached creation token is greater than or equal to
> the highest advertised token. Overall, this seems pretty safe:
>
> - If the value is (somehow) manually updated to something larger than it
> should be, objects will be fetched from the server that could have
> otherwise come from a bundle. Not ideal, but no worse than a regular
> fetch.
> - If the value is too small or invalid (i.e., not an unsigned integer),
> we'll download the first bundle, unbundle it, then overwrite the invalid
> 'fetch.bundlecreationtoken' with a new valid one.
>
> The latter is self-correcting, but should the former be documented
> somewhere? For example, if someone changes which bundle server they're using
> with a repo, the creation token numbering scheme might be completely
> different. In that case, a user would probably want to "reset" the
> 'fetch.bundlecreationtoken' and start fresh with a new bundle list (even if
> the recommended method is "manually delete the config key from your repo").
I can update the config documentation to include this.
>> diff --git a/t/t5558-clone-bundle-uri.sh b/t/t5558-clone-bundle-uri.sh
>> index 3f4d61a915c..0604d721f1b 100755
>> --- a/t/t5558-clone-bundle-uri.sh
>> +++ b/t/t5558-clone-bundle-uri.sh
>
> It isn't exactly related to this patch, but it looks like the second "clone
> bundle list (http, creationToken)" never received any updates to
> differentiate it from the first test with that name (I noted the duplicate
> name in patch 4 [2]). Is it just a leftover duplicate?
>
> [1] https://lore.kernel.org/git/ede340d1-bce4-0c1d-7afb-4874a67d1803@xxxxxxxxxx/
I'll be sure to follow up and make these test changes be of higher value,
avoiding these confusions.
Thanks,
-Stolee
