Re: [PATCH 2/2] trace2: randomize/timestamp trace2 targets

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 14, 2019, 06:07 UTC
Message-ID: <xmqqwol27zv4.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <20190314001600.GC31968@sigill.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 5 quoted lines

> This definitely seems useful. Could we drop the final "%" and make it
> either a single-character "%t" or "%(iso8601)" to match our other
> formatting strings? There's no _technical_ reason to do that, but it
> just seems like there's not much point in being unnecessarily
> inconsistent.

I do agree that %TOKEN% is not among the patterns we've used before, and I fully anticipated that it would attract paintbrushes when the proposal hits the public list ;-)

A big question is what we want to be consisten with, though. From the readability point-of-view, for-each-ref language %(token) is easier to read and extend. A secondary question is that there likely are things other than the timestamp of the time process started that may want to be interpolated, so we may want to pick some useful vocabulary upfront. If we can declare interpolation will be done ONLY for timestamps, as Ævar suggests, taking strftime pattern directly from the caller may be sufficiently simple to explain and useful, but I am not sure "only for timestamps" is a limitation we would want to adopt this early in the design process.

Show 8 quoted lines

> Doing this automatically for directories feels kind of magical. I'd have
> expected it to be just another placeholder. And in fact, I'd think using
> the process id as a placeholder would be pretty common. Between
> timestamp and pid, that's generally unique (though I'm not opposed to
> the random placeholder if somebody really wants to be thorough).
>
> That leaves the door open for being able to append to or overwrite
> existing trace files later, if we want to make that a possibility.

Thanks.