v2, 2 partsnotes: add notes.merge strategy option

> 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>

> 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'.

>  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.

>  --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;

> +
> +       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);

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

> 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;

>>  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.

> On Sat, Aug 1, 2015 at 7:46 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> >>  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.
> 
> This results in something incredibly jagged. I can't find a good way
> to split which minimizes the change. Would a 3rd patch which just
> re-flows this be an acceptable alternative
> 
> ie: add the words in one patch then re-flow afterwards in a second
> patch with no changes?
> 
> There is no good alternative as other re-flows I tried end up looking
> way too jagged, as compared to surrounding documentation.

> Don't worry too much about it. Consider it something to keep in mind for
> future patches. I reviewed the change and it seemed okay. I mentioned it
> because one of the goals of patch submission, in addition to making an
> actual change, is to ease the review process. If Junio is okay with
> accepting it as is, then it's probably not worth spending more time
> trying to refine it.
>
> Having said that, I came up with the following for those two paragraphs,
> which gives a much less noisy diff and doesn't look too jagged:
>

> From: Jacob Keller <jacob.keller@gmail.com>
>
> This series adds a default merge strategy option for git-notes, so that
> the user does not have to type "-s" every time. It is overridden by the
> -s option.

> In addition, I found a small documentation
> bug which is corrected in the first patch of the series.
>
> I Cc'd a couple more people in this version of the patch in order to
> hopefully get some more review.

>
>  Documentation/config.txt              | 11 +++++--
>  Documentation/git-notes.txt           | 33 +++++++++++++--------
>  builtin/notes.c                       | 55 +++++++++++++++++++++++++----------
>  notes-merge.h                         | 16 +++++-----
>  t/t3309-notes-merge-auto-resolve.sh   | 29 ++++++++++++++++++
>  t/t3310-notes-merge-manual-resolve.sh | 12 ++++++++
>  6 files changed, 119 insertions(+), 37 deletions(-)
>
> --
> 2.5.0.482.gfcd5645
>
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html