[PATCH v3 0/4] notes: add support for notes.merge option

[PATCH v3 0/4] notes: add support for notes.merge option

[Jacob Keller]

From: Jacob Keller <jacob.keller@gmail.com>

This series incorporates the feedback from both Johan and Eric. In
addition, I included an RFC implementing suggestion from Johan regarding
per-ref merge strategies.

I split the tests for --merge/--commit/--strategy out into their own
patch to help review and keep the "one commit = one change" logic.

I am not certain the RFC is ready yet, but the first 3 patches (now
based on next) should be ready to go.

Jacob Keller (4):
  notes: document cat_sort_uniq rewriteMode
  notes: add tests for --commit/--abort/--strategy exclusivity
  notes: add notes.merge option to select default strategy
  notes: add per-ref configuration of merge strategy

 Documentation/config.txt              |  18 +++-
 Documentation/git-notes.txt           |  23 ++++-
 builtin/notes.c                       | 167 ++++++++++++++++++++++++++++++----
 notes-merge.h                         |  16 ++--
 t/t3309-notes-merge-auto-resolve.sh   |  79 ++++++++++++++++
 t/t3310-notes-merge-manual-resolve.sh |  12 +++
 6 files changed, 288 insertions(+), 27 deletions(-)

-- 
2.5.0.482.gfcd5645

[PATCH v3 1/4] notes: document cat_sort_uniq rewriteMode

[Jacob Keller]

From: Jacob Keller <jacob.keller@gmail.com>

Teach documentation about the cat_sort_uniq rewriteMode that got added
at the same time as the equivalent merge strategy.

Signed-off-by: Jacob Keller <jacob.keller@gmail.com>
Cc: Johan Herland <johan@herland.net>
Cc: Michael Haggerty <mhagger@alum.mit.edu>
Cc: Eric Sunshine <sunshine@sunshineco.com>
---
 Documentation/config.txt    | 4 ++--
 Documentation/git-notes.txt | 3 ++-
 2 files changed, 4 insertions(+), 3 deletions(-)

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

[PATCH v3 2/4] notes: add tests for --commit/--abort/--strategy exclusivity

[Jacob Keller]

From: Jacob Keller <jacob.keller@gmail.com>

Add new tests to ensure that --commit, --abort, and --strategy are
mutually exclusive.

Signed-off-by: Jacob Keller <jacob.keller@gmail.com>
---
 t/t3310-notes-merge-manual-resolve.sh | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/t/t3310-notes-merge-manual-resolve.sh b/t/t3310-notes-merge-manual-resolve.sh
index 195bb97f859d..d5572121da69 100755
--- a/t/t3310-notes-merge-manual-resolve.sh
+++ b/t/t3310-notes-merge-manual-resolve.sh
@@ -314,6 +314,18 @@ y and z notes on 1st commit
 
 EOF
 
+test_expect_success 'do not allow mixing --commit and --abort' '
+	test_must_fail git notes merge --commit --abort
+'
+
+test_expect_success 'do not allow mixing --commit and --strategy' '
+	test_must_fail git notes merge --commit --strategy theirs
+'
+
+test_expect_success 'do not allow mixing --abort and --strategy' '
+	test_must_fail git notes merge --abort --strategy theirs
+'
+
 test_expect_success 'finalize conflicting merge (z => m)' '
 	# Resolve conflicts and finalize merge
 	cat >.git/NOTES_MERGE_WORKTREE/$commit_sha1 <<EOF &&
-- 
2.5.0.482.gfcd5645

[PATCH v3 3/4] notes: add notes.merge option to select default strategy

[Jacob Keller]

From: Jacob Keller <jacob.keller@gmail.com>

Teach git-notes about a new configuration option "notes.merge" for
selecting the default notes merge strategy. Document the option in
config.txt and git-notes.txt

Add tests for use of the configuration option. Include a test to ensure
that --strategy correctly overrides the configured setting.

Signed-off-by: Jacob Keller <jacob.keller@gmail.com>
Cc: Johan Herland <johan@herland.net>
Cc: Michael Haggerty <mhagger@alum.mit.edu>
Cc: Eric Sunshine <sunshine@sunshineco.com>
---
 Documentation/config.txt            |  7 +++++
 Documentation/git-notes.txt         | 14 +++++++++-
 builtin/notes.c                     | 56 ++++++++++++++++++++++++++-----------
 notes-merge.h                       | 16 ++++++-----
 t/t3309-notes-merge-auto-resolve.sh | 29 +++++++++++++++++++
 5 files changed, 98 insertions(+), 24 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 4ce5c553e50f..0fa88a29dd65 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1911,6 +1911,13 @@ mergetool.writeToTemp::
 mergetool.prompt::
 	Prompt before each invocation of the merge resolution program.
 
