On Sun, Nov 12, 2017 at 03:00:40PM +0000, Soukaina NAIT HMID wrote:
> From: Soukaina NAIT HMID <[email protected]>
>
> Signed-off-by: Soukaina NAIT HMID <[email protected]>
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".
> 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
