Re: [PATCH 8/8] bundle-uri: store fetch.bundleCreationToken

[Derrick Stolee <derrickstolee@github.com>]

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@github.com/

I'll be sure to follow up and make these test changes be of higher value,
avoiding these confusions.

Thanks,
-Stolee