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=+8m5s4efsxcabcy7upme5...@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.

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

[Junio C Hamano]

Jeff King  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

[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=+8m5s4efsxcabcy7upme5...@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(, 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

[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=+8m5s4efsxcabcy7upme5...@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

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

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

[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=+8m5s4efsxcabcy7upme5...@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 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 
---
  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(, 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(, "%.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(_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

[Ævar Arnfjörð Bjarmason]

On Fri, Jun 23 2017, Jeff King jotted:

> On Fri, Jun 23, 2017 at 02:46:03PM +, Æ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=+8m5s4efsxcabcy7upme5...@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 02:46:03PM +, Æ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

[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 
---
 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(, mode->strftime_fmt, tm, tz,
-   mode->local ? NULL : "");
+   mode->local ? 0 : 1);
else
strbuf_addf(, "%.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(_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