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

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Fri, Jun 23 2017, Jeff King jotted:

> On Fri, Jun 23, 2017 at 02:46:03PM +0000, Ævar Arnfjörð Bjarmason wrote:
>
>> Change the code for deciding what's to be done about %Z to stop
>> passing always either a NULL or "" char * to
>> strbuf_addftime(). Instead pass a boolean int to indicate whether the
>> strftime() %Z format should be omitted, which is what this code is
>> actually doing.
>>
>> This code grew organically between the changes in 9eafe86d58 ("Merge
>> branch 'rs/strbuf-addftime-zZ'", 2017-06-22) yielding an end result
>> that wasn't very readable. Out of context it looked as though the call
>> to strbuf_addstr() might be adding a custom tz_name to the string, but
>> actually tz_name would always be "", so the call to strbuf_addstr()
>> just to add an empty string to the format was pointless.
>
> 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).
>
> 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.

Closes the door on doing that via passing the char * of the prepared
custom tz_name to strbuf_addftime().

I have a WIP patch (which may not make it on-list, depending) playing
with the idea I proposed in
CACBZZX5OQc45fUyDVayE89rkT=+8m5S4efSXCAbCy7Upme5zLA@mail.gmail.com which
just inserts the custom TZ name based on the offset inside that `if
(omit_strftime_tz_name)` branch.

That seems like a more straightforward way to do it than passing the
name to strbuf_addftime().

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

Then we'd need to change this comment again if we had some patch like
the one I mentioned above, I thought it was better to just leave this
vague enough that we didn't need to do that.