+notes.merge::
+	Which merge strategy to choose by default when resolving notes
+	conflicts. Must be one of `manual`, `ours`, `theirs`, `union`,
+	or `cat_sort_uniq`. Defaults to `manual`. See "NOTES MERGE
+	STRATEGIES" section of linkgit:git-notes[1] for more information
+	on each strategy.
+
 notes.displayRef::
 	The (fully qualified) refname from which to show notes when
 	showing commit messages.  The value of this variable can be set
diff --git a/Documentation/git-notes.txt b/Documentation/git-notes.txt
index 674682b34b83..9c4f8536182f 100644
--- a/Documentation/git-notes.txt
+++ b/Documentation/git-notes.txt
@@ -101,7 +101,7 @@ merge::
 	any) into the current notes ref (called "local").
 +
 If conflicts arise and a strategy for automatically resolving
-conflicting notes (see the -s/--strategy option) is not given,
+conflicting notes (see the "NOTES MERGE STRATEGIES" section) is not given,
 the "manual" resolver is used. This resolver checks out the
 conflicting notes in a special worktree (`.git/NOTES_MERGE_WORKTREE`),
 and instructs the user to manually resolve the conflicts there.
@@ -183,6 +183,7 @@ OPTIONS
 	When merging notes, resolve notes conflicts using the given
 	strategy. The following strategies are recognized: "manual"
 	(default), "ours", "theirs", "union" and "cat_sort_uniq".
+	This option overrides the "notes.merge" configuration setting.
 	See the "NOTES MERGE STRATEGIES" section below for more
 	information on each notes merge strategy.
 
@@ -247,6 +248,9 @@ When done, the user can either finalize the merge with
 'git notes merge --commit', or abort the merge with
 'git notes merge --abort'.
 
+Users may select an automated merge strategy from among the following using
+either -s/--strategy option or configuring notes.merge accordingly:
+
 "ours" automatically resolves conflicting notes in favor of the local
 version (i.e. the current notes ref).
 
@@ -310,6 +314,14 @@ core.notesRef::
 	This setting can be overridden through the environment and
 	command line.
 
+notes.merge::
+	Which merge strategy to choose by default when resolving notes
+	conflicts. Must be one of `manual`, `ours`, `theirs`, `union`,
+	or `cat_sort_uniq`. Defaults to `manual`. See "NOTES MERGE
+	STRATEGIES" section above for more information on each strategy.
++
+This setting can be overridden by passing the `--strategy` option.
+
 notes.displayRef::
 	Which ref (or refs, if a glob or specified more than once), in
 	addition to the default set by `core.notesRef` or
