{"thread":{"id":"39988","subject":"[PATCH v2 0/2] notes: add notes.merge strategy option","startedAt":"2015-07-31T23:12:19Z","lastAt":"2015-08-02T08:20:32Z","messageCount":9,"participants":["Jacob Keller","Johan Herland","Eric Sunshine"],"isPatch":true,"patchVersion":2,"patchTotal":2},"messages":[{"id":"267240","messageId":"1438384341-2688-1-git-send-email-jacob.e.keller@intel.com","threadId":"39988","inReplyTo":null,"subject":"[PATCH v2 0/2] notes: add notes.merge strategy option","fromName":"Jacob Keller","fromEmail":"jacob.e.keller@intel.com","sentAt":"2015-07-31T23:12:19Z","receivedAt":"2015-07-31T23:12: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\nThis series adds a default merge strategy option for git-notes, so that\nthe user does not have to type \"-s\" every time. It is overridden by the\n-s option.\n\nI also added some tests to ensure that the \"--abort\" \"--commit\" and \"-s\"\noptions must be independent. In addition, I found a small documentation\nbug which is corrected in the first patch of the series.\n\nI Cc'd a couple more people in this version of the patch in order to\nhopefully get some more review. This is based on pu incase there were\nany other conflicts, but I can easily rebase if necessary.\n\nJacob Keller (2):\n  notes: document cat_sort_uniq rewriteMode\n  notes: add notes.merge option to select default strategy\n\n Documentation/config.txt              | 11 +++++--\n Documentation/git-notes.txt           | 33 +++++++++++++--------\n builtin/notes.c                       | 55 +++++++++++++++++++++++++----------\n notes-merge.h                         | 16 +++++-----\n t/t3309-notes-merge-auto-resolve.sh   | 29 ++++++++++++++++++\n t/t3310-notes-merge-manual-resolve.sh | 12 ++++++++\n 6 files changed, 119 insertions(+), 37 deletions(-)\n\n-- \n2.5.0.482.gfcd5645\n"},{"id":"267241","messageId":"1438384341-2688-2-git-send-email-jacob.e.keller@intel.com","threadId":"39988","inReplyTo":"1438384341-2688-1-git-send-email-jacob.e.keller@intel.com","subject":"[PATCH v2 1/2] notes: document cat_sort_uniq rewriteMode","fromName":"Jacob Keller","fromEmail":"jacob.e.keller@intel.com","sentAt":"2015-07-31T23:12:20Z","receivedAt":"2015-07-31T23:12: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 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>\nCc: Johan Herland <johan@herland.net>\nCc: Michael Haggerty <mhagger@alum.mit.edu>\nCc: Eric Sunshine <sunshine@sunshineco.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 6562c6ab09b9..3c1e4df09beb 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -1965,8 +1965,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":"267242","messageId":"1438384341-2688-3-git-send-email-jacob.e.keller@intel.com","threadId":"39988","inReplyTo":"1438384341-2688-1-git-send-email-jacob.e.keller@intel.com","subject":"[PATCH v2 2/2] notes: add notes.merge option to select default strategy","fromName":"Jacob Keller","fromEmail":"jacob.e.keller@intel.com","sentAt":"2015-07-31T23:12:21Z","receivedAt":"2015-07-31T23:12: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\nTeach git-notes about a new configuration option \"notes.merge\" for\nselecting the default notes merge strategy. Document the option in\nconfig.txt and git-notes.txt\n\nAdd tests for the configuration option. Ensure that command line\n--strategy option overrides the configured value. Ensure that -s can't\nbe passed with --commit or --abort. Ensure that --abort and --commit\ncan't be used together.\n\nSigned-off-by: Jacob Keller <jacob.keller@gmail.com>\nCc: Johan Herland <johan@herland.net>\nCc: Michael Haggerty <mhagger@alum.mit.edu>\nCc: Eric Sunshine <sunshine@sunshineco.com>\n---\n Documentation/config.txt              |  7 +++++\n Documentation/git-notes.txt           | 30 ++++++++++++-------\n builtin/notes.c                       | 55 +++++++++++++++++++++++++----------\n notes-merge.h                         | 16 +++++-----\n t/t3309-notes-merge-auto-resolve.sh   | 29 ++++++++++++++++++\n t/t3310-notes-merge-manual-resolve.sh | 12 ++++++++\n 6 files changed, 115 insertions(+), 34 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 3c1e4df09beb..85c15126e4ce 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -1937,6 +1937,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..d8944f5aec60 100644\n--- a/Documentation/git-notes.txt\n+++ b/Documentation/git-notes.txt\n@@ -101,13 +101,13 @@ 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-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-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+conflicting notes (see the -s/--strategy option or notes.merge\n+config option) is not given, the \"manual\" resolver is used.\n+This resolver checks out the conflicting notes in a special\n+worktree (`.git/NOTES_MERGE_WORKTREE`), and instructs the user\n+to manually resolve the conflicts there. When done, the user\n+can either finalize the merge with 'git notes merge --commit',\n+or abort the merge with 'git notes merge --abort'.\n \n remove::\n \tRemove the notes for given objects (defaults to HEAD). When\n@@ -181,10 +181,10 @@ OPTIONS\n -s <strategy>::\n --strategy=<strategy>::\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-\tSee the \"NOTES MERGE STRATEGIES\" section below for more\n-\tinformation on each notes merge strategy.\n+\tstrategy. Overrides \"notes.merge\". The following strategies are\n+\trecognized: `manual`, `ours`, `theirs`, `union` and\n+\t`cat_sort_uniq`. See the `NOTES MERGE STRATEGIES` section below\n+\tfor more information on each notes merge strategy.\n \n --commit::\n \tFinalize an in-progress 'git notes merge'. Use this option\n@@ -310,6 +310,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 88fdafb32bc0..728980bc79bf 100644\n--- a/builtin/notes.c\n+++ b/builtin/notes.c\n@@ -93,6 +93,8 @@ static const char * const git_notes_get_ref_usage[] = {\n static const char note_template[] =\n \t\"\\nWrite/edit the notes for the following object:\\n\";\n \n+static enum notes_merge_strategy merge_strategy;\n+\n struct note_data {\n \tint given;\n \tint use_editor;\n@@ -741,6 +743,25 @@ 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+\t}\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@@ -799,23 +820,13 @@ 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+\tif (strategy && parse_notes_strategy(strategy, &merge_strategy)) {\n+\t\terror(\"Unknown -s/--strategy: %s\", strategy);\n+\t\tusage_with_options(git_notes_merge_usage, options);\n \t}\n \n+\to.strategy = merge_strategy;\n+\n \tt = init_notes_check(\"merge\", NOTES_INIT_WRITABLE);\n \n \tstrbuf_addf(&msg, \"notes: Merged notes from %s into %s\",\n@@ -950,6 +961,18 @@ static int get_ref(int argc, const char **argv, const char *prefix)\n \treturn 0;\n }\n \n+static int git_notes_config(const char *var, const char *value, void *cb)\n+{\n+\tif (!strcmp(var, \"notes.merge\")) {\n+\t\tif (!value)\n+\t\t\treturn config_error_nonbool(var);\n+\t\tif (parse_notes_strategy(value, &merge_strategy))\n+\t\t\treturn error(\"Unknown notes merge strategy: %s\", value);\n+\t}\n+\n+\treturn git_default_config(var, value, cb);\n+}\n+\n int cmd_notes(int argc, const char **argv, const char *prefix)\n {\n \tint result;\n@@ -960,7 +983,7 @@ int cmd_notes(int argc, const char **argv, const char *prefix)\n \t\tOPT_END()\n \t};\n \n-\tgit_config(git_default_config, NULL);\n+\tgit_config(git_notes_config, NULL);\n \targc = parse_options(argc, argv, prefix, options, git_notes_usage,\n \t\t\t     PARSE_OPT_STOP_AT_NON_OPTION);\n \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\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":"267268","messageId":"CALKQrgeoUOLeiJi6EVF8-hz9D74iNA4pxzC8WYiX2_mN=CH8XA@mail.gmail.com","threadId":"39988","inReplyTo":"1438384341-2688-1-git-send-email-jacob.e.keller@intel.com","subject":"Re: [PATCH v2 0/2] notes: add notes.merge strategy option","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2015-08-01T13:30:11Z","receivedAt":"2015-08-01T13:30:11Z","isPatch":true,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"On Sat, Aug 1, 2015 at 1:12 AM, Jacob Keller <jacob.e.keller@intel.com> wrote:\n> From: Jacob Keller <jacob.keller@gmail.com>\n>\n> This series adds a default merge strategy option for git-notes, so that\n> the user does not have to type \"-s\" every time. It is overridden by the\n> -s option.\n\nI like this addition. A natural extension (i.e. future work, you\nneedn't worry about it for now) would be to allow this configuration\nto be per notes ref. Different notes refs are used to hold different\nkinds of data, and where cat_sort_uniq may be a good strategy for one\nparticular notes ref, it may be a poor default for other notes refs.\nSo we should consider adding notes.<notesref>.merge options to allow\nmore fine-grained control of which notes merge strategies apply to\nwhich notes refs.\n\n> I also added some tests to ensure that the \"--abort\" \"--commit\" and \"-s\"\n> options must be independent.\n\nGood. These could easily be split out into a separate commit, though,\nas they are independent of the notes.merge addition.\n\n> In addition, I found a small documentation\n> bug which is corrected in the first patch of the series.\n>\n> I Cc'd a couple more people in this version of the patch in order to\n> hopefully get some more review.\n\nThanks, appreciated (although AFAICS the cover letter was not CCed to me).\n\n> This is based on pu incase there were\n> any other conflicts, but I can easily rebase if necessary.\n\nJunio has the final word here, but I believe the preferred workflow is\nto base your patch series on master or next, as those do not jump\naround quite as much as pu does.\n\n>\n> Jacob Keller (2):\n>   notes: document cat_sort_uniq rewriteMode\n>   notes: add notes.merge option to select default strategy\n\nBoth patches Acked-by: Johan Herland <johan@herland.net>\n\n\n...Johan\n\n>\n>  Documentation/config.txt              | 11 +++++--\n>  Documentation/git-notes.txt           | 33 +++++++++++++--------\n>  builtin/notes.c                       | 55 +++++++++++++++++++++++++----------\n>  notes-merge.h                         | 16 +++++-----\n>  t/t3309-notes-merge-auto-resolve.sh   | 29 ++++++++++++++++++\n>  t/t3310-notes-merge-manual-resolve.sh | 12 ++++++++\n>  6 files changed, 119 insertions(+), 37 deletions(-)\n>\n> --\n> 2.5.0.482.gfcd5645\n>\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":"267273","messageId":"CAPig+cSws1EWRTuV2D49RCyy6reeC7yJ5YiOSO4ee3S1rjdhBg@mail.gmail.com","threadId":"39988","inReplyTo":"1438384341-2688-3-git-send-email-jacob.e.keller@intel.com","subject":"Re: [PATCH v2 2/2] notes: add notes.merge option to select default strategy","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-08-02T02:46:48Z","receivedAt":"2015-08-02T02:46:48Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Jul 31, 2015 at 7:12 PM, Jacob Keller <jacob.e.keller@intel.com> wrote:\n> Teach git-notes about a new configuration option \"notes.merge\" for\n> selecting the default notes merge strategy. Document the option in\n> config.txt and git-notes.txt\n>\n> Add tests for the configuration option. Ensure that command line\n> --strategy option overrides the configured value. Ensure that -s can't\n> be passed with --commit or --abort. Ensure that --abort and --commit\n> can't be used together.\n>\n> Signed-off-by: Jacob Keller <jacob.keller@gmail.com>\n> Cc: Johan Herland <johan@herland.net>\n> Cc: Michael Haggerty <mhagger@alum.mit.edu>\n> Cc: Eric Sunshine <sunshine@sunshineco.com>\n\nThanks, this looks better than the previous round. A few comments below...\n\n> diff --git a/Documentation/git-notes.txt b/Documentation/git-notes.txt\n> index 674682b34b83..d8944f5aec60 100644\n> --- a/Documentation/git-notes.txt\n> +++ b/Documentation/git-notes.txt\n> @@ -101,13 +101,13 @@ 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> -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> -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> +conflicting notes (see the -s/--strategy option or notes.merge\n> +config option) is not given, the \"manual\" resolver is used.\n> +This resolver checks out the conflicting notes in a special\n> +worktree (`.git/NOTES_MERGE_WORKTREE`), and instructs the user\n> +to manually resolve the conflicts there. When done, the user\n> +can either finalize the merge with 'git notes merge --commit',\n> +or abort the merge with 'git notes merge --abort'.\n\nWhen you re-wrap the text like this, it's difficult to spot your one\nlittle change in all the diff noise. For the sake of review, it's\noften better to minimize the change, even if it leaves the text more\njagged than you'd like.\n\n>  remove::\n>         Remove the notes for given objects (defaults to HEAD). When\n> @@ -181,10 +181,10 @@ OPTIONS\n>  -s <strategy>::\n>  --strategy=<strategy>::\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> -       See the \"NOTES MERGE STRATEGIES\" section below for more\n> -       information on each notes merge strategy.\n> +       strategy. Overrides \"notes.merge\". The following strategies are\n> +       recognized: `manual`, `ours`, `theirs`, `union` and\n> +       `cat_sort_uniq`. See the `NOTES MERGE STRATEGIES` section below\n> +       for more information on each notes merge strategy.\n\nDitto.\n\n>  --commit::\n>         Finalize an in-progress 'git notes merge'. Use this option\n> diff --git a/builtin/notes.c b/builtin/notes.c\n> index 88fdafb32bc0..728980bc79bf 100644\n> --- a/builtin/notes.c\n> +++ b/builtin/notes.c\n> @@ -741,6 +743,25 @@ 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\nIn this codebase, it's customary to return 0 on success and -1 on error\n\n> +       }\n\nStyle: drop unnecessary braces\n\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> @@ -950,6 +961,18 @@ static int get_ref(int argc, const char **argv, const char *prefix)\n>         return 0;\n>  }\n>\n> +static int git_notes_config(const char *var, const char *value, void *cb)\n> +{\n> +       if (!strcmp(var, \"notes.merge\")) {\n> +               if (!value)\n> +                       return config_error_nonbool(var);\n> +               if (parse_notes_strategy(value, &merge_strategy))\n> +                       return error(\"Unknown notes merge strategy: %s\", value);\n\nFor the non-error case, don't you want to 'return 0' here to indicate\nthat you handled the config variable rather than letting it fall\nthrough to git_default_config() below?\n\n> +       }\n> +\n> +       return git_default_config(var, value, cb);\n> +}\n> +\n>  int cmd_notes(int argc, const char **argv, const char *prefix)\n>  {\n>         int result;\n"},{"id":"267274","messageId":"CA+P7+xpV4JwCpUa3RGgxuWdpguLmQ60k_ToqM6cRXO8TNz+o-g@mail.gmail.com","threadId":"39988","inReplyTo":"CAPig+cSws1EWRTuV2D49RCyy6reeC7yJ5YiOSO4ee3S1rjdhBg@mail.gmail.com","subject":"Re: [PATCH v2 2/2] notes: add notes.merge option to select default strategy","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2015-08-02T04:12:40Z","receivedAt":"2015-08-02T04:12:40Z","isPatch":true,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Sat, Aug 1, 2015 at 7:46 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:\n> On Fri, Jul 31, 2015 at 7:12 PM, Jacob Keller <jacob.e.keller@intel.com> wrote:\n>> Teach git-notes about a new configuration option \"notes.merge\" for\n>> selecting the default notes merge strategy. Document the option in\n>> config.txt and git-notes.txt\n>>\n>> Add tests for the configuration option. Ensure that command line\n>> --strategy option overrides the configured value. Ensure that -s can't\n>> be passed with --commit or --abort. Ensure that --abort and --commit\n>> can't be used together.\n>>\n>> Signed-off-by: Jacob Keller <jacob.keller@gmail.com>\n>> Cc: Johan Herland <johan@herland.net>\n>> Cc: Michael Haggerty <mhagger@alum.mit.edu>\n>> Cc: Eric Sunshine <sunshine@sunshineco.com>\n>\n> Thanks, this looks better than the previous round. A few comments below...\n>\n>> diff --git a/Documentation/git-notes.txt b/Documentation/git-notes.txt\n>> index 674682b34b83..d8944f5aec60 100644\n>> --- a/Documentation/git-notes.txt\n>> +++ b/Documentation/git-notes.txt\n>> @@ -101,13 +101,13 @@ 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>> -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>> -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>> +conflicting notes (see the -s/--strategy option or notes.merge\n>> +config option) is not given, the \"manual\" resolver is used.\n>> +This resolver checks out the conflicting notes in a special\n>> +worktree (`.git/NOTES_MERGE_WORKTREE`), and instructs the user\n>> +to manually resolve the conflicts there. When done, the user\n>> +can either finalize the merge with 'git notes merge --commit',\n>> +or abort the merge with 'git notes merge --abort'.\n>\n> When you re-wrap the text like this, it's difficult to spot your one\n> little change in all the diff noise. For the sake of review, it's\n> often better to minimize the change, even if it leaves the text more\n> jagged than you'd like.\n>\n\nEven if it leaves it incredibly jagged? That is one of the downsides\nwith diff of flowed text like this :(\n\n>>  remove::\n>>         Remove the notes for given objects (defaults to HEAD). When\n>> @@ -181,10 +181,10 @@ OPTIONS\n>>  -s <strategy>::\n>>  --strategy=<strategy>::\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>> -       See the \"NOTES MERGE STRATEGIES\" section below for more\n>> -       information on each notes merge strategy.\n>> +       strategy. Overrides \"notes.merge\". The following strategies are\n>> +       recognized: `manual`, `ours`, `theirs`, `union` and\n>> +       `cat_sort_uniq`. See the `NOTES MERGE STRATEGIES` section below\n>> +       for more information on each notes merge strategy.\n>\n> Ditto.\n>\n>>  --commit::\n>>         Finalize an in-progress 'git notes merge'. Use this option\n>> diff --git a/builtin/notes.c b/builtin/notes.c\n>> index 88fdafb32bc0..728980bc79bf 100644\n>> --- a/builtin/notes.c\n>> +++ b/builtin/notes.c\n>> @@ -741,6 +743,25 @@ 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> In this codebase, it's customary to return 0 on success and -1 on error\n>\n\nFair enough.\n\n>> +       }\n>\n> Style: drop unnecessary braces\n>\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>> @@ -950,6 +961,18 @@ static int get_ref(int argc, const char **argv, const char *prefix)\n>>         return 0;\n>>  }\n>>\n>> +static int git_notes_config(const char *var, const char *value, void *cb)\n>> +{\n>> +       if (!strcmp(var, \"notes.merge\")) {\n>> +               if (!value)\n>> +                       return config_error_nonbool(var);\n>> +               if (parse_notes_strategy(value, &merge_strategy))\n>> +                       return error(\"Unknown notes merge strategy: %s\", value);\n>\n> For the non-error case, don't you want to 'return 0' here to indicate\n> that you handled the config variable rather than letting it fall\n> through to git_default_config() below?\n>\n\nMakes sense, I can fix that.\n\nRegards,\nJake\n\n>> +       }\n>> +\n>> +       return git_default_config(var, value, cb);\n>> +}\n>> +\n>>  int cmd_notes(int argc, const char **argv, const char *prefix)\n>>  {\n>>         int result;\n"},{"id":"267275","messageId":"CA+P7+xpTu6eKZEBTbzR9mg4gV3zAvTOc-3PTJP6QamEO_sA1=A@mail.gmail.com","threadId":"39988","inReplyTo":"CAPig+cSws1EWRTuV2D49RCyy6reeC7yJ5YiOSO4ee3S1rjdhBg@mail.gmail.com","subject":"Re: [PATCH v2 2/2] notes: add notes.merge option to select default strategy","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2015-08-02T07:41:08Z","receivedAt":"2015-08-02T07:41:08Z","isPatch":true,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Sat, Aug 1, 2015 at 7:46 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:\n>>  If conflicts arise and a strategy for automatically resolving\n>> -conflicting notes (see the -s/--strategy option) 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>> -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>> +conflicting notes (see the -s/--strategy option or notes.merge\n>> +config option) is not given, the \"manual\" resolver is used.\n>> +This resolver checks out the conflicting notes in a special\n>> +worktree (`.git/NOTES_MERGE_WORKTREE`), and instructs the user\n>> +to manually resolve the conflicts there. When done, the user\n>> +can either finalize the merge with 'git notes merge --commit',\n>> +or abort the merge with 'git notes merge --abort'.\n>\n> When you re-wrap the text like this, it's difficult to spot your one\n> little change in all the diff noise. For the sake of review, it's\n> often better to minimize the change, even if it leaves the text more\n> jagged than you'd like.\n\n\nThis results in something incredibly jagged. I can't find a good way\nto split which minimizes the change. Would a 3rd patch which just\nre-flows this be an acceptable alternative\n\nie: add the words in one patch then re-flow afterwards in a second\npatch with no changes?\n\nThere is no good alternative as other re-flows I tried end up looking\nway too jagged, as compared to surrounding documentation.\n\nRegards,\nJake\n"},{"id":"267276","messageId":"20150802080123.GA17440@flurp.local","threadId":"39988","inReplyTo":"CA+P7+xpTu6eKZEBTbzR9mg4gV3zAvTOc-3PTJP6QamEO_sA1=A@mail.gmail.com","subject":"Re: [PATCH v2 2/2] notes: add notes.merge option to select default strategy","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-08-02T08:01:23Z","receivedAt":"2015-08-02T08:01:23Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, Aug 02, 2015 at 12:41:08AM -0700, Jacob Keller wrote:\n> On Sat, Aug 1, 2015 at 7:46 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:\n> >>  If conflicts arise and a strategy for automatically resolving\n> >> -conflicting notes (see the -s/--strategy option) 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> >> -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> >> +conflicting notes (see the -s/--strategy option or notes.merge\n> >> +config option) is not given, the \"manual\" resolver is used.\n> >> +This resolver checks out the conflicting notes in a special\n> >> +worktree (`.git/NOTES_MERGE_WORKTREE`), and instructs the user\n> >> +to manually resolve the conflicts there. When done, the user\n> >> +can either finalize the merge with 'git notes merge --commit',\n> >> +or abort the merge with 'git notes merge --abort'.\n> >\n> > When you re-wrap the text like this, it's difficult to spot your one\n> > little change in all the diff noise. For the sake of review, it's\n> > often better to minimize the change, even if it leaves the text more\n> > jagged than you'd like.\n> \n> This results in something incredibly jagged. I can't find a good way\n> to split which minimizes the change. Would a 3rd patch which just\n> re-flows this be an acceptable alternative\n> \n> ie: add the words in one patch then re-flow afterwards in a second\n> patch with no changes?\n> \n> There is no good alternative as other re-flows I tried end up looking\n> way too jagged, as compared to surrounding documentation.\n\nDon't worry too much about it. Consider it something to keep in mind for\nfuture patches. I reviewed the change and it seemed okay. I mentioned it\nbecause one of the goals of patch submission, in addition to making an\nactual change, is to ease the review process. If Junio is okay with\naccepting it as is, then it's probably not worth spending more time\ntrying to refine it.\n\nHaving said that, I came up with the following for those two paragraphs,\nwhich gives a much less noisy diff and doesn't look too jagged:\n\n--- 8< ---\n If conflicts arise and a strategy for automatically resolving\n-conflicting notes (see the -s/--strategy option) is not given,\n-the \"manual\" resolver is used. This resolver checks out the\n+conflicting notes (see -s/--strategy or notes.merge configuration) is\n+not given, 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 When done, the user can either finalize the merge with\n--- 8< ---\n\n...and...\n\n--- 8< ---\n \tWhen merging notes, resolve notes conflicts using the given\n-\tstrategy. The following strategies are recognized: \"manual\"\n+\tstrategy. Overrides the \"notes.merge\" configuration variable.\n+\tThe following strategies are recognized: \"manual\"\n \t(default), \"ours\", \"theirs\", \"union\" and \"cat_sort_uniq\".\n \tSee the \"NOTES MERGE STRATEGIES\" section below for more\n \tinformation on each notes merge strategy.\n--- 8< ---\n"},{"id":"267277","messageId":"CA+P7+xoLpedQMiOg3V9m5S-0OShPpAnDS6_qb62E2c8A-if1Nw@mail.gmail.com","threadId":"39988","inReplyTo":"20150802080123.GA17440@flurp.local","subject":"Re: [PATCH v2 2/2] notes: add notes.merge option to select default strategy","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2015-08-02T08:20:32Z","receivedAt":"2015-08-02T08:20:32Z","isPatch":true,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Sun, Aug 2, 2015 at 1:01 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:\n> Don't worry too much about it. Consider it something to keep in mind for\n> future patches. I reviewed the change and it seemed okay. I mentioned it\n> because one of the goals of patch submission, in addition to making an\n> actual change, is to ease the review process. If Junio is okay with\n> accepting it as is, then it's probably not worth spending more time\n> trying to refine it.\n>\n> Having said that, I came up with the following for those two paragraphs,\n> which gives a much less noisy diff and doesn't look too jagged:\n>\n\nYea, I actually have re-worked it as well to something more suitable.\nI'm re-sending anyways as I fixed some of the other style issues and\nthe one bug about not returning 0 on good read of the notes.merge\nvalue.\n\nRegards\nJake\n"}]}