Re: [PATCH 2/3] format_config: simplify buffer handling

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 21, 2015, 17:42 UTC
Message-ID: <xmqqvbc85yi3.fsf@gitster.dls.corp.google.com>
In-Reply-To: <20150820144733.GB11913@sigill.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 17 quoted lines

> When formatting a config value into a strbuf, we may end
> up stringifying it into a fixed-size buffer using sprintf,
> and then copying that buffer into the strbuf. We can
> eliminate the middle-man (and drop some calls to sprintf!)
> by writing directly to the strbuf.
>
> The reason it was written this way in the first place is
> that we need to know before writing the value whether to
> insert a delimiter. Instead of delaying the write of the
> value, we speculatively write the delimiter, and roll it
> back in the single case that cares.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> I admit the rollback is a little gross. The other option would be adding
> the delimiter in each of the conditional branches, which is also kind of
> nasty.

I actually am fine with this rollback. The "variable alone stands for true" is not something a user can produce from the command line very easily, so having to rollback is a rare event anyway.

I wonder if we can do this instead

	if (!omit_values) {
-		if (show_keys)
+		if (show_keys && value_)
			strbuf_addch(buf, key_delim);

though.  That would eliminate the need for rolling back.

I briefly wondered how such a change would interact with

        if (types == TYPE_INT)
                strbuf_addf(buf, "%"PRId64,
                            git_config_int64(key_, value_ ? value_ : ""));

that immediately follows it, but this "turn NULL into an empty string" may be bogus in the first place, in the sense that git_config_int64() should complain about a NULL value_ the same way as it would complain about an empty string---both them are not an integer. And indeed:

 - git_parse_int64() that is called from git_config_int64() is
   prepared to take both "" and NULL and return failure with EINVAL;

 - die_bad_number() that is eventually called when parsing fails by
   git_config_int64() is prepared to take NULL and turns it to an
   empty string.

So perhaps we could do this squashed in?

 builtin/config.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/builtin/config.c b/builtin/config.c
index 71acc44..593b1ae 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -111,12 +111,12 @@ static int format_config(struct strbuf *buf, const char *key_, const char *value
 	if (show_keys)
 		strbuf_addstr(buf, key_);
 	if (!omit_values) {
-		if (show_keys)
+		if (show_keys && value_)
 			strbuf_addch(buf, key_delim);
 
 		if (types == TYPE_INT)
 			strbuf_addf(buf, "%"PRId64,
-				    git_config_int64(key_, value_ ? value_ : ""));
+				    git_config_int64(key_, value_));
 		else if (types == TYPE_BOOL)
 			strbuf_addstr(buf, git_config_bool(key_, value_) ?
 				      "true" : "false");
@@ -136,9 +136,8 @@ static int format_config(struct strbuf *buf, const char *key_, const char *value
 		} else if (value_) {
 			strbuf_addstr(buf, value_);
 		} else {
-			/* Just show the key name; back out delimiter */
-			if (show_keys)
-				strbuf_setlen(buf, buf->len - 1);
+			/* Just show the key name */
+			;
 		}
 	}
 	strbuf_addch(buf, term);