From: René Scharfe <l.s.r@web.de>
Date: Fri, 23 Jun 2017 16:22:59 GMT
Subject: Re: [PATCH] strbuf: change an always NULL/"" strbuf_addftime() param to bool
Message-ID: <eb492fbd-5cf8-4014-acee-d9aca84dea24@web.de>
In-Reply-To: <20170623152554.jybv2vtsnjsoedoi@sigill.intra.peff.net>

Am 23.06.2017 um 17:25 schrieb Jeff King:
> On Fri, Jun 23, 2017 at 05:20:19PM +0200, René Scharfe wrote:
> 
>>> diff --git a/strbuf.c b/strbuf.c
>>> index be3b9e37b1..81ff3570e2 100644
>>> --- a/strbuf.c
>>> +++ b/strbuf.c
>>> @@ -786,7 +786,7 @@ char *xstrfmt(const char *fmt, ...)
>>>    }
>>>    void strbuf_addftime(struct strbuf *sb, const char *fmt, const struct tm *tm,
>>> -		     int tz_offset, const char *tz_name)
>>> +		     int tz_offset, const int omit_strftime_tz_name)
>>
>> Why const?  And as written above, naming the parameter local would make
>> it easier to understand instead of exposing an implementation detail in
>> the interface.
> 
> I think calling it "local" isn't right. That's a decision the _caller_
> is making about whether to pass through %Z. But the actual
> implementation is more like "should the function fill tzname based on
> tz?" So some name along those lines would make sense.
> 
> In which case the caller would then pass "!mode->local" for the flag.

We only have a single caller currently, so responsibilities can still be
shifted, and it's a bit hard to draw the line.  "Here's a format and all
time information I have, expand!" is just as viable as "here's a format
and most time information, expand, and handle %Z in this particular way
when you see it!", I think.

René