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

[Johan Herland <johan@herland.net>]

On Fri, Aug 14, 2015 at 11:13 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>
> ---
>  Documentation/config.txt            |  7 ++++++
>  Documentation/git-notes.txt         | 14 +++++++++++-
>  builtin/notes.c                     | 45 ++++++++++++++++++++++++++++---------
>  notes-merge.h                       | 16 +++++++------
>  t/t3309-notes-merge-auto-resolve.sh | 29 ++++++++++++++++++++++++
>  5 files changed, 92 insertions(+), 19 deletions(-)
>
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index de67ad1fdedf..5e3e03459de7 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -1919,6 +1919,13 @@ mergetool.writeToTemp::
>  mergetool.prompt::
>         Prompt before each invocation of the merge resolution program.
>
> +notes.mergestrategy::

Just one small nit: Config keys are (AFAIK) case-insensitive, and we
can thus use
camelCasing in the documentation to make long keywords more readable (see e.g.
notes.displayRef below). So I suggest using notes.mergeStrategy here.

> +       Which merge strategy to choose by default when resolving notes
> +       conflicts. Must be one of `manual`, `ours`, `theirs`, `union`,
> +       or `cat_sort_uniq`. Defaults to `manual`. See "NOTES MERGE
> +       STRATEGIES" section of linkgit:git-notes[1] for more information
> +       on each strategy.
> +
>  notes.displayRef::
>         The (fully qualified) refname from which to show notes when
>         showing commit messages.  The value of this variable can be set
> diff --git a/Documentation/git-notes.txt b/Documentation/git-notes.txt
> index 674682b34b83..89c8829a0543 100644
> --- a/Documentation/git-notes.txt
> +++ b/Documentation/git-notes.txt
> @@ -101,7 +101,7 @@ 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,
> +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.mergestrategy" configuration setting.
>         See the "NOTES MERGE STRATEGIES" section below for more
>         information on each notes merge strategy.
>
> @@ -247,6 +248,9 @@ When done, the user can either finalize the merge with
>  'git notes merge --commit', or abort the merge with
>  'git notes merge --abort'.
>
> +Users may select an automated merge strategy from among the following using
> +either -s/--strategy option or configuring notes.mergestrategy accordingly:
> +
>  "ours" automatically resolves conflicting notes in favor of the local
>  version (i.e. the current notes ref).
>
> @@ -310,6 +314,14 @@ core.notesRef::
>         This setting can be overridden through the environment and
>         command line.
>
> +notes.mergestrategy::

Same here.

> +       Which merge strategy to choose by default when resolving notes
> +       conflicts. Must be one of `manual`, `ours`, `theirs`, `union`,
> +       or `cat_sort_uniq`. Defaults to `manual`. See "NOTES MERGE
> +       STRATEGIES" section above for more information on each strategy.
> ++
> +This setting can be overridden by passing the `--strategy` option.
> +
>  notes.displayRef::
>         Which ref (or refs, if a glob or specified more than once), in
>         addition to the default set by `core.notesRef` or
[...]

Otherwise, looks good to me.


...Johan


-- 
Johan Herland, <johan@herland.net>
www.herland.net