{"thread":{"id":"40063","subject":"[PATCH v4 1/4] notes: document cat_sort_uniq rewriteMode","startedAt":"2015-08-11T20:57:17Z","lastAt":"2015-08-12T22:51:50Z","messageCount":21,"participants":["Jacob Keller","Johan Herland","Eric Sunshine","Junio C Hamano"],"isPatch":true,"patchVersion":4,"patchTotal":4},"messages":[{"id":"267863","messageId":"1439326641-9447-1-git-send-email-jacob.e.keller@intel.com","threadId":"40063","inReplyTo":null,"subject":"[PATCH v4 0/4] add notes strategy configuration options","fromName":"Jacob Keller","fromEmail":"jacob.e.keller@intel.com","sentAt":"2015-08-11T20:57:17Z","receivedAt":"2015-08-11T20:57:17Z","isPatch":true,"sender":{"key":"jacob.e.keller@intel.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"From: Jacob Keller <jacob.keller@gmail.com>\n\nThis series of patches implements notes.merge and notes.<ref>.merge\noptions for configuring notes merge strategy such that user may avoid\ntyping \"-s\". It is (probably) most useful if the user wishes to always\nenforce cat_sort_uniq strategy.\n\nThis series now uses git_config_get_string_const() instead of trying to\nchange the git_default_config setup. In addition, the 4th patch is much\nsmaller and avoids a lot of heavy cruft due to this change.\n\nI tried to re-work everything I noticed from the email list, but please\nshout if I did not manage to recall your suggestion.\n\nThe new version should be much simpler to understand.\n\nChanges since v3:\n* don't use hash table, instead just call \"git_config_get_string_const\"\n* notes.<ref>.merge must always be fully qualified, (since notes doesn't\n  enforce the refs to be always in refs/notes, so we shouldn't either)\n* fixed documentation to reduce confusion over which strategies are\n  accepted for options\n* ensure tests cover \"notes.<ref>.merge\" overrides \"notes.merge\"\n\nI also tried to make sure the reviewers were all Cc'ed on every patch\nnot just the first one this time.\n\nJacob Keller (4):\n  notes: document cat_sort_uniq rewriteMode\n  notes: add tests for --commit/--abort/--strategy exclusivity\n  notes: add notes.merge option to select default strategy\n  notes: teach git-notes about notes.<ref>.merge option\n\n Documentation/config.txt              | 17 +++++++--\n Documentation/git-notes.txt           | 23 ++++++++++--\n builtin/notes.c                       | 49 ++++++++++++++++---------\n notes-merge.h                         | 16 +++++----\n t/t3309-notes-merge-auto-resolve.sh   | 68 +++++++++++++++++++++++++++++++++++\n t/t3310-notes-merge-manual-resolve.sh | 12 +++++++\n 6 files changed, 157 insertions(+), 28 deletions(-)\n\n-- \n2.5.0.482.gfcd5645\n"},{"id":"267862","messageId":"1439326641-9447-2-git-send-email-jacob.e.keller@intel.com","threadId":"40063","inReplyTo":"1439326641-9447-1-git-send-email-jacob.e.keller@intel.com","subject":"[PATCH v4 1/4] notes: document cat_sort_uniq rewriteMode","fromName":"Jacob Keller","fromEmail":"jacob.e.keller@intel.com","sentAt":"2015-08-11T20:57:18Z","receivedAt":"2015-08-11T20:57:18Z","isPatch":true,"sender":{"key":"jacob.e.keller@intel.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"From: Jacob Keller <jacob.keller@gmail.com>\n\nTeach documentation about the cat_sort_uniq rewriteMode that got added\nat the same time as the equivalent merge strategy.\n\nSigned-off-by: Jacob Keller <jacob.keller@gmail.com>\n---\n Documentation/config.txt    | 4 ++--\n Documentation/git-notes.txt | 3 ++-\n 2 files changed, 4 insertions(+), 3 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex da74ad0d0aca..b3df1b16491c 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -1933,8 +1933,8 @@ notes.rewriteMode::\n \tWhen copying notes during a rewrite (see the\n \t\"notes.rewrite.<command>\" option), determines what to do if\n \tthe target commit already has a note.  Must be one of\n-\t`overwrite`, `concatenate`, or `ignore`.  Defaults to\n-\t`concatenate`.\n+\t`overwrite`, `concatenate`, `cat_sort_uniq`, or `ignore`.\n+\tDefaults to `concatenate`.\n +\n This setting can be overridden with the `GIT_NOTES_REWRITE_MODE`\n environment variable.\ndiff --git a/Documentation/git-notes.txt b/Documentation/git-notes.txt\nindex 851518d531b5..674682b34b83 100644\n--- a/Documentation/git-notes.txt\n+++ b/Documentation/git-notes.txt\n@@ -331,7 +331,8 @@ environment variable.\n notes.rewriteMode::\n \tWhen copying notes during a rewrite, what to do if the target\n \tcommit already has a note.  Must be one of `overwrite`,\n-\t`concatenate`, and `ignore`.  Defaults to `concatenate`.\n+\t`concatenate`, `cat_sort_uniq`, or `ignore`.  Defaults to\n+\t`concatenate`.\n +\n This setting can be overridden with the `GIT_NOTES_REWRITE_MODE`\n environment variable.\n-- \n2.5.0.482.gfcd5645\n"},{"id":"267864","messageId":"1439326641-9447-3-git-send-email-jacob.e.keller@intel.com","threadId":"40063","inReplyTo":"1439326641-9447-1-git-send-email-jacob.e.keller@intel.com","subject":"[PATCH v4 2/4] notes: add tests for --commit/--abort/--strategy exclusivity","fromName":"Jacob Keller","fromEmail":"jacob.e.keller@intel.com","sentAt":"2015-08-11T20:57:19Z","receivedAt":"2015-08-11T20:57:19Z","isPatch":true,"sender":{"key":"jacob.e.keller@intel.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"From: Jacob Keller <jacob.keller@gmail.com>\n\nAdd new tests to ensure that --commit, --abort, and --strategy are\nmutually exclusive.\n\nSigned-off-by: Jacob Keller <jacob.keller@gmail.com>\n---\n t/t3310-notes-merge-manual-resolve.sh | 12 ++++++++++++\n 1 file changed, 12 insertions(+)\n\ndiff --git a/t/t3310-notes-merge-manual-resolve.sh b/t/t3310-notes-merge-manual-resolve.sh\nindex 195bb97f859d..d5572121da69 100755\n--- a/t/t3310-notes-merge-manual-resolve.sh\n+++ b/t/t3310-notes-merge-manual-resolve.sh\n@@ -314,6 +314,18 @@ y and z notes on 1st commit\n \n EOF\n \n+test_expect_success 'do not allow mixing --commit and --abort' '\n+\ttest_must_fail git notes merge --commit --abort\n+'\n+\n+test_expect_success 'do not allow mixing --commit and --strategy' '\n+\ttest_must_fail git notes merge --commit --strategy theirs\n+'\n+\n+test_expect_success 'do not allow mixing --abort and --strategy' '\n+\ttest_must_fail git notes merge --abort --strategy theirs\n+'\n+\n test_expect_success 'finalize conflicting merge (z => m)' '\n \t# Resolve conflicts and finalize merge\n \tcat >.git/NOTES_MERGE_WORKTREE/$commit_sha1 <<EOF &&\n-- \n2.5.0.482.gfcd5645\n"},{"id":"267865","messageId":"1439326641-9447-4-git-send-email-jacob.e.keller@intel.com","threadId":"40063","inReplyTo":"1439326641-9447-1-git-send-email-jacob.e.keller@intel.com","subject":"[PATCH v4 3/4] notes: add notes.merge option to select default strategy","fromName":"Jacob Keller","fromEmail":"jacob.e.keller@intel.com","sentAt":"2015-08-11T20:57:20Z","receivedAt":"2015-08-11T20:57:20Z","isPatch":true,"sender":{"key":"jacob.e.keller@intel.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"From: Jacob Keller <jacob.keller@gmail.com>\n\nTeach git-notes about \"notes.merge\" to select a general strategy for all\nnotes merges. This enables a user to always get expected merge strategy\nsuch as \"cat_sort_uniq\" without having to pass the \"-s\" option manually.\n\nSigned-off-by: Jacob Keller <jacob.keller@gmail.com>\n---\n Documentation/config.txt            |  7 ++++++\n Documentation/git-notes.txt         | 14 +++++++++++-\n builtin/notes.c                     | 44 +++++++++++++++++++++++--------------\n notes-merge.h                       | 16 ++++++++------\n t/t3309-notes-merge-auto-resolve.sh | 29 ++++++++++++++++++++++++\n 5 files changed, 86 insertions(+), 24 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex b3df1b16491c..488c2e8eec1b 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -1905,6 +1905,13 @@ mergetool.writeToTemp::\n mergetool.prompt::\n \tPrompt before each invocation of the merge resolution program.\n \n+notes.merge::\n+\tWhich merge strategy to choose by default when resolving notes\n+\tconflicts. Must be one of `manual`, `ours`, `theirs`, `union`,\n+\tor `cat_sort_uniq`. Defaults to `manual`. See \"NOTES MERGE\n+\tSTRATEGIES\" section of linkgit:git-notes[1] for more information\n+\ton each strategy.\n+\n notes.displayRef::\n \tThe (fully qualified) refname from which to show notes when\n \tshowing commit messages.  The value of this variable can be set\ndiff --git a/Documentation/git-notes.txt b/Documentation/git-notes.txt\nindex 674682b34b83..9c4f8536182f 100644\n--- a/Documentation/git-notes.txt\n+++ b/Documentation/git-notes.txt\n@@ -101,7 +101,7 @@ merge::\n \tany) into the current notes ref (called \"local\").\n +\n If conflicts arise and a strategy for automatically resolving\n-conflicting notes (see the -s/--strategy option) is not given,\n+conflicting notes (see the \"NOTES MERGE STRATEGIES\" section) is not given,\n the \"manual\" resolver is used. This resolver checks out the\n conflicting notes in a special worktree (`.git/NOTES_MERGE_WORKTREE`),\n and instructs the user to manually resolve the conflicts there.\n@@ -183,6 +183,7 @@ OPTIONS\n \tWhen merging notes, resolve notes conflicts using the given\n \tstrategy. The following strategies are recognized: \"manual\"\n \t(default), \"ours\", \"theirs\", \"union\" and \"cat_sort_uniq\".\n+\tThis option overrides the \"notes.merge\" configuration setting.\n \tSee the \"NOTES MERGE STRATEGIES\" section below for more\n \tinformation on each notes merge strategy.\n \n@@ -247,6 +248,9 @@ When done, the user can either finalize the merge with\n 'git notes merge --commit', or abort the merge with\n 'git notes merge --abort'.\n \n+Users may select an automated merge strategy from among the following using\n+either -s/--strategy option or configuring notes.merge accordingly:\n+\n \"ours\" automatically resolves conflicting notes in favor of the local\n version (i.e. the current notes ref).\n \n@@ -310,6 +314,14 @@ core.notesRef::\n \tThis setting can be overridden through the environment and\n \tcommand line.\n \n+notes.merge::\n+\tWhich merge strategy to choose by default when resolving notes\n+\tconflicts. Must be one of `manual`, `ours`, `theirs`, `union`,\n+\tor `cat_sort_uniq`. Defaults to `manual`. See \"NOTES MERGE\n+\tSTRATEGIES\" section above for more information on each strategy.\n++\n+This setting can be overridden by passing the `--strategy` option.\n+\n notes.displayRef::\n \tWhich ref (or refs, if a glob or specified more than once), in\n \taddition to the default set by `core.notesRef` or\ndiff --git a/builtin/notes.c b/builtin/notes.c\nindex 63f95fc55439..3c705f5e26ff 100644\n--- a/builtin/notes.c\n+++ b/builtin/notes.c\n@@ -737,6 +737,24 @@ static int merge_commit(struct notes_merge_options *o)\n \treturn ret;\n }\n \n+static int parse_notes_strategy(const char *arg, enum notes_merge_strategy *strategy)\n+{\n+\tif (!strcmp(arg, \"manual\"))\n+\t\t*strategy = NOTES_MERGE_RESOLVE_MANUAL;\n+\telse if (!strcmp(arg, \"ours\"))\n+\t\t*strategy = NOTES_MERGE_RESOLVE_OURS;\n+\telse if (!strcmp(arg, \"theirs\"))\n+\t\t*strategy = NOTES_MERGE_RESOLVE_THEIRS;\n+\telse if (!strcmp(arg, \"union\"))\n+\t\t*strategy = NOTES_MERGE_RESOLVE_UNION;\n+\telse if (!strcmp(arg, \"cat_sort_uniq\"))\n+\t\t*strategy = NOTES_MERGE_RESOLVE_CAT_SORT_UNIQ;\n+\telse\n+\t\treturn -1;\n+\n+\treturn 0;\n+}\n+\n static int merge(int argc, const char **argv, const char *prefix)\n {\n \tstruct strbuf remote_ref = STRBUF_INIT, msg = STRBUF_INIT;\n@@ -745,7 +763,7 @@ static int merge(int argc, const char **argv, const char *prefix)\n \tstruct notes_merge_options o;\n \tint do_merge = 0, do_commit = 0, do_abort = 0;\n \tint verbosity = 0, result;\n-\tconst char *strategy = NULL;\n+\tconst char *strategy = NULL, *configured_strategy = NULL;\n \tstruct option options[] = {\n \t\tOPT_GROUP(N_(\"General options\")),\n \t\tOPT__VERBOSITY(&verbosity),\n@@ -795,21 +813,15 @@ static int merge(int argc, const char **argv, const char *prefix)\n \texpand_notes_ref(&remote_ref);\n \to.remote_ref = remote_ref.buf;\n \n-\tif (strategy) {\n-\t\tif (!strcmp(strategy, \"manual\"))\n-\t\t\to.strategy = NOTES_MERGE_RESOLVE_MANUAL;\n-\t\telse if (!strcmp(strategy, \"ours\"))\n-\t\t\to.strategy = NOTES_MERGE_RESOLVE_OURS;\n-\t\telse if (!strcmp(strategy, \"theirs\"))\n-\t\t\to.strategy = NOTES_MERGE_RESOLVE_THEIRS;\n-\t\telse if (!strcmp(strategy, \"union\"))\n-\t\t\to.strategy = NOTES_MERGE_RESOLVE_UNION;\n-\t\telse if (!strcmp(strategy, \"cat_sort_uniq\"))\n-\t\t\to.strategy = NOTES_MERGE_RESOLVE_CAT_SORT_UNIQ;\n-\t\telse {\n-\t\t\terror(\"Unknown -s/--strategy: %s\", strategy);\n-\t\t\tusage_with_options(git_notes_merge_usage, options);\n-\t\t}\n+\tgit_config_get_string_const(\"notes.merge\", &configured_strategy);\n+\n+\tif (configured_strategy &&\n+\t    parse_notes_strategy(configured_strategy, &o.strategy))\n+\t\tdie(\"Unknown notes merge strategy: %s\", configured_strategy);\n+\n+\tif (strategy && parse_notes_strategy(strategy, &o.strategy)) {\n+\t\terror(\"Unknown -s/--strategy: %s\", strategy);\n+\t\tusage_with_options(git_notes_merge_usage, options);\n \t}\n \n \tt = init_notes_check(\"merge\");\ndiff --git a/notes-merge.h b/notes-merge.h\nindex 1d01f6aacf54..bda8c0c8d348 100644\n--- a/notes-merge.h\n+++ b/notes-merge.h\n@@ -8,18 +8,20 @@ enum notes_merge_verbosity {\n \tNOTES_MERGE_VERBOSITY_MAX = 5\n };\n \n+enum notes_merge_strategy {\n+\tNOTES_MERGE_RESOLVE_MANUAL = 0,\n+\tNOTES_MERGE_RESOLVE_OURS,\n+\tNOTES_MERGE_RESOLVE_THEIRS,\n+\tNOTES_MERGE_RESOLVE_UNION,\n+\tNOTES_MERGE_RESOLVE_CAT_SORT_UNIQ\n+};\n+\n struct notes_merge_options {\n \tconst char *local_ref;\n \tconst char *remote_ref;\n \tstruct strbuf commit_msg;\n \tint verbosity;\n-\tenum {\n-\t\tNOTES_MERGE_RESOLVE_MANUAL = 0,\n-\t\tNOTES_MERGE_RESOLVE_OURS,\n-\t\tNOTES_MERGE_RESOLVE_THEIRS,\n-\t\tNOTES_MERGE_RESOLVE_UNION,\n-\t\tNOTES_MERGE_RESOLVE_CAT_SORT_UNIQ\n-\t} strategy;\n+\tenum notes_merge_strategy strategy;\n \tunsigned has_worktree:1;\n };\n \ndiff --git a/t/t3309-notes-merge-auto-resolve.sh b/t/t3309-notes-merge-auto-resolve.sh\nindex 461fd84755d7..a773b01b73db 100755\n--- a/t/t3309-notes-merge-auto-resolve.sh\n+++ b/t/t3309-notes-merge-auto-resolve.sh\n@@ -298,6 +298,13 @@ test_expect_success 'merge z into y with invalid strategy => Fail/No changes' '\n \tverify_notes y y\n '\n \n+test_expect_success 'merge z into y with invalid configuration option => Fail/No changes' '\n+\tgit config core.notesRef refs/notes/y &&\n+\ttest_must_fail git -c notes.merge=\"foo\" notes merge z &&\n+\t# Verify no changes (y)\n+\tverify_notes y y\n+'\n+\n cat <<EOF | sort >expect_notes_ours\n 68b8630d25516028bed862719855b3d6768d7833 $commit_sha15\n 5de7ea7ad4f47e7ff91989fb82234634730f75df $commit_sha14\n@@ -365,6 +372,17 @@ test_expect_success 'reset to pre-merge state (y)' '\n \tverify_notes y y\n '\n \n+test_expect_success 'merge z into y with \"ours\" configuration option => Non-conflicting 3-way merge' '\n+\tgit -c notes.merge=\"ours\" notes merge z &&\n+\tverify_notes y ours\n+'\n+\n+test_expect_success 'reset to pre-merge state (y)' '\n+\tgit update-ref refs/notes/y refs/notes/y^1 &&\n+\t# Verify pre-merge state\n+\tverify_notes y y\n+'\n+\n cat <<EOF | sort >expect_notes_theirs\n 9b4b2c61f0615412da3c10f98ff85b57c04ec765 $commit_sha15\n 5de7ea7ad4f47e7ff91989fb82234634730f75df $commit_sha14\n@@ -432,6 +450,17 @@ test_expect_success 'reset to pre-merge state (y)' '\n \tverify_notes y y\n '\n \n+test_expect_success 'merge z into y with \"theirs\" strategy overriding configuration option \"ours\" => Non-conflicting 3-way merge' '\n+\tgit -c notes.merge=\"ours\" notes merge --strategy=theirs z &&\n+\tverify_notes y theirs\n+'\n+\n+test_expect_success 'reset to pre-merge state (y)' '\n+\tgit update-ref refs/notes/y refs/notes/y^1 &&\n+\t# Verify pre-merge state\n+\tverify_notes y y\n+'\n+\n cat <<EOF | sort >expect_notes_union\n 7c4e546efd0fe939f876beb262ece02797880b54 $commit_sha15\n 5de7ea7ad4f47e7ff91989fb82234634730f75df $commit_sha14\n-- \n2.5.0.482.gfcd5645\n"},{"id":"267866","messageId":"1439326641-9447-5-git-send-email-jacob.e.keller@intel.com","threadId":"40063","inReplyTo":"1439326641-9447-1-git-send-email-jacob.e.keller@intel.com","subject":"[PATCH v4 4/4] notes: teach git-notes about notes.<ref>.merge option","fromName":"Jacob Keller","fromEmail":"jacob.e.keller@intel.com","sentAt":"2015-08-11T20:57:21Z","receivedAt":"2015-08-11T20:57:21Z","isPatch":true,"sender":{"key":"jacob.e.keller@intel.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"From: Jacob Keller <jacob.keller@gmail.com>\n\nAdd new option \"notes.<ref>.merge\" option which specifies the merge\nstrategy for merging into a given notes ref. This option enables\nselection of merge strategy for particular notes refs, rather than all\nnotes ref merges, as user may not want cat_sort_uniq for all refs, but\nonly some. Note that the <ref> is the local reference we are merging\ninto, not the remote ref we merged from. The assumption is that users\nwill mostly want to configure separate local ref merge strategies rather\nthan strategies depending on which remote ref they merge from. Also,\nnotes.<ref>.merge overrides the general behavior as it is more specific.\n\nSigned-off-by: Jacob Keller <jacob.keller@gmail.com>\n---\n Documentation/config.txt            |  6 ++++++\n Documentation/git-notes.txt         |  6 ++++++\n builtin/notes.c                     |  7 +++++--\n t/t3309-notes-merge-auto-resolve.sh | 39 +++++++++++++++++++++++++++++++++++++\n 4 files changed, 56 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 488c2e8eec1b..2c283ebc309e 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -1912,6 +1912,12 @@ notes.merge::\n \tSTRATEGIES\" section of linkgit:git-notes[1] for more information\n \ton each strategy.\n \n+notes.<localref>.merge::\n+\tWhich merge strategy to choose if the local ref for a notes merge\n+\tmatches <localref>, overriding \"notes.merge\". <localref> just be a\n+\tfully qualified refname. See \"NOTES MERGE STRATEGIES\" section in\n+\tlinkgit:git-notes[1] for more information on the available strategies.\n+\n notes.displayRef::\n \tThe (fully qualified) refname from which to show notes when\n \tshowing commit messages.  The value of this variable can be set\ndiff --git a/Documentation/git-notes.txt b/Documentation/git-notes.txt\nindex 9c4f8536182f..68fae8d22555 100644\n--- a/Documentation/git-notes.txt\n+++ b/Documentation/git-notes.txt\n@@ -322,6 +322,12 @@ notes.merge::\n +\n This setting can be overridden by passing the `--strategy` option.\n \n+notes.<localref>.merge::\n+\tWhich strategy to choose when merging into <localref>. The set of\n+\tallowed values is the same as \"notes.merge\". <localref> must be a fully\n+\tqualified refname. See \"NOTES MERGE STRATEGIES\" section above for more\n+\tinformation about each strategy.\n+\n notes.displayRef::\n \tWhich ref (or refs, if a glob or specified more than once), in\n \taddition to the default set by `core.notesRef` or\ndiff --git a/builtin/notes.c b/builtin/notes.c\nindex 3c705f5e26ff..2f2d7e87ef47 100644\n--- a/builtin/notes.c\n+++ b/builtin/notes.c\n@@ -757,7 +757,7 @@ static int parse_notes_strategy(const char *arg, enum notes_merge_strategy *stra\n \n static int merge(int argc, const char **argv, const char *prefix)\n {\n-\tstruct strbuf remote_ref = STRBUF_INIT, msg = STRBUF_INIT;\n+\tstruct strbuf remote_ref = STRBUF_INIT, msg = STRBUF_INIT, merge_key = STRBUF_INIT;\n \tunsigned char result_sha1[20];\n \tstruct notes_tree *t;\n \tstruct notes_merge_options o;\n@@ -813,7 +813,10 @@ static int merge(int argc, const char **argv, const char *prefix)\n \texpand_notes_ref(&remote_ref);\n \to.remote_ref = remote_ref.buf;\n \n-\tgit_config_get_string_const(\"notes.merge\", &configured_strategy);\n+\tstrbuf_addf(&merge_key, \"notes.%s.merge\", o.local_ref);\n+\n+\tif (git_config_get_string_const(merge_key.buf, &configured_strategy))\n+\t\tgit_config_get_string_const(\"notes.merge\", &configured_strategy);\n \n \tif (configured_strategy &&\n \t    parse_notes_strategy(configured_strategy, &o.strategy))\ndiff --git a/t/t3309-notes-merge-auto-resolve.sh b/t/t3309-notes-merge-auto-resolve.sh\nindex a773b01b73db..b0bbc0a6bac2 100755\n--- a/t/t3309-notes-merge-auto-resolve.sh\n+++ b/t/t3309-notes-merge-auto-resolve.sh\n@@ -383,6 +383,17 @@ test_expect_success 'reset to pre-merge state (y)' '\n \tverify_notes y y\n '\n \n+test_expect_success 'merge z into y with \"ours\" per-ref configuration option => Non-conflicting 3-way merge' '\n+\tgit -c notes.refs/notes/y.merge=\"ours\" notes merge z &&\n+\tverify_notes y ours\n+'\n+\n+test_expect_success 'reset to pre-merge state (y)' '\n+\tgit update-ref refs/notes/y refs/notes/y^1 &&\n+\t# Verify pre-merge state\n+\tverify_notes y y\n+'\n+\n cat <<EOF | sort >expect_notes_theirs\n 9b4b2c61f0615412da3c10f98ff85b57c04ec765 $commit_sha15\n 5de7ea7ad4f47e7ff91989fb82234634730f75df $commit_sha14\n@@ -534,6 +545,34 @@ test_expect_success 'reset to pre-merge state (y)' '\n \tverify_notes y y\n '\n \n+test_expect_success 'merge z into y with \"union\" strategy overriding per-ref configuration => Non-conflicting 3-way merge' '\n+\tgit -c notes.refs/notes/y.merge=\"theirs\" notes merge --strategy=union z &&\n+\tverify_notes y union\n+'\n+\n+test_expect_success 'reset to pre-merge state (y)' '\n+\tgit update-ref refs/notes/y refs/notes/y^1 &&\n+\t# Verify pre-merge state\n+\tverify_notes y y\n+'\n+\n+test_expect_success 'merge z into y with \"union\" per-ref overriding general configuration => Non-conflicting 3-way merge' '\n+\tgit -c notes.refs/notes/y.merge=\"union\" -c notes.merge=\"theirs\" notes merge z &&\n+\tverify_notes y union\n+'\n+\n+test_expect_success 'reset to pre-merge state (y)' '\n+\tgit update-ref refs/notes/y refs/notes/y^1 &&\n+\t# Verify pre-merge state\n+\tverify_notes y y\n+'\n+\n+test_expect_success 'merge z into y with \"manual\" per-ref only checks specific ref configuration => Conflicting 3-way merge' '\n+\ttest_must_fail git -c notes.refs/notes/z.merge=\"union\" notes merge z &&\n+\tgit notes merge --abort &&\n+\tverify_notes y y\n+'\n+\n cat <<EOF | sort >expect_notes_union2\n d682107b8bf7a7aea1e537a8d5cb6a12b60135f1 $commit_sha15\n 5de7ea7ad4f47e7ff91989fb82234634730f75df $commit_sha14\n-- \n2.5.0.482.gfcd5645\n"},{"id":"267885","messageId":"CALKQrgcYpbae0eu2sT_rOZYq3DsnVSpYua1SZr=TC=dYfG_fpw@mail.gmail.com","threadId":"40063","inReplyTo":"1439326641-9447-4-git-send-email-jacob.e.keller@intel.com","subject":"Re: [PATCH v4 3/4] notes: add notes.merge option to select default strategy","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2015-08-12T00:04:21Z","receivedAt":"2015-08-12T00:04:21Z","isPatch":true,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"On Tue, Aug 11, 2015 at 10:57 PM, Jacob Keller <jacob.e.keller@intel.com> wrote:\n> From: Jacob Keller <jacob.keller@gmail.com>\n>\n> Teach git-notes about \"notes.merge\" to select a general strategy for all\n> notes merges. This enables a user to always get expected merge strategy\n> such as \"cat_sort_uniq\" without having to pass the \"-s\" option manually.\n>\n> Signed-off-by: Jacob Keller <jacob.keller@gmail.com>\n> ---\n>  Documentation/config.txt            |  7 ++++++\n>  Documentation/git-notes.txt         | 14 +++++++++++-\n>  builtin/notes.c                     | 44 +++++++++++++++++++++++--------------\n>  notes-merge.h                       | 16 ++++++++------\n>  t/t3309-notes-merge-auto-resolve.sh | 29 ++++++++++++++++++++++++\n>  5 files changed, 86 insertions(+), 24 deletions(-)\n>\n[...]\n> diff --git a/builtin/notes.c b/builtin/notes.c\n> index 63f95fc55439..3c705f5e26ff 100644\n> --- a/builtin/notes.c\n> +++ b/builtin/notes.c\n> @@ -737,6 +737,24 @@ static int merge_commit(struct notes_merge_options *o)\n>         return ret;\n>  }\n>\n> +static int parse_notes_strategy(const char *arg, enum notes_merge_strategy *strategy)\n> +{\n> +       if (!strcmp(arg, \"manual\"))\n> +               *strategy = NOTES_MERGE_RESOLVE_MANUAL;\n> +       else if (!strcmp(arg, \"ours\"))\n> +               *strategy = NOTES_MERGE_RESOLVE_OURS;\n> +       else if (!strcmp(arg, \"theirs\"))\n> +               *strategy = NOTES_MERGE_RESOLVE_THEIRS;\n> +       else if (!strcmp(arg, \"union\"))\n> +               *strategy = NOTES_MERGE_RESOLVE_UNION;\n> +       else if (!strcmp(arg, \"cat_sort_uniq\"))\n> +               *strategy = NOTES_MERGE_RESOLVE_CAT_SORT_UNIQ;\n> +       else\n> +               return -1;\n> +\n> +       return 0;\n> +}\n> +\n>  static int merge(int argc, const char **argv, const char *prefix)\n>  {\n>         struct strbuf remote_ref = STRBUF_INIT, msg = STRBUF_INIT;\n> @@ -745,7 +763,7 @@ static int merge(int argc, const char **argv, const char *prefix)\n>         struct notes_merge_options o;\n>         int do_merge = 0, do_commit = 0, do_abort = 0;\n>         int verbosity = 0, result;\n> -       const char *strategy = NULL;\n> +       const char *strategy = NULL, *configured_strategy = NULL;\n>         struct option options[] = {\n>                 OPT_GROUP(N_(\"General options\")),\n>                 OPT__VERBOSITY(&verbosity),\n> @@ -795,21 +813,15 @@ static int merge(int argc, const char **argv, const char *prefix)\n>         expand_notes_ref(&remote_ref);\n>         o.remote_ref = remote_ref.buf;\n>\n> -       if (strategy) {\n> -               if (!strcmp(strategy, \"manual\"))\n> -                       o.strategy = NOTES_MERGE_RESOLVE_MANUAL;\n> -               else if (!strcmp(strategy, \"ours\"))\n> -                       o.strategy = NOTES_MERGE_RESOLVE_OURS;\n> -               else if (!strcmp(strategy, \"theirs\"))\n> -                       o.strategy = NOTES_MERGE_RESOLVE_THEIRS;\n> -               else if (!strcmp(strategy, \"union\"))\n> -                       o.strategy = NOTES_MERGE_RESOLVE_UNION;\n> -               else if (!strcmp(strategy, \"cat_sort_uniq\"))\n> -                       o.strategy = NOTES_MERGE_RESOLVE_CAT_SORT_UNIQ;\n> -               else {\n> -                       error(\"Unknown -s/--strategy: %s\", strategy);\n> -                       usage_with_options(git_notes_merge_usage, options);\n> -               }\n> +       git_config_get_string_const(\"notes.merge\", &configured_strategy);\n> +\n> +       if (configured_strategy &&\n> +           parse_notes_strategy(configured_strategy, &o.strategy))\n> +               die(\"Unknown notes merge strategy: %s\", configured_strategy);\n> +\n> +       if (strategy && parse_notes_strategy(strategy, &o.strategy)) {\n> +               error(\"Unknown -s/--strategy: %s\", strategy);\n> +               usage_with_options(git_notes_merge_usage, options);\n>         }\n\nDo you need to look at the notes.merge config variable at all if\n-s/--strategy is given?\nI'd expect the above to rather look something like:\n\nif (strategy) {\n        if (parse_notes_strategy(strategy, &o.strategy)) {\n                error(\"Unknown -s/--strategy: %s\", strategy);\n                usage_with_options(git_notes_merge_usage, options);\n        }\n} else if (configured_strategy) {\n        if (parse_notes_strategy(configured_strategy, &o.strategy))\n                die(\"Unknown notes merge strategy: %s\", configured_strategy);\n} /* else o.strategy = NOTES_MERGE_RESOLVE_MANUAL; */\n\n\nOtherwise, this patch looks good to me.\n\n...Johan\n\n\n-- \nJohan Herland, <johan@herland.net>\nwww.herland.net\n"},{"id":"267886","messageId":"CAPig+cRCmuxqVSqFfF7v-aOKEohfPjAYFAaos5XOz4uWD_psdQ@mail.gmail.com","threadId":"40063","inReplyTo":"1439326641-9447-5-git-send-email-jacob.e.keller@intel.com","subject":"Re: [PATCH v4 4/4] notes: teach git-notes about notes.<ref>.merge option","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-08-12T00:11:12Z","receivedAt":"2015-08-12T00:11:12Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Aug 11, 2015 at 4:57 PM, Jacob Keller <jacob.e.keller@intel.com> wrote:\n> Add new option \"notes.<ref>.merge\" option which specifies the merge\n> strategy for merging into a given notes ref. This option enables\n> selection of merge strategy for particular notes refs, rather than all\n> notes ref merges, as user may not want cat_sort_uniq for all refs, but\n> only some. Note that the <ref> is the local reference we are merging\n> into, not the remote ref we merged from. The assumption is that users\n> will mostly want to configure separate local ref merge strategies rather\n> than strategies depending on which remote ref they merge from. Also,\n> notes.<ref>.merge overrides the general behavior as it is more specific.\n>\n> Signed-off-by: Jacob Keller <jacob.keller@gmail.com>\n> ---\n> diff --git a/Documentation/config.txt b/Documentation/config.txt\n> index 488c2e8eec1b..2c283ebc309e 100644\n> --- a/Documentation/config.txt\n> +++ b/Documentation/config.txt\n> @@ -1912,6 +1912,12 @@ notes.merge::\n>         STRATEGIES\" section of linkgit:git-notes[1] for more information\n>         on each strategy.\n>\n> +notes.<localref>.merge::\n> +       Which merge strategy to choose if the local ref for a notes merge\n> +       matches <localref>, overriding \"notes.merge\". <localref> just be a\n\ns/just/must/\n\n> +       fully qualified refname. See \"NOTES MERGE STRATEGIES\" section in\n> +       linkgit:git-notes[1] for more information on the available strategies.\n> +\n"},{"id":"267887","messageId":"CALKQrgeDuRkXm2LzDOuZDZLOBRXjLmmRvhtXfXScWfLKX+9t=g@mail.gmail.com","threadId":"40063","inReplyTo":"1439326641-9447-5-git-send-email-jacob.e.keller@intel.com","subject":"Re: [PATCH v4 4/4] notes: teach git-notes about notes.<ref>.merge option","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2015-08-12T00:34:32Z","receivedAt":"2015-08-12T00:34:32Z","isPatch":true,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"On Tue, Aug 11, 2015 at 10:57 PM, Jacob Keller <jacob.e.keller@intel.com> wrote:\n> From: Jacob Keller <jacob.keller@gmail.com>\n>\n> Add new option \"notes.<ref>.merge\" option which specifies the merge\n> strategy for merging into a given notes ref. This option enables\n> selection of merge strategy for particular notes refs, rather than all\n> notes ref merges, as user may not want cat_sort_uniq for all refs, but\n> only some. Note that the <ref> is the local reference we are merging\n> into, not the remote ref we merged from. The assumption is that users\n> will mostly want to configure separate local ref merge strategies rather\n> than strategies depending on which remote ref they merge from. Also,\n> notes.<ref>.merge overrides the general behavior as it is more specific.\n\nThanks for working on this, and I apologize for not properly reviewing\nthis part of the series before now. More comments below.\n\n>\n> Signed-off-by: Jacob Keller <jacob.keller@gmail.com>\n> ---\n>  Documentation/config.txt            |  6 ++++++\n>  Documentation/git-notes.txt         |  6 ++++++\n>  builtin/notes.c                     |  7 +++++--\n>  t/t3309-notes-merge-auto-resolve.sh | 39 +++++++++++++++++++++++++++++++++++++\n>  4 files changed, 56 insertions(+), 2 deletions(-)\n>\n> diff --git a/Documentation/config.txt b/Documentation/config.txt\n> index 488c2e8eec1b..2c283ebc309e 100644\n> --- a/Documentation/config.txt\n> +++ b/Documentation/config.txt\n> @@ -1912,6 +1912,12 @@ notes.merge::\n>         STRATEGIES\" section of linkgit:git-notes[1] for more information\n>         on each strategy.\n>\n> +notes.<localref>.merge::\n> +       Which merge strategy to choose if the local ref for a notes merge\n> +       matches <localref>, overriding \"notes.merge\". <localref> just be a\n\ns/just/must/\n\n> +       fully qualified refname. See \"NOTES MERGE STRATEGIES\" section in\n> +       linkgit:git-notes[1] for more information on the available strategies.\n\nI sort of get the reason for \"fully qualified refname\", but I think\n\n  notes.refs/notes/commits.merge\n\nlooks much uglier than\n\n  notes.commits.merge\n\nEspecially since we have the opposite precedence for branch.<name>.*,\ne.g. we already have\n\n  branch.master.merge\n\nand not\n\n  branch.refs/heads/master.merge\n\nI know that we don't yet have a \"proper\" place to put remote notes refs,\nbut the <ref> in notes.<ref>.merge _must_ be a \"local\" notes ref (you even\nuse the <localref> notation in the documentation below). Thus, I believe\nwe can presume that the local notes ref must live under refs/notes/*,\nand hence drop the \"refs/notes/\" prefix from the config key (just like\nbranch.<name>.* can presume that <name> lives under refs/heads/*).\n\n...\n\nWhich brings me to another small gripe about the naming here:\nbranch.<name>.merge names the remote ref (at branch.<name>.remote)\nthat we will pull from, but notes.<ref>.merge has a very different\nmeaning.\n\nIf we - in the future - were to provide a similar config mechanism for a\nhypothetical \"git notes pull\" command, then it would be natural to model\nits config similarly: notes.<name>.remote and notes.<name>.merge\nspecifies whence we fetch, and what we (notes-)merge, respectively.\n\nExcept that this patch in its current form will occupy the .merge config\nkey...\n\nCan you rename to notes.<name>.mergestrategy instead? Or even better,\ntake inspiration from branch.<name>.mergeoptions, and provide\nnotes.<name>.mergeoptions instead, which you can then set like:\n\n  git config notes.foo.mergeoptions \"--strategy=cat_sort_uniq\"\n\nEven though 'git notes merge' don't yet accept any other options that\nshould be configurable, I think it's worth emulating the mechanisms\nthat alread exist for branches. It gives is some amount of future-\nproofing as well.\n\n\n...Johan\n\n\n-- \nJohan Herland, <johan@herland.net>\nwww.herland.net\n"},{"id":"267888","messageId":"CALKQrgciJWg_mhOjUtvWHTQtNM1YfWNQM7xH+FgkGOWBrmL+9A@mail.gmail.com","threadId":"40063","inReplyTo":"1439326641-9447-2-git-send-email-jacob.e.keller@intel.com","subject":"Re: [PATCH v4 1/4] notes: document cat_sort_uniq rewriteMode","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2015-08-12T00:35:59Z","receivedAt":"2015-08-12T00:35:59Z","isPatch":true,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"On Tue, Aug 11, 2015 at 10:57 PM, Jacob Keller <jacob.e.keller@intel.com> wrote:\n> From: Jacob Keller <jacob.keller@gmail.com>\n>\n> Teach documentation about the cat_sort_uniq rewriteMode that got added\n> at the same time as the equivalent merge strategy.\n>\n> Signed-off-by: Jacob Keller <jacob.keller@gmail.com>\n\nReviewed-by: Johan Herland <johan@herland.net>\n\n-- \nJohan Herland, <johan@herland.net>\nwww.herland.net\n"},{"id":"267889","messageId":"CALKQrgfC7Bko9NOtbCrNwq2UfS6tStsO0scN150NixEfFbmc8A@mail.gmail.com","threadId":"40063","inReplyTo":"1439326641-9447-3-git-send-email-jacob.e.keller@intel.com","subject":"Re: [PATCH v4 2/4] notes: add tests for --commit/--abort/--strategy exclusivity","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2015-08-12T00:36:26Z","receivedAt":"2015-08-12T00:36:26Z","isPatch":true,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"On Tue, Aug 11, 2015 at 10:57 PM, Jacob Keller <jacob.e.keller@intel.com> wrote:\n> From: Jacob Keller <jacob.keller@gmail.com>\n>\n> Add new tests to ensure that --commit, --abort, and --strategy are\n> mutually exclusive.\n>\n> Signed-off-by: Jacob Keller <jacob.keller@gmail.com>\n\nReviewed-by: Johan Herland <johan@herland.net>\n\n-- \nJohan Herland, <johan@herland.net>\nwww.herland.net\n"},{"id":"267890","messageId":"xmqqy4hhmedb.fsf@gitster.dls.corp.google.com","threadId":"40063","inReplyTo":"CALKQrgeDuRkXm2LzDOuZDZLOBRXjLmmRvhtXfXScWfLKX+9t=g@mail.gmail.com","subject":"Re: [PATCH v4 4/4] notes: teach git-notes about notes.<ref>.merge option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-08-12T02:26:08Z","receivedAt":"2015-08-12T02:26:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johan Herland <johan@herland.net> writes:\n\n> I know that we don't yet have a \"proper\" place to put remote notes refs,\n> but the <ref> in notes.<ref>.merge _must_ be a \"local\" notes ref (you even\n> use the <localref> notation in the documentation below). Thus, I believe\n> we can presume that the local notes ref must live under refs/notes/*,\n> and hence drop the \"refs/notes/\" prefix from the config key (just like\n> branch.<name>.* can presume that <name> lives under refs/heads/*).\n\nI am OK going in that direction, as long as we promise that \"notes\nmerge\" will forever refuse to work on --notes=$ref where $ref does\nnot begin with refs/notes/.\n\n> Except that this patch in its current form will occupy the .merge config\n> key...\n>\n> Can you rename to notes.<name>.mergestrategy instead?\n\nThis is an excellent suggestion.\n\n> Or even better, take inspiration from branch.<name>.mergeoptions,\n\nPlease don't.\n\nThat is one of the design mistakes that was copied from another\ndesign mistake (remotes.*.tagopt).  I'd want to see us not to repeat\nthese design mistakes.\n\nThese configuration variables were made to take free-form text value\nthat is split according to shell rules, primarily because it was\nexpedient to implement.  Read its value into a $variable and put it\nat the end of the command line to let the shell split it.  \"tagopt\"\nwas done a bit more carefully in that it made to react only with a\nfixed string \"--no-tags\", so it was hard to abuse, but \"mergeoptions\"\nallowed you to override something that you wouldn't want to (e.g. it\neven allowed you to feed '--message=foo').\n\nOnce you start from such a broken design, it would be hard to later\nmake it saner, even if you wanted to.  You have to retroactively\nforbid something that \"worked\" (with some definition of \"working\"),\nor you have to split, parse and then reject something that does not\nmake sense yourself, reimplementing dequote/split rule used in the\nshell---which is especially problematic when you no longer write in\nshell scripts.\n\nSo a single string value that names one of the supported strategy\nstored in notes.<name>.mergestrategy is an excellent choice.  An\narbitrary string in notes.<name>.mergeoptions is to be avoided.\n\nThanks.\n"},{"id":"267933","messageId":"CA+P7+xrSAo43=-3q-7SPxerYPKU3+T75dG5dTE=H8odicRDxSg@mail.gmail.com","threadId":"40063","inReplyTo":"xmqqy4hhmedb.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v4 4/4] notes: teach git-notes about notes.<ref>.merge option","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2015-08-12T19:05:50Z","receivedAt":"2015-08-12T19:05:50Z","isPatch":true,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Tue, Aug 11, 2015 at 7:26 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Johan Herland <johan@herland.net> writes:\n>\n>> I know that we don't yet have a \"proper\" place to put remote notes refs,\n>> but the <ref> in notes.<ref>.merge _must_ be a \"local\" notes ref (you even\n>> use the <localref> notation in the documentation below). Thus, I believe\n>> we can presume that the local notes ref must live under refs/notes/*,\n>> and hence drop the \"refs/notes/\" prefix from the config key (just like\n>> branch.<name>.* can presume that <name> lives under refs/heads/*).\n>\n> I am OK going in that direction, as long as we promise that \"notes\n> merge\" will forever refuse to work on --notes=$ref where $ref does\n> not begin with refs/notes/.\n>\n\nIt appears that notes merge already allows operating on refs not in \"refs/notes\"\n\nthe way you select what you are merging into is via either --ref or\nthe environment variable.I chose to use the full refs/notes/* format\nas it was the only one that also supported all layouts.\n\nSo, should we enforce that default_notes_ref always requires refs\ninside refs/notes? that doesn't seem a bad idea here.\n\nIf we only disallow it for merge I think that would be very confusing.\n\nRegards,\nJake\n"},{"id":"267934","messageId":"xmqqa8twl3x9.fsf@gitster.dls.corp.google.com","threadId":"40063","inReplyTo":"CA+P7+xrSAo43=-3q-7SPxerYPKU3+T75dG5dTE=H8odicRDxSg@mail.gmail.com","subject":"Re: [PATCH v4 4/4] notes: teach git-notes about notes.<ref>.merge option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-08-12T19:09:22Z","receivedAt":"2015-08-12T19:09:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jacob Keller <jacob.keller@gmail.com> writes:\n\n> On Tue, Aug 11, 2015 at 7:26 PM, Junio C Hamano <gitster@pobox.com> wrote:\n>> Johan Herland <johan@herland.net> writes:\n>>\n>>> I know that we don't yet have a \"proper\" place to put remote notes refs,\n>>> but the <ref> in notes.<ref>.merge _must_ be a \"local\" notes ref (you even\n>>> use the <localref> notation in the documentation below). Thus, I believe\n>>> we can presume that the local notes ref must live under refs/notes/*,\n>>> and hence drop the \"refs/notes/\" prefix from the config key (just like\n>>> branch.<name>.* can presume that <name> lives under refs/heads/*).\n>>\n>> I am OK going in that direction, as long as we promise that \"notes\n>> merge\" will forever refuse to work on --notes=$ref where $ref does\n>> not begin with refs/notes/.\n>\n> It appears that notes merge already allows operating on refs not in \"refs/notes\"\n\nIf that is the case, then the only sane choice left to us is to\naccept only fully qualified refname, e.g. refs/notes/commit, in\nnotes.<ref>.mergestrategy, without shortening, I would think.\n"},{"id":"267938","messageId":"CA+P7+xournf+1Cqnr-AAO+EzUee3KfEevFQLhXkqyfQGBvPRpQ@mail.gmail.com","threadId":"40063","inReplyTo":"xmqqa8twl3x9.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v4 4/4] notes: teach git-notes about notes.<ref>.merge option","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2015-08-12T19:16:43Z","receivedAt":"2015-08-12T19:16:43Z","isPatch":true,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Wed, Aug 12, 2015 at 12:09 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Jacob Keller <jacob.keller@gmail.com> writes:\n>\n>> On Tue, Aug 11, 2015 at 7:26 PM, Junio C Hamano <gitster@pobox.com> wrote:\n>>> Johan Herland <johan@herland.net> writes:\n>>>\n>>>> I know that we don't yet have a \"proper\" place to put remote notes refs,\n>>>> but the <ref> in notes.<ref>.merge _must_ be a \"local\" notes ref (you even\n>>>> use the <localref> notation in the documentation below). Thus, I believe\n>>>> we can presume that the local notes ref must live under refs/notes/*,\n>>>> and hence drop the \"refs/notes/\" prefix from the config key (just like\n>>>> branch.<name>.* can presume that <name> lives under refs/heads/*).\n>>>\n>>> I am OK going in that direction, as long as we promise that \"notes\n>>> merge\" will forever refuse to work on --notes=$ref where $ref does\n>>> not begin with refs/notes/.\n>>\n>> It appears that notes merge already allows operating on refs not in \"refs/notes\"\n>\n> If that is the case, then the only sane choice left to us is to\n> accept only fully qualified refname, e.g. refs/notes/commit, in\n> notes.<ref>.mergestrategy, without shortening, I would think.\n\nOh interesting. I did a test. If you provide a fully qualified ref not\ninside refs/notes, then it assumes you meant refs/notes/refs/foo/y\nrather than refs/foo/y\n\nI need to do some more digging on this to determine the exact thing going on...\n\nRegards,\nJake\n"},{"id":"267948","messageId":"CA+P7+xpsjX=4j=7w7DJ+EnO-uBfq53_Bmwk8AZ4frAy=iTQ7ZQ@mail.gmail.com","threadId":"40063","inReplyTo":"CA+P7+xournf+1Cqnr-AAO+EzUee3KfEevFQLhXkqyfQGBvPRpQ@mail.gmail.com","subject":"Re: [PATCH v4 4/4] notes: teach git-notes about notes.<ref>.merge option","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2015-08-12T21:43:17Z","receivedAt":"2015-08-12T21:43:17Z","isPatch":true,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Wed, Aug 12, 2015 at 12:16 PM, Jacob Keller <jacob.keller@gmail.com> wrote:\n> Oh interesting. I did a test. If you provide a fully qualified ref not\n> inside refs/notes, then it assumes you meant refs/notes/refs/foo/y\n> rather than refs/foo/y\n>\n> I need to do some more digging on this to determine the exact thing going on...\n>\n> Regards,\n> Jake\n\nI did some more digging. If you pass a notes ref to \"--refs\" option,\nthat requires all notes to be bound to refs/notes/* and does not allow\npassing of arbitrary refs. However, you can set the environment\nvariable GIT_NOTES_REF or core.notesRef to a fully qualified\nreference.\n\nThat seems very arbitrary that --ref works by expanding notes and the\nenvironment variable and configuration option do not... [1]\n\nI think this inconsistency is very weird, because *most* people will\nnot use refs/notes/* etc. This makes it so that --refs forces you to\nuse refs/notes/* or it will prefix it for you... ie: you can use\nnotes/x, refs/notes/x, x, but if you use refs/tags/x it will DWIM into\nrefs/notes/refs/tags/x\n\nI think this is very confusing that --refs doesn't behave the same as\nother sections... either we should enforce this on all refs or we\nshould fix the DWIM-ery to be consistent.\n\nthat is, we should fix DWIM-ery to be:\n\n(1) if it starts with refs/* leave it alone\n\n(2) if it starts with notes/*, prefix it with refs/\n\n(3) otherwise prefix it with refs/notes/\n\nBut that way, refs/some-other-notes/ will work fine instead of\nbecoming something else.\n\nWe should also fix reads of environment variable etc such taht we\nenforce these values always are fully qualified and begin with refs.\nOtherwise, use of --refs and the environment variable don't allow the\nsame formats.\n\nRegards,\nJake\n\n[1] 8ef313e1ec3b (\"builtin/notes.c: Split notes ref DWIMmery into a\nseparate function\")\n"},{"id":"267949","messageId":"CALKQrgf2hdvNExVbvnP5sVUM4sEh7thj9HLw93LbYWSStNjeYg@mail.gmail.com","threadId":"40063","inReplyTo":"xmqqy4hhmedb.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v4 4/4] notes: teach git-notes about notes.<ref>.merge option","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2015-08-12T21:46:43Z","receivedAt":"2015-08-12T21:46:43Z","isPatch":true,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"On Wed, Aug 12, 2015 at 4:26 AM, Junio C Hamano <gitster@pobox.com> wrote:\n> Johan Herland <johan@herland.net> writes:\n>> I know that we don't yet have a \"proper\" place to put remote notes refs,\n>> but the <ref> in notes.<ref>.merge _must_ be a \"local\" notes ref (you even\n>> use the <localref> notation in the documentation below). Thus, I believe\n>> we can presume that the local notes ref must live under refs/notes/*,\n>> and hence drop the \"refs/notes/\" prefix from the config key (just like\n>> branch.<name>.* can presume that <name> lives under refs/heads/*).\n>\n> I am OK going in that direction, as long as we promise that \"notes\n> merge\" will forever refuse to work on --notes=$ref where $ref does\n> not begin with refs/notes/.\n\nIf we don't already refuse to merge into a ref outside refs/notes, then\nI would consider that a bug to be fixed, and not some corner use case that\nwe must preserve for all future.\n\nAfter all, we do already have a test in t3308 named 'fail to merge into\nvarious non-notes refs', where one of the non-notes ref being tested are:\n\n  test_must_fail git -c \"core.notesRef=refs/heads/master\" notes merge x\n\n>> Except that this patch in its current form will occupy the .merge config\n>> key...\n>>\n>> Can you rename to notes.<name>.mergestrategy instead?\n>\n> This is an excellent suggestion.\n>\n>> Or even better, take inspiration from branch.<name>.mergeoptions,\n>\n> Please don't.\n>\n> That is one of the design mistakes that was copied from another\n> design mistake (remotes.*.tagopt).  I'd want to see us not to repeat\n> these design mistakes.\n>\n> These configuration variables were made to take free-form text value\n> that is split according to shell rules, primarily because it was\n> expedient to implement.  Read its value into a $variable and put it\n> at the end of the command line to let the shell split it.  \"tagopt\"\n> was done a bit more carefully in that it made to react only with a\n> fixed string \"--no-tags\", so it was hard to abuse, but \"mergeoptions\"\n> allowed you to override something that you wouldn't want to (e.g. it\n> even allowed you to feed '--message=foo').\n>\n> Once you start from such a broken design, it would be hard to later\n> make it saner, even if you wanted to.  You have to retroactively\n> forbid something that \"worked\" (with some definition of \"working\"),\n> or you have to split, parse and then reject something that does not\n> make sense yourself, reimplementing dequote/split rule used in the\n> shell---which is especially problematic when you no longer write in\n> shell scripts.\n>\n> So a single string value that names one of the supported strategy\n> stored in notes.<name>.mergestrategy is an excellent choice.  An\n> arbitrary string in notes.<name>.mergeoptions is to be avoided.\n\nUnderstood. And agreed.\n\n...Johan\n\n-- \nJohan Herland, <johan@herland.net>\nwww.herland.net\n"},{"id":"267951","messageId":"CA+P7+xrQnrQdE3OOhdc2-2__V3Huzc+HfGEXKBZULy2JkQR37Q@mail.gmail.com","threadId":"40063","inReplyTo":"CALKQrgf2hdvNExVbvnP5sVUM4sEh7thj9HLw93LbYWSStNjeYg@mail.gmail.com","subject":"Re: [PATCH v4 4/4] notes: teach git-notes about notes.<ref>.merge option","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2015-08-12T21:57:01Z","receivedAt":"2015-08-12T21:57:01Z","isPatch":true,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Wed, Aug 12, 2015 at 2:46 PM, Johan Herland <johan@herland.net> wrote:\n> If we don't already refuse to merge into a ref outside refs/notes, then\n> I would consider that a bug to be fixed, and not some corner use case that\n> we must preserve for all future.\n>\n> After all, we do already have a test in t3308 named 'fail to merge into\n> various non-notes refs', where one of the non-notes ref being tested are:\n>\n>   test_must_fail git -c \"core.notesRef=refs/heads/master\" notes merge x\n>\n\nThis test is checking if the ref pointed at by refs/heads/master *is*\na note. But you could create a ref outside of refs/notes which is a\nnote but which isn't inside refs/notes\n\nI did just find that we expand remote-ref using expand_notes_ref, and\nit does *not* currently let us reference refs outside of refs/notes..\nso we can merge IN to a ref not inside refs/notes (using the\nenvironment variable) but we can't merge FROM\nrefs/tracking/origin/notes/y for example, which means currently all\nnotes we merge from have to be located into refs/notes/*\n\nThere are some weird issues here.\n\nRegards,\nJake\n"},{"id":"267957","messageId":"CA+P7+xp0zhqM4CHmFpKuPyNMTx3DBxiLSOsiwz=byrfTEDj3sA@mail.gmail.com","threadId":"40063","inReplyTo":"CA+P7+xrQnrQdE3OOhdc2-2__V3Huzc+HfGEXKBZULy2JkQR37Q@mail.gmail.com","subject":"Re: [PATCH v4 4/4] notes: teach git-notes about notes.<ref>.merge option","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2015-08-12T22:03:51Z","receivedAt":"2015-08-12T22:03:51Z","isPatch":true,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Wed, Aug 12, 2015 at 2:57 PM, Jacob Keller <jacob.keller@gmail.com> wrote:\n> On Wed, Aug 12, 2015 at 2:46 PM, Johan Herland <johan@herland.net> wrote:\n>> If we don't already refuse to merge into a ref outside refs/notes, then\n>> I would consider that a bug to be fixed, and not some corner use case that\n>> we must preserve for all future.\n>>\n>> After all, we do already have a test in t3308 named 'fail to merge into\n>> various non-notes refs', where one of the non-notes ref being tested are:\n>>\n>>   test_must_fail git -c \"core.notesRef=refs/heads/master\" notes merge x\n>>\n>\n> This test is checking if the ref pointed at by refs/heads/master *is*\n> a note. But you could create a ref outside of refs/notes which is a\n> note but which isn't inside refs/notes\n>\n> I did just find that we expand remote-ref using expand_notes_ref, and\n> it does *not* currently let us reference refs outside of refs/notes..\n> so we can merge IN to a ref not inside refs/notes (using the\n> environment variable) but we can't merge FROM\n> refs/tracking/origin/notes/y for example, which means currently all\n> notes we merge from have to be located into refs/notes/*\n>\n> There are some weird issues here.\n>\n> Regards,\n> Jake\n\n\nI spoke to soon. We have an \"init_notes_check\" function which shows\nthat it does refuse to merge outside of refs/notes/* It prevents all\nnotes operations outside of refs/notes\n\nSince this is the case, I would prefer to modify the DWIM to be as I\nsuggested, and use this DWIM for the notes.\n\nWe will need to modify the DWIM so that it doesn't change refs/* even\nif this will fail later, as we use expand_notes_ref for the remote_ref\nof a merge, and we probably want to allow notes refs to be located\nsomewhere outside of notes such as refs/tracking/<origin>/notes or\nsomething in the future.\n\nSo we can make our config option take only unqualified values.\n\nThoughts?\n\nRegards,\nJake\n"},{"id":"267958","messageId":"CALKQrgfyxRG0dMBKUcsfV5ybu16nVhWPwa9rCBjT=ba93p+aEQ@mail.gmail.com","threadId":"40063","inReplyTo":"CA+P7+xpsjX=4j=7w7DJ+EnO-uBfq53_Bmwk8AZ4frAy=iTQ7ZQ@mail.gmail.com","subject":"Re: [PATCH v4 4/4] notes: teach git-notes about notes.<ref>.merge option","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2015-08-12T22:04:36Z","receivedAt":"2015-08-12T22:04:36Z","isPatch":true,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"On Wed, Aug 12, 2015 at 11:43 PM, Jacob Keller <jacob.keller@gmail.com> wrote:\n> On Wed, Aug 12, 2015 at 12:16 PM, Jacob Keller <jacob.keller@gmail.com> wrote:\n>> Oh interesting. I did a test. If you provide a fully qualified ref not\n>> inside refs/notes, then it assumes you meant refs/notes/refs/foo/y\n>> rather than refs/foo/y\n>>\n>> I need to do some more digging on this to determine the exact thing going on...\n>>\n>> Regards,\n>> Jake\n>\n> I did some more digging. If you pass a notes ref to \"--refs\" option,\n\nYou're referring to the --ref option to 'git notes'?\n\n> that requires all notes to be bound to refs/notes/* and does not allow\n> passing of arbitrary refs. However, you can set the environment\n> variable GIT_NOTES_REF or core.notesRef to a fully qualified\n> reference.\n>\n> That seems very arbitrary that --ref works by expanding notes and the\n> environment variable and configuration option do not... [1]\n\nI believe the intention here was to provide the DWIM-ing at the most\nend-user-facing interface (leaving the two other interfaces as possible\n\"loopholes\" for e.g. scripts that \"known what they're doing\" and don't\nwant the DWIM-ery to get in their way). Looking back at it now, that\napproach was probably misguided.\n\n> I think this inconsistency is very weird, because *most* people will\n> not use refs/notes/* etc. This makes it so that --refs forces you to\n> use refs/notes/* or it will prefix it for you... ie: you can use\n> notes/x, refs/notes/x, x, but if you use refs/tags/x it will DWIM into\n> refs/notes/refs/tags/x\n>\n> I think this is very confusing that --refs doesn't behave the same as\n> other sections... either we should enforce this on all refs or we\n> should fix the DWIM-ery to be consistent.\n>\n> that is, we should fix DWIM-ery to be:\n>\n> (1) if it starts with refs/* leave it alone\n>\n> (2) if it starts with notes/*, prefix it with refs/\n>\n> (3) otherwise prefix it with refs/notes/\n>\n> But that way, refs/some-other-notes/ will work fine instead of\n> becoming something else.\n\nYes, that is probably a better way to do the DWIM-ery.\n\nHowever, we then need to provide an additional layer of safety/checks\nthat prevent notes operations from manipulating non-notes refs. First:\n\n - As mentioned elsewhere in the thread, git notes merge should certainly\n   refuse to merge into anything not under refs/notes/*.\n\n - Preferably, all notes _manipulation_ should be limited to only operating\n   under refs/notes/* (although I haven't fully thought through all of the\n   ramifications of that).\n\n - Notes _querying_ (as opposed to manipulation) should be allowed both\n   within and outside refs/notes/*\n\nI think this should cover the use cases where you fetch notes from a remote\nand put them in e.g. refs/remote-notes/* (or refs/remotes/origin/notes/*).\nAfter all, you should not manipulate those notes directly (just as you\ndon't manipulate your remote-tracking branches directly), but you should\ndefinitely be able to query them, and merge them into a _local_ notes ref\n(living under refs/notes/*).\n\nDoes that make sense?\n\n> We should also fix reads of environment variable etc such taht we\n> enforce these values always are fully qualified and begin with refs.\n> Otherwise, use of --refs and the environment variable don't allow the\n> same formats.\n\nAgreed.\n\n\n...Johan\n\n> Regards,\n> Jake\n>\n> [1] 8ef313e1ec3b (\"builtin/notes.c: Split notes ref DWIMmery into a\n> separate function\")\n> --\n> To unsubscribe from this list: send the line \"unsubscribe git\" in\n> the body of a message to majordomo@vger.kernel.org\n> More majordomo info at  http://vger.kernel.org/majordomo-info.html\n\n\n\n-- \nJohan Herland, <johan@herland.net>\nwww.herland.net\n"},{"id":"267959","messageId":"xmqqmvxwjfjb.fsf@gitster.dls.corp.google.com","threadId":"40063","inReplyTo":"CA+P7+xp0zhqM4CHmFpKuPyNMTx3DBxiLSOsiwz=byrfTEDj3sA@mail.gmail.com","subject":"Re: [PATCH v4 4/4] notes: teach git-notes about notes.<ref>.merge option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-08-12T22:41:28Z","receivedAt":"2015-08-12T22:41:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jacob Keller <jacob.keller@gmail.com> writes:\n\n> I spoke to soon. We have an \"init_notes_check\" function which shows\n> that it does refuse to merge outside of refs/notes/* It prevents all\n> notes operations outside of refs/notes\n\nOK.  Then it is OK to limit notes.<ref>.mergestrategy so that <ref>\nrefers to what comes after refs/notes/, because we will not allow\nmerging to happen outside the hierarchy.\n\nIf you are planning to break that promise, however, <ref> must be\nalways spelled fully (i.e. with refs/notes/ prefix for those inside\nthe hierarchy) to avoid ambiguity.  Otherwise it will be hard to\ninterpret a configuration that does something like this (note that\nthese could come from multiple places, e.g. $HOME/.gitconfig and\n$GIT_DIR/config):\n\n    [notes \"commits\"]\n        mergestrategy = concatenate\n    [notes \"notes/commits\"]\n        mergestrategy = cat_sort_uniq\n    [notes \"refs/notes/commits\"]\n        mergestrategy = overwrite\n\nThe three entries in the above example obviously are all meant to\nrefer to the same refs/notes/commits notes tree, and the usual \"last\none wins\" rule should apply.  But with the recent git_config_get_*()\ninterface, you cannot tell which one among them was given the last,\noverriding the previous entries.\n"},{"id":"267962","messageId":"CA+P7+xqa-DFHhoCvi4qRhPNGDpAmb9T3k28GOwuW6gzc4VfPbQ@mail.gmail.com","threadId":"40063","inReplyTo":"xmqqmvxwjfjb.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v4 4/4] notes: teach git-notes about notes.<ref>.merge option","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2015-08-12T22:51:50Z","receivedAt":"2015-08-12T22:51:50Z","isPatch":true,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Wed, Aug 12, 2015 at 3:41 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Jacob Keller <jacob.keller@gmail.com> writes:\n>\n>> I spoke to soon. We have an \"init_notes_check\" function which shows\n>> that it does refuse to merge outside of refs/notes/* It prevents all\n>> notes operations outside of refs/notes\n>\n> OK.  Then it is OK to limit notes.<ref>.mergestrategy so that <ref>\n> refers to what comes after refs/notes/, because we will not allow\n> merging to happen outside the hierarchy.\n>\n> If you are planning to break that promise, however, <ref> must be\n> always spelled fully (i.e. with refs/notes/ prefix for those inside\n> the hierarchy) to avoid ambiguity.  Otherwise it will be hard to\n> interpret a configuration that does something like this (note that\n> these could come from multiple places, e.g. $HOME/.gitconfig and\n> $GIT_DIR/config):\n>\n\nAgreed. Today, we do not allow any notes operations at all that\nfunction outside of refs/notes/*\n\nI suggest we enforce that all configs for merge strategy must be the\nunqualified notation, and not allow the variance of refs/notes/* and\nsuch that DWIM does on the command line.\n\n>     [notes \"commits\"]\n>         mergestrategy = concatenate\n>     [notes \"notes/commits\"]\n>         mergestrategy = cat_sort_uniq\n>     [notes \"refs/notes/commits\"]\n>         mergestrategy = overwrite\n>\n> The three entries in the above example obviously are all meant to\n> refer to the same refs/notes/commits notes tree, and the usual \"last\n> one wins\" rule should apply.  But with the recent git_config_get_*()\n> interface, you cannot tell which one among them was given the last,\n> overriding the previous entries.\n\nYa, I'd like to avoid this if possible.\n\nRegards,\nJake\n"}]}