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

From: René Scharfe <l.s.r@web.de>
Date: Jun 24, 2017, 13:17 UTC
Message-ID: <9f3720e5-b8be-978d-4f6c-8c082aef3680@web.de>
In-Reply-To: <20170624121452.7952-2-avarab@gmail.com>

Am 24.06.2017 um 14:14 schrieb Ævar Arnfjörð Bjarmason:

Show 11 quoted lines

> 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 suppressed by converting it to an empty
> string, 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). The intent was to use
> this API in the future to pass a custom leave the door open to pass a
> custom timezone name to the function (see my [1] and related
> messages).

"leave the door open to pass a" seems redundant.

> But that's not what this code does now, and this strbuf_addstr() call
> always being redundant makes it hard to understand the current
> functionality. So simplify this internal API to match its use, we can
> always change it in the future if it gets a different use-case.

I don't understand the confusion, but of course I'm biased. And I don't like binary parameters in general and would use named flags or two function names in most cases. But that aside I find the description hard to follow (perhaps I should do something about my attention span).

Here's an attempt at a commit message that would have be easier to understand for me:

   strbuf_addstr() allows callers to pass a time zone name for expanding
   %Z.  The only current caller either passes the empty string or NULL,
   in which case %Z is handed over verbatim to strftime(3).  Replace that
   string parameter with a flag controlling whether to remove %Z from the
   format specification.  This simplifies the code.

René