Re: [PATCH] strbuf: change an always NULL/"" strbuf_addftime() param to bool

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 23, 2017, 17:13 UTC
Message-ID: <xmqqa84yfwzt.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20170623145102.h7rt6zaqajfzuhsk@sigill.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 5 quoted lines

> The idea was that eventually the caller might be able to come up with a
> TZ that is not blank, but is also not what strftime("%Z") would produce.
> Conceivably that could be done if Git commits carried the "%Z"
> information (not likely), or if we used a reverse-lookup table (also not
> likely).

The former might be conceivable, but I do not think the latter is workable. Knowing that a location happened to be using a particular GMT offset at a specific point in time simply is not sufficient to give us a zone name; the whole idea of a zone name being that it will give us rules that would apply to other timestamps, not just the one we are paring with GMT offset in our committer and author timestamp fields.

A third possibility is the information may come out of band. Something like "When gitster is in +0900 he is in JST" can be maintained per project and supplied by the caller.

> This closes the door on that.  Since we don't have immediate plans to go
> that route, I'm OK with this patch. It would be easy enough to re-open
> the door if we change our minds later.

I agree that it is not too much hassle to revert this change. I actually would not have minded if René's original were written with a boolean flag. But I do not see the value in flipping ("" vs NULL) with a bool now. The benefit we are gaining (other than closing the door) is unclear to me.

Show 12 quoted lines

>>  /**
>>   * Add the time specified by `tm`, as formatted by `strftime`.
>> - * `tz_name` is used to expand %Z internally unless it's NULL.
>>   * `tz_offset` is in decimal hhmm format, e.g. -600 means six hours west
>>   * of Greenwich, and it's used to expand %z internally.  However, tokens
>>   * with modifiers (e.g. %Ez) are passed to `strftime`.
>> + * `omit_strftime_tz_name` when set, means don't let `strftime` format
>> + * %Z, instead do our own formatting.
>
> Since we now always turn it into a blank string, perhaps "do our own
> formatting" could be more descriptive: we convert it into the empty
> string.

Yeah, that reads better. I am also OK if this said that an empty string is an accepted POSIX way to fallback when the information is unavailable---and we really are in that "information is unavailable" situation in this code.