On Tue, Jul 13 2021, Jeff Hostetler wrote:
> On 7/1/21 6:58 PM, Ævar Arnfjörð Bjarmason wrote:
>> On Thu, Jul 01 2021, Jeff Hostetler via GitGitGadget wrote:
>>
>>> + if (!test_env_value) {
>>> + struct timeval tv;
>>> + struct tm tm;
>>> + time_t secs;
>>> +
>>> + gettimeofday(&tv, NULL);
>>> + secs = tv.tv_sec;
>>> + gmtime_r(&secs, &tm);
>>> +
>>> + strbuf_addf(&token->token_id,
>>> + "%"PRIu64".%d.%4d%02d%02dT%02d%02d%02d.%06ldZ",
>>> + flush_count++,
>>> + getpid(),
>>> + tm.tm_year + 1900, tm.tm_mon + 1, tm.tm_mday,
>>> + tm.tm_hour, tm.tm_min, tm.tm_sec,
>>> + (long)tv.tv_usec);
>> Just bikeshedding, but can we have tokens that mostly sort
>> numeric-wise
>> by time order? So time at the start, not the flush_count/getpid.
>
> As I described in a rather large comment in the code, tokens are opaque
> strings -- without a less-than / greater-than relationship -- just a
> random string that the daemon can use (along with a sequence number) to
> ensure that a later request is well-defined.
>
> Here I'm using a counter, pid, and date-stamp. I'd prefer using a GUID
> or UUID just to drive that home, but I didn't want to add a new .lib or
> .a to the build if not necessary.
>
> Perhaps I should compute this portion as hex(hash(time())) to remove the
> temptation to look inside my opaque token ??
Why does it matter if someone looks inside your opaque token if the code
is treating it as opaque by just doing a strcmp(old,new) on it?
I just suggested this as a debugging aid, i.e. when you the human (as
opposed to the program) are looking at this behavior it's handy to look
at the token and see that your cookies don't match, and that they look
to be N seconds apart.
And furthermore, if git crashes or whatever you can now easily look up
what process crashed if you've got the leftover cookie, if you've also
got trace2 logs.
>> Maybe I'm missing something, but couldn't we just re-use the trace2
>> SID
>> + a more trivial trailer? It would have the nice property that you could
>> find the trace2 SID whenever you looked at such a token (could
>> e.g. split them by "/" too), and add the tv_usec, flush_count+whatever
>> else is needed to make it unique after the "/", no?
>>
>
> I would rather keep Trace2 out of this. The SID is another opaque
> string and I don't want to reach inside it.
For the purposes of the git.git codebase it's fine to reach inside of
it, especially for a "I'd like a near-enough-UUID, and I know the trace2
SID already does that per-program", so you just need e.g. a sequence
counter within the program to ensure global uniqueness with other git
processes for such a cookie.
