Re: [PATCH v2 2/2] notes: add notes.merge option to select default strategy

[Eric Sunshine <sunshine@sunshineco.com>]

On Fri, Jul 31, 2015 at 7:12 PM, Jacob Keller <jacob.e.keller@intel.com> wrote:
> 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 the configuration option. Ensure that command line
> --strategy option overrides the configured value. Ensure that -s can't
> be passed with --commit or --abort. Ensure that --abort and --commit
> can't be used together.
>
> Signed-off-by: Jacob Keller <jacob.keller@gmail.com>
> Cc: Johan Herland <johan@herland.net>
> Cc: Michael Haggerty <mhagger@alum.mit.edu>
> Cc: Eric Sunshine <sunshine@sunshineco.com>

Thanks, this looks better than the previous round. A few comments below...

> diff --git a/Documentation/git-notes.txt b/Documentation/git-notes.txt
> index 674682b34b83..d8944f5aec60 100644
> --- a/Documentation/git-notes.txt
> +++ b/Documentation/git-notes.txt
> @@ -101,13 +101,13 @@ merge::
>         any) into the current notes ref (called "local").
>  +
>  If conflicts arise and a strategy for automatically resolving
> -conflicting notes (see the -s/--strategy option) 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.
> -When done, the user can either finalize the merge with
> -'git notes merge --commit', or abort the merge with
> -'git notes merge --abort'.
> +conflicting notes (see the -s/--strategy option or notes.merge
> +config option) 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. When done, the user
> +can either finalize the merge with 'git notes merge --commit',
> +or abort the merge with 'git notes merge --abort'.

When you re-wrap the text like this, it's difficult to spot your one
little change in all the diff noise. For the sake of review, it's
often better to minimize the change, even if it leaves the text more
jagged than you'd like.

>  remove::
>         Remove the notes for given objects (defaults to HEAD). When
> @@ -181,10 +181,10 @@ OPTIONS
>  -s <strategy>::
>  --strategy=<strategy>::
>         When merging notes, resolve notes conflicts using the given
> -       strategy. The following strategies are recognized: "manual"
> -       (default), "ours", "theirs", "union" and "cat_sort_uniq".
> -       See the "NOTES MERGE STRATEGIES" section below for more
> -       information on each notes merge strategy.
> +       strategy. Overrides "notes.merge". The following strategies are
> +       recognized: `manual`, `ours`, `theirs`, `union` and
> +       `cat_sort_uniq`. See the `NOTES MERGE STRATEGIES` section below
> +       for more information on each notes merge strategy.

Ditto.

>  --commit::
>         Finalize an in-progress 'git notes merge'. Use this option
> diff --git a/builtin/notes.c b/builtin/notes.c
> index 88fdafb32bc0..728980bc79bf 100644
> --- a/builtin/notes.c
> +++ b/builtin/notes.c
> @@ -741,6 +743,25 @@ static int merge_commit(struct notes_merge_options *o)
>         return ret;
>  }
>
> +static int parse_notes_strategy(const char *arg, enum notes_merge_strategy *strategy)
> +{
> +       if (!strcmp(arg, "manual"))
> +               *strategy = NOTES_MERGE_RESOLVE_MANUAL;
> +       else if (!strcmp(arg, "ours"))
> +               *strategy = NOTES_MERGE_RESOLVE_OURS;
> +       else if (!strcmp(arg, "theirs"))
> +               *strategy = NOTES_MERGE_RESOLVE_THEIRS;
> +       else if (!strcmp(arg, "union"))
> +               *strategy = NOTES_MERGE_RESOLVE_UNION;
> +       else if (!strcmp(arg, "cat_sort_uniq"))
> +               *strategy = NOTES_MERGE_RESOLVE_CAT_SORT_UNIQ;
> +       else {
> +               return 1;

In this codebase, it's customary to return 0 on success and -1 on error

> +       }

Style: drop unnecessary braces

> +
> +       return 0;
> +}
> +
>  static int merge(int argc, const char **argv, const char *prefix)
>  {
>         struct strbuf remote_ref = STRBUF_INIT, msg = STRBUF_INIT;
> @@ -950,6 +961,18 @@ 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);

For the non-error case, don't you want to 'return 0' here to indicate
that you handled the config variable rather than letting it fall
through to git_default_config() below?

> +       }
> +
> +       return git_default_config(var, value, cb);
> +}
> +
>  int cmd_notes(int argc, const char **argv, const char *prefix)
>  {
>         int result;