From: Junio C Hamano <gitster@pobox•com>
To: "René Scharfe" <l.s.r@web•de>
Cc: "Ævar Arnfjörð Bjarmason" <avarab@gmail•com>,
git@vger•kernel.org, "Jeff King" <peff@peff•net>
Subject: Re: [PATCH v5 2/2] strbuf: change an always NULL/"" strbuf_addftime() param to bool
Date: Sat, 24 Jun 2017 11:21:15 -0700 [thread overview]
Message-ID: <xmqqk2419rhg.fsf@gitster.mtv.corp.google.com> (raw)
In-Reply-To: <9f3720e5-b8be-978d-4f6c-8c082aef3680@web.de> ("René Scharfe"'s message of "Sat, 24 Jun 2017 15:17:20 +0200")
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.
next prev parent reply other threads:[~2017-06-24 18:21 UTC|newest]
Thread overview: 29+ messages / expand[flat|nested] mbox.gz Atom feed top
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 [this message]
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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=xmqqk2419rhg.fsf@gitster.mtv.corp.google.com \
--to=gitster@pobox$(echo .)com \
--cc=avarab@gmail$(echo .)com \
--cc=git@vger$(echo .)kernel.org \
--cc=l.s.r@web$(echo .)de \
--cc=peff@peff$(echo .)net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox