Re: [add-default-config 4/5] add defaults for path/int/bool

[Jeff King <peff@peff.net>]

On Sun, Nov 12, 2017 at 03:00:40PM +0000, Soukaina NAIT HMID wrote:

> @@ -256,8 +258,15 @@ static int get_value(const char *key_, const char *regex_)
>  			fwrite(buf->buf, 1, buf->len, stdout);
>  		strbuf_release(buf);
>  	}
> -	free(values.items);
>  
> +	if (values.nr == 0 && default_value) {
> +		if(types == TYPE_INT || types == TYPE_BOOL || types == TYPE_BOOL_OR_INT || types == TYPE_PATH ) {
> +			char* xstr = normalize_value(key, default_value);
> +			fwrite(xstr, 1, strlen(xstr), stdout);
> +			fwrite("\n", 1, 1, stdout);
> +		}
> +	}
> +	free(values.items);

OK, this location makes more sense to me for handling --default. I think
it's still a bit lower in the function than I'd expect, though.

The loop just above the code you added is showing all of the values we
found. So I think that's the spot where'd say "we didn't find any
values; pretend like we found default_value". Including, I think, making
sure that the return value from the function is still 0. So realy right
after config_with_options() returns, I'd expect to check something like:

  if (!values.nr && default_value) {
          /* insert default_value into values list */
  }

We'd also want to use format_config(), not normalize_config(). We do
format_config() to show values, whereas normalize_config() is usually
for values we're putting into a config file (so for example TYPE_PATH
doesn't get normalized, but we would want it converted here to show the
user).

I'm also not sure that we need to check "types" as you have here.
Wouldn't we want to apply the default regardless of type, and let
format_config() handle it?

> @@ -272,6 +281,7 @@ static int get_value(const char *key_, const char *regex_)
>  	return ret;
>  }
>  
> +
>  static char *normalize_value(const char *key, const char *value)

Watch out for stray changes like this one creeping into your commits.

> diff --git a/t/t4026-color2.sh b/t/t4026-color2.sh
> deleted file mode 100755
> index 695ce9dd6f8d4..0000000000000

This part is obviously good and rectifying the problem from patch 3.
Once they're squashed together, we won't see it at all. :)

-Peff