diff --git a/builtin/notes.c b/builtin/notes.c
index 63f95fc55439..de0caa00df1b 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -92,6 +92,8 @@ static const char * const git_notes_get_ref_usage[] = {
 static const char note_template[] =
 	"\nWrite/edit the notes for the following object:\n";
 
+static enum notes_merge_strategy merge_strategy;
+
 struct note_data {
 	int given;
 	int use_editor;
@@ -737,6 +739,24 @@ static int merge_commit(struct notes_merge_options *o)
 	return ret;
 }
 
+static int parse_notes_strategy(const char *arg, enum notes_merge_strategy *strategy)
+{
+	if (!strcmp(arg, "manual"))
+		*strategy = NOTES_MERGE_RESOLVE_MANUAL;
+	else if (!strcmp(arg, "ours"))
+		*strategy = NOTES_MERGE_RESOLVE_OURS;
+	else if (!strcmp(arg, "theirs"))
+		*strategy = NOTES_MERGE_RESOLVE_THEIRS;
+	else if (!strcmp(arg, "union"))
+		*strategy = NOTES_MERGE_RESOLVE_UNION;
+	else if (!strcmp(arg, "cat_sort_uniq"))
+		*strategy = NOTES_MERGE_RESOLVE_CAT_SORT_UNIQ;
+	else
+		return -1;
+
+	return 0;
+}
+
 static int merge(int argc, const char **argv, const char *prefix)
 {
 	struct strbuf remote_ref = STRBUF_INIT, msg = STRBUF_INIT;
@@ -795,23 +815,13 @@ static int merge(int argc, const char **argv, const char *prefix)
 	expand_notes_ref(&remote_ref);
 	o.remote_ref = remote_ref.buf;
 
-	if (strategy) {
-		if (!strcmp(strategy, "manual"))
-			o.strategy = NOTES_MERGE_RESOLVE_MANUAL;
-		else if (!strcmp(strategy, "ours"))
-			o.strategy = NOTES_MERGE_RESOLVE_OURS;
-		else if (!strcmp(strategy, "theirs"))
-			o.strategy = NOTES_MERGE_RESOLVE_THEIRS;
-		else if (!strcmp(strategy, "union"))
-			o.strategy = NOTES_MERGE_RESOLVE_UNION;
-		else if (!strcmp(strategy, "cat_sort_uniq"))
-			o.strategy = NOTES_MERGE_RESOLVE_CAT_SORT_UNIQ;
-		else {
-			error("Unknown -s/--strategy: %s", strategy);
-			usage_with_options(git_notes_merge_usage, options);
-		}
+	if (strategy && parse_notes_strategy(strategy, &merge_strategy)) {
+		error("Unknown -s/--strategy: %s", strategy);
+		usage_with_options(git_notes_merge_usage, options);
 	}
 
+	o.strategy = merge_strategy;
+
 	t = init_notes_check("merge");
 
 	strbuf_addf(&msg, "notes: Merged notes from %s into %s",
@@ -945,6 +955,20 @@ static int get_ref(int argc, const char **argv, const char *prefix)
 	return 0;
 }
 
+static int git_notes_config(const char *var, const char *value, void *cb)
+{
+	if (!strcmp(var, "notes.merge")) {
+		if (!value)
+			return config_error_nonbool(var);
+		if (parse_notes_strategy(value, &merge_strategy))
+			return error("Unknown notes merge strategy: %s", value);
+		else
+			return 0;
+	}
+
+	return git_default_config(var, value, cb);
+}
+
 int cmd_notes(int argc, const char **argv, const char *prefix)
 {
 	int result;
@@ -955,7 +979,7 @@ int cmd_notes(int argc, const char **argv, const char *prefix)
 		OPT_END()
 	};
 
-	git_config(git_default_config, NULL);
+	git_config(git_notes_config, NULL);
 	argc = parse_options(argc, argv, prefix, options, git_notes_usage,
 			     PARSE_OPT_STOP_AT_NON_OPTION);
 
diff --git a/notes-merge.h b/notes-merge.h
index 1d01f6aacf54..bda8c0c8d348 100644
--- a/notes-merge.h
+++ b/notes-merge.h
@@ -8,18 +8,20 @@ enum notes_merge_verbosity {
 	NOTES_MERGE_VERBOSITY_MAX = 5
 };
 
+enum notes_merge_strategy {
+	NOTES_MERGE_RESOLVE_MANUAL = 0,
+	NOTES_MERGE_RESOLVE_OURS,
+	NOTES_MERGE_RESOLVE_THEIRS,
+	NOTES_MERGE_RESOLVE_UNION,
+	NOTES_MERGE_RESOLVE_CAT_SORT_UNIQ
+};
+
 struct notes_merge_options {
 	const char *local_ref;
 	const char *remote_ref;
 	struct strbuf commit_msg;
 	int verbosity;
-	enum {
-		NOTES_MERGE_RESOLVE_MANUAL = 0,
-		NOTES_MERGE_RESOLVE_OURS,
-		NOTES_MERGE_RESOLVE_THEIRS,
-		NOTES_MERGE_RESOLVE_UNION,
-		NOTES_MERGE_RESOLVE_CAT_SORT_UNIQ
-	} strategy;
+	enum notes_merge_strategy strategy;
 	unsigned has_worktree:1;
 };
 
diff --git a/t/t3309-notes-merge-auto-resolve.sh b/t/t3309-notes-merge-auto-resolve.sh
index 461fd84755d7..a773b01b73db 100755
--- a/t/t3309-notes-merge-auto-resolve.sh
+++ b/t/t3309-notes-merge-auto-resolve.sh
@@ -298,6 +298,13 @@ test_expect_success 'merge z into y with invalid strategy => Fail/No changes' '
 	verify_notes y y
 '
 
+test_expect_success 'merge z into y with invalid configuration option => Fail/No changes' '
+	git config core.notesRef refs/notes/y &&
+	test_must_fail git -c notes.merge="foo" notes merge z &&
+	# Verify no changes (y)
+	verify_notes y y
+'
+
 cat <<EOF | sort >expect_notes_ours
 68b8630d25516028bed862719855b3d6768d7833 $commit_sha15
 5de7ea7ad4f47e7ff91989fb82234634730f75df $commit_sha14
@@ -365,6 +372,17 @@ test_expect_success 'reset to pre-merge state (y)' '
 	verify_notes y y
 '
 
+test_expect_success 'merge z into y with "ours" configuration option => Non-conflicting 3-way merge' '
+	git -c notes.merge="ours" notes merge z &&
+	verify_notes y ours
+'
+
+test_expect_success 'reset to pre-merge state (y)' '
+	git update-ref refs/notes/y refs/notes/y^1 &&
+	# Verify pre-merge state
+	verify_notes y y
+'
+
 cat <<EOF | sort >expect_notes_theirs
 9b4b2c61f0615412da3c10f98ff85b57c04ec765 $commit_sha15
 5de7ea7ad4f47e7ff91989fb82234634730f75df $commit_sha14
@@ -432,6 +450,17 @@ test_expect_success 'reset to pre-merge state (y)' '
 	verify_notes y y
 '
 
+test_expect_success 'merge z into y with "theirs" strategy overriding configuration option "ours" => Non-conflicting 3-way merge' '
+	git -c notes.merge="ours" notes merge --strategy=theirs z &&
+	verify_notes y theirs
+'
+
+test_expect_success 'reset to pre-merge state (y)' '
+	git update-ref refs/notes/y refs/notes/y^1 &&
+	# Verify pre-merge state
+	verify_notes y y
+'
+
 cat <<EOF | sort >expect_notes_union
 7c4e546efd0fe939f876beb262ece02797880b54 $commit_sha15
 5de7ea7ad4f47e7ff91989fb82234634730f75df $commit_sha14
-- 
2.5.0.482.gfcd5645

[PATCH RFC 4/4] notes: add per-ref configuration of merge strategy

[Jacob Keller]

From: Jacob Keller <jacob.keller@gmail.com>

Teach git-notes about a new configuration option
"notes.<localref>.merge" which selects the merge strategy for a
particular ref. This allows selection of merge strategy different for
each note reference, in addition to the default strategy for all
references.

Signed-off-by: Jacob Keller <jacob.keller@gmail.com>
Cc: Johan Herland <johan@herland.net>
Cc: Michael Haggerty <mhagger@alum.mit.edu>
Cc: Eric Sunshine <sunshine@sunshineco.com>
---
 Documentation/config.txt            |   7 +++
 Documentation/git-notes.txt         |   6 ++
 builtin/notes.c                     | 115 +++++++++++++++++++++++++++++++++++-
 t/t3309-notes-merge-auto-resolve.sh |  50 ++++++++++++++++
 4 files changed, 176 insertions(+), 2 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 0fa88a29dd65..c53ec4538cd3 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1918,6 +1918,13 @@ notes.merge::
 	STRATEGIES" section of linkgit:git-notes[1] for more information
 	on each strategy.
 
+notes.<localref>.merge::
+	Which merge strategy to choose if the local ref for a notes merge
+	matches <localref>. Is overridden by notes.merge and takes the same
+	values. <localref> may be fully qualified or just under refs/notes/.
+	See "NOTES MERGE STRATEGIES" section in linkgit:git-notes[1] for more
+	information on each strategy.
+
 notes.displayRef::
 	The (fully qualified) refname from which to show notes when
 	showing commit messages.  The value of this variable can be set
diff --git a/Documentation/git-notes.txt b/Documentation/git-notes.txt
index 9c4f8536182f..1e001807f9d9 100644
--- a/Documentation/git-notes.txt
+++ b/Documentation/git-notes.txt
@@ -322,6 +322,12 @@ notes.merge::
 +
 This setting can be overridden by passing the `--strategy` option.
 
+notes.<localref>.merge::
+	Which strategy to choose when merging into <localref>. Uses the same
+	values as notes.merge. <localref> may be either a fully qualified ref
+	or the shortname under "refs/notes/". See "NOTES MERGE STRATEGIES"
+	section above for more information about each strategy.
+
 notes.displayRef::
 	Which ref (or refs, if a glob or specified more than once), in
 	addition to the default set by `core.notesRef` or
diff --git a/builtin/notes.c b/builtin/notes.c
index de0caa00df1b..b0174d1024dc 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -92,6 +92,10 @@ static const char * const git_notes_get_ref_usage[] = {
 static const char note_template[] =
 	"\nWrite/edit the notes for the following object:\n";
 
+static struct note_ref **note_refs;
+static int note_refs_alloc;
+static int note_refs_nr;
+static struct hashmap note_refs_hash;
 static enum notes_merge_strategy merge_strategy;
 
 struct note_data {
@@ -757,12 +761,87 @@ static int parse_notes_strategy(const char *arg, enum notes_merge_strategy *stra
 	return 0;
 }
 
+struct note_refs_hash_key {
+	const char *str;
+	int len;
+};
+
+struct note_ref {
+	struct hashmap_entry ent; /* must be first */
+
+	const char *name;
+	enum notes_merge_strategy merge_strategy;
+};
+
+static int note_refs_hash_cmp(const struct note_ref *a, const struct note_ref *b, const struct note_refs_hash_key *key)
+{
+	if (key)
+		return strncmp(a->name, key->str, key->len) || a->name[key->len];
+	else
+		return strcmp(a->name, b->name);
+}
+
+static inline void init_note_refs_hash(void)
+{
+	if (!note_refs_hash.cmpfn)
+		hashmap_init(&note_refs_hash, (hashmap_cmp_fn)note_refs_hash_cmp, 0);
+}
+
+static struct note_ref *make_note_ref(const char *name, int len)
+{
+	struct note_ref *ret, *replaced;
+	struct note_refs_hash_key lookup;
+	struct hashmap_entry lookup_entry;
+
+	if (!len)
+		len = strlen(name);
+
+	init_note_refs_hash();
+	lookup.str = name;
+	lookup.len = len;
+	hashmap_entry_init(&lookup_entry, memhash(name, len));
+
+	if ((ret = hashmap_get(&note_refs_hash, &lookup_entry, &lookup)) != NULL)
+		return ret;
+
+	ret = xcalloc(1, sizeof(struct note_ref));
+	ALLOC_GROW(note_refs, note_refs_nr + 1, note_refs_alloc);
+	note_refs[note_refs_nr++] = ret;
+	ret->name = xstrndup(name, len);
+
+	hashmap_entry_init(ret, lookup_entry.hash);
+	replaced = hashmap_put(&note_refs_hash, ret);
+	assert(replaced == NULL);  /* no previous entry overwritten */
+	return ret;
+}
+
+static void set_strategy_for_ref(const char *ref)
+{
+	const char *name = ref;
+	struct note_refs_hash_key lookup;
+	struct hashmap_entry lookup_entry;
+	struct note_ref *note;
+
+	skip_prefix(ref, "refs/notes/", &name);
+
+	init_note_refs_hash();
+	lookup.str = name;
+	lookup.len = strlen(name);
+
+	hashmap_entry_init(&lookup_entry, memhash(name, lookup.len));
+
+	note = hashmap_get(&note_refs_hash, &lookup_entry, &lookup);
+	if (note != NULL)
+		merge_strategy = note->merge_strategy;
+}
+
 static int merge(int argc, const char **argv, const char *prefix)
 {
 	struct strbuf remote_ref = STRBUF_INIT, msg = STRBUF_INIT;
 	unsigned char result_sha1[20];
 	struct notes_tree *t;
 	struct notes_merge_options o;
+	struct note_ref;
 	int do_merge = 0, do_commit = 0, do_abort = 0;
 	int verbosity = 0, result;
 	const char *strategy = NULL;
@@ -815,6 +894,8 @@ static int merge(int argc, const char **argv, const char *prefix)
 	expand_notes_ref(&remote_ref);
 	o.remote_ref = remote_ref.buf;
 
+	set_strategy_for_ref(o.local_ref);
+
 	if (strategy && parse_notes_strategy(strategy, &merge_strategy)) {
 		error("Unknown -s/--strategy: %s", strategy);
 		usage_with_options(git_notes_merge_usage, options);
@@ -957,7 +1038,16 @@ static int get_ref(int argc, const char **argv, const char *prefix)
 
 static int git_notes_config(const char *var, const char *value, void *cb)
 {
-	if (!strcmp(var, "notes.merge")) {
+	const char *ref;
+	const char *subkey;
+	struct note_ref *note;
+
+	if (!starts_with(var, "notes."))
+		return git_default_config(var, value, cb);
+	ref = var + 6;
+
+	/* Handle notes.* variables */
+	if (!strcmp(ref, "merge")) {
 		if (!value)
 			return config_error_nonbool(var);
 		if (parse_notes_strategy(value, &merge_strategy))
@@ -966,7 +1056,28 @@ static int git_notes_config(const char *var, const char *value, void *cb)
 			return 0;
 	}
 
-	return git_default_config(var, value, cb);
+	if (*ref == '/') {
+		warning("Config notes ref cannot begin with '/': %s", ref);
+		return 0;
+	}
+	subkey = strrchr(ref, '.');
+	if (!subkey)
+		return 0;
+
+	/* skip refs/notes prefix if provided */
+	skip_prefix(ref, "refs/notes/", &ref);
+
+	note = make_note_ref(ref, subkey - ref);
+	if (!strcmp(subkey, ".merge")) {
+		if (!value)
+			return config_error_nonbool(var);
+		if (parse_notes_strategy(value, &note->merge_strategy))
+			return error("Unknown notes merge strategy: %s", value);
+		else
+			return 0;
+	}
+
+	return 0;
 }
 
 int cmd_notes(int argc, const char **argv, const char *prefix)
diff --git a/t/t3309-notes-merge-auto-resolve.sh b/t/t3309-notes-merge-auto-resolve.sh
index a773b01b73db..15dd539cd141 100755
--- a/t/t3309-notes-merge-auto-resolve.sh
+++ b/t/t3309-notes-merge-auto-resolve.sh
@@ -383,6 +383,28 @@ test_expect_success 'reset to pre-merge state (y)' '
 	verify_notes y y
 '
 
+test_expect_success 'merge z into y with "ours" per-ref configuration option => Non-conflicting 3-way merge' '
+	git -c notes.y.merge="ours" notes merge z &&
+	verify_notes y ours
+'
+
+test_expect_success 'reset to pre-merge state (y)' '
+	git update-ref refs/notes/y refs/notes/y^1 &&
+	# Verify pre-merge state
+	verify_notes y y
+'
+
+test_expect_success 'merge z into y with "ours" per-ref configuration option prefixed with "refs/notes" => Non-conflicting 3-way merge' '
+	git -c notes.refs/notes/y.merge="ours" notes merge z &&
+	verify_notes y ours
+'
+
+test_expect_success 'reset to pre-merge state (y)' '
+	git update-ref refs/notes/y refs/notes/y^1 &&
+	# Verify pre-merge state
+	verify_notes y y
+'
+
 cat <<EOF | sort >expect_notes_theirs
 9b4b2c61f0615412da3c10f98ff85b57c04ec765 $commit_sha15
 5de7ea7ad4f47e7ff91989fb82234634730f75df $commit_sha14
@@ -534,6 +556,34 @@ test_expect_success 'reset to pre-merge state (y)' '
 	verify_notes y y
 '
 
+test_expect_success 'merge z into y with "union" strategy overriding per-ref configuration => Non-conflicting 3-way merge' '
+	git -c notes.refs/notes/y.merge="theirs" notes merge --strategy=union z &&
+	verify_notes y union
+'
+
+test_expect_success 'reset to pre-merge state (y)' '
+	git update-ref refs/notes/y refs/notes/y^1 &&
+	# Verify pre-merge state
+	verify_notes y y
+'
+
+test_expect_success 'merge z into y with "union" per-ref overriding general configuration => Non-conflicting 3-way merge' '
+	git -c notes.refs/notes/y.merge="union" -c notes.merge="theirs" notes merge z &&
+	verify_notes y union
+'
+
+test_expect_success 'reset to pre-merge state (y)' '
+	git update-ref refs/notes/y refs/notes/y^1 &&
+	# Verify pre-merge state
+	verify_notes y y
+'
+
+test_expect_success 'merge z into y with "manual" per-ref only checks specific ref configuration => Conflicting 3-way merge' '
+	test_must_fail git -c notes.refs/notes/z.merge="union" notes merge z &&
+	git notes merge --abort &&
+	verify_notes y y
+'
+
 cat <<EOF | sort >expect_notes_union2
 d682107b8bf7a7aea1e537a8d5cb6a12b60135f1 $commit_sha15
 5de7ea7ad4f47e7ff91989fb82234634730f75df $commit_sha14
-- 
2.5.0.482.gfcd5645

Re: [PATCH v3 3/4] notes: add notes.merge option to select default strategy

[Junio C Hamano]

Jacob Keller <jacob.e.keller@intel.com> writes:

> From: Jacob Keller <jacob.keller@gmail.com>
>
> Teach git-notes about a new configuration option "notes.merge" for
> selecting the default notes merge strategy. Document the option in
> config.txt and git-notes.txt
>
> Add tests for use of the configuration option. Include a test to ensure
> that --strategy correctly overrides the configured setting.
>
> Signed-off-by: Jacob Keller <jacob.keller@gmail.com>
> Cc: Johan Herland <johan@herland.net>
> Cc: Michael Haggerty <mhagger@alum.mit.edu>
> Cc: Eric Sunshine <sunshine@sunshineco.com>
> ---

Perhaps I am biased because we do not usually use Cc: around here,
but the above looks in a somewhat strange order.  Shouldn't your
sign-off be at the end?

> +static enum notes_merge_strategy merge_strategy;

OK.

> +static int parse_notes_strategy(const char *arg, enum notes_merge_strategy *strategy)
> +{
> +	if (!strcmp(arg, "manual"))
> +		*strategy = NOTES_MERGE_RESOLVE_MANUAL;
> +	else if (!strcmp(arg, "ours"))
> +		*strategy = NOTES_MERGE_RESOLVE_OURS;
> +	else if (!strcmp(arg, "theirs"))
> +		*strategy = NOTES_MERGE_RESOLVE_THEIRS;
> +	else if (!strcmp(arg, "union"))
> +		*strategy = NOTES_MERGE_RESOLVE_UNION;
> +	else if (!strcmp(arg, "cat_sort_uniq"))
> +		*strategy = NOTES_MERGE_RESOLVE_CAT_SORT_UNIQ;
> +	else
> +		return -1;
> +
> +	return 0;
> +}

OK.

>  static int merge(int argc, const char **argv, const char *prefix)
>  {
>  	struct strbuf remote_ref = STRBUF_INIT, msg = STRBUF_INIT;
> @@ -795,23 +815,13 @@ static int merge(int argc, const char **argv, const char *prefix)
>  	expand_notes_ref(&remote_ref);
>  	o.remote_ref = remote_ref.buf;
>  
> -	if (strategy) {
> -		if (!strcmp(strategy, "manual"))
> -			o.strategy = NOTES_MERGE_RESOLVE_MANUAL;
> -		else if (!strcmp(strategy, "ours"))
> -			o.strategy = NOTES_MERGE_RESOLVE_OURS;
> -		else if (!strcmp(strategy, "theirs"))
> -			o.strategy = NOTES_MERGE_RESOLVE_THEIRS;
> -		else if (!strcmp(strategy, "union"))
> -			o.strategy = NOTES_MERGE_RESOLVE_UNION;
> -		else if (!strcmp(strategy, "cat_sort_uniq"))
> -			o.strategy = NOTES_MERGE_RESOLVE_CAT_SORT_UNIQ;
> -		else {
> -			error("Unknown -s/--strategy: %s", strategy);
> -			usage_with_options(git_notes_merge_usage, options);
> -		}
> +	if (strategy && parse_notes_strategy(strategy, &merge_strategy)) {
> +		error("Unknown -s/--strategy: %s", strategy);
> +		usage_with_options(git_notes_merge_usage, options);
>  	}
>  
> +	o.strategy = merge_strategy;
> +

This may be a minor taste thing, but initializing "o.strategy" with
merge_strategy at the same place as "o.remote_ref" is initialized,
and then passing &o.merge_strategy to parse_notes_strategy(), may be
easier to follow.

Renaming the global "merge_strategy" to "configured_merge_strategy"
might make it even easier to follow.  If anybody uses the variable
instead of o.strategy after this point, it would be immediately
obvious that it is either a bug or it is deliberately using the
value from the configuration file, not command line.

Thanks.

Re: [PATCH RFC 4/4] notes: add per-ref configuration of merge strategy

[Junio C Hamano]

Jacob Keller <jacob.e.keller@intel.com> writes:

> +notes.<localref>.merge::
> +	Which merge strategy to choose if the local ref for a notes merge
> +	matches <localref>. Is overridden by notes.merge and takes the same
> +	values. <localref> may be fully qualified or just under refs/notes/.
> +	See "NOTES MERGE STRATEGIES" section in linkgit:git-notes[1] for more
> +	information on each strategy.

If I have notes.refs/notes/commit.merge and notes.merge specified,
I'd expect the former overrides the latter.  The second sentence may
need correcting.

I think it is a mistake to allow both notes.refs/notes/commit.merge
and notes.commit.merge.  You'd end up needing to implement quite a
complicated "the last one wins" rule if you did so.

> +notes.<localref>.merge::
> +	Which strategy to choose when merging into <localref>. Uses the same
> +	values as notes.merge. <localref> may be either a fully qualified ref
> +	or the shortname under "refs/notes/". See "NOTES MERGE STRATEGIES"
> +	section above for more information about each strategy.

As a reviewer, I can tell that "Uses the same values" wants to say
that the set of allowed values is the same, but a casual reader is
bound to read it as "notes.commit.merge must be set to the same
value as the value set to notes.merge".

> diff --git a/builtin/notes.c b/builtin/notes.c
> index de0caa00df1b..b0174d1024dc 100644
> --- a/builtin/notes.c
> +++ b/builtin/notes.c
> @@ -92,6 +92,10 @@ static const char * const git_notes_get_ref_usage[] = {
>  static const char note_template[] =
>  	"\nWrite/edit the notes for the following object:\n";
>  
> +static struct note_ref **note_refs;
> +static int note_refs_alloc;
> +static int note_refs_nr;
> +static struct hashmap note_refs_hash;
>  static enum notes_merge_strategy merge_strategy;
>  
>  struct note_data {
> @@ -757,12 +761,87 @@ static int parse_notes_strategy(const char *arg, enum notes_merge_strategy *stra
>  	return 0;
>  }
>  ...
> +struct note_refs_hash_key {
> +	const char *str;
> +	int len;
> +};
> + ...
> +static void set_strategy_for_ref(const char *ref)
> +{
> + ...
> +}

Hmmm, I do not see why you need all the complexity above.

When you come to merge(), after calling default_notes_ref(), you
know exactly which notes ref you are merging into, no?  Shouldn't
then the change required for this feature just the matter of asking
the configuration system values for notes.$remote_ref.merge and
notes.merge?

IOW,

	struct strbuf key = STRBUF_INIT;
	char *value = NULL;

        strbuf_addf(&key, "notes.%s.merge", remote_ref.buf);

	git_config_get_string(key.buf, &value) ||
	git_config_get_string_const("notes.merge", &value));

	if (value)
        	parse_notes_strategy(value, &configured_merge_strategy);

	...

        parse_options();
	if (strategy)
        	parse_notes_strategy(value, &configured_merge_strategy);

or something?

Re: [PATCH v3 3/4] notes: add notes.merge option to select default strategy

[Eric Sunshine]

On Sun, Aug 2, 2015 at 6:10 AM, Jacob Keller <jacob.e.keller@intel.com> wrote:
> Teach git-notes about a new configuration option "notes.merge" for
> selecting the default notes merge strategy. Document the option in
> config.txt and git-notes.txt
>
> Add tests for use of the configuration option. Include a test to ensure
> that --strategy correctly overrides the configured setting.
>
> Signed-off-by: Jacob Keller <jacob.keller@gmail.com>
> ---
> diff --git a/Documentation/git-notes.txt b/Documentation/git-notes.txt
> index 674682b34b83..9c4f8536182f 100644
> --- a/Documentation/git-notes.txt
> +++ b/Documentation/git-notes.txt
> @@ -101,7 +101,7 @@ merge::
>  If conflicts arise and a strategy for automatically resolving
> -conflicting notes (see the -s/--strategy option) is not given,
> +conflicting notes (see the "NOTES MERGE STRATEGIES" section) is not given,
>  the "manual" resolver is used. This resolver checks out the
>  conflicting notes in a special worktree (`.git/NOTES_MERGE_WORKTREE`),
>  and instructs the user to manually resolve the conflicts there.
> @@ -183,6 +183,7 @@ OPTIONS
>         When merging notes, resolve notes conflicts using the given
>         strategy. The following strategies are recognized: "manual"
>         (default), "ours", "theirs", "union" and "cat_sort_uniq".
> +       This option overrides the "notes.merge" configuration setting.
>         See the "NOTES MERGE STRATEGIES" section below for more
>         information on each notes merge strategy.

These two documentation updates are much easier to digest than the
noisy-diff versions of the previous attempt; and the patch overall is
a more pleasant read than v1.

> diff --git a/builtin/notes.c b/builtin/notes.c
> index 63f95fc55439..de0caa00df1b 100644
> --- a/builtin/notes.c
> +++ b/builtin/notes.c
> @@ -945,6 +955,20 @@ static int get_ref(int argc, const char **argv, const char *prefix)
>         return 0;
>  }
>
> +static int git_notes_config(const char *var, const char *value, void *cb)
> +{
> +       if (!strcmp(var, "notes.merge")) {
> +               if (!value)
> +                       return config_error_nonbool(var);
> +               if (parse_notes_strategy(value, &merge_strategy))
> +                       return error("Unknown notes merge strategy: %s", value);
> +               else
> +                       return 0;

A purely subjective stylistic suggestion, which you can freely ignore
if your preference differs:

    if (!value)
        return ...;
    if (parse_notes_strategy(...))
        return ...;
    return 0;

> +       }
> +
> +       return git_default_config(var, value, cb);
> +}
> +