Re: [PATCHv3 1/2] config: add '--names-only' option to list only variable names

Quoting Junio C Hamano [off-list ref]:

SZEDER Gábor [off-list ref] writes:

quoted

'git config' can only show values or name-value pairs, so if a shell
script needs the names of set config variables it has to run 'git config
--list' or '--get-regexp' and parse the output to separate config
variable names from their values.  However, such a parsing can't cope
with multi-line values.  Though 'git config' can produce null-terminated
output for newline-safe parsing, that's of no use in such a case, becase

s/becase/because/;

OK.

quoted

shells can't cope with null characters.

Even our own bash completion script suffers from these issues.

Help the completion script, and shell scripts in general, by introducing
the '--names-only' option to modify the output of '--list' and
'--get-regexp' to list only the names of config variables, so they don't
have to perform error-prone post processing to separate variable names
from their values anymore.

I agree with Peff that "--names-only" has a subtle difference with
an existing and well known subcommand option and it would be a bit
irritating to remember which options is for which command.

OK.

quoted

diff --git a/builtin/config.c b/builtin/config.c
index 7188405f7e..307980ab50 100644
--- a/builtin/config.c
+++ b/builtin/config.c

@@ -13,6 +13,7 @@ static char *key;
 static regex_t *key_regexp;
 static regex_t *regexp;
 static int show_keys;
+static int omit_values;
 static int use_key_regexp;
 static int do_all;
 static int do_not_match;

...

@@ -91,7 +93,7 @@ static void check_argc(int argc, int min, int max) {

 static int show_all_config(const char *key_, const char *value_, void *cb)
 {
-	if (value_)
+	if (!omit_values && value_)

Hmmmm.  As we have "show_keys",

	if (show_values && value_)

would be a lot more intuitive, no?

Well, the name 'omit_values' was suggested by Peff after the first  
round.  I'm happy to rename it to whatever you agree upon :)

quoted

@@ -117,6 +119,10 @@ static int format_config(struct strbuf *buf,  

const char *key_, const char *value
 		strbuf_addstr(buf, key_);
 		must_print_delim = 1;
 	}
+	if (omit_values) {
+		strbuf_addch(buf, term);
+		return 0;
+	}

This hunk makes me wonder what the assignment to "must_print_delim"
is about.  When the code is told to show only keys and not values,
it shouldn't even have to worry about key_delim, but that assignment
is done to control exactly that.  It happens that you are lucky that
you can "return 0" early here so that the assignment does not have
any effect, but still conceptually the code structure is made ugly
by this patch.

How about restructuring the function like this?  Perhaps even better  
than a tri-state toggle would be.
(showing the result instead of the diff, because all the indentation  
changes make the diff hard to read).

static int format_config(struct strbuf *buf, const char *key_, const  
char *value_)
{
         strbuf_init(buf, 0);

         if (show_keys)
                 strbuf_addstr(buf, key_);
         if (!omit_values) {                  // or show_values
                 int must_free_vptr = 0;
                 int must_add_delim = show_keys;
                 char value[256];
                 const char *vptr = value;

                 if (types == TYPE_INT)
                         sprintf(value, "%"PRId64,
                                 git_config_int64(key_, value_ ? value_ : ""));
                 else if (types == TYPE_BOOL)
                         vptr = git_config_bool(key_, value_) ? "true"  
: "false";
                 else if (types == TYPE_BOOL_OR_INT) {
                         int is_bool, v;
                         v = git_config_bool_or_int(key_, value_, &is_bool);
                         if (is_bool)
                                 vptr = v ? "true" : "false";
                         else
                                 sprintf(value, "%d", v);
                 } else if (types == TYPE_PATH) {
                         if (git_config_pathname(&vptr, key_, value_) < 0)
                                 return -1;
                         must_free_vptr = 1;
                 } else if (value_) {
                         vptr = value_;
                 } else {
                         /* Just show the key name */
                         vptr = "";
                         must_add_delim = 0;
                 }

                 if (must_add_delim)
                         strbuf_addch(buf, key_delim);
                 strbuf_addstr(buf, vptr);

                 if (must_free_vptr)
                         free((char *)vptr);
         }
         strbuf_addch(buf, term);
         return 0;
}

Isn't it more like the existing "show_keys" can be replaced/enhanced
with a single "show" tri-state toggle that chooses one among:

    * show both keys and values (for --list)
    * show only keys (for your new feature)
    * show only value (for --get)

perhaps?

I see get_urlmatch() abuses show_keys variable in a strange way, and
it may not be as trivial as removing show_keys and replacing it with
a new tri-state toggle, though.