v7, 4 partsnotes: document cat_sort_uniq rewriteMode

> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index 75ec02e8e90a..de67ad1fdedf 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -1947,8 +1947,8 @@ notes.rewriteMode::
>  	When copying notes during a rewrite (see the
>  	"notes.rewrite.<command>" option), determines what to do if
>  	the target commit already has a note.  Must be one of
> -	`overwrite`, `concatenate`, or `ignore`.  Defaults to
> -	`concatenate`.
> +	`overwrite`, `concatenate`, `cat_sort_uniq`, or `ignore`.
> +	Defaults to `concatenate`.
>  +
>  This setting can be overridden with the `GIT_NOTES_REWRITE_MODE`
>  environment variable.
> diff --git a/Documentation/git-notes.txt b/Documentation/git-notes.txt
> index 851518d531b5..674682b34b83 100644
> --- a/Documentation/git-notes.txt
> +++ b/Documentation/git-notes.txt
> @@ -331,7 +331,8 @@ environment variable.
>  notes.rewriteMode::
>  	When copying notes during a rewrite, what to do if the target
>  	commit already has a note.  Must be one of `overwrite`,
> -	`concatenate`, and `ignore`.  Defaults to `concatenate`.
> +	`concatenate`, `cat_sort_uniq`, or `ignore`.  Defaults to
> +	`concatenate`.
>  +
>  This setting can be overridden with the `GIT_NOTES_REWRITE_MODE`
>  environment variable.

> Jacob Keller <jacob.e.keller@intel.com> writes:
>
>> diff --git a/Documentation/config.txt b/Documentation/config.txt
>> index 75ec02e8e90a..de67ad1fdedf 100644
>> --- a/Documentation/config.txt
>> +++ b/Documentation/config.txt
>> @@ -1947,8 +1947,8 @@ notes.rewriteMode::
>>       When copying notes during a rewrite (see the
>>       "notes.rewrite.<command>" option), determines what to do if
>>       the target commit already has a note.  Must be one of
>> -     `overwrite`, `concatenate`, or `ignore`.  Defaults to
>> -     `concatenate`.
>> +     `overwrite`, `concatenate`, `cat_sort_uniq`, or `ignore`.
>> +     Defaults to `concatenate`.
>>  +
>>  This setting can be overridden with the `GIT_NOTES_REWRITE_MODE`
>>  environment variable.
>> diff --git a/Documentation/git-notes.txt b/Documentation/git-notes.txt
>> index 851518d531b5..674682b34b83 100644
>> --- a/Documentation/git-notes.txt
>> +++ b/Documentation/git-notes.txt
>> @@ -331,7 +331,8 @@ environment variable.
>>  notes.rewriteMode::
>>       When copying notes during a rewrite, what to do if the target
>>       commit already has a note.  Must be one of `overwrite`,
>> -     `concatenate`, and `ignore`.  Defaults to `concatenate`.
>> +     `concatenate`, `cat_sort_uniq`, or `ignore`.  Defaults to
>> +     `concatenate`.
>>  +
>>  This setting can be overridden with the `GIT_NOTES_REWRITE_MODE`
>>  environment variable.
>
> This obviously is not a problem introduced by this patch, but I
> wonder why we have two similar but different set of modes for
> rewrtie and merge.  Isn't 'overwrite' like 'ours', 'ignore' like
> 'theirs', and 'concat' like 'union', and if these are similar
> enough, perhaps it would be helpful to the end user if we unified
> the terms (or accepted both as synonyms for backward compatibility)?
>
> Also I notice that you cannot manually reconcile while rewriting;
> don't we want to have 'manual' there, too, I wonder?
>
> [jc: Cc'ed Thomas who invented rewrite back when merge was not even
> there, and Johan who added merge]
>

