Re: [PATCH 3/8] bundle-uri: parse bundle.<id>.creationToken values

From: Derrick Stolee <derrickstolee@github.com>
Date: Jan 9, 2023, 14:41 UTC
Message-ID: <3c0db749-5040-24eb-1b26-969e7c768906@github.com>
In-Reply-To: <xmqq8ricxv52.fsf@gitster.g>

On 1/8/2023 10:08 PM, Junio C Hamano wrote:

Show 12 quoted lines

> "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.

Show 12 quoted lines

>> +	/**
>> +	 * 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