Re: [add-default-config 2/5] adding default to color
- From
Jeff King <peff@peff.net>
- Date
- Nov 12, 2017, 15:37 UTC
- Message-ID
- <20171112153659.lt77rn6h6faeqfpb@sigill.intra.peff.net>
- In-Reply-To
- <0102015fb0bf3002-3462777a-2363-40cf-af37-80e26c0db65b-000000@eu-west-1.amazonses.com>
On Sun, Nov 12, 2017 at 03:00:40PM +0000, Soukaina NAIT HMID wrote:
Show 11 quoted lines
> 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.
Show 5 quoted lines
> @@ -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.
Show 5 quoted lines
> +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