Re: [PATCH 8/8] bundle-uri: store fetch.bundleCreationToken
- From
Derrick Stolee <derrickstolee@github.com>
- Date
- Jan 20, 2023, 15:53 UTC
- Message-ID
- <7853f7f3-d999-5b39-ab6f-c43055c65085@github.com>
- In-Reply-To
- <f7a777f0-3f37-86a1-f520-1ab6853b9ed1@github.com>
On 1/19/2023 5:24 PM, Victoria Dye wrote:
> Derrick Stolee via GitGitGadget wrote:
Show 31 quoted lines
>> + /*
>> + * 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.
Show 11 quoted lines
>> 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@github.com/
I'll be sure to follow up and make these test changes be of higher value, avoiding these confusions.
Thanks, -Stolee