{"thread":{"id":"40092","subject":"[PATCH v7 1/4] notes: document cat_sort_uniq rewriteMode","startedAt":"2015-08-14T21:13:51Z","lastAt":"2015-08-17T17:33:35Z","messageCount":16,"participants":["Jacob Keller","Junio C Hamano","Eric Sunshine","Johan Herland"],"isPatch":true,"patchVersion":7,"patchTotal":4},"messages":[{"id":"268094","messageId":"1439586835-15712-1-git-send-email-jacob.e.keller@intel.com","threadId":"40092","inReplyTo":null,"subject":"[PATCH v7 0/4] notes.mergestrategy option(s)","fromName":"Jacob Keller","fromEmail":"jacob.e.keller@intel.com","sentAt":"2015-08-14T21:13:51Z","receivedAt":"2015-08-14T21:13:51Z","isPatch":true,"sender":{"key":"jacob.e.keller@intel.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"From: Jacob Keller \n\nChanges since v6\n* Eric suggested a more stream-lined approach for\n git_config_get_notes_strategy\n\nJacob Keller (4):\n notes: document cat_sort_uniq rewriteMode\n notes: add tests for --commit/--abort/--strategy exclusivity\n notes: add notes.mergestrategy option to select default strategy\n notes: teach git-notes about notes..mergestrategy option\n\n Documentation/config.txt | 18 ++++++++--\n Documentation/git-notes.txt | 23 ++++++++++--\n builtin/notes.c | 56 ++++++++++++++++++++++-------\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, 169 insertions(+), 24 deletions(-)\n\n-- \n2.5.0.280.g4aaba03\n"},{"id":"268091","messageId":"1439586835-15712-2-git-send-email-jacob.e.keller@intel.com","threadId":"40092","inReplyTo":"1439586835-15712-1-git-send-email-jacob.e.keller@intel.com","subject":"[PATCH v7 1/4] notes: document cat_sort_uniq rewriteMode","fromName":"Jacob Keller","fromEmail":"jacob.e.keller@intel.com","sentAt":"2015-08-14T21:13:52Z","receivedAt":"2015-08-14T21:13:52Z","isPatch":true,"sender":{"key":"jacob.e.keller@intel.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"From: Jacob Keller \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 \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 75ec02e8e90a..de67ad1fdedf 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -1947,8 +1947,8 @@ notes.rewriteMode::\n \tWhen copying notes during a rewrite (see the\n \t\"notes.rewrite.\" 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.280.g4aaba03\n"},{"id":"268095","messageId":"1439586835-15712-3-git-send-email-jacob.e.keller@intel.com","threadId":"40092","inReplyTo":"1439586835-15712-1-git-send-email-jacob.e.keller@intel.com","subject":"[PATCH v7 2/4] notes: add tests for --commit/--abort/--strategy exclusivity","fromName":"Jacob Keller","fromEmail":"jacob.e.keller@intel.com","sentAt":"2015-08-14T21:13:53Z","receivedAt":"2015-08-14T21:13:53Z","isPatch":true,"sender":{"key":"jacob.e.keller@intel.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"From: Jacob Keller \n\nAdd new tests to ensure that --commit, --abort, and --strategy are\nmutually exclusive.\n\nSigned-off-by: Jacob Keller \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 <\n\nTeach git-notes about \"notes.mergestrategy\" to select a general strategy\nfor all notes merges. This enables a user to always get expected merge\nstrategy such as \"cat_sort_uniq\" without having to pass the \"-s\" option\nmanually.\n\nSigned-off-by: Jacob Keller \n---\n Documentation/config.txt | 7 ++++++\n Documentation/git-notes.txt | 14 +++++++++++-\n builtin/notes.c | 45 ++++++++++++++++++++++++++++---------\n notes-merge.h | 16 +++++++------\n t/t3309-notes-merge-auto-resolve.sh | 29 ++++++++++++++++++++++++\n 5 files changed, 92 insertions(+), 19 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex de67ad1fdedf..5e3e03459de7 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -1919,6 +1919,13 @@ mergetool.writeToTemp::\n mergetool.prompt::\n \tPrompt before each invocation of the merge resolution program.\n \n+notes.mergestrategy::\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..89c8829a0543 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.mergestrategy\" 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.mergestrategy 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.mergestrategy::\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 042348082709..12a42b583f98 100644\n--- a/builtin/notes.c\n+++ b/builtin/notes.c\n@@ -738,6 +738,37 @@ 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 git_config_get_notes_strategy(const char *key,\n+\t\t\t\t\t enum notes_merge_strategy *strategy)\n+{\n+\tconst char *value;\n+\n+\tif (git_config_get_string_const(key, &value))\n+\t\treturn 1;\n+\tif (parse_notes_strategy(value, strategy))\n+\t\tgit_die_config(key, \"unknown notes merge strategy %s\", value);\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@@ -797,20 +828,12 @@ static int merge(int argc, const char **argv, const char *prefix)\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\tif (parse_notes_strategy(strategy, &o.strategy)) {\n \t\t\terror(\"Unknown -s/--strategy: %s\", strategy);\n \t\t\tusage_with_options(git_notes_merge_usage, options);\n \t\t}\n+\t} else {\n+\t\tgit_config_get_notes_strategy(\"notes.mergestrategy\", &o.strategy);\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..476b9f5306f1 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.mergestrategy=\"foo\" notes merge z &&\n+\t# Verify no changes (y)\n+\tverify_notes y y\n+'\n+\n cat <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.mergestrategy=\"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 <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.mergestrategy=\"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 <expect_notes_union\n 7c4e546efd0fe939f876beb262ece02797880b54 $commit_sha15\n 5de7ea7ad4f47e7ff91989fb82234634730f75df $commit_sha14\n-- \n2.5.0.280.g4aaba03\n"},{"id":"268092","messageId":"1439586835-15712-5-git-send-email-jacob.e.keller@intel.com","threadId":"40092","inReplyTo":"1439586835-15712-1-git-send-email-jacob.e.keller@intel.com","subject":"[PATCH v7 4/4] notes: teach git-notes about notes..mergestrategy option","fromName":"Jacob Keller","fromEmail":"jacob.e.keller@intel.com","sentAt":"2015-08-14T21:13:55Z","receivedAt":"2015-08-14T21:13:55Z","isPatch":true,"sender":{"key":"jacob.e.keller@intel.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"From: Jacob Keller \n\nAdd new option \"notes..mergestrategy\" 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 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..merge overrides the general behavior as it is more specific.\n\nSigned-off-by: Jacob Keller \n---\n Documentation/config.txt | 7 +++++++\n Documentation/git-notes.txt | 6 ++++++\n builtin/notes.c | 13 ++++++++++---\n t/t3309-notes-merge-auto-resolve.sh | 39 +++++++++++++++++++++++++++++++++++++\n 4 files changed, 62 insertions(+), 3 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 5e3e03459de7..47478311367e 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -1926,6 +1926,13 @@ notes.mergestrategy::\n \tSTRATEGIES\" section of linkgit:git-notes[1] for more information\n \ton each strategy.\n \n+notes..mergestrategy::\n+\tWhich merge strategy to choose if the local ref for a notes merge\n+\tmatches , overriding \"notes.mergestrategy\". must\n+\tbe the short name of a ref under refs/notes/. See \"NOTES MERGE\n+\tSTRATEGIES\" section in linkgit:git-notes[1] for more information on the\n+\tavailable 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 89c8829a0543..b99809fc81a6 100644\n--- a/Documentation/git-notes.txt\n+++ b/Documentation/git-notes.txt\n@@ -322,6 +322,12 @@ notes.mergestrategy::\n +\n This setting can be overridden by passing the `--strategy` option.\n \n+notes..mergestrategy::\n+\tWhich strategy to choose when merging into . The set of\n+\tallowed values is the same as \"notes.mergestrategy\". must be\n+\tthe short name of a ref under refs/notes/. See \"NOTES MERGE STRATEGIES\"\n+\tsection above for more information 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 12a42b583f98..bdfd9c7d29b4 100644\n--- a/builtin/notes.c\n+++ b/builtin/notes.c\n@@ -771,13 +771,13 @@ static int git_config_get_notes_strategy(const char *key,\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 \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, *short_ref = NULL;\n \tstruct option options[] = {\n \t\tOPT_GROUP(N_(\"General options\")),\n \t\tOPT__VERBOSITY(&verbosity),\n@@ -833,7 +833,14 @@ static int merge(int argc, const char **argv, const char *prefix)\n \t\t\tusage_with_options(git_notes_merge_usage, options);\n \t\t}\n \t} else {\n-\t\tgit_config_get_notes_strategy(\"notes.mergestrategy\", &o.strategy);\n+\t\tif (!skip_prefix(o.local_ref, \"refs/notes/\", &short_ref))\n+\t\t\tdie(\"Refusing to merge notes into %s (outside of refs/notes/)\",\n+\t\t\t o.local_ref);\n+\n+\t\tstrbuf_addf(&merge_key, \"notes.%s.mergestrategy\", short_ref);\n+\n+\t\tif (git_config_get_notes_strategy(merge_key.buf, &o.strategy))\n+\t\t\tgit_config_get_notes_strategy(\"notes.mergestrategy\", &o.strategy);\n \t}\n \n \tt = init_notes_check(\"merge\");\ndiff --git a/t/t3309-notes-merge-auto-resolve.sh b/t/t3309-notes-merge-auto-resolve.sh\nindex 476b9f5306f1..560e75259798 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.y.mergestrategy=\"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 <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.y.mergestrategy=\"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.y.mergestrategy=\"union\" -c notes.mergestrategy=\"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.z.mergestrategy=\"union\" notes merge z &&\n+\tgit notes merge --abort &&\n+\tverify_notes y y\n+'\n+\n cat <expect_notes_union2\n d682107b8bf7a7aea1e537a8d5cb6a12b60135f1 $commit_sha15\n 5de7ea7ad4f47e7ff91989fb82234634730f75df $commit_sha14\n-- \n2.5.0.280.g4aaba03\n"},{"id":"268098","messageId":"xmqq8u9dh6lq.fsf@gitster.dls.corp.google.com","threadId":"40092","inReplyTo":"1439586835-15712-5-git-send-email-jacob.e.keller@intel.com","subject":"Re: [PATCH v7 4/4] notes: teach git-notes about notes..mergestrategy option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-08-14T22:01:53Z","receivedAt":"2015-08-14T22:01:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jacob Keller writes:\n\n> diff --git a/builtin/notes.c b/builtin/notes.c\n> index 12a42b583f98..bdfd9c7d29b4 100644\n> --- a/builtin/notes.c\n> +++ b/builtin/notes.c\n> ...\n> @@ -833,7 +833,14 @@ static int merge(int argc, const char **argv, const char *prefix)\n> \t\t\tusage_with_options(git_notes_merge_usage, options);\n> \t\t}\n> \t} else {\n> -\t\tgit_config_get_notes_strategy(\"notes.mergestrategy\", &o.strategy);\n> +\t\tif (!skip_prefix(o.local_ref, \"refs/notes/\", &short_ref))\n> +\t\t\tdie(\"Refusing to merge notes into %s (outside of refs/notes/)\",\n> +\t\t\t o.local_ref);\n> +\n\nSorry, but I lost track. \n\nDo I understand correctly the consensus on the previous discussion?\nMy understanding is:\n\n (1) We do not currently refuse to merge notes into anywhere outside\n of refs/notes/;\n\n (2) But that is not a designed behaviour---we simply forgot to\n check it---we should start checking and refusing.\n\nIf that is the concensus, having this check somewhere in the merge()\nfunction is indeed necessary, but this looks very out of place.\nThink what happens if the user passes \"--stratagy manual\" from the\ncommand line. This check is not even performed, is it?\n\nI'd prefer to see:\n\n * \"Let's start making sure that we do not allow touching outside\n refs/notes/\" as a separate patch, perhaps as a preparatory step.\n\n * Have the check apply consistently, regardless of where the\n strategy comes from.\n\n * That separate patch to add this restriction should test that\n the refusal triggers correctly when it should, and it does not\n trigger when it shouldn't.\n\n> +\t\tstrbuf_addf(&merge_key, \"notes.%s.mergestrategy\", short_ref);\n> +\n> +\t\tif (git_config_get_notes_strategy(merge_key.buf, &o.strategy))\n> +\t\t\tgit_config_get_notes_strategy(\"notes.mergestrategy\", &o.strategy);\n> \t}\n\nI think you are leaking merge_key after you are done using it.\n\nIt is tempting to suggest writing the above like so:\n\n\t\tgit_config_get_notes_strategy(merge_key.buf, &o.strategy)) ||\n git_config_get_notes_strategy(\"notes.mergestrategy\", &o.strategy);\n\nwhich might make it more obvious what is going on, but I do not care\ntoo deeply about it. To be honest, I am not sure which one is\neasier to read in the longer term myself ;-).\n\nThanks.\n"},{"id":"268100","messageId":"CAPig+cTxmFCRChzahQZVpMeJ=3N0PjHAcamFBm394OgTR8LnLw@mail.gmail.com","threadId":"40092","inReplyTo":"xmqq8u9dh6lq.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v7 4/4] notes: teach git-notes about notes..mergestrategy option","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-08-14T22:10:48Z","receivedAt":"2015-08-14T22:10:48Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Aug 14, 2015 at 6:01 PM, Junio C Hamano wrote:\n> Jacob Keller writes:\n>> diff --git a/builtin/notes.c b/builtin/notes.c\n>> index 12a42b583f98..bdfd9c7d29b4 100644\n>> --- a/builtin/notes.c\n>> +++ b/builtin/notes.c\n>> + strbuf_addf(&merge_key, \"notes.%s.mergestrategy\", short_ref);\n>> +\n>> + if (git_config_get_notes_strategy(merge_key.buf, &o.strategy))\n>> + git_config_get_notes_strategy(\"notes.mergestrategy\", &o.strategy);\n>> }\n>\n> I think you are leaking merge_key after you are done using it.\n\nIn addition to fixing the leak, since 'merge_key' is only used within\nthis block, it might also make sense to declare it in this block\nrather than at the top of the function.\n"},{"id":"268101","messageId":"xmqq4mk1h66i.fsf@gitster.dls.corp.google.com","threadId":"40092","inReplyTo":"1439586835-15712-2-git-send-email-jacob.e.keller@intel.com","subject":"Re: [PATCH v7 1/4] notes: document cat_sort_uniq rewriteMode","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-08-14T22:11:01Z","receivedAt":"2015-08-14T22:11:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jacob Keller writes:\n\n> diff --git a/Documentation/config.txt b/Documentation/config.txt\n> index 75ec02e8e90a..de67ad1fdedf 100644\n> --- a/Documentation/config.txt\n> +++ b/Documentation/config.txt\n> @@ -1947,8 +1947,8 @@ notes.rewriteMode::\n> \tWhen copying notes during a rewrite (see the\n> \t\"notes.rewrite.\" 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.\n> diff --git a/Documentation/git-notes.txt b/Documentation/git-notes.txt\n> index 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\nThis obviously is not a problem introduced by this patch, but I\nwonder why we have two similar but different set of modes for\nrewrtie and merge. Isn't 'overwrite' like 'ours', 'ignore' like\n'theirs', and 'concat' like 'union', and if these are similar\nenough, perhaps it would be helpful to the end user if we unified\nthe terms (or accepted both as synonyms for backward compatibility)?\n\nAlso I notice that you cannot manually reconcile while rewriting;\ndon't we want to have 'manual' there, too, I wonder?\n\n[jc: Cc'ed Thomas who invented rewrite back when merge was not even\nthere, and Johan who added merge]\n"},{"id":"268105","messageId":"CA+P7+xqHzE5b7_yEqnCDq_vVJGSnsHPV8qJrXqUAGW=h_5C0mQ@mail.gmail.com","threadId":"40092","inReplyTo":"xmqq8u9dh6lq.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v7 4/4] notes: teach git-notes about notes..mergestrategy option","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2015-08-14T22:48:54Z","receivedAt":"2015-08-14T22:48:54Z","isPatch":true,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Fri, Aug 14, 2015 at 3:01 PM, Junio C Hamano wrote:\n> Jacob Keller writes:\n>\n>> diff --git a/builtin/notes.c b/builtin/notes.c\n>> index 12a42b583f98..bdfd9c7d29b4 100644\n>> --- a/builtin/notes.c\n>> +++ b/builtin/notes.c\n>> ...\n>> @@ -833,7 +833,14 @@ static int merge(int argc, const char **argv, const char *prefix)\n>> usage_with_options(git_notes_merge_usage, options);\n>> }\n>> } else {\n>> - git_config_get_notes_strategy(\"notes.mergestrategy\", &o.strategy);\n>> + if (!skip_prefix(o.local_ref, \"refs/notes/\", &short_ref))\n>> + die(\"Refusing to merge notes into %s (outside of refs/notes/)\",\n>> + o.local_ref);\n>> +\n>\n> Sorry, but I lost track.\n>\n> Do I understand correctly the consensus on the previous discussion?\n> My understanding is:\n>\n> (1) We do not currently refuse to merge notes into anywhere outside\n> of refs/notes/;\n>\n\n\nWe do. I mis understood the original code. We check inside\n\"init_notes_check()\", which will check if the ref is under refs/notes/\n\n> (2) But that is not a designed behaviour---we simply forgot to\n> check it---we should start checking and refusing.\n>\n> If that is the concensus, having this check somewhere in the merge()\n> function is indeed necessary, but this looks very out of place.\n> Think what happens if the user passes \"--stratagy manual\" from the\n> command line. This check is not even performed, is it?\n>\n\nIt is checked (also) in init_notes_check(). I just happen to re-check\nhere because I didn't want to out-right ignore it in some weird flow\nwhere it was incorrect.\n\n> I'd prefer to see:\n>\n> * \"Let's start making sure that we do not allow touching outside\n> refs/notes/\" as a separate patch, perhaps as a preparatory step.\n>\n\nWe already don't allow it. See init_notes_check()\n\n> * Have the check apply consistently, regardless of where the\n> strategy comes from.\n\nIt already does. There is just a second check. I could completely\nremove that check if you like, but then we'd check config since we\ndon't run init_notes_check until after we find the merge strategy.\n\n>\n> * That separate patch to add this restriction should test that\n> the refusal triggers correctly when it should, and it does not\n> trigger when it shouldn't.\n>\n>> + strbuf_addf(&merge_key, \"notes.%s.mergestrategy\", short_ref);\n>> +\n>> + if (git_config_get_notes_strategy(merge_key.buf, &o.strategy))\n>> + git_config_get_notes_strategy(\"notes.mergestrategy\", &o.strategy);\n>> }\n>\n> I think you are leaking merge_key after you are done using it.\n>\n> It is tempting to suggest writing the above like so:\n>\n> git_config_get_notes_strategy(merge_key.buf, &o.strategy)) ||\n> git_config_get_notes_strategy(\"notes.mergestrategy\", &o.strategy);\n>\n> which might make it more obvious what is going on, but I do not care\n> too deeply about it. To be honest, I am not sure which one is\n> easier to read in the longer term myself ;-).\n>\n\n the || strategy results in a warning that we are checking and then\ndropping the outcome.\n\n> Thanks.\n\nRegards,\nJake\n"},{"id":"268106","messageId":"CA+P7+xofitJ2tTxqtRyWitcSKt4sKZCH5tygJxXScuW8wkW=SA@mail.gmail.com","threadId":"40092","inReplyTo":"CAPig+cTxmFCRChzahQZVpMeJ=3N0PjHAcamFBm394OgTR8LnLw@mail.gmail.com","subject":"Re: [PATCH v7 4/4] notes: teach git-notes about notes..mergestrategy option","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2015-08-14T22:50:36Z","receivedAt":"2015-08-14T22:50:36Z","isPatch":true,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Fri, Aug 14, 2015 at 3:10 PM, Eric Sunshine wrote:\n> On Fri, Aug 14, 2015 at 6:01 PM, Junio C Hamano wrote:\n>> Jacob Keller writes:\n>>> diff --git a/builtin/notes.c b/builtin/notes.c\n>>> index 12a42b583f98..bdfd9c7d29b4 100644\n>>> --- a/builtin/notes.c\n>>> +++ b/builtin/notes.c\n>>> + strbuf_addf(&merge_key, \"notes.%s.mergestrategy\", short_ref);\n>>> +\n>>> + if (git_config_get_notes_strategy(merge_key.buf, &o.strategy))\n>>> + git_config_get_notes_strategy(\"notes.mergestrategy\", &o.strategy);\n>>> }\n>>\n>> I think you are leaking merge_key after you are done using it.\n>\n> In addition to fixing the leak, since 'merge_key' is only used within\n> this block, it might also make sense to declare it in this block\n> rather than at the top of the function.\n\nI can do that.\n\nHow do you feel about having the duplicate check for the short_ref? We\n*already* check this inside init_notes_check() which is called right\nafter this.\n\nI think that we should keep it but can't find a consistent way to\navoid the duplication.\n\nIn addition, we already provide the tests for merging into and from\nnon-notes refs.\n\nRegards,\nJake\n"},{"id":"268107","messageId":"CA+P7+xoSB0um3FkhRAXGF1t0ZoYk0zaxAvAOvFcwn+CWQ-gyfg@mail.gmail.com","threadId":"40092","inReplyTo":"xmqq4mk1h66i.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v7 1/4] notes: document cat_sort_uniq rewriteMode","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2015-08-14T22:53:48Z","receivedAt":"2015-08-14T22:53:48Z","isPatch":true,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Fri, Aug 14, 2015 at 3:11 PM, Junio C Hamano wrote:\n> Jacob Keller writes:\n>\n>> diff --git a/Documentation/config.txt b/Documentation/config.txt\n>> index 75ec02e8e90a..de67ad1fdedf 100644\n>> --- a/Documentation/config.txt\n>> +++ b/Documentation/config.txt\n>> @@ -1947,8 +1947,8 @@ notes.rewriteMode::\n>> When copying notes during a rewrite (see the\n>> \"notes.rewrite.\" option), determines what to do if\n>> the target commit already has a note. Must be one of\n>> - `overwrite`, `concatenate`, or `ignore`. Defaults to\n>> - `concatenate`.\n>> + `overwrite`, `concatenate`, `cat_sort_uniq`, or `ignore`.\n>> + Defaults to `concatenate`.\n>> +\n>> This setting can be overridden with the `GIT_NOTES_REWRITE_MODE`\n>> environment variable.\n>> diff --git a/Documentation/git-notes.txt b/Documentation/git-notes.txt\n>> index 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>> When copying notes during a rewrite, what to do if the target\n>> commit already has a note. Must be one of `overwrite`,\n>> - `concatenate`, and `ignore`. Defaults to `concatenate`.\n>> + `concatenate`, `cat_sort_uniq`, or `ignore`. Defaults to\n>> + `concatenate`.\n>> +\n>> This setting can be overridden with the `GIT_NOTES_REWRITE_MODE`\n>> environment variable.\n>\n> This obviously is not a problem introduced by this patch, but I\n> wonder why we have two similar but different set of modes for\n> rewrtie and merge. Isn't 'overwrite' like 'ours', 'ignore' like\n> 'theirs', and 'concat' like 'union', and if these are similar\n> enough, perhaps it would be helpful to the end user if we unified\n> the terms (or accepted both as synonyms for backward compatibility)?\n>\n> Also I notice that you cannot manually reconcile while rewriting;\n> don't we want to have 'manual' there, too, I wonder?\n>\n> [jc: Cc'ed Thomas who invented rewrite back when merge was not even\n> there, and Johan who added merge]\n>\n\nI was not sure. I believe that re-write doesn't do the same thing as\nmerge? I think we could make all of them handle the \"overwrite\", which\nis basically a synonym of, I think \"theirs\" depending on the direction\nof the \"merge\".\n\nI don't know if re-write actually supports manual mode at all!\n\nMaybe we could make merge support the other names as synonyms, and\nthen code re-write in terms of merging?\n\nI wasn't sure so I chose only to document the mode that was missing.\n\nRegards,\nJake\n"},{"id":"268120","messageId":"CALKQrgcAmA021a6CS4eLwUwYh8C2BB7xXcfg9Hf7tTFwFw3KLg@mail.gmail.com","threadId":"40092","inReplyTo":"1439586835-15712-4-git-send-email-jacob.e.keller@intel.com","subject":"Re: [PATCH v7 3/4] notes: add notes.mergestrategy option to select default strategy","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2015-08-15T09:09:58Z","receivedAt":"2015-08-15T09:09:58Z","isPatch":true,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"On Fri, Aug 14, 2015 at 11:13 PM, Jacob Keller wrote:\n> From: Jacob Keller \n>\n> Teach git-notes about \"notes.mergestrategy\" to select a general strategy\n> for all notes merges. This enables a user to always get expected merge\n> strategy such as \"cat_sort_uniq\" without having to pass the \"-s\" option\n> manually.\n>\n> Signed-off-by: Jacob Keller \n> ---\n> Documentation/config.txt | 7 ++++++\n> Documentation/git-notes.txt | 14 +++++++++++-\n> builtin/notes.c | 45 ++++++++++++++++++++++++++++---------\n> notes-merge.h | 16 +++++++------\n> t/t3309-notes-merge-auto-resolve.sh | 29 ++++++++++++++++++++++++\n> 5 files changed, 92 insertions(+), 19 deletions(-)\n>\n> diff --git a/Documentation/config.txt b/Documentation/config.txt\n> index de67ad1fdedf..5e3e03459de7 100644\n> --- a/Documentation/config.txt\n> +++ b/Documentation/config.txt\n> @@ -1919,6 +1919,13 @@ mergetool.writeToTemp::\n> mergetool.prompt::\n> Prompt before each invocation of the merge resolution program.\n>\n> +notes.mergestrategy::\n\nJust one small nit: Config keys are (AFAIK) case-insensitive, and we\ncan thus use\ncamelCasing in the documentation to make long keywords more readable (see e.g.\nnotes.displayRef below). So I suggest using notes.mergeStrategy here.\n\n> + Which merge strategy to choose by default when resolving notes\n> + conflicts. Must be one of `manual`, `ours`, `theirs`, `union`,\n> + or `cat_sort_uniq`. Defaults to `manual`. See \"NOTES MERGE\n> + STRATEGIES\" section of linkgit:git-notes[1] for more information\n> + on each strategy.\n> +\n> notes.displayRef::\n> The (fully qualified) refname from which to show notes when\n> showing commit messages. The value of this variable can be set\n> diff --git a/Documentation/git-notes.txt b/Documentation/git-notes.txt\n> index 674682b34b83..89c8829a0543 100644\n> --- a/Documentation/git-notes.txt\n> +++ b/Documentation/git-notes.txt\n> @@ -101,7 +101,7 @@ merge::\n> any) 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> When merging notes, resolve notes conflicts using the given\n> strategy. The following strategies are recognized: \"manual\"\n> (default), \"ours\", \"theirs\", \"union\" and \"cat_sort_uniq\".\n> + This option overrides the \"notes.mergestrategy\" configuration setting.\n> See the \"NOTES MERGE STRATEGIES\" section below for more\n> information 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.mergestrategy 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> This setting can be overridden through the environment and\n> command line.\n>\n> +notes.mergestrategy::\n\nSame here.\n\n> + Which merge strategy to choose by default when resolving notes\n> + conflicts. Must be one of `manual`, `ours`, `theirs`, `union`,\n> + or `cat_sort_uniq`. Defaults to `manual`. See \"NOTES MERGE\n> + STRATEGIES\" 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> Which ref (or refs, if a glob or specified more than once), in\n> addition to the default set by `core.notesRef` or\n[...]\n\nOtherwise, looks good to me.\n\n\n...Johan\n\n\n-- \nJohan Herland, \nwww.herland.net\n"},{"id":"268123","messageId":"CALKQrgfA4qfBV7yb1QNNtBvA9BK1gcDMFwdGRLxiVwk4SLWfHA@mail.gmail.com","threadId":"40092","inReplyTo":"1439586835-15712-5-git-send-email-jacob.e.keller@intel.com","subject":"Re: [PATCH v7 4/4] notes: teach git-notes about notes..mergestrategy option","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2015-08-15T09:25:44Z","receivedAt":"2015-08-15T09:25:44Z","isPatch":true,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"On Fri, Aug 14, 2015 at 11:13 PM, Jacob Keller wrote:\n> From: Jacob Keller \n>\n> Add new option \"notes..mergestrategy\" 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 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..merge overrides the general behavior as it is more specific.\n>\n> Signed-off-by: Jacob Keller \n> ---\n> Documentation/config.txt | 7 +++++++\n> Documentation/git-notes.txt | 6 ++++++\n> builtin/notes.c | 13 ++++++++++---\n> t/t3309-notes-merge-auto-resolve.sh | 39 +++++++++++++++++++++++++++++++++++++\n> 4 files changed, 62 insertions(+), 3 deletions(-)\n>\n> diff --git a/Documentation/config.txt b/Documentation/config.txt\n> index 5e3e03459de7..47478311367e 100644\n> --- a/Documentation/config.txt\n> +++ b/Documentation/config.txt\n> @@ -1926,6 +1926,13 @@ notes.mergestrategy::\n> STRATEGIES\" section of linkgit:git-notes[1] for more information\n> on each strategy.\n>\n> +notes..mergestrategy::\n\nNit: mergeStrategy\n\n> + Which merge strategy to choose if the local ref for a notes merge\n> + matches , overriding \"notes.mergestrategy\". must\n> + be the short name of a ref under refs/notes/. See \"NOTES MERGE\n\nAn example would be useful here, methinks:\n\n must be the short name of a ref under refs/notes/, e.g. for\nconfiguring\nthe merge strategy for refs/notes/commits, notes.commits.mergeStrategy must\nbe set.\n\n\nOtherwise, the patch looks good to me.\n\n\n...Johan\n\n-- \nJohan Herland, \nwww.herland.net\n"},{"id":"268124","messageId":"CALKQrgd=qVBPMKu_bZxVHgqsdYGtJcOvnrQaB2aH9z67ymv=Uw@mail.gmail.com","threadId":"40092","inReplyTo":"CA+P7+xoSB0um3FkhRAXGF1t0ZoYk0zaxAvAOvFcwn+CWQ-gyfg@mail.gmail.com","subject":"Re: [PATCH v7 1/4] notes: document cat_sort_uniq rewriteMode","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2015-08-15T10:06:14Z","receivedAt":"2015-08-15T10:06:14Z","isPatch":true,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"On Sat, Aug 15, 2015 at 12:53 AM, Jacob Keller wrote:\n> On Fri, Aug 14, 2015 at 3:11 PM, Junio C Hamano wrote:\n>> Jacob Keller writes:\n>>\n>>> diff --git a/Documentation/config.txt b/Documentation/config.txt\n>>> index 75ec02e8e90a..de67ad1fdedf 100644\n>>> --- a/Documentation/config.txt\n>>> +++ b/Documentation/config.txt\n>>> @@ -1947,8 +1947,8 @@ notes.rewriteMode::\n>>> When copying notes during a rewrite (see the\n>>> \"notes.rewrite.\" option), determines what to do if\n>>> the target commit already has a note. Must be one of\n>>> - `overwrite`, `concatenate`, or `ignore`. Defaults to\n>>> - `concatenate`.\n>>> + `overwrite`, `concatenate`, `cat_sort_uniq`, or `ignore`.\n>>> + Defaults to `concatenate`.\n>>> +\n>>> This setting can be overridden with the `GIT_NOTES_REWRITE_MODE`\n>>> environment variable.\n>>> diff --git a/Documentation/git-notes.txt b/Documentation/git-notes.txt\n>>> index 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>>> When copying notes during a rewrite, what to do if the target\n>>> commit already has a note. Must be one of `overwrite`,\n>>> - `concatenate`, and `ignore`. Defaults to `concatenate`.\n>>> + `concatenate`, `cat_sort_uniq`, or `ignore`. Defaults to\n>>> + `concatenate`.\n>>> +\n>>> This setting can be overridden with the `GIT_NOTES_REWRITE_MODE`\n>>> environment variable.\n>>\n>> This obviously is not a problem introduced by this patch, but I\n>> wonder why we have two similar but different set of modes for\n>> rewrtie and merge.\n\nWe do. Rewrite builds directly on top of the combine_notes_* functions\nthat are part of the core/low-level notes code, added in 73f464b5\n(2010-02-13, Refactor notes concatenation into a flexible interface for\ncombining notes). Notes merge (and its various strategies) were added\nlater, and also build on top of these combine_notes_* functions (see\nmerge_one_change() in notes-merge.c). In addition, notes-merge adds the\n'manual' strategy, which depends on surrounding machinery that\nnotes-merge provides, but that the low-level combine_notes_* functions\ncannot depend on.\n\n\n>> Isn't 'overwrite' like 'ours', 'ignore' like\n>> 'theirs', and 'concat' like 'union', and if these are similar\n>> enough, perhaps it would be helpful to the end user if we unified\n>> the terms (or accepted both as synonyms for backward compatibility)?\n\nThe mapping (which is contained in merge_one_change() in notes-merge.c)\nis:\n\n 'manual' -> [custom handling in notes-merge]\n 'ours' -> combine_notes_ignore [or simply a no-op]\n 'theirs' -> combine_notes_overwrite\n 'union' -> combine_notes_concatenate\n 'cat_sort_uniq' -> combine_notes_cat_sort_uniq\n\n>>\n>> Also I notice that you cannot manually reconcile while rewriting;\n>> don't we want to have 'manual' there, too, I wonder?\n\nNo, as stated above, 'manual' requires some external mechanism for\ncreating a \"worktree\" where the notes conflicts can be checked out\nand manipulated by the user. The guts of notes.c is not the correct\nplace to do that. I don't know if it's easy to rewrite \"rewrite\" to\ndo a (partial) notes merge instead of using the combine_notes_*\nfunctions directly, but I imagine that would be the best way forward.\n\n>>\n>> [jc: Cc'ed Thomas who invented rewrite back when merge was not even\n>> there, and Johan who added merge]\n>>\n>\n> I was not sure. I believe that re-write doesn't do the same thing as\n> merge? I think we could make all of them handle the \"overwrite\", which\n> is basically a synonym of, I think \"theirs\" depending on the direction\n> of the \"merge\".\n\nCorrect.\n\n> I don't know if re-write actually supports manual mode at all!\n\nIt doesn't.\n\n> Maybe we could make merge support the other names as synonyms, and\n> then code re-write in terms of merging?\n\nAdding the synonyms is fine, but I wouldn't publicize it heavily, as\nthe meaning of words like \"overwrite\" and \"ignore\" can become quite\nconfusing in the context of a merge.\n\nReimplementing re-write in terms of merge is probably a good idea,\nthough.\n\n> I wasn't sure so I chose only to document the mode that was missing.\n\nIMHO, that's good for now.\n\n\n...Johan\n\n\n-- \nJohan Herland, \nwww.herland.net\n"},{"id":"268193","messageId":"xmqqpp2lesns.fsf@gitster.dls.corp.google.com","threadId":"40092","inReplyTo":"CA+P7+xqHzE5b7_yEqnCDq_vVJGSnsHPV8qJrXqUAGW=h_5C0mQ@mail.gmail.com","subject":"Re: [PATCH v7 4/4] notes: teach git-notes about notes..mergestrategy option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-08-17T17:22:47Z","receivedAt":"2015-08-17T17:22:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jacob Keller writes:\n\n>> Sorry, but I lost track.\n>>\n>> Do I understand correctly the consensus on the previous discussion?\n>> My understanding is:\n>>\n>> (1) We do not currently refuse to merge notes into anywhere outside\n>> of refs/notes/;\n>\n> We do. I mis understood the original code. We check inside\n> \"init_notes_check()\", which will check if the ref is under refs/notes/\n\nOK, then we are in a much better shape than I thought ;-). Thanks.\n"},{"id":"268194","messageId":"xmqqlhd9es5s.fsf@gitster.dls.corp.google.com","threadId":"40092","inReplyTo":"CA+P7+xofitJ2tTxqtRyWitcSKt4sKZCH5tygJxXScuW8wkW=SA@mail.gmail.com","subject":"Re: [PATCH v7 4/4] notes: teach git-notes about notes..mergestrategy option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-08-17T17:33:35Z","receivedAt":"2015-08-17T17:33:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jacob Keller writes:\n\n> How do you feel about having the duplicate check for the short_ref? We\n> *already* check this inside init_notes_check() which is called right\n> after this.\n\nI thought you were trying to enforce a new rule (i.e. must be under\n\"refs/notes/\") with this, but it isn't. It is \"we are going to\nstrip the prefix known to us, just make sure the caller did not feed\nus something bogus\" safety, and the placement of this new check in\nyour patch (i.e. only when strategy was not given so we need to\ncheck which short-ref we are dealing with) is the best place.\n\nThanks.\n"}]}