> On Fri, Aug 14, 2015 at 3:11 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Jacob Keller <jacob.e.keller@intel.com> writes:
>>
>>> diff --git a/Documentation/config.txt b/Documentation/config.txt
>>> index 75ec02e8e90a..de67ad1fdedf 100644
>>> --- a/Documentation/config.txt
>>> +++ b/Documentation/config.txt
>>> @@ -1947,8 +1947,8 @@ notes.rewriteMode::
>>>       When copying notes during a rewrite (see the
>>>       "notes.rewrite.<command>" option), determines what to do if
>>>       the target commit already has a note.  Must be one of
>>> -     `overwrite`, `concatenate`, or `ignore`.  Defaults to
>>> -     `concatenate`.
>>> +     `overwrite`, `concatenate`, `cat_sort_uniq`, or `ignore`.
>>> +     Defaults to `concatenate`.
>>>  +
>>>  This setting can be overridden with the `GIT_NOTES_REWRITE_MODE`
>>>  environment variable.
>>> diff --git a/Documentation/git-notes.txt b/Documentation/git-notes.txt
>>> index 851518d531b5..674682b34b83 100644
>>> --- a/Documentation/git-notes.txt
>>> +++ b/Documentation/git-notes.txt
>>> @@ -331,7 +331,8 @@ environment variable.
>>>  notes.rewriteMode::
>>>       When copying notes during a rewrite, what to do if the target
>>>       commit already has a note.  Must be one of `overwrite`,
>>> -     `concatenate`, and `ignore`.  Defaults to `concatenate`.
>>> +     `concatenate`, `cat_sort_uniq`, or `ignore`.  Defaults to
>>> +     `concatenate`.
>>>  +
>>>  This setting can be overridden with the `GIT_NOTES_REWRITE_MODE`
>>>  environment variable.
>>
>> This obviously is not a problem introduced by this patch, but I
>> wonder why we have two similar but different set of modes for
>> rewrtie and merge.

>>
>> [jc: Cc'ed Thomas who invented rewrite back when merge was not even
>> there, and Johan who added merge]
>>
>
> I was not sure. I believe that re-write doesn't do the same thing as
> merge? I think we could make all of them handle the "overwrite", which
> is basically a synonym of, I think "theirs" depending on the direction
> of the "merge".

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

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

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

> diff --git a/builtin/notes.c b/builtin/notes.c
> index 12a42b583f98..bdfd9c7d29b4 100644
> --- a/builtin/notes.c
> +++ b/builtin/notes.c
> ...
> @@ -833,7 +833,14 @@ static int merge(int argc, const char **argv, const char *prefix)
>  			usage_with_options(git_notes_merge_usage, options);
>  		}
>  	} else {
> -		git_config_get_notes_strategy("notes.mergestrategy", &o.strategy);
> +		if (!skip_prefix(o.local_ref, "refs/notes/", &short_ref))
> +			die("Refusing to merge notes into %s (outside of refs/notes/)",
> +			    o.local_ref);
> +

> +		strbuf_addf(&merge_key, "notes.%s.mergestrategy", short_ref);
> +
> +		if (git_config_get_notes_strategy(merge_key.buf, &o.strategy))
> +			git_config_get_notes_strategy("notes.mergestrategy", &o.strategy);
>  	}

> Jacob Keller <jacob.e.keller@intel.com> writes:
>> diff --git a/builtin/notes.c b/builtin/notes.c
>> index 12a42b583f98..bdfd9c7d29b4 100644
>> --- a/builtin/notes.c
>> +++ b/builtin/notes.c
>> +             strbuf_addf(&merge_key, "notes.%s.mergestrategy", short_ref);
>> +
>> +             if (git_config_get_notes_strategy(merge_key.buf, &o.strategy))
>> +                     git_config_get_notes_strategy("notes.mergestrategy", &o.strategy);
>>       }
>
> I think you are leaking merge_key after you are done using it.

> On Fri, Aug 14, 2015 at 6:01 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Jacob Keller <jacob.e.keller@intel.com> writes:
>>> diff --git a/builtin/notes.c b/builtin/notes.c
>>> index 12a42b583f98..bdfd9c7d29b4 100644
>>> --- a/builtin/notes.c
>>> +++ b/builtin/notes.c
>>> +             strbuf_addf(&merge_key, "notes.%s.mergestrategy", short_ref);
>>> +
>>> +             if (git_config_get_notes_strategy(merge_key.buf, &o.strategy))
>>> +                     git_config_get_notes_strategy("notes.mergestrategy", &o.strategy);
>>>       }
>>
>> I think you are leaking merge_key after you are done using it.
>
> In addition to fixing the leak, since 'merge_key' is only used within
> this block, it might also make sense to declare it in this block
> rather than at the top of the function.

