* [PATCH] strbuf: change an always NULL/"" strbuf_addftime() param to bool @ 2017-06-23 14:46 Ævar Arnfjörð Bjarmason 2017-06-23 14:51 ` Jeff King 2017-06-23 15:20 ` René Scharfe 0 siblings, 2 replies; 29+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2017-06-23 14:46 UTC (permalink / raw) To: git Cc: Junio C Hamano, Jeff King, René Scharfe, Æ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 ^ permalink raw reply related [flat|nested] 29+ messages in thread
* Re: [PATCH] strbuf: change an always NULL/"" strbuf_addftime() param to bool 2017-06-23 14:46 [PATCH] strbuf: change an always NULL/"" strbuf_addftime() param to bool Ævar Arnfjörð Bjarmason @ 2017-06-23 14:51 ` Jeff King 2017-06-23 15:13 ` Ævar Arnfjörð Bjarmason 2017-06-23 17:13 ` [PATCH] " Junio C Hamano 2017-06-23 15:20 ` René Scharfe 1 sibling, 2 replies; 29+ messages in thread From: Jeff King @ 2017-06-23 14:51 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason Cc: git, Junio C Hamano, René Scharfe 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 ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH] strbuf: change an always NULL/"" strbuf_addftime() param to bool 2017-06-23 14:51 ` Jeff King @ 2017-06-23 15:13 ` Ævar Arnfjörð Bjarmason 2017-06-23 15:23 ` Jeff King 2017-06-23 17:13 ` [PATCH] " Junio C Hamano 1 sibling, 1 reply; 29+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2017-06-23 15:13 UTC (permalink / raw) To: Jeff King; +Cc: git, Junio C Hamano, René Scharfe 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. ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH] strbuf: change an always NULL/"" strbuf_addftime() param to bool 2017-06-23 15:13 ` Ævar Arnfjörð Bjarmason @ 2017-06-23 15:23 ` Jeff King 2017-06-23 16:23 ` René Scharfe 2017-06-23 16:36 ` [PATCH -v2] " Ævar Arnfjörð Bjarmason 0 siblings, 2 replies; 29+ messages in thread From: Jeff King @ 2017-06-23 15:23 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason Cc: git, Junio C Hamano, René Scharfe 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 ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH] strbuf: change an always NULL/"" strbuf_addftime() param to bool 2017-06-23 15:23 ` Jeff King @ 2017-06-23 16:23 ` René Scharfe 2017-06-23 16:37 ` Jeff King 2017-06-24 11:11 ` Ævar Arnfjörð Bjarmason 2017-06-23 16:36 ` [PATCH -v2] " Ævar Arnfjörð Bjarmason 1 sibling, 2 replies; 29+ messages in thread From: René Scharfe @ 2017-06-23 16:23 UTC (permalink / raw) To: Jeff King, Ævar Arnfjörð Bjarmason; +Cc: git, Junio C Hamano 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é ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH] strbuf: change an always NULL/"" strbuf_addftime() param to bool 2017-06-23 16:23 ` René Scharfe @ 2017-06-23 16:37 ` Jeff King 2017-06-24 11:11 ` Ævar Arnfjörð Bjarmason 1 sibling, 0 replies; 29+ messages in thread From: Jeff King @ 2017-06-23 16:37 UTC (permalink / raw) To: René Scharfe Cc: Ævar Arnfjörð Bjarmason, git, Junio C Hamano 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 ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH] strbuf: change an always NULL/"" strbuf_addftime() param to bool 2017-06-23 16:23 ` René Scharfe 2017-06-23 16:37 ` Jeff King @ 2017-06-24 11:11 ` Ævar Arnfjörð Bjarmason 1 sibling, 0 replies; 29+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2017-06-24 11:11 UTC (permalink / raw) To: René Scharfe; +Cc: Jeff King, git, Junio C Hamano 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. ^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH -v2] strbuf: change an always NULL/"" strbuf_addftime() param to bool 2017-06-23 15:23 ` Jeff King 2017-06-23 16:23 ` René Scharfe @ 2017-06-23 16:36 ` Ævar Arnfjörð Bjarmason 2017-06-23 16:44 ` Jeff King 1 sibling, 1 reply; 29+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2017-06-23 16:36 UTC (permalink / raw) To: git Cc: Junio C Hamano, Jeff King, René Scharfe, Æ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 ^ permalink raw reply related [flat|nested] 29+ messages in thread
* Re: [PATCH -v2] strbuf: change an always NULL/"" strbuf_addftime() param to bool 2017-06-23 16:36 ` [PATCH -v2] " Ævar Arnfjörð Bjarmason @ 2017-06-23 16:44 ` Jeff King 2017-06-24 11:36 ` [PATCH v3 1/2] strbuf.h comment: discuss strbuf_addftime() arguments in order Ævar Arnfjörð Bjarmason 2017-06-24 11:36 ` [PATCH v3 2/2] strbuf: change an always NULL/"" strbuf_addftime() param to bool Ævar Arnfjörð Bjarmason 0 siblings, 2 replies; 29+ messages in thread From: Jeff King @ 2017-06-23 16:44 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason Cc: git, Junio C Hamano, René Scharfe 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 ^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH v3 1/2] strbuf.h comment: discuss strbuf_addftime() arguments in order 2017-06-23 16:44 ` Jeff King @ 2017-06-24 11:36 ` Ævar Arnfjörð Bjarmason 2017-06-24 11:36 ` [PATCH v3 2/2] strbuf: change an always NULL/"" strbuf_addftime() param to bool Ævar Arnfjörð Bjarmason 1 sibling, 0 replies; 29+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2017-06-24 11:36 UTC (permalink / raw) To: git Cc: Junio C Hamano, Jeff King, René Scharfe, Æ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 ^ permalink raw reply related [flat|nested] 29+ messages in thread
* [PATCH v3 2/2] strbuf: change an always NULL/"" strbuf_addftime() param to bool 2017-06-23 16:44 ` Jeff King 2017-06-24 11:36 ` [PATCH v3 1/2] strbuf.h comment: discuss strbuf_addftime() arguments in order Ævar Arnfjörð Bjarmason @ 2017-06-24 11:36 ` Ævar Arnfjörð Bjarmason 2017-06-24 12:02 ` Jeff King 1 sibling, 1 reply; 29+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2017-06-24 11:36 UTC (permalink / raw) To: git Cc: Junio C Hamano, Jeff King, René Scharfe, Æ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 ^ permalink raw reply related [flat|nested] 29+ messages in thread
* Re: [PATCH v3 2/2] strbuf: change an always NULL/"" strbuf_addftime() param to bool 2017-06-24 11:36 ` [PATCH v3 2/2] strbuf: change an always NULL/"" strbuf_addftime() param to bool Ævar Arnfjörð Bjarmason @ 2017-06-24 12:02 ` Jeff King 2017-06-24 12:10 ` [PATCH v4 1/2] strbuf.h comment: discuss strbuf_addftime() arguments in order Ævar Arnfjörð Bjarmason 2017-06-24 12:10 ` [PATCH v4 2/2] strbuf: change an always NULL/"" strbuf_addftime() param to bool Ævar Arnfjörð Bjarmason 0 siblings, 2 replies; 29+ messages in thread From: Jeff King @ 2017-06-24 12:02 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason Cc: git, Junio C Hamano, René Scharfe 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 ^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH v4 1/2] strbuf.h comment: discuss strbuf_addftime() arguments in order 2017-06-24 12:02 ` Jeff King @ 2017-06-24 12:10 ` Ævar Arnfjörð Bjarmason 2017-06-24 12:10 ` [PATCH v4 2/2] strbuf: change an always NULL/"" strbuf_addftime() param to bool Ævar Arnfjörð Bjarmason 1 sibling, 0 replies; 29+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2017-06-24 12:10 UTC (permalink / raw) To: git Cc: Junio C Hamano, Jeff King, René Scharfe, Æ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 ^ permalink raw reply related [flat|nested] 29+ messages in thread
* [PATCH v4 2/2] strbuf: change an always NULL/"" strbuf_addftime() param to bool 2017-06-24 12:02 ` Jeff King 2017-06-24 12:10 ` [PATCH v4 1/2] strbuf.h comment: discuss strbuf_addftime() arguments in order Ævar Arnfjörð Bjarmason @ 2017-06-24 12:10 ` Ævar Arnfjörð Bjarmason 2017-06-24 12:12 ` Jeff King ` (2 more replies) 1 sibling, 3 replies; 29+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2017-06-24 12:10 UTC (permalink / raw) To: git Cc: Junio C Hamano, Jeff King, René Scharfe, Æ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 ^ permalink raw reply related [flat|nested] 29+ messages in thread
* Re: [PATCH v4 2/2] strbuf: change an always NULL/"" strbuf_addftime() param to bool 2017-06-24 12:10 ` [PATCH v4 2/2] strbuf: change an always NULL/"" strbuf_addftime() param to bool Ævar Arnfjörð Bjarmason @ 2017-06-24 12:12 ` Jeff King 2017-06-24 12:14 ` [PATCH v5 1/2] strbuf.h comment: discuss strbuf_addftime() arguments in order Ævar Arnfjörð Bjarmason 2017-06-24 12:14 ` [PATCH v5 2/2] strbuf: change an always NULL/"" strbuf_addftime() param to bool Ævar Arnfjörð Bjarmason 2 siblings, 0 replies; 29+ messages in thread From: Jeff King @ 2017-06-24 12:12 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason Cc: git, Junio C Hamano, René Scharfe 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 ^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH v5 1/2] strbuf.h comment: discuss strbuf_addftime() arguments in order 2017-06-24 12:10 ` [PATCH v4 2/2] strbuf: change an always NULL/"" strbuf_addftime() param to bool Ævar Arnfjörð Bjarmason 2017-06-24 12:12 ` Jeff King @ 2017-06-24 12:14 ` Ævar Arnfjörð Bjarmason 2017-06-24 12:14 ` [PATCH v5 2/2] strbuf: change an always NULL/"" strbuf_addftime() param to bool Ævar Arnfjörð Bjarmason 2 siblings, 0 replies; 29+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2017-06-24 12:14 UTC (permalink / raw) To: git Cc: Junio C Hamano, Jeff King, René Scharfe, Æ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 ^ permalink raw reply related [flat|nested] 29+ messages in thread
* [PATCH v5 2/2] strbuf: change an always NULL/"" strbuf_addftime() param to bool 2017-06-24 12:10 ` [PATCH v4 2/2] strbuf: change an always NULL/"" strbuf_addftime() param to bool Ævar Arnfjörð Bjarmason 2017-06-24 12:12 ` Jeff King 2017-06-24 12:14 ` [PATCH v5 1/2] strbuf.h comment: discuss strbuf_addftime() arguments in order Ævar Arnfjörð Bjarmason @ 2017-06-24 12:14 ` Ævar Arnfjörð Bjarmason 2017-06-24 12:22 ` Jeff King 2017-06-24 13:17 ` René Scharfe 2 siblings, 2 replies; 29+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2017-06-24 12:14 UTC (permalink / raw) To: git Cc: Junio C Hamano, Jeff King, René Scharfe, Æ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 ^ permalink raw reply related [flat|nested] 29+ messages in thread
* Re: [PATCH v5 2/2] strbuf: change an always NULL/"" strbuf_addftime() param to bool 2017-06-24 12:14 ` [PATCH v5 2/2] strbuf: change an always NULL/"" strbuf_addftime() param to bool Ævar Arnfjörð Bjarmason @ 2017-06-24 12:22 ` Jeff King 2017-06-24 13:17 ` René Scharfe 1 sibling, 0 replies; 29+ messages in thread From: Jeff King @ 2017-06-24 12:22 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason Cc: git, Junio C Hamano, René Scharfe 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 ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v5 2/2] strbuf: change an always NULL/"" strbuf_addftime() param to bool 2017-06-24 12:14 ` [PATCH v5 2/2] strbuf: change an always NULL/"" strbuf_addftime() param to bool Ævar Arnfjörð Bjarmason 2017-06-24 12:22 ` Jeff King @ 2017-06-24 13:17 ` René Scharfe 2017-06-24 18:21 ` Junio C Hamano 1 sibling, 1 reply; 29+ messages in thread From: René Scharfe @ 2017-06-24 13:17 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason, git; +Cc: Junio C Hamano, Jeff King 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é ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v5 2/2] strbuf: change an always NULL/"" strbuf_addftime() param to bool 2017-06-24 13:17 ` René Scharfe @ 2017-06-24 18:21 ` Junio C Hamano 2017-07-01 12:55 ` [PATCH v6 1/2] strbuf.h comment: discuss strbuf_addftime() arguments in order Ævar Arnfjörð Bjarmason 2017-07-01 12:55 ` [PATCH v6 2/2] strbuf: change an always NULL/"" strbuf_addftime() param to bool Ævar Arnfjörð Bjarmason 0 siblings, 2 replies; 29+ messages in thread From: Junio C Hamano @ 2017-06-24 18:21 UTC (permalink / raw) To: René Scharfe; +Cc: Ævar Arnfjörð Bjarmason, git, Jeff King 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. ^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH v6 1/2] strbuf.h comment: discuss strbuf_addftime() arguments in order 2017-06-24 18:21 ` Junio C Hamano @ 2017-07-01 12:55 ` Ævar Arnfjörð Bjarmason 2017-07-01 12:55 ` [PATCH v6 2/2] strbuf: change an always NULL/"" strbuf_addftime() param to bool Ævar Arnfjörð Bjarmason 1 sibling, 0 replies; 29+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2017-07-01 12:55 UTC (permalink / raw) To: git Cc: Junio C Hamano, Jeff King, René Scharfe, Æ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 ^ permalink raw reply related [flat|nested] 29+ messages in thread
* [PATCH v6 2/2] strbuf: change an always NULL/"" strbuf_addftime() param to bool 2017-06-24 18:21 ` Junio C Hamano 2017-07-01 12:55 ` [PATCH v6 1/2] strbuf.h comment: discuss strbuf_addftime() arguments in order Ævar Arnfjörð Bjarmason @ 2017-07-01 12:55 ` Ævar Arnfjörð Bjarmason 2017-07-01 13:00 ` René Scharfe 1 sibling, 1 reply; 29+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2017-07-01 12:55 UTC (permalink / raw) To: git Cc: Junio C Hamano, Jeff King, René Scharfe, Æ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 ^ permalink raw reply related [flat|nested] 29+ messages in thread
* Re: [PATCH v6 2/2] strbuf: change an always NULL/"" strbuf_addftime() param to bool 2017-07-01 12:55 ` [PATCH v6 2/2] strbuf: change an always NULL/"" strbuf_addftime() param to bool Ævar Arnfjörð Bjarmason @ 2017-07-01 13:00 ` René Scharfe 2017-07-01 13:15 ` [PATCH v7 1/2] strbuf.h comment: discuss strbuf_addftime() arguments in order Ævar Arnfjörð Bjarmason 2017-07-01 13:15 ` [PATCH v7 2/2] strbuf: change an always NULL/"" strbuf_addftime() param to bool Ævar Arnfjörð Bjarmason 0 siblings, 2 replies; 29+ messages in thread From: René Scharfe @ 2017-07-01 13:00 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason, git; +Cc: Junio C Hamano, Jeff King 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é ^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH v7 1/2] strbuf.h comment: discuss strbuf_addftime() arguments in order 2017-07-01 13:00 ` René Scharfe @ 2017-07-01 13:15 ` Ævar Arnfjörð Bjarmason 2017-07-01 13:15 ` [PATCH v7 2/2] strbuf: change an always NULL/"" strbuf_addftime() param to bool Ævar Arnfjörð Bjarmason 1 sibling, 0 replies; 29+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2017-07-01 13:15 UTC (permalink / raw) To: git Cc: Junio C Hamano, Jeff King, René Scharfe, Æ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 ^ permalink raw reply related [flat|nested] 29+ messages in thread
* [PATCH v7 2/2] strbuf: change an always NULL/"" strbuf_addftime() param to bool 2017-07-01 13:00 ` René Scharfe 2017-07-01 13:15 ` [PATCH v7 1/2] strbuf.h comment: discuss strbuf_addftime() arguments in order Ævar Arnfjörð Bjarmason @ 2017-07-01 13:15 ` Ævar Arnfjörð Bjarmason 1 sibling, 0 replies; 29+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2017-07-01 13:15 UTC (permalink / raw) To: git Cc: Junio C Hamano, Jeff King, René Scharfe, Æ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 ^ permalink raw reply related [flat|nested] 29+ messages in thread
* Re: [PATCH] strbuf: change an always NULL/"" strbuf_addftime() param to bool 2017-06-23 14:51 ` Jeff King 2017-06-23 15:13 ` Ævar Arnfjörð Bjarmason @ 2017-06-23 17:13 ` Junio C Hamano 1 sibling, 0 replies; 29+ messages in thread From: Junio C Hamano @ 2017-06-23 17:13 UTC (permalink / raw) To: Jeff King; +Cc: Ævar Arnfjörð Bjarmason, git, René Scharfe 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. ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH] strbuf: change an always NULL/"" strbuf_addftime() param to bool 2017-06-23 14:46 [PATCH] strbuf: change an always NULL/"" strbuf_addftime() param to bool Ævar Arnfjörð Bjarmason 2017-06-23 14:51 ` Jeff King @ 2017-06-23 15:20 ` René Scharfe 2017-06-23 15:25 ` Jeff King 1 sibling, 1 reply; 29+ messages in thread From: René Scharfe @ 2017-06-23 15:20 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason, git; +Cc: Junio C Hamano, Jeff King 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. > ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH] strbuf: change an always NULL/"" strbuf_addftime() param to bool 2017-06-23 15:20 ` René Scharfe @ 2017-06-23 15:25 ` Jeff King 2017-06-23 16:22 ` René Scharfe 0 siblings, 1 reply; 29+ messages in thread From: Jeff King @ 2017-06-23 15:25 UTC (permalink / raw) To: René Scharfe Cc: Ævar Arnfjörð Bjarmason, git, Junio C Hamano 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 ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH] strbuf: change an always NULL/"" strbuf_addftime() param to bool 2017-06-23 15:25 ` Jeff King @ 2017-06-23 16:22 ` René Scharfe 0 siblings, 0 replies; 29+ messages in thread From: René Scharfe @ 2017-06-23 16:22 UTC (permalink / raw) To: Jeff King; +Cc: Ævar Arnfjörð Bjarmason, git, Junio C Hamano 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é ^ permalink raw reply [flat|nested] 29+ messages in thread
end of thread, other threads:[~2017-07-01 13:16 UTC | newest] Thread overview: 29+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-06-23 14:46 [PATCH] strbuf: change an always NULL/"" strbuf_addftime() param to bool Ævar Arnfjörð Bjarmason 2017-06-23 14:51 ` Jeff King 2017-06-23 15:13 ` Ævar Arnfjörð Bjarmason 2017-06-23 15:23 ` Jeff King 2017-06-23 16:23 ` René Scharfe 2017-06-23 16:37 ` Jeff King 2017-06-24 11:11 ` Ævar Arnfjörð Bjarmason 2017-06-23 16:36 ` [PATCH -v2] " Ævar Arnfjörð Bjarmason 2017-06-23 16:44 ` Jeff King 2017-06-24 11:36 ` [PATCH v3 1/2] strbuf.h comment: discuss strbuf_addftime() arguments in order Ævar Arnfjörð Bjarmason 2017-06-24 11:36 ` [PATCH v3 2/2] strbuf: change an always NULL/"" strbuf_addftime() param to bool Ævar Arnfjörð Bjarmason 2017-06-24 12:02 ` Jeff King 2017-06-24 12:10 ` [PATCH v4 1/2] strbuf.h comment: discuss strbuf_addftime() arguments in order Ævar Arnfjörð Bjarmason 2017-06-24 12:10 ` [PATCH v4 2/2] strbuf: change an always NULL/"" strbuf_addftime() param to bool Ævar Arnfjörð Bjarmason 2017-06-24 12:12 ` Jeff King 2017-06-24 12:14 ` [PATCH v5 1/2] strbuf.h comment: discuss strbuf_addftime() arguments in order Ævar Arnfjörð Bjarmason 2017-06-24 12:14 ` [PATCH v5 2/2] strbuf: change an always NULL/"" strbuf_addftime() param to bool Ævar Arnfjörð Bjarmason 2017-06-24 12:22 ` Jeff King 2017-06-24 13:17 ` René Scharfe 2017-06-24 18:21 ` Junio C Hamano 2017-07-01 12:55 ` [PATCH v6 1/2] strbuf.h comment: discuss strbuf_addftime() arguments in order Ævar Arnfjörð Bjarmason 2017-07-01 12:55 ` [PATCH v6 2/2] strbuf: change an always NULL/"" strbuf_addftime() param to bool Ævar Arnfjörð Bjarmason 2017-07-01 13:00 ` René Scharfe 2017-07-01 13:15 ` [PATCH v7 1/2] strbuf.h comment: discuss strbuf_addftime() arguments in order Ævar Arnfjörð Bjarmason 2017-07-01 13:15 ` [PATCH v7 2/2] strbuf: change an always NULL/"" strbuf_addftime() param to bool Ævar Arnfjörð Bjarmason 2017-06-23 17:13 ` [PATCH] " Junio C Hamano 2017-06-23 15:20 ` René Scharfe 2017-06-23 15:25 ` Jeff King 2017-06-23 16:22 ` René Scharfe
Code repositories for project(s) associated with this public inbox https://80x24.org/mirrors/git.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).