Re: [add-default-config 1/5] add --color option to git config

From: Jeff King <peff@peff.net>
Date: Nov 12, 2017, 15:22 UTC
Message-ID: <20171112152245.fetxqionsqqg2obp@sigill.intra.peff.net>
In-Reply-To: <0102015fb0bf2f74-cb456171-fe65-4d83-8784-b553c7c9e584-000000@eu-west-1.amazonses.com>

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

> From: Soukaina NAIT HMID <snaithmid@bloomberg.net>
> 
> Signed-off-by: Soukaina NAIT HMID <snaithmid@bloomberg.net>

Hi Soukaina, and welcome to the list! Thanks for working on these patches.

I'll go through and make inline comments on your patches, but first a few overall issues:

 - you have five patches in your series, some of which are backing out
   changes made by the other patches. It's normal in the Git community
   to use "git rebase -i" to squash down your changes to "clean" patches
   that form a sequence. For your topic, I'd expect to see two patches
   in the end:

     1. Add a "config --default" option (and documentation and tests).

     2. Add a "config --color" type (and documentation and tests).

 - your commit messages are mostly empty. :) This is a good place to
   describe the motivation for the patch, document any design decisions,
   discuss any alternatives, etc. This is helpful for reviewers to
   understand what you're trying to achieve, and for people later who
   discover your commit from "git log".

Show 9 quoted lines

> diff --git a/builtin/config.c b/builtin/config.c
> index d13daeeb55927..124a682d50fa8 100644
> --- a/builtin/config.c
> +++ b/builtin/config.c
> @@ -80,6 +80,7 @@ static struct option builtin_config_options[] = {
>  	OPT_BIT(0, "int", &types, N_("value is decimal number"), TYPE_INT),
>  	OPT_BIT(0, "bool-or-int", &types, N_("value is --bool or --int"), TYPE_BOOL_OR_INT),
>  	OPT_BIT(0, "path", &types, N_("value is a path (file or directory name)"), TYPE_PATH),
> +	OPT_BIT(0, "color", &actions, N_("find the color configured"), ACTION_GET_COLOR),

I think we'd want this "--color" to actually be a type, not a separate action. I.e., so that it behaves --int, --bool, etc. Part of the goal (well, my goal) was to make accessing color config more like other types of config.

-Peff