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

From: Jeff King <peff@peff.net>
Date: Jun 24, 2017, 12:02 UTC
Message-ID: <20170624120259.5kbmkww66tymxb5i@sigill.intra.peff.net>
In-Reply-To: <20170624113635.16360-2-avarab@gmail.com>

On Sat, Jun 24, 2017 at 11:36:35AM +0000, Ævar Arnfjörð Bjarmason wrote:

Show 8 quoted lines

> >>  extern void strbuf_addftime(struct strbuf *sb, const char *fmt,
> >>  			    const struct tm *tm, int tz_offset,
> >> -			    const char *tz_name);
> >> +			    const int omit_strftime_tz_name);
> >
> > This would need the new name, too (whatever it is).
> 
> *Nod*. Now the parameter is called suppress_tz_name.

Thanks. That sounds good (and your initial re-ordering patch looks fine, too). One minor typo:

Show 11 quoted lines

> diff --git a/strbuf.h b/strbuf.h
> index 6708cef0f9..d3e6e65123 100644
> --- a/strbuf.h
> +++ b/strbuf.h
> @@ -343,11 +343,12 @@ extern void strbuf_vaddf(struct strbuf *sb, const char *fmt, va_list ap);
>   * `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`.
> - * `tz_name` is used to expand %Z internally unless it's NULL.
> + * `suppress_tz_name` when set, means let suppress the `strftime` %Z
> + * format and replace it with an empty string.

I couldn't quite parse "let suppress". I'm not sure if it was supposed to be "let's". Probably "means to suppress the strftime..." would be more clear. I'd probably have written it more like:

  `suppress_tz_name`, when set, expands %Z internally to the empty
  string rather than passing it to `strftime`.

-Peff