[PATCH v5 2/2] strbuf: change an always NULL/"" strbuf_addftime() param to bool
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=+8m5s4efsxcabcy7upme5...@mail.gmail.com
(https://public-inbox.org/git/CACBZZX5OQc45fUyDVayE89rkT=+8m5s4efsxcabcy7upme5...@mail.gmail.com/)
Signed-off-by: Ævar Arnfjörð Bjarmason
---
On Sat, Jun 24, 2017 at 2:10 PM, Ævar Arnfjörð Bjarmason
wrote:
> Thanks. Docs fixed per your suggestion. I sent a v4 of 1/2 too, but
> that's unchanged, just thought it was simpler than having just one
> patch have a v4...
Urgh, mistake on my end, sent v3 again as v4. Here's v5 with the
*actual* fixes. Sorry.
date.c | 2 +-
strbuf.c | 5 ++---
strbuf.h | 5 +++--
3 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/date.c b/date.c
index 1fd6d66375..c3e673fd04 100644
--- a/date.c
+++ b/date.c
@@ -256,7 +256,7 @@ const char *show_date(timestamp_t time, int tz, const
struct date_mode *mode)
tm->tm_hour, tm->tm_min, tm->tm_sec, tz);
else if (mode->type == DATE_STRFTIME)
strbuf_addftime(&timebuf, mode->strftime_fmt, tm, tz,
- mode->local ? NULL : "");
+ !mode->local);
else
strbuf_addf(&timebuf, "%.3s %.3s %d %02d:%02d:%02d %d%c%+05d",
weekday_names[tm->tm_wday],
diff --git a/strbuf.c b/strbuf.c
index be3b9e37b1..89e40bb496 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -786,7 +786,7 @@ char *xstrfmt(const char *fmt, ...)
}
void strbuf_addftime(struct strbuf *sb, const char *fmt, const struct tm *tm,
-int tz_offset, const char *tz_name)
+int tz_offset, int suppress_tz_name)
{
struct strbuf munged_fmt = STRBUF_INIT;
size_t hint = 128;
@@ -815,8 +815,7 @@ void strbuf_addftime(struct strbuf *sb, const char *fmt,
const struct tm *tm,
fmt++;
break;
case 'Z':
- if (tz_name) {
- strbuf_addstr(&munged_fmt, tz_name);
+ if (suppress_tz_name) {
fmt++;
break;
}
diff --git a/strbuf.h b/strbuf.h
index 6708cef0f9..9262615ca0 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -343,11 +343,12 @@ extern void strbuf_vaddf(struct strbuf *sb, const char
*fmt, va_list ap);
* `tz_offset` is in decimal hhmm format, e.g. -600 means six hours west
* of Greenwich, and it's used to expand %z internally. However, tokens
* with modifiers (e.g. %Ez) are passed to `strftime`.
- * `tz_name` is used to expand %Z internally unless it's NULL.
+ * `suppress_tz_name`, when set, expands %Z internally to the empty
+ * string rather than passing it to `strftime`.
*/
extern void strbuf_addftime(struct strbuf *sb, const char *fmt,
const struct tm *tm, int tz_offset,
- const char *tz_name);
+ int suppress_tz_name);
/**
* Read a given size of data from a FILE* pointer to the buffer.
--
2.13.1.611.g7e3b11ae1
Re: [PATCH v5 2/2] strbuf: change an always NULL/"" strbuf_addftime() param to bool
On Sat, Jun 24, 2017 at 12:14:52PM +, Ævar Arnfjörð Bjarmason wrote: > On Sat, Jun 24, 2017 at 2:10 PM, Ævar Arnfjörð Bjarmason > wrote: > > Thanks. Docs fixed per your suggestion. I sent a v4 of 1/2 too, but > > that's unchanged, just thought it was simpler than having just one > > patch have a v4... > > Urgh, mistake on my end, sent v3 again as v4. Here's v5 with the > *actual* fixes. Sorry. Heh, I skimmed over v4 again and thought "this looks good", but the one thing I _didn't_ read was the final hunk that actually changed from v3. Yikes. So much for my code review skills. -Peff
Re: [PATCH v5 2/2] strbuf: change an always NULL/"" strbuf_addftime() param to bool
Am 24.06.2017 um 14:14 schrieb Ævar Arnfjörð Bjarmason:
Change the code for deciding what's to be done about %Z to stop
passing always either a NULL or "" char * to
strbuf_addftime(). Instead pass a boolean int to indicate whether the
strftime() %Z format should be suppressed by converting it to an empty
string, which is what this code is actually doing.
This code grew organically between the changes in 9eafe86d58 ("Merge
branch 'rs/strbuf-addftime-zZ'", 2017-06-22). The intent was to use
this API in the future to pass a custom leave the door open to pass a
custom timezone name to the function (see my [1] and related
messages).
"leave the door open to pass a" seems redundant.
But that's not what this code does now, and this strbuf_addstr() call
always being redundant makes it hard to understand the current
functionality. So simplify this internal API to match its use, we can
always change it in the future if it gets a different use-case.
I don't understand the confusion, but of course I'm biased. And I don't
like binary parameters in general and would use named flags or two
function names in most cases. But that aside I find the description
hard to follow (perhaps I should do something about my attention span).
Here's an attempt at a commit message that would have be easier to
understand for me:
strbuf_addstr() allows callers to pass a time zone name for expanding
%Z. The only current caller either passes the empty string or NULL,
in which case %Z is handed over verbatim to strftime(3). Replace that
string parameter with a flag controlling whether to remove %Z from the
format specification. This simplifies the code.
René
Re: [PATCH v5 2/2] strbuf: change an always NULL/"" strbuf_addftime() param to bool
René Scharfe 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.
