Subject: Re: [PATCH v3 05/11] bundle-uri: parse bundle.<id>.creationToken values

From: Junio C Hamano <[email-masked]>
Date: Tue, 31 Jan 2023 13:22:45 -0800

"Derrick Stolee via GitGitGadget" <[email-masked]> 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.