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

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

[Ævar Arnfjörð Bjarmason]

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.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 date.c   | 2 +-
 strbuf.c | 5 ++---
 strbuf.h | 5 +++--
 3 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/date.c b/date.c
index 1fd6d66375..5f09743bad 100644
--- a/date.c
+++ b/date.c
@@ -256,7 +256,7 @@ const char *show_date(timestamp_t time, int tz, const struct date_mode *mode)
 			tm->tm_hour, tm->tm_min, tm->tm_sec, tz);
 	else if (mode->type == DATE_STRFTIME)
 		strbuf_addftime(&timebuf, mode->strftime_fmt, tm, tz,
-				mode->local ? NULL : "");
+				mode->local ? 0 : 1);
 	else
 		strbuf_addf(&timebuf, "%.3s %.3s %d %02d:%02d:%02d %d%c%+05d",
 				weekday_names[tm->tm_wday],
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)
 {
 	struct strbuf munged_fmt = STRBUF_INIT;
 	size_t hint = 128;
@@ -815,8 +815,7 @@ void strbuf_addftime(struct strbuf *sb, const char *fmt, const struct tm *tm,
 			fmt++;
 			break;
 		case 'Z':
-			if (tz_name) {
-				strbuf_addstr(&munged_fmt, tz_name);
+			if (omit_strftime_tz_name) {
 				fmt++;
 				break;
 			}
diff --git a/strbuf.h b/strbuf.h
index 4559035c47..bad698058a 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -340,14 +340,15 @@ extern void strbuf_vaddf(struct strbuf *sb, const char *fmt, va_list ap);
 
 /**
  * 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.
  */
 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);
 
 /**
  * Read a given size of data from a FILE* pointer to the buffer.
-- 
2.13.1.611.g7e3b11ae1

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

[Jeff King]

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.

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

-Peff

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

[Ævar Arnfjörð Bjarmason]

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.

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

[Jeff King]

On Fri, Jun 23, 2017 at 05:13:38PM +0200, Ævar Arnfjörð Bjarmason wrote:

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

OK. I'd assumed that would all happen outside of strbuf_addftime(). But
if it happens inside, then I agree a flag is better.

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

Right, if you're going to do your own formatting inside the function,
then I agree the wording should be kept. But then "omit" is not really
the right word. Isn't it "tzname_from_tz" or something?

-Peff

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

[René Scharfe]

Am 23.06.2017 um 17:23 schrieb Jeff King:
> On Fri, Jun 23, 2017 at 05:13:38PM +0200, Ævar Arnfjörð Bjarmason wrote:
> 
>>> 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.
> 
> OK. I'd assumed that would all happen outside of strbuf_addftime(). But
> if it happens inside, then I agree a flag is better.

Oh, so the interface that was meant to allow better time zone names
without having to make strbuf_addftime() even bigger than it already is
turns out to be too ugly for its purpose?  I'm sorry. :(

René

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

[Jeff King]

On Fri, Jun 23, 2017 at 06:23:10PM +0200, René Scharfe wrote:

> > > 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.
> > 
> > OK. I'd assumed that would all happen outside of strbuf_addftime(). But
> > if it happens inside, then I agree a flag is better.
> 
> Oh, so the interface that was meant to allow better time zone names
> without having to make strbuf_addftime() even bigger than it already is
> turns out to be too ugly for its purpose?  I'm sorry. :(

I haven't seen Ævar's patch, but I agree that if the caller did:

  if (mode->local)
	tzname = NULL; /* let strftime handle it */
  else
	tzname = fake_tz_from_offset(tz);
  ...
  strbuf_addftime(&buf, fmt, tm, tz, tzname);

that would be pretty clean (and what I was expecting with the "I'd
assumed" above).

-Peff

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

[Ævar Arnfjörð Bjarmason]

On Fri, Jun 23 2017, René Scharfe jotted:

> Am 23.06.2017 um 17:23 schrieb Jeff King:
>> On Fri, Jun 23, 2017 at 05:13:38PM +0200, Ævar Arnfjörð Bjarmason wrote:
>>
>>>> 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.
>>
>> OK. I'd assumed that would all happen outside of strbuf_addftime(). But
>> if it happens inside, then I agree a flag is better.
>
> Oh, so the interface that was meant to allow better time zone names
> without having to make strbuf_addftime() even bigger than it already is
> turns out to be too ugly for its purpose?  I'm sorry. :(

I don't think it's ugly. My motivation for sending this patch is that I
started playing with this code and was confused because I thought that
strbuf_addstr(...) actually did something to the string, but it never
did.

Since it's a purely internal API used in just one place I thought it
made sense to adjust the prototype / code to its current usage for ease
of readability, if we want to do something else with it in the future
it'll be trivial to adjust it then.

But I don't feel strongly about this patch at all, it's just a minor
fixup I submitted while reading / playing with the code.

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

[Ævar Arnfjörð Bjarmason]

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 expanded 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) 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.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---

I believe this addresses the comments in the thread so far. Also Re:
René's "why const?" in a2673ce4-5cf8-6b40-d4db-8e2a49518138@web.de:
Because tzname_from_tz isn't changed in the body of the function, only
read.

 date.c   | 2 +-
 strbuf.c | 5 ++---
 strbuf.h | 5 +++--
 3 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/date.c b/date.c
index 1fd6d66375..17db07d905 100644
--- a/date.c
+++ b/date.c
@@ -256,7 +256,7 @@ const char *show_date(timestamp_t time, int tz, const struct date_mode *mode)
 			tm->tm_hour, tm->tm_min, tm->tm_sec, tz);
 	else if (mode->type == DATE_STRFTIME)
 		strbuf_addftime(&timebuf, mode->strftime_fmt, tm, tz,
-				mode->local ? NULL : "");
+				mode->local);
 	else
 		strbuf_addf(&timebuf, "%.3s %.3s %d %02d:%02d:%02d %d%c%+05d",
 				weekday_names[tm->tm_wday],
diff --git a/strbuf.c b/strbuf.c
index be3b9e37b1..92b7bda772 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 tzname_from_tz)
 {
 	struct strbuf munged_fmt = STRBUF_INIT;
 	size_t hint = 128;
@@ -815,8 +815,7 @@ void strbuf_addftime(struct strbuf *sb, const char *fmt, const struct tm *tm,
 			fmt++;
 			break;
 		case 'Z':
-			if (tz_name) {
-				strbuf_addstr(&munged_fmt, tz_name);
+			if (!tzname_from_tz) {
 				fmt++;
 				break;
 			}
diff --git a/strbuf.h b/strbuf.h
index 4559035c47..eba5d59a77 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -340,14 +340,15 @@ extern void strbuf_vaddf(struct strbuf *sb, const char *fmt, va_list ap);
 
 /**
  * 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`.
+ * `tzname_from_tz` when set, means let `strftime` format %Z, instead
+ * of intercepting it and doing our own formatting.
  */
 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);
 
 /**
  * Read a given size of data from a FILE* pointer to the buffer.
-- 
2.13.1.611.g7e3b11ae1

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

[Jeff King]

On Fri, Jun 23, 2017 at 04:36:06PM +0000, Ævar Arnfjörð Bjarmason wrote:

> I believe this addresses the comments in the thread so far. Also Re:
> René's "why const?" in a2673ce4-5cf8-6b40-d4db-8e2a49518138@web.de:
> Because tzname_from_tz isn't changed in the body of the function, only
> read.

Sure, it's not wrong. But that property is also held by 99% of the
parameters that are passed by value. It's the normal style in our code
base (and in most C code bases I know of) to never declare pass-by-value
as const. It pollutes the interface and isn't something the caller cares
about.

Without passing judgement on whether that style is good or not (though
IMHO it is), making this one case different than all the others is a bad
idea. It makes the reader wonder why it's different.

> diff --git a/date.c b/date.c
> index 1fd6d66375..17db07d905 100644
> --- a/date.c
> +++ b/date.c
> @@ -256,7 +256,7 @@ const char *show_date(timestamp_t time, int tz, const struct date_mode *mode)
>  			tm->tm_hour, tm->tm_min, tm->tm_sec, tz);
>  	else if (mode->type == DATE_STRFTIME)
>  		strbuf_addftime(&timebuf, mode->strftime_fmt, tm, tz,
> -				mode->local ? NULL : "");
> +				mode->local);

You flipped the boolean here. That's OK by me. But in the definition...

>  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 tzname_from_tz)

Wouldn't tzname_from_tz only happen when we're _not_ in local mode? I
suggested that name anticipating your second patch to actually compute
it based on "tz". In local-mode it's not coming from tz, it's coming
from secret unportable magic (the combination of localtime() and
strftime()).

> @@ -815,8 +815,7 @@ void strbuf_addftime(struct strbuf *sb, const char *fmt, const struct tm *tm,
>  			fmt++;
>  			break;
>  		case 'Z':
> -			if (tz_name) {
> -				strbuf_addstr(&munged_fmt, tz_name);
> +			if (!tzname_from_tz) {
>  				fmt++;
>  				break;
>  			}

This logic matches your inversion in the caller, so it does the right
thing. But I think the name is wrong, as above.

> index 4559035c47..eba5d59a77 100644
> --- a/strbuf.h
> +++ b/strbuf.h
> @@ -340,14 +340,15 @@ extern void strbuf_vaddf(struct strbuf *sb, const char *fmt, va_list ap);
>  
>  /**
>   * 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`.
> + * `tzname_from_tz` when set, means let `strftime` format %Z, instead
> + * of intercepting it and doing our own formatting.
>   */
>  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).

-Peff

[PATCH v3 1/2] strbuf.h comment: discuss strbuf_addftime() arguments in order

[Ævar Arnfjörð Bjarmason]

Change the comment documenting the strbuf_addftime() function to
discuss the parameters in the order in which they appear, which makes
this easier to read than discussing them out of order.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---

I though it was more readable to split out this change into its own
patch.

 strbuf.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/strbuf.h b/strbuf.h
index 4559035c47..6708cef0f9 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -340,10 +340,10 @@ extern void strbuf_vaddf(struct strbuf *sb, const char *fmt, va_list ap);
 
 /**
  * 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`.
+ * `tz_name` is used to expand %Z internally unless it's NULL.
  */
 extern void strbuf_addftime(struct strbuf *sb, const char *fmt,
 			    const struct tm *tm, int tz_offset,
-- 
2.13.1.611.g7e3b11ae1

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

[Ævar Arnfjörð Bjarmason]

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).

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.

1. CACBZZX5OQc45fUyDVayE89rkT=+8m5S4efSXCAbCy7Upme5zLA@mail.gmail.com
   (https://public-inbox.org/git/CACBZZX5OQc45fUyDVayE89rkT=+8m5S4efSXCAbCy7Upme5zLA@mail.gmail.com/)

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---

On Fri, Jun 23 2017, Jeff King jotted:

> On Fri, Jun 23, 2017 at 04:36:06PM +0000, Ævar Arnfjörð Bjarmason wrote:
>
>> I believe this addresses the comments in the thread so far. Also Re:
>> René's "why const?" in a2673ce4-5cf8-6b40-d4db-8e2a49518138@web.de:
>> Because tzname_from_tz isn't changed in the body of the function, only
>> read.
>
> Sure, it's not wrong. But that property is also held by 99% of the
> parameters that are passed by value. It's the normal style in our code
> base (and in most C code bases I know of) to never declare pass-by-value
> as const. It pollutes the interface and isn't something the caller cares
> about.
>
> Without passing judgement on whether that style is good or not (though
> IMHO it is), making this one case different than all the others is a bad
> idea. It makes the reader wonder why it's different.

Makes sense. I wasn't trying to be snary or curt or whatever. I'd just
never noticed this pattern in the codebase.

Seems a bit odd to me to not make use of the compiler guarding against
accidental assignments and giving it a strong hint to inline the value
where possible, but whatever, makes sense to have it stylistically be
consistent. So this version does that.

>> diff --git a/date.c b/date.c
>> index 1fd6d66375..17db07d905 100644
>> --- a/date.c
>> +++ b/date.c
>> @@ -256,7 +256,7 @@ const char *show_date(timestamp_t time, int tz, const struct date_mode *mode)
>>  			tm->tm_hour, tm->tm_min, tm->tm_sec, tz);
>>  	else if (mode->type == DATE_STRFTIME)
>>  		strbuf_addftime(&timebuf, mode->strftime_fmt, tm, tz,
>> -				mode->local ? NULL : "");
>> +				mode->local);
>
> You flipped the boolean here. That's OK by me. But in the definition...
>
>>  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 tzname_from_tz)
>
> Wouldn't tzname_from_tz only happen when we're _not_ in local mode? I
> suggested that name anticipating your second patch to actually compute
> it based on "tz". In local-mode it's not coming from tz, it's coming
> from secret unportable magic (the combination of localtime() and
> strftime()).