> Jacob Keller <jacob.e.keller@intel.com> writes:
>
>> diff --git a/builtin/notes.c b/builtin/notes.c
>> index 12a42b583f98..bdfd9c7d29b4 100644
>> --- a/builtin/notes.c
>> +++ b/builtin/notes.c
>> ...
>> @@ -833,7 +833,14 @@ static int merge(int argc, const char **argv, const char *prefix)
>>                       usage_with_options(git_notes_merge_usage, options);
>>               }
>>       } else {
>> -             git_config_get_notes_strategy("notes.mergestrategy", &o.strategy);
>> +             if (!skip_prefix(o.local_ref, "refs/notes/", &short_ref))
>> +                     die("Refusing to merge notes into %s (outside of refs/notes/)",
>> +                         o.local_ref);
>> +
>
> Sorry, but I lost track.
>
> Do I understand correctly the consensus on the previous discussion?
> My understanding is:
>
>  (1) We do not currently refuse to merge notes into anywhere outside
>      of refs/notes/;
>

>  (2) But that is not a designed behaviour---we simply forgot to
>      check it---we should start checking and refusing.
>
> If that is the concensus, having this check somewhere in the merge()
> function is indeed necessary, but this looks very out of place.
> Think what happens if the user passes "--stratagy manual" from the
> command line.  This check is not even performed, is it?
>

> I'd prefer to see:
>
>  * "Let's start making sure that we do not allow touching outside
>    refs/notes/" as a separate patch, perhaps as a preparatory step.
>

>
>  * That separate patch to add this restriction should test that
>    the refusal triggers correctly when it should, and it does not
>    trigger when it shouldn't.
>
>> +             strbuf_addf(&merge_key, "notes.%s.mergestrategy", short_ref);
>> +
>> +             if (git_config_get_notes_strategy(merge_key.buf, &o.strategy))
>> +                     git_config_get_notes_strategy("notes.mergestrategy", &o.strategy);
>>       }
>
> I think you are leaking merge_key after you are done using it.
>
> It is tempting to suggest writing the above like so:
>
>                 git_config_get_notes_strategy(merge_key.buf, &o.strategy)) ||
>                 git_config_get_notes_strategy("notes.mergestrategy", &o.strategy);
>
> which might make it more obvious what is going on, but I do not care
> too deeply about it.  To be honest, I am not sure which one is
> easier to read in the longer term myself ;-).
>

>> Sorry, but I lost track.
>>
>> Do I understand correctly the consensus on the previous discussion?
>> My understanding is:
>>
>>  (1) We do not currently refuse to merge notes into anywhere outside
>>      of refs/notes/;
>
> We do. I mis understood the original code. We check inside
> "init_notes_check()", which will check if the ref is under refs/notes/

> From: Jacob Keller <jacob.keller@gmail.com>
>
> Add new option "notes.<ref>.mergestrategy" option which specifies the merge
> strategy for merging into a given notes ref. This option enables
> selection of merge strategy for particular notes refs, rather than all
> notes ref merges, as user may not want cat_sort_uniq for all refs, but
> only some. Note that the <ref> is the local reference we are merging
> into, not the remote ref we merged from. The assumption is that users
> will mostly want to configure separate local ref merge strategies rather
> than strategies depending on which remote ref they merge from. Also,
> notes.<ref>.merge overrides the general behavior as it is more specific.
>
> Signed-off-by: Jacob Keller <jacob.keller@gmail.com>
> ---
>  Documentation/config.txt            |  7 +++++++
>  Documentation/git-notes.txt         |  6 ++++++
>  builtin/notes.c                     | 13 ++++++++++---
>  t/t3309-notes-merge-auto-resolve.sh | 39 +++++++++++++++++++++++++++++++++++++
>  4 files changed, 62 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index 5e3e03459de7..47478311367e 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -1926,6 +1926,13 @@ notes.mergestrategy::
>         STRATEGIES" section of linkgit:git-notes[1] for more information
>         on each strategy.
>
> +notes.<localref>.mergestrategy::