git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date
Jun 23, 2017, 15:13 UTC
Message-ID
<8760fmka8t.fsf@gmail.com>
In-Reply-To
<20170623145102.h7rt6zaqajfzuhsk@sigill.intra.peff.net>
On Fri, Jun 23 2017, Jeff King jotted:
Show 24 quoted lines
> 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().

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.

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.

Previous: Jeff KingNext: Jeff King
Message 3 of 29 in “strbuf: change an always NULL/"" strbuf_addftime() param to bool”
  1. strbuf: change an always NULL/"" strbuf_addftime() param to boolÆvar Arnfjörð Bjarmason, Jun 23, 2017
  2. Jeff KingJun 23, 2017
  3. Ævar Arnfjörð BjarmasonJun 23, 2017
  4. Jeff KingJun 23, 2017
  5. René ScharfeJun 23, 2017
  6. Jeff KingJun 23, 2017
  7. Ævar Arnfjörð BjarmasonJun 24, 2017
  8. strbuf: change an always NULL/"" strbuf_addftime() param to boolÆvar Arnfjörð Bjarmason, Jun 23, 2017
  9. Jeff KingJun 23, 2017
  10. 1/2 strbuf.h comment: discuss strbuf_addftime() arguments in orderÆvar Arnfjörð Bjarmason, Jun 24, 2017
  11. 2/2 strbuf: change an always NULL/"" strbuf_addftime() param to boolÆvar Arnfjörð Bjarmason, Jun 24, 2017
  12. Jeff KingJun 24, 2017
  13. 1/2 strbuf.h comment: discuss strbuf_addftime() arguments in orderÆvar Arnfjörð Bjarmason, Jun 24, 2017
  14. 2/2 strbuf: change an always NULL/"" strbuf_addftime() param to boolÆvar Arnfjörð Bjarmason, Jun 24, 2017
  15. Jeff KingJun 24, 2017
  16. 1/2 strbuf.h comment: discuss strbuf_addftime() arguments in orderÆvar Arnfjörð Bjarmason, Jun 24, 2017
  17. 2/2 strbuf: change an always NULL/"" strbuf_addftime() param to boolÆvar Arnfjörð Bjarmason, Jun 24, 2017
  18. Jeff KingJun 24, 2017
  19. René ScharfeJun 24, 2017
  20. Junio C HamanoJun 24, 2017
  21. 1/2 strbuf.h comment: discuss strbuf_addftime() arguments in orderÆvar Arnfjörð Bjarmason, Jul 1, 2017
  22. 2/2 strbuf: change an always NULL/"" strbuf_addftime() param to boolÆvar Arnfjörð Bjarmason, Jul 1, 2017
  23. René ScharfeJul 1, 2017
  24. 1/2 strbuf.h comment: discuss strbuf_addftime() arguments in orderÆvar Arnfjörð Bjarmason, Jul 1, 2017
  25. 2/2 strbuf: change an always NULL/"" strbuf_addftime() param to boolÆvar Arnfjörð Bjarmason, Jul 1, 2017
  26. Junio C HamanoJun 23, 2017
  27. René ScharfeJun 23, 2017
  28. Jeff KingJun 23, 2017
  29. René ScharfeJun 23, 2017

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.