From: Eric Sunshine <sunshine@sunshineco.com>
Date: Fri, 14 Aug 2015 21:06:11 GMT
Subject: Re: [PATCH v6 3/4] notes: add notes.mergestrategy option to select default strategy
Message-ID: <CAPig+cTjQu2LRuR5cC+VSihVB00k-bjew3eCqA4Ee_tRuDK_=w@mail.gmail.com>
In-Reply-To: <1439585329-10542-4-git-send-email-jacob.e.keller@intel.com>

On Fri, Aug 14, 2015 at 4:48 PM, Jacob Keller <jacob.e.keller@intel.com> wrote:
> From: Jacob Keller <jacob.keller@gmail.com>
>
> Teach git-notes about "notes.mergestrategy" to select a general strategy
> for all notes merges. This enables a user to always get expected merge
> strategy such as "cat_sort_uniq" without having to pass the "-s" option
> manually.
>
> Signed-off-by: Jacob Keller <jacob.keller@gmail.com>
> ---
> diff --git a/builtin/notes.c b/builtin/notes.c
> index 042348082709..d65134f89b40 100644
> --- a/builtin/notes.c
> +++ b/builtin/notes.c
> @@ -738,6 +738,41 @@ static int merge_commit(struct notes_merge_options *o)
> +static int git_config_get_notes_strategy(const char *key,
> +                                        enum notes_merge_strategy *strategy)
> +{
> +       const char *value = NULL;
> +
> +       git_config_get_string_const(key, &value);
> +
> +       if (value) {
> +               if (parse_notes_strategy(value, strategy))
> +                       git_die_config(key, "unknown notes merge strategy '%s'", value);
> +               else
> +                       return 0;
> +       }
> +
> +       return 1;
> +}

Nice. This seems like a better (and more user-helpful) approach.

Instead of initializing value to NULL and ignoring the return value of
git_config_get_string_const(), would it instead make sense to respect
the return of git_config_get_string_const(), like this?

    const char *value;

    if (!git_config_get_string_const(key, &value)) {
        if (parse_notes_strategy(value, strategy))
            git_die_config(key, "unknown notes merge strategy '%s'", value);
        return 0;
    }
    return 1;

Or, the equivalent, but less indented:

    if (git_config_get_string_const(key, &value))
        return 1;
    if (parse_notes_strategy(value, strategy))
        git_die_config(key, "unknown notes merge strategy '%s'", value);
    return 0;