v4, 4 partsnotes: document cat_sort_uniq rewriteMode

> From: Jacob Keller <jacob.keller@gmail.com>
>
> Teach documentation about the cat_sort_uniq rewriteMode that got added
> at the same time as the equivalent merge strategy.
>
> Signed-off-by: Jacob Keller <jacob.keller@gmail.com>

> From: Jacob Keller <jacob.keller@gmail.com>
>
> Add new tests to ensure that --commit, --abort, and --strategy are
> mutually exclusive.
>
> Signed-off-by: Jacob Keller <jacob.keller@gmail.com>

> From: Jacob Keller <jacob.keller@gmail.com>
>
> Teach git-notes about "notes.merge" 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                     | 44 +++++++++++++++++++++++--------------
>  notes-merge.h                       | 16 ++++++++------
>  t/t3309-notes-merge-auto-resolve.sh | 29 ++++++++++++++++++++++++
>  5 files changed, 86 insertions(+), 24 deletions(-)
>

> diff --git a/builtin/notes.c b/builtin/notes.c
> index 63f95fc55439..3c705f5e26ff 100644
> --- a/builtin/notes.c
> +++ b/builtin/notes.c
> @@ -737,6 +737,24 @@ 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;
> @@ -745,7 +763,7 @@ static int merge(int argc, const char **argv, const char *prefix)
>         struct notes_merge_options o;
>         int do_merge = 0, do_commit = 0, do_abort = 0;
>         int verbosity = 0, result;
> -       const char *strategy = NULL;
> +       const char *strategy = NULL, *configured_strategy = NULL;
>         struct option options[] = {
>                 OPT_GROUP(N_("General options")),
>                 OPT__VERBOSITY(&verbosity),
> @@ -795,21 +813,15 @@ static int merge(int argc, const char **argv, const char *prefix)
>         expand_notes_ref(&remote_ref);
>         o.remote_ref = remote_ref.buf;
>
> -       if (strategy) {
> -               if (!strcmp(strategy, "manual"))
> -                       o.strategy = NOTES_MERGE_RESOLVE_MANUAL;
> -               else if (!strcmp(strategy, "ours"))
> -                       o.strategy = NOTES_MERGE_RESOLVE_OURS;
> -               else if (!strcmp(strategy, "theirs"))
> -                       o.strategy = NOTES_MERGE_RESOLVE_THEIRS;
> -               else if (!strcmp(strategy, "union"))
> -                       o.strategy = NOTES_MERGE_RESOLVE_UNION;
> -               else if (!strcmp(strategy, "cat_sort_uniq"))
> -                       o.strategy = NOTES_MERGE_RESOLVE_CAT_SORT_UNIQ;
> -               else {
> -                       error("Unknown -s/--strategy: %s", strategy);
> -                       usage_with_options(git_notes_merge_usage, options);
> -               }
> +       git_config_get_string_const("notes.merge", &configured_strategy);
> +
> +       if (configured_strategy &&
> +           parse_notes_strategy(configured_strategy, &o.strategy))
> +               die("Unknown notes merge strategy: %s", configured_strategy);
> +
> +       if (strategy && parse_notes_strategy(strategy, &o.strategy)) {
> +               error("Unknown -s/--strategy: %s", strategy);
> +               usage_with_options(git_notes_merge_usage, options);
>         }

> Add new option "notes.<ref>.merge" 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>
> ---
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index 488c2e8eec1b..2c283ebc309e 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -1912,6 +1912,12 @@ notes.merge::
>         STRATEGIES" section of linkgit:git-notes[1] for more information
>         on each strategy.
>
> +notes.<localref>.merge::
> +       Which merge strategy to choose if the local ref for a notes merge
> +       matches <localref>, overriding "notes.merge". <localref> just be a

> From: Jacob Keller <jacob.keller@gmail.com>
>
> Add new option "notes.<ref>.merge" 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            |  6 ++++++
>  Documentation/git-notes.txt         |  6 ++++++
>  builtin/notes.c                     |  7 +++++--
>  t/t3309-notes-merge-auto-resolve.sh | 39 +++++++++++++++++++++++++++++++++++++
>  4 files changed, 56 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index 488c2e8eec1b..2c283ebc309e 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -1912,6 +1912,12 @@ notes.merge::
>         STRATEGIES" section of linkgit:git-notes[1] for more information
>         on each strategy.
>
> +notes.<localref>.merge::
> +       Which merge strategy to choose if the local ref for a notes merge
> +       matches <localref>, overriding "notes.merge". <localref> just be a

> I know that we don't yet have a "proper" place to put remote notes refs,
> but the <ref> in notes.<ref>.merge _must_ be a "local" notes ref (you even
> use the <localref> notation in the documentation below). Thus, I believe
> we can presume that the local notes ref must live under refs/notes/*,
> and hence drop the "refs/notes/" prefix from the config key (just like
> branch.<name>.* can presume that <name> lives under refs/heads/*).

> Johan Herland <johan@herland.net> writes:
>
>> I know that we don't yet have a "proper" place to put remote notes refs,
>> but the <ref> in notes.<ref>.merge _must_ be a "local" notes ref (you even
>> use the <localref> notation in the documentation below). Thus, I believe
>> we can presume that the local notes ref must live under refs/notes/*,
>> and hence drop the "refs/notes/" prefix from the config key (just like
>> branch.<name>.* can presume that <name> lives under refs/heads/*).
>
> I am OK going in that direction, as long as we promise that "notes
> merge" will forever refuse to work on --notes=$ref where $ref does
> not begin with refs/notes/.
>

> On Tue, Aug 11, 2015 at 7:26 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Johan Herland <johan@herland.net> writes:
>>
>>> I know that we don't yet have a "proper" place to put remote notes refs,
>>> but the <ref> in notes.<ref>.merge _must_ be a "local" notes ref (you even
>>> use the <localref> notation in the documentation below). Thus, I believe
>>> we can presume that the local notes ref must live under refs/notes/*,
>>> and hence drop the "refs/notes/" prefix from the config key (just like
>>> branch.<name>.* can presume that <name> lives under refs/heads/*).
>>
>> I am OK going in that direction, as long as we promise that "notes
>> merge" will forever refuse to work on --notes=$ref where $ref does
>> not begin with refs/notes/.
>
> It appears that notes merge already allows operating on refs not in "refs/notes"

> Jacob Keller <jacob.keller@gmail.com> writes:
>
>> On Tue, Aug 11, 2015 at 7:26 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>> Johan Herland <johan@herland.net> writes:
>>>
>>>> I know that we don't yet have a "proper" place to put remote notes refs,
>>>> but the <ref> in notes.<ref>.merge _must_ be a "local" notes ref (you even
>>>> use the <localref> notation in the documentation below). Thus, I believe
>>>> we can presume that the local notes ref must live under refs/notes/*,
>>>> and hence drop the "refs/notes/" prefix from the config key (just like
>>>> branch.<name>.* can presume that <name> lives under refs/heads/*).
>>>
>>> I am OK going in that direction, as long as we promise that "notes
>>> merge" will forever refuse to work on --notes=$ref where $ref does
>>> not begin with refs/notes/.
>>
>> It appears that notes merge already allows operating on refs not in "refs/notes"
>
> If that is the case, then the only sane choice left to us is to
> accept only fully qualified refname, e.g. refs/notes/commit, in
> notes.<ref>.mergestrategy, without shortening, I would think.

> Oh interesting. I did a test. If you provide a fully qualified ref not
> inside refs/notes, then it assumes you meant refs/notes/refs/foo/y
> rather than refs/foo/y
>
> I need to do some more digging on this to determine the exact thing going on...
>
> Regards,
> Jake

> On Wed, Aug 12, 2015 at 12:16 PM, Jacob Keller <jacob.keller@gmail.com> wrote:
>> Oh interesting. I did a test. If you provide a fully qualified ref not
>> inside refs/notes, then it assumes you meant refs/notes/refs/foo/y
>> rather than refs/foo/y
>>
>> I need to do some more digging on this to determine the exact thing going on...
>>
>> Regards,
>> Jake
>
> I did some more digging. If you pass a notes ref to "--refs" option,

> that requires all notes to be bound to refs/notes/* and does not allow
> passing of arbitrary refs. However, you can set the environment
> variable GIT_NOTES_REF or core.notesRef to a fully qualified
> reference.
>
> That seems very arbitrary that --ref works by expanding notes and the
> environment variable and configuration option do not... [1]

> I think this inconsistency is very weird, because *most* people will
> not use refs/notes/* etc. This makes it so that --refs forces you to
> use refs/notes/* or it will prefix it for you... ie: you can use
> notes/x, refs/notes/x, x, but if you use refs/tags/x it will DWIM into
> refs/notes/refs/tags/x
>
> I think this is very confusing that --refs doesn't behave the same as
> other sections... either we should enforce this on all refs or we
> should fix the DWIM-ery to be consistent.
>
> that is, we should fix DWIM-ery to be:
>
> (1) if it starts with refs/* leave it alone
>
> (2) if it starts with notes/*, prefix it with refs/
>
> (3) otherwise prefix it with refs/notes/
>
> But that way, refs/some-other-notes/ will work fine instead of
> becoming something else.

> Regards,
> Jake
>
> [1] 8ef313e1ec3b ("builtin/notes.c: Split notes ref DWIMmery into a
> separate function")
> --
> 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

> Johan Herland <johan@herland.net> writes:
>> I know that we don't yet have a "proper" place to put remote notes refs,
>> but the <ref> in notes.<ref>.merge _must_ be a "local" notes ref (you even
>> use the <localref> notation in the documentation below). Thus, I believe
>> we can presume that the local notes ref must live under refs/notes/*,
>> and hence drop the "refs/notes/" prefix from the config key (just like
>> branch.<name>.* can presume that <name> lives under refs/heads/*).
>
> I am OK going in that direction, as long as we promise that "notes
> merge" will forever refuse to work on --notes=$ref where $ref does
> not begin with refs/notes/.

>> Except that this patch in its current form will occupy the .merge config
>> key...
>>
>> Can you rename to notes.<name>.mergestrategy instead?
>
> This is an excellent suggestion.
>
>> Or even better, take inspiration from branch.<name>.mergeoptions,
>
> Please don't.
>
> That is one of the design mistakes that was copied from another
> design mistake (remotes.*.tagopt).  I'd want to see us not to repeat
> these design mistakes.
>
> These configuration variables were made to take free-form text value
> that is split according to shell rules, primarily because it was
> expedient to implement.  Read its value into a $variable and put it
> at the end of the command line to let the shell split it.  "tagopt"
> was done a bit more carefully in that it made to react only with a
> fixed string "--no-tags", so it was hard to abuse, but "mergeoptions"
> allowed you to override something that you wouldn't want to (e.g. it
> even allowed you to feed '--message=foo').
>
> Once you start from such a broken design, it would be hard to later
> make it saner, even if you wanted to.  You have to retroactively
> forbid something that "worked" (with some definition of "working"),
> or you have to split, parse and then reject something that does not
> make sense yourself, reimplementing dequote/split rule used in the
> shell---which is especially problematic when you no longer write in
> shell scripts.
>
> So a single string value that names one of the supported strategy
> stored in notes.<name>.mergestrategy is an excellent choice.  An
> arbitrary string in notes.<name>.mergeoptions is to be avoided.

> If we don't already refuse to merge into a ref outside refs/notes, then
> I would consider that a bug to be fixed, and not some corner use case that
> we must preserve for all future.
>
> After all, we do already have a test in t3308 named 'fail to merge into
> various non-notes refs', where one of the non-notes ref being tested are:
>
>   test_must_fail git -c "core.notesRef=refs/heads/master" notes merge x
>

> On Wed, Aug 12, 2015 at 2:46 PM, Johan Herland <johan@herland.net> wrote:
>> If we don't already refuse to merge into a ref outside refs/notes, then
>> I would consider that a bug to be fixed, and not some corner use case that
>> we must preserve for all future.
>>
>> After all, we do already have a test in t3308 named 'fail to merge into
>> various non-notes refs', where one of the non-notes ref being tested are:
>>
>>   test_must_fail git -c "core.notesRef=refs/heads/master" notes merge x
>>
>
> This test is checking if the ref pointed at by refs/heads/master *is*
> a note. But you could create a ref outside of refs/notes which is a
> note but which isn't inside refs/notes
>
> I did just find that we expand remote-ref using expand_notes_ref, and
> it does *not* currently let us reference refs outside of refs/notes..
> so we can merge IN to a ref not inside refs/notes (using the
> environment variable) but we can't merge FROM
> refs/tracking/origin/notes/y for example, which means currently all
> notes we merge from have to be located into refs/notes/*
>
> There are some weird issues here.
>
> Regards,
> Jake

> Jacob Keller <jacob.keller@gmail.com> writes:
>
>> I spoke to soon. We have an "init_notes_check" function which shows
>> that it does refuse to merge outside of refs/notes/* It prevents all
>> notes operations outside of refs/notes
>
> OK.  Then it is OK to limit notes.<ref>.mergestrategy so that <ref>
> refers to what comes after refs/notes/, because we will not allow
> merging to happen outside the hierarchy.
>
> If you are planning to break that promise, however, <ref> must be
> always spelled fully (i.e. with refs/notes/ prefix for those inside
> the hierarchy) to avoid ambiguity.  Otherwise it will be hard to
> interpret a configuration that does something like this (note that
> these could come from multiple places, e.g. $HOME/.gitconfig and
> $GIT_DIR/config):
>

>     [notes "commits"]
>         mergestrategy = concatenate
>     [notes "notes/commits"]
>         mergestrategy = cat_sort_uniq
>     [notes "refs/notes/commits"]
>         mergestrategy = overwrite
>
> The three entries in the above example obviously are all meant to
> refer to the same refs/notes/commits notes tree, and the usual "last
> one wins" rule should apply.  But with the recent git_config_get_*()
> interface, you cannot tell which one among them was given the last,
> overriding the previous entries.