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

Jeff King [off-list ref] writes:

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 <redacted>
---
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);