Re: [PATCH v3 3/4] notes: add notes.merge option to select default strategy

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Aug 6, 2015, 22:37 UTC
Message-ID: <CAPig+cR==VEdBPoraqQAb_dp+3rd_NoZZidthDRn2Zd9BYJRCw@mail.gmail.com>
In-Reply-To: <1438510226-1163-4-git-send-email-jacob.e.keller@intel.com>

On Sun, Aug 2, 2015 at 6:10 AM, Jacob Keller <jacob.e.keller@intel.com> wrote:

Show 27 quoted lines

> Teach git-notes about a new configuration option "notes.merge" for
> selecting the default notes merge strategy. Document the option in
> config.txt and git-notes.txt
>
> Add tests for use of the configuration option. Include a test to ensure
> that --strategy correctly overrides the configured setting.
>
> Signed-off-by: Jacob Keller <jacob.keller@gmail.com>
> ---
> diff --git a/Documentation/git-notes.txt b/Documentation/git-notes.txt
> index 674682b34b83..9c4f8536182f 100644
> --- a/Documentation/git-notes.txt
> +++ b/Documentation/git-notes.txt
> @@ -101,7 +101,7 @@ merge::
>  If conflicts arise and a strategy for automatically resolving
> -conflicting notes (see the -s/--strategy option) is not given,
> +conflicting notes (see the "NOTES MERGE STRATEGIES" section) is not given,
>  the "manual" resolver is used. This resolver checks out the
>  conflicting notes in a special worktree (`.git/NOTES_MERGE_WORKTREE`),
>  and instructs the user to manually resolve the conflicts there.
> @@ -183,6 +183,7 @@ OPTIONS
>         When merging notes, resolve notes conflicts using the given
>         strategy. The following strategies are recognized: "manual"
>         (default), "ours", "theirs", "union" and "cat_sort_uniq".
> +       This option overrides the "notes.merge" configuration setting.
>         See the "NOTES MERGE STRATEGIES" section below for more
>         information on each notes merge strategy.

These two documentation updates are much easier to digest than the noisy-diff versions of the previous attempt; and the patch overall is a more pleasant read than v1.

Show 17 quoted lines

> diff --git a/builtin/notes.c b/builtin/notes.c
> index 63f95fc55439..de0caa00df1b 100644
> --- a/builtin/notes.c
> +++ b/builtin/notes.c
> @@ -945,6 +955,20 @@ static int get_ref(int argc, const char **argv, const char *prefix)
>         return 0;
>  }
>
> +static int git_notes_config(const char *var, const char *value, void *cb)
> +{
> +       if (!strcmp(var, "notes.merge")) {
> +               if (!value)
> +                       return config_error_nonbool(var);
> +               if (parse_notes_strategy(value, &merge_strategy))
> +                       return error("Unknown notes merge strategy: %s", value);
> +               else
> +                       return 0;

A purely subjective stylistic suggestion, which you can freely ignore if your preference differs:

    if (!value)
        return ...;
    if (parse_notes_strategy(...))
        return ...;
    return 0;

Show 5 quoted lines

> +       }
> +
> +       return git_default_config(var, value, cb);
> +}
> +