Web lists-archives.com

Re: [add-default-config 2/5] adding default to color




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

> diff --git a/builtin/config.c b/builtin/config.c
> index 124a682d50fa8..9df2d9c43bcad 100644
> --- a/builtin/config.c
> +++ b/builtin/config.c
> @@ -30,6 +30,7 @@ static int end_null;
>  static int respect_includes_opt = -1;
>  static struct config_options config_options;
>  static int show_origin;
> +static const char *default_value;
> [...]
> +	OPT_STRING(0, "default", &default_value, N_("default-value"), N_("sets default for bool/int/path/color when no value is returned from config")),

These hunks make sense. We're adding a new "--default" option that would
kick in when you try to look up a key and it isn't present.

I think we can skip the "bool/int/path/color" thing in the help string.
We would want this to kick in for every type, right?  The only
constraint is that we are doing a "get" operation. It wouldn't make any
sense to use "--default" when setting a variable, listing, etc. Should
we catch these cases and return an error?

We'd also want to mention this in Documentation/git-config.txt.

> @@ -47,6 +48,7 @@ static int show_origin;
>  #define ACTION_GET_COLOR (1<<13)
>  #define ACTION_GET_COLORBOOL (1<<14)
>  #define ACTION_GET_URLMATCH (1<<15)
> +#define ACTION_GET_COLORORDEFAULT (1<<16)

I'm not sure I understand this part, though. Providing a default should
be something that goes along with a "get" action, but isn't its own
action.

> +static void get_color_default(const char *var)
> +{
> +	get_color(var, default_value);
> +}
> +

And here we're just applying --default to colors, but we'd eventually
want them for everything. I think that's fixed later in the series, so
I'll keep reading. But I'd expect a function like get_value() to be
detecting the case where we got no hits and filling in the default_value
there, as if we had read it from the config file.

-Peff