I misread (I think) an earlier E-Mail of yours and thought this was
what you were suggesting. This version hopefully looks OK.

>> @@ -815,8 +815,7 @@ void strbuf_addftime(struct strbuf *sb, const char *fmt, const struct tm *tm,
>>  			fmt++;
>>  			break;
>>  		case 'Z':
>> -			if (tz_name) {
>> -				strbuf_addstr(&munged_fmt, tz_name);
>> +			if (!tzname_from_tz) {
>>  				fmt++;
>>  				break;
>>  			}
>
> This logic matches your inversion in the caller, so it does the right
> thing. But I think the name is wrong, as above.

Fixed.

>> index 4559035c47..eba5d59a77 100644
>> --- a/strbuf.h
>> +++ b/strbuf.h
>> @@ -340,14 +340,15 @@ extern void strbuf_vaddf(struct strbuf *sb, const char *fmt, va_list ap);
>>  
>>  /**
>>   * 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`.
>> + * `tzname_from_tz` when set, means let `strftime` format %Z, instead
>> + * of intercepting it and doing our own formatting.
>>   */
>>  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.

 date.c   | 2 +-
 strbuf.c | 5 ++---
 strbuf.h | 5 +++--
 3 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/date.c b/date.c
index 1fd6d66375..c3e673fd04 100644
--- a/date.c
+++ b/date.c
@@ -256,7 +256,7 @@ const char *show_date(timestamp_t time, int tz, const struct date_mode *mode)
 			tm->tm_hour, tm->tm_min, tm->tm_sec, tz);
 	else if (mode->type == DATE_STRFTIME)
 		strbuf_addftime(&timebuf, mode->strftime_fmt, tm, tz,
-				mode->local ? NULL : "");
+				!mode->local);
 	else
 		strbuf_addf(&timebuf, "%.3s %.3s %d %02d:%02d:%02d %d%c%+05d",
 				weekday_names[tm->tm_wday],
diff --git a/strbuf.c b/strbuf.c
index be3b9e37b1..89e40bb496 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, int suppress_tz_name)
 {
 	struct strbuf munged_fmt = STRBUF_INIT;
 	size_t hint = 128;
@@ -815,8 +815,7 @@ void strbuf_addftime(struct strbuf *sb, const char *fmt, const struct tm *tm,
 			fmt++;
 			break;
 		case 'Z':
-			if (tz_name) {
-				strbuf_addstr(&munged_fmt, tz_name);
+			if (suppress_tz_name) {
 				fmt++;
 				break;
 			}
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.
  */
 extern void strbuf_addftime(struct strbuf *sb, const char *fmt,
 			    const struct tm *tm, int tz_offset,
-			    const char *tz_name);
+			    int suppress_tz_name);
 
 /**
  * Read a given size of data from a FILE* pointer to the buffer.
-- 
2.13.1.611.g7e3b11ae1

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

[Jeff King]

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

> >>  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:

> 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

[PATCH v4 1/2] strbuf.h comment: discuss strbuf_addftime() arguments in order

[Ævar Arnfjörð Bjarmason]

Change the comment documenting the strbuf_addftime() function to
discuss the parameters in the order in which they appear, which makes
this easier to read than discussing them out of order.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 strbuf.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/strbuf.h b/strbuf.h
index 4559035c47..6708cef0f9 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -340,10 +340,10 @@ extern void strbuf_vaddf(struct strbuf *sb, const char *fmt, va_list ap);
 
 /**
  * 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`.
+ * `tz_name` is used to expand %Z internally unless it's NULL.
  */
 extern void strbuf_addftime(struct strbuf *sb, const char *fmt,
 			    const struct tm *tm, int tz_offset,
-- 
2.13.1.611.g7e3b11ae1

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

[Ævar Arnfjörð Bjarmason]

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).

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.

1. CACBZZX5OQc45fUyDVayE89rkT=+8m5S4efSXCAbCy7Upme5zLA@mail.gmail.com
   (https://public-inbox.org/git/CACBZZX5OQc45fUyDVayE89rkT=+8m5S4efSXCAbCy7Upme5zLA@mail.gmail.com/)

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---

On Sat, Jun 24, 2017 at 2:02 PM, Jeff King <peff@peff.net> wrote:
> On Sat, Jun 24, 2017 at 11:36:35AM +0000, Ævar Arnfjörð Bjarmason wrote:
>
>> >>  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:
>
>> 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`.

Thanks. Docs fixed per your suggestion. I sent a v4 of 1/2 too, but
that's unchanged, just thought it was simpler than having just one
patch have a v4...

 date.c   | 2 +-
 strbuf.c | 5 ++---
 strbuf.h | 5 +++--
 3 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/date.c b/date.c
index 1fd6d66375..c3e673fd04 100644
--- a/date.c
+++ b/date.c
@@ -256,7 +256,7 @@ const char *show_date(timestamp_t time, int tz, const struct date_mode *mode)
 			tm->tm_hour, tm->tm_min, tm->tm_sec, tz);
 	else if (mode->type == DATE_STRFTIME)
 		strbuf_addftime(&timebuf, mode->strftime_fmt, tm, tz,
-				mode->local ? NULL : "");
+				!mode->local);
 	else
 		strbuf_addf(&timebuf, "%.3s %.3s %d %02d:%02d:%02d %d%c%+05d",
 				weekday_names[tm->tm_wday],
diff --git a/strbuf.c b/strbuf.c
index be3b9e37b1..89e40bb496 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, int suppress_tz_name)
 {
 	struct strbuf munged_fmt = STRBUF_INIT;
 	size_t hint = 128;
@@ -815,8 +815,7 @@ void strbuf_addftime(struct strbuf *sb, const char *fmt, const struct tm *tm,
 			fmt++;
 			break;
 		case 'Z':
-			if (tz_name) {
-				strbuf_addstr(&munged_fmt, tz_name);
+			if (suppress_tz_name) {
 				fmt++;
 				break;
 			}
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.
  */
 extern void strbuf_addftime(struct strbuf *sb, const char *fmt,
 			    const struct tm *tm, int tz_offset,
-			    const char *tz_name);
+			    int suppress_tz_name);
 
 /**
  * Read a given size of data from a FILE* pointer to the buffer.
-- 
2.13.1.611.g7e3b11ae1

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

[Jeff King]

On Sat, Jun 24, 2017 at 12:10:23PM +0000, Ævar Arnfjörð Bjarmason wrote:

> > 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`.
> 
> Thanks. Docs fixed per your suggestion. I sent a v4 of 1/2 too, but
> that's unchanged, just thought it was simpler than having just one
> patch have a v4...

Thanks, both of the v4 patches look OK to me.

-Peff

[PATCH v5 1/2] strbuf.h comment: discuss strbuf_addftime() arguments in order

[Ævar Arnfjörð Bjarmason]

Change the comment documenting the strbuf_addftime() function to
discuss the parameters in the order in which they appear, which makes
this easier to read than discussing them out of order.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 strbuf.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/strbuf.h b/strbuf.h
index 4559035c47..6708cef0f9 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -340,10 +340,10 @@ extern void strbuf_vaddf(struct strbuf *sb, const char *fmt, va_list ap);
 
 /**
  * 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`.
+ * `tz_name` is used to expand %Z internally unless it's NULL.
  */
 extern void strbuf_addftime(struct strbuf *sb, const char *fmt,
 			    const struct tm *tm, int tz_offset,
-- 
2.13.1.611.g7e3b11ae1

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

[Ævar Arnfjörð Bjarmason]

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).

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.

1. CACBZZX5OQc45fUyDVayE89rkT=+8m5S4efSXCAbCy7Upme5zLA@mail.gmail.com
   (https://public-inbox.org/git/CACBZZX5OQc45fUyDVayE89rkT=+8m5S4efSXCAbCy7Upme5zLA@mail.gmail.com/)

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---

On Sat, Jun 24, 2017 at 2:10 PM, Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
> Thanks. Docs fixed per your suggestion. I sent a v4 of 1/2 too, but
> that's unchanged, just thought it was simpler than having just one
> patch have a v4...

Urgh, mistake on my end, sent v3 again as v4. Here's v5 with the
*actual* fixes. Sorry.

 date.c   | 2 +-
 strbuf.c | 5 ++---
 strbuf.h | 5 +++--
 3 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/date.c b/date.c
index 1fd6d66375..c3e673fd04 100644
--- a/date.c
+++ b/date.c
@@ -256,7 +256,7 @@ const char *show_date(timestamp_t time, int tz, const struct date_mode *mode)
 			tm->tm_hour, tm->tm_min, tm->tm_sec, tz);
 	else if (mode->type == DATE_STRFTIME)
 		strbuf_addftime(&timebuf, mode->strftime_fmt, tm, tz,
-				mode->local ? NULL : "");
+				!mode->local);
 	else
 		strbuf_addf(&timebuf, "%.3s %.3s %d %02d:%02d:%02d %d%c%+05d",
 				weekday_names[tm->tm_wday],
diff --git a/strbuf.c b/strbuf.c
index be3b9e37b1..89e40bb496 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, int suppress_tz_name)
 {
 	struct strbuf munged_fmt = STRBUF_INIT;
 	size_t hint = 128;
@@ -815,8 +815,7 @@ void strbuf_addftime(struct strbuf *sb, const char *fmt, const struct tm *tm,
 			fmt++;
 			break;
 		case 'Z':
-			if (tz_name) {
-				strbuf_addstr(&munged_fmt, tz_name);
+			if (suppress_tz_name) {
 				fmt++;
 				break;
 			}
diff --git a/strbuf.h b/strbuf.h
index 6708cef0f9..9262615ca0 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, expands %Z internally to the empty
+ * string rather than passing it to `strftime`.
  */
 extern void strbuf_addftime(struct strbuf *sb, const char *fmt,
 			    const struct tm *tm, int tz_offset,
-			    const char *tz_name);
+			    int suppress_tz_name);
 
 /**
  * Read a given size of data from a FILE* pointer to the buffer.
-- 
2.13.1.611.g7e3b11ae1

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

[Jeff King]

On Sat, Jun 24, 2017 at 12:14:52PM +0000, Ævar Arnfjörð Bjarmason wrote:

> On Sat, Jun 24, 2017 at 2:10 PM, Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
> > Thanks. Docs fixed per your suggestion. I sent a v4 of 1/2 too, but
> > that's unchanged, just thought it was simpler than having just one
> > patch have a v4...
> 
> Urgh, mistake on my end, sent v3 again as v4. Here's v5 with the
> *actual* fixes. Sorry.

Heh, I skimmed over v4 again and thought "this looks good", but the one
thing I _didn't_ read was the final hunk that actually changed from v3.
Yikes.  So much for my code review skills.

-Peff

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

[René Scharfe]

Am 24.06.2017 um 14:14 schrieb Ævar Arnfjörð Bjarmason:
> 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é

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

[Junio C Hamano]

René Scharfe <l.s.r@web.de> writes:

> Am 24.06.2017 um 14:14 schrieb Ævar Arnfjörð Bjarmason:
>> 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.

    The intent was to use this API in the future to leave the door open
    to pass a custom timezone name to the function (see my [1] and
    related messages).

perhaps?

>> 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).

I share this feeling.

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

I think the first one is strbuf_addftime(); other than that, I think
this version explains what is going on in this patch than the
original.

I'll wait for Ævar to respond, but my inclination is to take the
patch with the above tweaks to the log message, as the change is
easy to revert if we find it necessary.

[PATCH v6 1/2] strbuf.h comment: discuss strbuf_addftime() arguments in order

[Ævar Arnfjörð Bjarmason]

Change the comment documenting the strbuf_addftime() function to
discuss the parameters in the order in which they appear, which makes
this easier to read than discussing them out of order.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 strbuf.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/strbuf.h b/strbuf.h
index 3646a6291b..6809d7daa8 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -334,10 +334,10 @@ extern void strbuf_vaddf(struct strbuf *sb, const char *fmt, va_list ap);
 
 /**
  * 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`.
+ * `tz_name` is used to expand %Z internally unless it's NULL.
  */
 extern void strbuf_addftime(struct strbuf *sb, const char *fmt,
 			    const struct tm *tm, int tz_offset,
-- 
2.13.1.611.g7e3b11ae1

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

[Ævar Arnfjörð Bjarmason]

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.

Commit-message-by: René Scharfe <l.s.r@web.de>
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---

On Sat, Jun 24 2017, Junio C. Hamano jotted:
> René Scharfe <l.s.r@web.de> writes:
>> 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.
>
> I think the first one is strbuf_addftime(); other than that, I think
> this version explains what is going on in this patch than the
> original.
>
> I'll wait for Ævar to respond, but my inclination is to take the
> patch with the above tweaks to the log message, as the change is
> easy to revert if we find it necessary.

Thanks both. Here's a v6 with those changes, sorry about the delay.

 date.c   | 2 +-
 strbuf.c | 5 ++---
 strbuf.h | 5 +++--
 3 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/date.c b/date.c
index 1fd6d66375..c3e673fd04 100644
--- a/date.c
+++ b/date.c
@@ -256,7 +256,7 @@ const char *show_date(timestamp_t time, int tz, const struct date_mode *mode)
 			tm->tm_hour, tm->tm_min, tm->tm_sec, tz);
 	else if (mode->type == DATE_STRFTIME)
 		strbuf_addftime(&timebuf, mode->strftime_fmt, tm, tz,
-				mode->local ? NULL : "");
+				!mode->local);
 	else
 		strbuf_addf(&timebuf, "%.3s %.3s %d %02d:%02d:%02d %d%c%+05d",
 				weekday_names[tm->tm_wday],
diff --git a/strbuf.c b/strbuf.c
index c4e91a6656..89d22e3b09 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -779,7 +779,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, int suppress_tz_name)
 {
 	struct strbuf munged_fmt = STRBUF_INIT;
 	size_t hint = 128;
@@ -808,8 +808,7 @@ void strbuf_addftime(struct strbuf *sb, const char *fmt, const struct tm *tm,
 			fmt++;
 			break;
 		case 'Z':
-			if (tz_name) {
-				strbuf_addstr(&munged_fmt, tz_name);
+			if (suppress_tz_name) {
 				fmt++;
 				break;
 			}
diff --git a/strbuf.h b/strbuf.h
index 6809d7daa8..2075384e0b 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -337,11 +337,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, expands %Z internally to the empty
+ * string rather than passing it to `strftime`.
  */
 extern void strbuf_addftime(struct strbuf *sb, const char *fmt,
 			    const struct tm *tm, int tz_offset,
-			    const char *tz_name);
+			    int suppress_tz_name);
 
 /**
  * Read a given size of data from a FILE* pointer to the buffer.
-- 
2.13.1.611.g7e3b11ae1

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

[René Scharfe]

Am 01.07.2017 um 14:55 schrieb Ævar Arnfjörð Bjarmason:
> strbuf_addstr() allows callers to pass a time zone name for expanding
   ^^^^^^^^^^^^^^^
That should be "strbuf_addftime()" instead (my typo), as Junio noted.

> %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.
> 
> Commit-message-by: René Scharfe <l.s.r@web.de>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
> 
> On Sat, Jun 24 2017, Junio C. Hamano jotted:
>> René Scharfe <l.s.r@web.de> writes:
>>> 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.
>>
>> I think the first one is strbuf_addftime(); other than that, I think
>> this version explains what is going on in this patch than the
>> original.

René

[PATCH v7 1/2] strbuf.h comment: discuss strbuf_addftime() arguments in order

[Ævar Arnfjörð Bjarmason]

Change the comment documenting the strbuf_addftime() function to
discuss the parameters in the order in which they appear, which makes
this easier to read than discussing them out of order.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 strbuf.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/strbuf.h b/strbuf.h
index 3646a6291b..6809d7daa8 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -334,10 +334,10 @@ extern void strbuf_vaddf(struct strbuf *sb, const char *fmt, va_list ap);
 
 /**
  * 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`.
+ * `tz_name` is used to expand %Z internally unless it's NULL.
  */
 extern void strbuf_addftime(struct strbuf *sb, const char *fmt,
 			    const struct tm *tm, int tz_offset,
-- 
2.13.1.611.g7e3b11ae1

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

[Ævar Arnfjörð Bjarmason]

strbuf_addftime() 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.

Commit-message-by: René Scharfe <l.s.r@web.de>
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---

On Sat, Jul 01 2017, René Scharfe jotted:

> Am 01.07.2017 um 14:55 schrieb Ævar Arnfjörð Bjarmason:
>> strbuf_addstr() allows callers to pass a time zone name for expanding
>   ^^^^^^^^^^^^^^^
> That should be "strbuf_addftime()" instead (my typo), as Junio noted.

Oops. Fixed.

 date.c   | 2 +-
 strbuf.c | 5 ++---
 strbuf.h | 5 +++--
 3 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/date.c b/date.c
index 1fd6d66375..c3e673fd04 100644
--- a/date.c
+++ b/date.c
@@ -256,7 +256,7 @@ const char *show_date(timestamp_t time, int tz, const struct date_mode *mode)
 			tm->tm_hour, tm->tm_min, tm->tm_sec, tz);
 	else if (mode->type == DATE_STRFTIME)
 		strbuf_addftime(&timebuf, mode->strftime_fmt, tm, tz,
-				mode->local ? NULL : "");
+				!mode->local);
 	else
 		strbuf_addf(&timebuf, "%.3s %.3s %d %02d:%02d:%02d %d%c%+05d",
 				weekday_names[tm->tm_wday],
diff --git a/strbuf.c b/strbuf.c
index c4e91a6656..89d22e3b09 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -779,7 +779,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, int suppress_tz_name)
 {
 	struct strbuf munged_fmt = STRBUF_INIT;
 	size_t hint = 128;
@@ -808,8 +808,7 @@ void strbuf_addftime(struct strbuf *sb, const char *fmt, const struct tm *tm,
 			fmt++;
 			break;
 		case 'Z':
-			if (tz_name) {
-				strbuf_addstr(&munged_fmt, tz_name);
+			if (suppress_tz_name) {
 				fmt++;
 				break;
 			}
diff --git a/strbuf.h b/strbuf.h
index 6809d7daa8..2075384e0b 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -337,11 +337,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, expands %Z internally to the empty
+ * string rather than passing it to `strftime`.
  */
 extern void strbuf_addftime(struct strbuf *sb, const char *fmt,
 			    const struct tm *tm, int tz_offset,
-			    const char *tz_name);
+			    int suppress_tz_name);
 
 /**
  * Read a given size of data from a FILE* pointer to the buffer.
-- 
2.13.1.611.g7e3b11ae1

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> 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).

The former might be conceivable, but I do not think the latter is
workable.  Knowing that a location happened to be using a particular
GMT offset at a specific point in time simply is not sufficient to
give us a zone name; the whole idea of a zone name being that it
will give us rules that would apply to other timestamps, not just
the one we are paring with GMT offset in our committer and author
timestamp fields.

A third possibility is the information may come out of band.
Something like "When gitster is in +0900 he is in JST" can be
maintained per project and supplied by the caller.

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

I agree that it is not too much hassle to revert this change.  I
actually would not have minded if René's original were written with
a boolean flag.  But I do not see the value in flipping ("" vs NULL)
with a bool now.  The benefit we are gaining (other than closing the
door) is unclear to me.

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

Yeah, that reads better.  I am also OK if this said that an empty
string is an accepted POSIX way to fallback when the information is
unavailable---and we really are in that "information is unavailable"
situation in this code.

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

[René Scharfe]

Am 23.06.2017 um 16:46 schrieb Ævar Arnfjörð Bjarmason:
> 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.

"Omitting" sounds not quite right somehow.  We expand %Z to the empty
string because that's the best we can do -- which amounts to a removal,
but that's not the intent, just an implementation detail.  Calling it
"handling %Z internally" would be better, I think.

> 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.
> 
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>   date.c   | 2 +-
>   strbuf.c | 5 ++---
>   strbuf.h | 5 +++--
>   3 files changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/date.c b/date.c
> index 1fd6d66375..5f09743bad 100644
> --- a/date.c
> +++ b/date.c
> @@ -256,7 +256,7 @@ const char *show_date(timestamp_t time, int tz, const struct date_mode *mode)
>   			tm->tm_hour, tm->tm_min, tm->tm_sec, tz);
>   	else if (mode->type == DATE_STRFTIME)
>   		strbuf_addftime(&timebuf, mode->strftime_fmt, tm, tz,
> -				mode->local ? NULL : "");
> +				mode->local ? 0 : 1);

I don't see how this is more readable -- both look about equally ugly to
me.  Passing mode->local unchanged would be better.

>   	else
>   		strbuf_addf(&timebuf, "%.3s %.3s %d %02d:%02d:%02d %d%c%+05d",
>   				weekday_names[tm->tm_wday],
> 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.

>   {
>   	struct strbuf munged_fmt = STRBUF_INIT;
>   	size_t hint = 128;
> @@ -815,8 +815,7 @@ void strbuf_addftime(struct strbuf *sb, const char *fmt, const struct tm *tm,
>   			fmt++;
>   			break;
>   		case 'Z':
> -			if (tz_name) {
> -				strbuf_addstr(&munged_fmt, tz_name);
> +			if (omit_strftime_tz_name) {

Getting rid of this strbuf_addstr call is nice, but as Peff mentioned in
his reply it also reduces the flexibility of the function.  While it's
unlikely to be needed I'm not convinced that we should already block
this path (even though it could be easily reopened).

>   				fmt++;
>   				break;
>   			}
> diff --git a/strbuf.h b/strbuf.h
> index 4559035c47..bad698058a 100644
> --- a/strbuf.h
> +++ b/strbuf.h
> @@ -340,14 +340,15 @@ extern void strbuf_vaddf(struct strbuf *sb, const char *fmt, va_list ap);
>   
>   /**
>    * 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.
>    */
>   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);
>   
>   /**
>    * Read a given size of data from a FILE* pointer to the buffer.
> 

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

[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.

-Peff

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

[René Scharfe]

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é