git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v8 0/8] implement notes.mergeStrategy option
@ 2015-08-17  8:46 Jacob Keller
  2015-08-17  8:46 ` [PATCH v8 1/8] notes: document cat_sort_uniq rewriteMode Jacob Keller
                   ` (7 more replies)
  0 siblings, 8 replies; 14+ messages in thread
From: Jacob Keller @ 2015-08-17  8:46 UTC (permalink / raw)
  To: git; +Cc: Johan Herland, Eric Sunshine, Jacob Keller

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

- Changes since v7 -
* add patches to make rewrite and merge take same options
* camel case mergeStrategy
* move init_notes_check above reading git-config in merge()
  This is necessary as it ensures refs are inside refs/notes/*

It should be noted that git-notes already blocks all operations outside
of refs/notes, including merging from refs outside of refs/notes. This
series leaves the ability to merge from non-refs/notes refs as an
enhancement for a future author. However, git-notes also (correctly)
prevents merge (and any other option) into non refs/notes/* refs. You
may notice a "BUG:" print statement after skip_prefix, this is only as a
failsafe and is marked BUG since it should never be true.

Hopefully everything looks good now. I chose to add patches which
combine rewrite and notes-merge options to take the same parameters.
This does cause some weirdness since ours and theirs is swapped much
like in a rebase merge conflict.

If we dont' like this, I am ok with re-writing this to not document the
synonyms. However, I think that a good future direction would be to make
rewrite use the exact same flow as merge, and support the `manual` mode
as well using the full notes-merge harness.

Jacob Keller (8):
  notes: document cat_sort_uniq rewriteMode
  notes: extract enum notes_merge_strategy to notes-utils.h
  note: extract parse_notes_merge_strategy to notes-utils
  notes: allow use of the "rewrite" terminology for merge strategies
  notes: implement parse_combine_rewrite_fn using
    parse_notes_merge_strategy
  notes: add tests for --commit/--abort/--strategy exclusivity
  notes: add notes.mergeStrategy option to select default strategy
  notes: teach git-notes about notes.<ref>.mergeStrategy option

 Documentation/config.txt              |  22 +++++-
 Documentation/git-notes.txt           |  43 +++++++++--
 builtin/notes.c                       |  43 +++++++----
 notes-merge.h                         |  10 +--
 notes-utils.c                         |  50 ++++++++++---
 notes-utils.h                         |   9 +++
 t/t3301-notes.sh                      |  55 ++++++++++++++
 t/t3309-notes-merge-auto-resolve.sh   | 135 ++++++++++++++++++++++++++++++++++
 t/t3310-notes-merge-manual-resolve.sh |  12 +++
 9 files changed, 341 insertions(+), 38 deletions(-)

-- 
2.5.0.280.g4aaba03

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH v8 1/8] notes: document cat_sort_uniq rewriteMode
  2015-08-17  8:46 [PATCH v8 0/8] implement notes.mergeStrategy option Jacob Keller
@ 2015-08-17  8:46 ` Jacob Keller
  2015-08-17  8:46 ` [PATCH v8 2/8] notes: extract enum notes_merge_strategy to notes-utils.h Jacob Keller
                   ` (6 subsequent siblings)
  7 siblings, 0 replies; 14+ messages in thread
From: Jacob Keller @ 2015-08-17  8:46 UTC (permalink / raw)
  To: git; +Cc: Johan Herland, Eric Sunshine, 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>
---
 Documentation/config.txt    | 4 ++--
 Documentation/git-notes.txt | 5 +++--
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 75ec02e8e90a..de67ad1fdedf 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1947,8 +1947,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..00c84be33ca9 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.
@@ -368,7 +369,7 @@ does not match any refs is silently ignored.
 'GIT_NOTES_REWRITE_MODE'::
 	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`.
+	Must be one of `overwrite`, `concatenate`, `cat_sort_uniq`, or `ignore`.
 	This overrides the `core.rewriteMode` setting.
 
 'GIT_NOTES_REWRITE_REF'::
-- 
2.5.0.280.g4aaba03

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH v8 2/8] notes: extract enum notes_merge_strategy to notes-utils.h
  2015-08-17  8:46 [PATCH v8 0/8] implement notes.mergeStrategy option Jacob Keller
  2015-08-17  8:46 ` [PATCH v8 1/8] notes: document cat_sort_uniq rewriteMode Jacob Keller
@ 2015-08-17  8:46 ` Jacob Keller
  2015-08-17  8:46 ` [PATCH v8 3/8] note: extract parse_notes_merge_strategy to notes-utils Jacob Keller
                   ` (5 subsequent siblings)
  7 siblings, 0 replies; 14+ messages in thread
From: Jacob Keller @ 2015-08-17  8:46 UTC (permalink / raw)
  To: git; +Cc: Johan Herland, Eric Sunshine, Jacob Keller

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

A future patch will extract parsing of the --strategy string into a
helper function in notes.c and will require the enumeration definition.

Signed-off-by: Jacob Keller <jacob.keller@gmail.com>
---
 notes-merge.h | 10 +++-------
 notes-utils.h |  8 ++++++++
 2 files changed, 11 insertions(+), 7 deletions(-)

diff --git a/notes-merge.h b/notes-merge.h
index 1d01f6aacf54..0d890563b5f4 100644
--- a/notes-merge.h
+++ b/notes-merge.h
@@ -1,6 +1,8 @@
 #ifndef NOTES_MERGE_H
 #define NOTES_MERGE_H
 
+#include "notes-utils.h"
+
 #define NOTES_MERGE_WORKTREE "NOTES_MERGE_WORKTREE"
 
 enum notes_merge_verbosity {
@@ -13,13 +15,7 @@ struct notes_merge_options {
 	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/notes-utils.h b/notes-utils.h
index 890ddb33e13a..db5811e3f718 100644
--- a/notes-utils.h
+++ b/notes-utils.h
@@ -19,6 +19,14 @@ void create_notes_commit(struct notes_tree *t, struct commit_list *parents,
 
 void commit_notes(struct notes_tree *t, const char *msg);
 
+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_rewrite_cfg {
 	struct notes_tree **trees;
 	const char *cmd;
-- 
2.5.0.280.g4aaba03

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH v8 3/8] note: extract parse_notes_merge_strategy to notes-utils
  2015-08-17  8:46 [PATCH v8 0/8] implement notes.mergeStrategy option Jacob Keller
  2015-08-17  8:46 ` [PATCH v8 1/8] notes: document cat_sort_uniq rewriteMode Jacob Keller
  2015-08-17  8:46 ` [PATCH v8 2/8] notes: extract enum notes_merge_strategy to notes-utils.h Jacob Keller
@ 2015-08-17  8:46 ` Jacob Keller
  2015-08-17  8:46 ` [PATCH v8 4/8] notes: allow use of the "rewrite" terminology for merge strategies Jacob Keller
                   ` (4 subsequent siblings)
  7 siblings, 0 replies; 14+ messages in thread
From: Jacob Keller @ 2015-08-17  8:46 UTC (permalink / raw)
  To: git; +Cc: Johan Herland, Eric Sunshine, Jacob Keller

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

Allow future code to re-use the parsing functionality.

Signed-off-by: Jacob Keller <jacob.keller@gmail.com>
---
 builtin/notes.c | 12 +-----------
 notes-utils.c   | 18 ++++++++++++++++++
 notes-utils.h   |  1 +
 3 files changed, 20 insertions(+), 11 deletions(-)

diff --git a/builtin/notes.c b/builtin/notes.c
index 042348082709..0e7aba98f4d7 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -797,17 +797,7 @@ static int merge(int argc, const char **argv, const char *prefix)
 	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 {
+		if (parse_notes_merge_strategy(strategy, &o.strategy)) {
 			error("Unknown -s/--strategy: %s", strategy);
 			usage_with_options(git_notes_merge_usage, options);
 		}
diff --git a/notes-utils.c b/notes-utils.c
index ccbf0737a34e..299e34bccc58 100644
--- a/notes-utils.c
+++ b/notes-utils.c
@@ -54,6 +54,24 @@ void commit_notes(struct notes_tree *t, const char *msg)
 	strbuf_release(&buf);
 }
 
+int parse_notes_merge_strategy(const char *v, enum notes_merge_strategy *s)
+{
+	if (!strcmp(v, "manual"))
+		*s = NOTES_MERGE_RESOLVE_MANUAL;
+	else if (!strcmp(v, "ours"))
+		*s = NOTES_MERGE_RESOLVE_OURS;
+	else if (!strcmp(v, "theirs"))
+		*s = NOTES_MERGE_RESOLVE_THEIRS;
+	else if (!strcmp(v, "union"))
+		*s = NOTES_MERGE_RESOLVE_UNION;
+	else if (!strcmp(v, "cat_sort_uniq"))
+		*s = NOTES_MERGE_RESOLVE_CAT_SORT_UNIQ;
+	else
+		return -1;
+
+	return 0;
+}
+
 static combine_notes_fn parse_combine_notes_fn(const char *v)
 {
 	if (!strcasecmp(v, "overwrite"))
diff --git a/notes-utils.h b/notes-utils.h
index db5811e3f718..fa538e1d9502 100644
--- a/notes-utils.h
+++ b/notes-utils.h
@@ -37,6 +37,7 @@ struct notes_rewrite_cfg {
 	int mode_from_env;
 };
 
+int parse_notes_merge_strategy(const char *v, enum notes_merge_strategy *s);
 struct notes_rewrite_cfg *init_copy_notes_for_rewrite(const char *cmd);
 int copy_note_for_rewrite(struct notes_rewrite_cfg *c,
 			  const unsigned char *from_obj, const unsigned char *to_obj);
-- 
2.5.0.280.g4aaba03

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH v8 4/8] notes: allow use of the "rewrite" terminology for merge strategies
  2015-08-17  8:46 [PATCH v8 0/8] implement notes.mergeStrategy option Jacob Keller
                   ` (2 preceding siblings ...)
  2015-08-17  8:46 ` [PATCH v8 3/8] note: extract parse_notes_merge_strategy to notes-utils Jacob Keller
@ 2015-08-17  8:46 ` Jacob Keller
  2015-08-17 12:54   ` Johan Herland
  2015-08-17  8:46 ` [PATCH v8 5/8] notes: implement parse_combine_rewrite_fn using parse_notes_merge_strategy Jacob Keller
                   ` (3 subsequent siblings)
  7 siblings, 1 reply; 14+ messages in thread
From: Jacob Keller @ 2015-08-17  8:46 UTC (permalink / raw)
  To: git; +Cc: Johan Herland, Eric Sunshine, Jacob Keller

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

notes-merge.c already re-uses the same functions for the automatic merge
strategies used by the rewrite functionality. Teach the -s/--strategy
option how to interpret the equivalent rewrite terminology for
consistency.

Add tests for the new synonyms.

Teaching rewrite how to understand merge terminology is left for a
following patch.

Signed-off-by: Jacob Keller <jacob.keller@gmail.com>
---
 Documentation/git-notes.txt         |  6 +++++
 notes-utils.c                       |  6 +++++
 t/t3309-notes-merge-auto-resolve.sh | 45 +++++++++++++++++++++++++++++++++++++
 3 files changed, 57 insertions(+)

diff --git a/Documentation/git-notes.txt b/Documentation/git-notes.txt
index 00c84be33ca9..5028e9355de5 100644
--- a/Documentation/git-notes.txt
+++ b/Documentation/git-notes.txt
@@ -250,13 +250,19 @@ When done, the user can either finalize the merge with
 "ours" automatically resolves conflicting notes in favor of the local
 version (i.e. the current notes ref).
 
+"ignore" is an alternative spelling for "ours".
+
 "theirs" automatically resolves notes conflicts in favor of the remote
 version (i.e. the given notes ref being merged into the current notes
 ref).
 
+"overwrite" is an alternative spelling for "theirs".
+
 "union" automatically resolves notes conflicts by concatenating the
 local and remote versions.
 
+"concatenate" is an alternative spelling for "union".
+
 "cat_sort_uniq" is similar to "union", but in addition to concatenating
 the local and remote versions, this strategy also sorts the resulting
 lines, and removes duplicate lines from the result. This is equivalent
diff --git a/notes-utils.c b/notes-utils.c
index 299e34bccc58..656b0ea152e2 100644
--- a/notes-utils.c
+++ b/notes-utils.c
@@ -60,10 +60,16 @@ int parse_notes_merge_strategy(const char *v, enum notes_merge_strategy *s)
 		*s = NOTES_MERGE_RESOLVE_MANUAL;
 	else if (!strcmp(v, "ours"))
 		*s = NOTES_MERGE_RESOLVE_OURS;
+	else if (!strcmp(v, "ignore"))
+		*s = NOTES_MERGE_RESOLVE_OURS;
 	else if (!strcmp(v, "theirs"))
 		*s = NOTES_MERGE_RESOLVE_THEIRS;
+	else if (!strcmp(v, "overwrite"))
+		*s = NOTES_MERGE_RESOLVE_THEIRS;
 	else if (!strcmp(v, "union"))
 		*s = NOTES_MERGE_RESOLVE_UNION;
+	else if (!strcmp(v, "concatenate"))
+		*s = NOTES_MERGE_RESOLVE_UNION;
 	else if (!strcmp(v, "cat_sort_uniq"))
 		*s = NOTES_MERGE_RESOLVE_CAT_SORT_UNIQ;
 	else
diff --git a/t/t3309-notes-merge-auto-resolve.sh b/t/t3309-notes-merge-auto-resolve.sh
index 461fd84755d7..909900672446 100755
--- a/t/t3309-notes-merge-auto-resolve.sh
+++ b/t/t3309-notes-merge-auto-resolve.sh
@@ -365,6 +365,17 @@ test_expect_success 'reset to pre-merge state (y)' '
 	verify_notes y y
 '
 
+test_expect_success 'merge z into y with "ignore" strategy => Non-conflicting 3-way merge' '
+	git notes merge --strategy=ignore 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 +443,17 @@ test_expect_success 'reset to pre-merge state (y)' '
 	verify_notes y y
 '
 
+test_expect_success 'merge z into y with "overwrite" strategy => Non-conflicting 3-way merge' '
+	git notes merge --strategy=overwrite 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
@@ -505,6 +527,17 @@ test_expect_success 'reset to pre-merge state (y)' '
 	verify_notes y y
 '
 
+test_expect_success 'merge z into y with "concatenate" strategy => Non-conflicting 3-way merge' '
+	git notes merge --strategy=concatenate 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
+'
+
 cat <<EOF | sort >expect_notes_union2
 d682107b8bf7a7aea1e537a8d5cb6a12b60135f1 $commit_sha15
 5de7ea7ad4f47e7ff91989fb82234634730f75df $commit_sha14
@@ -579,6 +612,18 @@ test_expect_success 'reset to pre-merge state (z)' '
 	verify_notes z z
 '
 
+test_expect_success 'merge y into z with "concatenate" strategy => Non-conflicting 3-way merge' '
+	git config core.notesRef refs/notes/z &&
+	git notes merge --strategy=concatenate y &&
+	verify_notes z union2
+'
+
+test_expect_success 'reset to pre-merge state (z)' '
+	git update-ref refs/notes/z refs/notes/z^1 &&
+	# Verify pre-merge state
+	verify_notes z z
+'
+
 cat <<EOF | sort >expect_notes_cat_sort_uniq
 6be90240b5f54594203e25d9f2f64b7567175aee $commit_sha15
 5de7ea7ad4f47e7ff91989fb82234634730f75df $commit_sha14
-- 
2.5.0.280.g4aaba03

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH v8 5/8] notes: implement parse_combine_rewrite_fn using parse_notes_merge_strategy
  2015-08-17  8:46 [PATCH v8 0/8] implement notes.mergeStrategy option Jacob Keller
                   ` (3 preceding siblings ...)
  2015-08-17  8:46 ` [PATCH v8 4/8] notes: allow use of the "rewrite" terminology for merge strategies Jacob Keller
@ 2015-08-17  8:46 ` Jacob Keller
  2015-08-17  8:46 ` [PATCH v8 6/8] notes: add tests for --commit/--abort/--strategy exclusivity Jacob Keller
                   ` (2 subsequent siblings)
  7 siblings, 0 replies; 14+ messages in thread
From: Jacob Keller @ 2015-08-17  8:46 UTC (permalink / raw)
  To: git; +Cc: Johan Herland, Eric Sunshine, Jacob Keller

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

Teach the rewrite combine notes to use the same names as git-notes
merge. This will support all current names plus a few new synonyms.

Update documentation to point to NOTES MERGE STRATEGIES to explain the
various rewrite options available.

Implementing rewrite functionality completely in terms of merging is
left as an exercise for a future contributor.

Signed-off-by: Jacob Keller <jacob.keller@gmail.com>
---
 Documentation/config.txt    |  8 ++++---
 Documentation/git-notes.txt | 18 ++++++++++-----
 notes-utils.c               | 26 +++++++++++++--------
 t/t3301-notes.sh            | 55 +++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 89 insertions(+), 18 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index de67ad1fdedf..4daa804b1eab 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1946,9 +1946,11 @@ notes.rewrite.<command>::
 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`, `cat_sort_uniq`, or `ignore`.
-	Defaults to `concatenate`.
+	the target commit already has a note.  With the exception of `manual`,
+	any automatic merge strategy may be chosen.  Beware that in the
+	re-write context the typical notion of `ours` and `theirs` is reversed.
+	See the "NOTES MERGE STRATEGIES" section above for information on each
+	of the available strategies.
 +
 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 5028e9355de5..678dadfdf3c3 100644
--- a/Documentation/git-notes.txt
+++ b/Documentation/git-notes.txt
@@ -336,9 +336,11 @@ 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`, `cat_sort_uniq`, or `ignore`.  Defaults to
-	`concatenate`.
+	commit already has a note.  With the exception of `manual`, any
+	automatic merge strategy may be chosen.  Beware that in the re-write
+	context the typical notion of `ours` and `theirs` is reversed .  See
+	the "NOTES MERGE STRATEGIES" section above for information on each of
+	the available strategies.
 +
 This setting can be overridden with the `GIT_NOTES_REWRITE_MODE`
 environment variable.
@@ -374,9 +376,13 @@ does not match any refs is silently ignored.
 
 'GIT_NOTES_REWRITE_MODE'::
 	When copying notes during a rewrite, what to do if the target
-	commit already has a note.
-	Must be one of `overwrite`, `concatenate`, `cat_sort_uniq`, or `ignore`.
-	This overrides the `core.rewriteMode` setting.
+	commit already has a note.  With the exception of `manual`, any
+	automatic merge strategy may be chosen.  Beware that in the re-write
+	context the typical notion of `ours` and `theirs` is reversed .  See
+	the "NOTES MERGE STRATEGIES" section above for information on each of
+	the available strategies.
++
+Overrides the notes.rewriteMode configuration option.
 
 'GIT_NOTES_REWRITE_REF'::
 	When rewriting commits, which notes to copy from the original
diff --git a/notes-utils.c b/notes-utils.c
index 656b0ea152e2..8d4cc8909bcf 100644
--- a/notes-utils.c
+++ b/notes-utils.c
@@ -80,16 +80,24 @@ int parse_notes_merge_strategy(const char *v, enum notes_merge_strategy *s)
 
 static combine_notes_fn parse_combine_notes_fn(const char *v)
 {
-	if (!strcasecmp(v, "overwrite"))
-		return combine_notes_overwrite;
-	else if (!strcasecmp(v, "ignore"))
-		return combine_notes_ignore;
-	else if (!strcasecmp(v, "concatenate"))
-		return combine_notes_concatenate;
-	else if (!strcasecmp(v, "cat_sort_uniq"))
-		return combine_notes_cat_sort_uniq;
-	else
+	enum notes_merge_strategy s;
+
+	if (parse_notes_merge_strategy(v, &s))
 		return NULL;
+
+	switch (s) {
+	case NOTES_MERGE_RESOLVE_OURS:
+		return combine_notes_ignore;
+	case NOTES_MERGE_RESOLVE_THEIRS:
+		return combine_notes_overwrite;
+	case NOTES_MERGE_RESOLVE_UNION:
+		return combine_notes_concatenate;
+	case NOTES_MERGE_RESOLVE_CAT_SORT_UNIQ:
+		return combine_notes_cat_sort_uniq;
+	case NOTES_MERGE_RESOLVE_MANUAL:
+	default:
+		return NULL;
+	}
 }
 
 static int notes_rewrite_config(const char *k, const char *v, void *cb)
diff --git a/t/t3301-notes.sh b/t/t3301-notes.sh
index 8cffd35fb03d..eb1b5824a422 100755
--- a/t/t3301-notes.sh
+++ b/t/t3301-notes.sh
@@ -997,6 +997,26 @@ test_expect_success 'git notes copy --for-rewrite (overwrite)' '
 	test_cmp expect actual
 '
 
+test_expect_success 'git notes copy --for-rewrite (theirs)' '
+	cat >expect <<-EOF &&
+		commit 4acf42e847e7fffbbf89ee365c20ac7caf40de89
+		Author: A U Thor <author@example.com>
+		Date:   Thu Apr 7 15:27:13 2005 -0700
+
+		${indent}15th
+
+		Notes:
+		${indent}a fresh note
+	EOF
+	git notes add -f -m"a fresh note" HEAD^ &&
+	test_config notes.rewriteMode theirs &&
+	test_config notes.rewriteRef "refs/notes/*" &&
+	echo $(git rev-parse HEAD^) $(git rev-parse HEAD) |
+	git notes copy --for-rewrite=foo &&
+	git log -1 >actual &&
+	test_cmp expect actual
+'
+
 test_expect_success 'git notes copy --for-rewrite (ignore)' '
 	test_config notes.rewriteMode ignore &&
 	test_config notes.rewriteRef "refs/notes/*" &&
@@ -1006,6 +1026,15 @@ test_expect_success 'git notes copy --for-rewrite (ignore)' '
 	test_cmp expect actual
 '
 
+test_expect_success 'git notes copy --for-rewrite (ours)' '
+	test_config notes.rewriteMode ours &&
+	test_config notes.rewriteRef "refs/notes/*" &&
+	echo $(git rev-parse HEAD^) $(git rev-parse HEAD) |
+	git notes copy --for-rewrite=foo &&
+	git log -1 >actual &&
+	test_cmp expect actual
+'
+
 test_expect_success 'git notes copy --for-rewrite (append)' '
 	cat >expect <<-EOF &&
 		commit 4acf42e847e7fffbbf89ee365c20ac7caf40de89
@@ -1028,6 +1057,30 @@ test_expect_success 'git notes copy --for-rewrite (append)' '
 	test_cmp expect actual
 '
 
+test_expect_success 'git notes copy --for-rewrite (union append)' '
+	cat >expect <<-EOF &&
+		commit 4acf42e847e7fffbbf89ee365c20ac7caf40de89
+		Author: A U Thor <author@example.com>
+		Date:   Thu Apr 7 15:27:13 2005 -0700
+
+		${indent}15th
+
+		Notes:
+		${indent}a fresh note
+		${indent}
+		${indent}another fresh note
+		${indent}
+		${indent}yet again a fresh note
+	EOF
+	git notes add -f -m"yet again a fresh note" HEAD^ &&
+	test_config notes.rewriteMode union &&
+	test_config notes.rewriteRef "refs/notes/*" &&
+	echo $(git rev-parse HEAD^) $(git rev-parse HEAD) |
+	git notes copy --for-rewrite=foo &&
+	git log -1 >actual &&
+	test_cmp expect actual
+'
+
 test_expect_success 'git notes copy --for-rewrite (append two to one)' '
 	cat >expect <<-EOF &&
 		commit 4acf42e847e7fffbbf89ee365c20ac7caf40de89
@@ -1041,6 +1094,8 @@ test_expect_success 'git notes copy --for-rewrite (append two to one)' '
 		${indent}
 		${indent}another fresh note
 		${indent}
+		${indent}yet again a fresh note
+		${indent}
 		${indent}append 1
 		${indent}
 		${indent}append 2
-- 
2.5.0.280.g4aaba03

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH v8 6/8] notes: add tests for --commit/--abort/--strategy exclusivity
  2015-08-17  8:46 [PATCH v8 0/8] implement notes.mergeStrategy option Jacob Keller
                   ` (4 preceding siblings ...)
  2015-08-17  8:46 ` [PATCH v8 5/8] notes: implement parse_combine_rewrite_fn using parse_notes_merge_strategy Jacob Keller
@ 2015-08-17  8:46 ` Jacob Keller
  2015-08-17  8:46 ` [PATCH v8 7/8] notes: add notes.mergeStrategy option to select default strategy Jacob Keller
  2015-08-17  8:46 ` [PATCH v8 8/8] notes: teach git-notes about notes.<ref>.mergeStrategy option Jacob Keller
  7 siblings, 0 replies; 14+ messages in thread
From: Jacob Keller @ 2015-08-17  8:46 UTC (permalink / raw)
  To: git; +Cc: Johan Herland, Eric Sunshine, 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.280.g4aaba03

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH v8 7/8] notes: add notes.mergeStrategy option to select default strategy
  2015-08-17  8:46 [PATCH v8 0/8] implement notes.mergeStrategy option Jacob Keller
                   ` (5 preceding siblings ...)
  2015-08-17  8:46 ` [PATCH v8 6/8] notes: add tests for --commit/--abort/--strategy exclusivity Jacob Keller
@ 2015-08-17  8:46 ` Jacob Keller
  2015-08-17  8:46 ` [PATCH v8 8/8] notes: teach git-notes about notes.<ref>.mergeStrategy option Jacob Keller
  7 siblings, 0 replies; 14+ messages in thread
From: Jacob Keller @ 2015-08-17  8:46 UTC (permalink / raw)
  To: git; +Cc: Johan Herland, Eric Sunshine, Jacob Keller

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

Teach git-notes about "notes.mergeStrategy" to select a general strategy
for all notes merges. This enables a user to always get expected merge
strategy such as "cat_sort_uniq" without having to pass the "-s" option
manually.

Signed-off-by: Jacob Keller <jacob.keller@gmail.com>
---
 Documentation/config.txt            |  7 +++++
 Documentation/git-notes.txt         | 14 +++++++++-
 builtin/notes.c                     | 19 ++++++++++++--
 t/t3309-notes-merge-auto-resolve.sh | 51 +++++++++++++++++++++++++++++++++++++
 4 files changed, 88 insertions(+), 3 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 4daa804b1eab..56e20446f587 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1919,6 +1919,13 @@ mergetool.writeToTemp::
 mergetool.prompt::
 	Prompt before each invocation of the merge resolution program.
 
+notes.mergeStrategy::
+	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 678dadfdf3c3..66b5017939c6 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.mergeStrategy" 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.mergeStrategy accordingly:
+
 "ours" automatically resolves conflicting notes in favor of the local
 version (i.e. the current notes ref).
 
@@ -316,6 +320,14 @@ core.notesRef::
 	This setting can be overridden through the environment and
 	command line.
 
+notes.mergeStrategy::
+	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 0e7aba98f4d7..91f7a6547b0f 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -738,6 +738,19 @@ static int merge_commit(struct notes_merge_options *o)
 	return ret;
 }
 
+static int git_config_get_notes_strategy(const char *key,
+					 enum notes_merge_strategy *strategy)
+{
+	const char *value;
+
+	if (git_config_get_string_const(key, &value))
+		return 1;
+	if (parse_notes_merge_strategy(value, strategy))
+		git_die_config(key, "unknown notes merge strategy %s", value);
+
+	return 0;
+}
+
 static int merge(int argc, const char **argv, const char *prefix)
 {
 	struct strbuf remote_ref = STRBUF_INIT, msg = STRBUF_INIT;
@@ -796,15 +809,17 @@ static int merge(int argc, const char **argv, const char *prefix)
 	expand_notes_ref(&remote_ref);
 	o.remote_ref = remote_ref.buf;
 
+	t = init_notes_check("merge");
+
 	if (strategy) {
 		if (parse_notes_merge_strategy(strategy, &o.strategy)) {
 			error("Unknown -s/--strategy: %s", strategy);
 			usage_with_options(git_notes_merge_usage, options);
 		}
+	} else {
+		git_config_get_notes_strategy("notes.mergeStrategy", &o.strategy);
 	}
 
-	t = init_notes_check("merge");
-
 	strbuf_addf(&msg, "notes: Merged notes from %s into %s",
 		    remote_ref.buf, default_notes_ref());
 	strbuf_add(&(o.commit_msg), msg.buf + 7, msg.len - 7); /* skip "notes: " */
diff --git a/t/t3309-notes-merge-auto-resolve.sh b/t/t3309-notes-merge-auto-resolve.sh
index 909900672446..1bd71d947e60 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.mergeStrategy="foo" notes merge z &&
+	# Verify no changes (y)
+	verify_notes y y
+'
+
 cat <<EOF | sort >expect_notes_ours
 68b8630d25516028bed862719855b3d6768d7833 $commit_sha15
 5de7ea7ad4f47e7ff91989fb82234634730f75df $commit_sha14
@@ -376,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" configuration option => Non-conflicting 3-way merge' '
+	git -c notes.mergeStrategy="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 "ignore" configuration option => Non-conflicting 3-way merge' '
+	git -c notes.mergeStrategy="ignore" 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
@@ -454,6 +483,28 @@ 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.mergeStrategy="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
+'
+
+test_expect_success 'merge z into y with "overwrite" strategy overriding configuration option "ours" => Non-conflicting 3-way merge' '
+	git -c notes.mergeStrategy="ours" notes merge --strategy=overwrite 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.280.g4aaba03

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH v8 8/8] notes: teach git-notes about notes.<ref>.mergeStrategy option
  2015-08-17  8:46 [PATCH v8 0/8] implement notes.mergeStrategy option Jacob Keller
                   ` (6 preceding siblings ...)
  2015-08-17  8:46 ` [PATCH v8 7/8] notes: add notes.mergeStrategy option to select default strategy Jacob Keller
@ 2015-08-17  8:46 ` Jacob Keller
  2015-08-17 13:21   ` Johan Herland
  7 siblings, 1 reply; 14+ messages in thread
From: Jacob Keller @ 2015-08-17  8:46 UTC (permalink / raw)
  To: git; +Cc: Johan Herland, Eric Sunshine, Jacob Keller

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

Add new option "notes.<ref>.mergeStrategy" option which specifies the merge
strategy for merging into a given notes ref. This option enables
selection of merge strategy for particular notes refs, rather than all
notes ref merges, as user may not want cat_sort_uniq for all refs, but
only some. Note that the <ref> is the local reference we are merging
into, not the remote ref we merged from. The assumption is that users
will mostly want to configure separate local ref merge strategies rather
than strategies depending on which remote ref they merge from. Also,
notes.<ref>.merge overrides the general behavior as it is more specific.

Signed-off-by: Jacob Keller <jacob.keller@gmail.com>
---
 Documentation/config.txt            |  7 +++++++
 Documentation/git-notes.txt         |  6 ++++++
 builtin/notes.c                     | 14 ++++++++++++-
 t/t3309-notes-merge-auto-resolve.sh | 39 +++++++++++++++++++++++++++++++++++++
 4 files changed, 65 insertions(+), 1 deletion(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 56e20446f587..a48c111d3ce0 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1926,6 +1926,13 @@ notes.mergeStrategy::
 	STRATEGIES" section of linkgit:git-notes[1] for more information
 	on each strategy.
 
+notes.<localref>.mergeStrategy::
+	Which merge strategy to choose if the local ref for a notes merge
+	matches <localref>, overriding "notes.mergeStrategy". <localref> must
+	be the short name of a ref under refs/notes/. See the "NOTES MERGE
+	STRATEGIES" section in linkgit:git-notes[1] for more information on the
+	available strategies.
+
 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 66b5017939c6..0167bc6e347b 100644
--- a/Documentation/git-notes.txt
+++ b/Documentation/git-notes.txt
@@ -328,6 +328,12 @@ notes.mergeStrategy::
 +
 This setting can be overridden by passing the `--strategy` option.
 
+notes.<localref>.mergeStrategy::
+	Which strategy to choose when merging into <localref>. The set of
+	allowed values is the same as "notes.mergeStrategy". <localref> must be
+	the short name of a ref under refs/notes/. See the "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 91f7a6547b0f..3608c6478528 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -817,7 +817,19 @@ static int merge(int argc, const char **argv, const char *prefix)
 			usage_with_options(git_notes_merge_usage, options);
 		}
 	} else {
-		git_config_get_notes_strategy("notes.mergeStrategy", &o.strategy);
+		struct strbuf merge_key = STRBUF_INIT;
+		const char *short_ref = NULL;
+
+		if (!skip_prefix(o.local_ref, "refs/notes/", &short_ref))
+			die("BUG: local ref %s is outside of refs/notes/",
+			    o.local_ref);
+
+		strbuf_addf(&merge_key, "notes.%s.mergeStrategy", short_ref);
+
+		if (git_config_get_notes_strategy(merge_key.buf, &o.strategy))
+			git_config_get_notes_strategy("notes.mergeStrategy", &o.strategy);
+
+		strbuf_release(&merge_key);
 	}
 
 	strbuf_addf(&msg, "notes: Merged notes from %s into %s",
diff --git a/t/t3309-notes-merge-auto-resolve.sh b/t/t3309-notes-merge-auto-resolve.sh
index 1bd71d947e60..44898ffba9fb 100755
--- a/t/t3309-notes-merge-auto-resolve.sh
+++ b/t/t3309-notes-merge-auto-resolve.sh
@@ -405,6 +405,17 @@ 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.mergeStrategy="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
@@ -589,6 +600,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.y.mergeStrategy="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.y.mergeStrategy="union" -c notes.mergeStrategy="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.z.mergeStrategy="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.280.g4aaba03

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [PATCH v8 4/8] notes: allow use of the "rewrite" terminology for merge strategies
  2015-08-17  8:46 ` [PATCH v8 4/8] notes: allow use of the "rewrite" terminology for merge strategies Jacob Keller
@ 2015-08-17 12:54   ` Johan Herland
  2015-08-17 18:28     ` Jacob Keller
  2015-08-17 19:35     ` Junio C Hamano
  0 siblings, 2 replies; 14+ messages in thread
From: Johan Herland @ 2015-08-17 12:54 UTC (permalink / raw)
  To: Jacob Keller
  Cc: Git mailing list, Eric Sunshine, Jacob Keller, Junio C Hamano

On Mon, Aug 17, 2015 at 10:46 AM, Jacob Keller <jacob.e.keller@intel.com> wrote:
> From: Jacob Keller <jacob.keller@gmail.com>
>
> notes-merge.c already re-uses the same functions for the automatic merge
> strategies used by the rewrite functionality. Teach the -s/--strategy
> option how to interpret the equivalent rewrite terminology for
> consistency.

I'm somewhat negative to this patch. IMHO, adding the rewrite modes as
merge strategy synonyms adds no benefit - only potential confusion -
to the existing merge strategies. Words that have a sensible meaning
in the context of rewrite, do not necessarily have the same sensible
meaning in the context of merge (and vice versa). I'd rather have the
rewrite code map ignore/overwrite/concatenate to ours/theirs/union,
without teaching the notes-merge code about these words. Or maybe even
drop this patch (and the next?) entirely, and let the future author
(who implements notes rewrite in terms of notes merge) decide how to
deal with this? By committing to these synonyms now, you might
actually be making things harder for the future author: once the
synonyms are part of the user-visible and documented interface, they
cannot easily be removed/changed again.

...Johan

> Add tests for the new synonyms.
>
> Teaching rewrite how to understand merge terminology is left for a
> following patch.
>
> Signed-off-by: Jacob Keller <jacob.keller@gmail.com>



-- 
Johan Herland, <johan@herland.net>
www.herland.net

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v8 8/8] notes: teach git-notes about notes.<ref>.mergeStrategy option
  2015-08-17  8:46 ` [PATCH v8 8/8] notes: teach git-notes about notes.<ref>.mergeStrategy option Jacob Keller
@ 2015-08-17 13:21   ` Johan Herland
  2015-08-17 18:25     ` Jacob Keller
  0 siblings, 1 reply; 14+ messages in thread
From: Johan Herland @ 2015-08-17 13:21 UTC (permalink / raw)
  To: Jacob Keller; +Cc: Git mailing list, Eric Sunshine, Jacob Keller

Allow me to suggest a different wording, somewhat inspired by the
branch.<name>.* documentation...

On Mon, Aug 17, 2015 at 10:46 AM, Jacob Keller <jacob.e.keller@intel.com> wrote:
> From: Jacob Keller <jacob.keller@gmail.com>
>
> Add new option "notes.<ref>.mergeStrategy" option which specifies the merge
> strategy for merging into a given notes ref.

Add new "notes.<name>.mergeStrategy" config, which specifies the merge
strategy for notes merges into refs/notes/<name>.

> This option enables
> selection of merge strategy for particular notes refs, rather than all
> notes ref merges, as user may not want cat_sort_uniq for all refs, but
> only some. Note that the <ref> is the local reference we are merging

s/<ref>/<name>/

> into, not the remote ref we merged from. The assumption is that users
> will mostly want to configure separate local ref merge strategies rather
> than strategies depending on which remote ref they merge from. Also,
> notes.<ref>.merge overrides the general behavior as it is more specific.

same here

>
> Signed-off-by: Jacob Keller <jacob.keller@gmail.com>
> ---
>  Documentation/config.txt            |  7 +++++++
>  Documentation/git-notes.txt         |  6 ++++++
>  builtin/notes.c                     | 14 ++++++++++++-
>  t/t3309-notes-merge-auto-resolve.sh | 39 +++++++++++++++++++++++++++++++++++++
>  4 files changed, 65 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index 56e20446f587..a48c111d3ce0 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -1926,6 +1926,13 @@ notes.mergeStrategy::
>         STRATEGIES" section of linkgit:git-notes[1] for more information
>         on each strategy.
>
> +notes.<localref>.mergeStrategy::
> +       Which merge strategy to choose if the local ref for a notes merge
> +       matches <localref>, overriding "notes.mergeStrategy". <localref> must
> +       be the short name of a ref under refs/notes/.

notes.<name>.mergeStrategy::
        Which merge strategy to use when doing a notes merge into
        refs/notes/<name>. This overrides the more general
"notes.mergeStrategy".

Otherwise, the series (except possibly #4/#5, see separate discussion)
looks good to me.


...Johan



-- 
Johan Herland, <johan@herland.net>
www.herland.net

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v8 8/8] notes: teach git-notes about notes.<ref>.mergeStrategy option
  2015-08-17 13:21   ` Johan Herland
@ 2015-08-17 18:25     ` Jacob Keller
  0 siblings, 0 replies; 14+ messages in thread
From: Jacob Keller @ 2015-08-17 18:25 UTC (permalink / raw)
  To: Johan Herland; +Cc: Jacob Keller, Git mailing list, Eric Sunshine

Hi,

On Mon, Aug 17, 2015 at 6:21 AM, Johan Herland <johan@herland.net> wrote:
> Allow me to suggest a different wording, somewhat inspired by the
> branch.<name>.* documentation...
>
> On Mon, Aug 17, 2015 at 10:46 AM, Jacob Keller <jacob.e.keller@intel.com> wrote:
>> From: Jacob Keller <jacob.keller@gmail.com>
>>
>> Add new option "notes.<ref>.mergeStrategy" option which specifies the merge
>> strategy for merging into a given notes ref.
>
> Add new "notes.<name>.mergeStrategy" config, which specifies the merge
> strategy for notes merges into refs/notes/<name>.
>
>> This option enables
>> selection of merge strategy for particular notes refs, rather than all
>> notes ref merges, as user may not want cat_sort_uniq for all refs, but
>> only some. Note that the <ref> is the local reference we are merging
>
> s/<ref>/<name>/
>
>> into, not the remote ref we merged from. The assumption is that users
>> will mostly want to configure separate local ref merge strategies rather
>> than strategies depending on which remote ref they merge from. Also,
>> notes.<ref>.merge overrides the general behavior as it is more specific.
>
> same here

Agreed, will fix in the next respin.

Regards,
Jake

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v8 4/8] notes: allow use of the "rewrite" terminology for merge strategies
  2015-08-17 12:54   ` Johan Herland
@ 2015-08-17 18:28     ` Jacob Keller
  2015-08-17 19:35     ` Junio C Hamano
  1 sibling, 0 replies; 14+ messages in thread
From: Jacob Keller @ 2015-08-17 18:28 UTC (permalink / raw)
  To: Johan Herland
  Cc: Jacob Keller, Git mailing list, Eric Sunshine, Junio C Hamano

Hi,

On Mon, Aug 17, 2015 at 5:54 AM, Johan Herland <johan@herland.net> wrote:
> On Mon, Aug 17, 2015 at 10:46 AM, Jacob Keller <jacob.e.keller@intel.com> wrote:
>> From: Jacob Keller <jacob.keller@gmail.com>
>>
>> notes-merge.c already re-uses the same functions for the automatic merge
>> strategies used by the rewrite functionality. Teach the -s/--strategy
>> option how to interpret the equivalent rewrite terminology for
>> consistency.
>
> I'm somewhat negative to this patch. IMHO, adding the rewrite modes as
> merge strategy synonyms adds no benefit - only potential confusion -
> to the existing merge strategies. Words that have a sensible meaning
> in the context of rewrite, do not necessarily have the same sensible
> meaning in the context of merge (and vice versa). I'd rather have the
> rewrite code map ignore/overwrite/concatenate to ours/theirs/union,
> without teaching the notes-merge code about these words. Or maybe even
> drop this patch (and the next?) entirely, and let the future author
> (who implements notes rewrite in terms of notes merge) decide how to
> deal with this? By committing to these synonyms now, you might
> actually be making things harder for the future author: once the
> synonyms are part of the user-visible and documented interface, they
> cannot easily be removed/changed again.
>
> ...Johan
>

I am ok dropping these, I really only added them after Junio brought
it up. I think that documenting union/concatenate is fine, but I think
that ours/theirs is very confusing. However, I think you're right that
a future author can deal with this when working on rewrite -> merge. I
don't think we need to do this now.

I'll drop these patches, but I will leave the
parse_notes_merge_strategy in it's location here.

We could document concatenate as a synonym of union but I don't think
that is a big deal.

Regards,
Jake

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v8 4/8] notes: allow use of the "rewrite" terminology for merge strategies
  2015-08-17 12:54   ` Johan Herland
  2015-08-17 18:28     ` Jacob Keller
@ 2015-08-17 19:35     ` Junio C Hamano
  1 sibling, 0 replies; 14+ messages in thread
From: Junio C Hamano @ 2015-08-17 19:35 UTC (permalink / raw)
  To: Johan Herland; +Cc: Jacob Keller, Git mailing list, Eric Sunshine, Jacob Keller

Johan Herland <johan@herland.net> writes:

> On Mon, Aug 17, 2015 at 10:46 AM, Jacob Keller <jacob.e.keller@intel.com> wrote:
>> From: Jacob Keller <jacob.keller@gmail.com>
>>
>> notes-merge.c already re-uses the same functions for the automatic merge
>> strategies used by the rewrite functionality. Teach the -s/--strategy
>> option how to interpret the equivalent rewrite terminology for
>> consistency.
>
> I'm somewhat negative to this patch. IMHO, adding the rewrite modes as
> merge strategy synonyms adds no benefit - only potential confusion -
> to the existing merge strategies.  ...
> ... By committing to these synonyms now, you might
> actually be making things harder for the future author: once the
> synonyms are part of the user-visible and documented interface, they
> cannot easily be removed/changed again.

OK.  Thanks.

^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2015-08-17 19:35 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-08-17  8:46 [PATCH v8 0/8] implement notes.mergeStrategy option Jacob Keller
2015-08-17  8:46 ` [PATCH v8 1/8] notes: document cat_sort_uniq rewriteMode Jacob Keller
2015-08-17  8:46 ` [PATCH v8 2/8] notes: extract enum notes_merge_strategy to notes-utils.h Jacob Keller
2015-08-17  8:46 ` [PATCH v8 3/8] note: extract parse_notes_merge_strategy to notes-utils Jacob Keller
2015-08-17  8:46 ` [PATCH v8 4/8] notes: allow use of the "rewrite" terminology for merge strategies Jacob Keller
2015-08-17 12:54   ` Johan Herland
2015-08-17 18:28     ` Jacob Keller
2015-08-17 19:35     ` Junio C Hamano
2015-08-17  8:46 ` [PATCH v8 5/8] notes: implement parse_combine_rewrite_fn using parse_notes_merge_strategy Jacob Keller
2015-08-17  8:46 ` [PATCH v8 6/8] notes: add tests for --commit/--abort/--strategy exclusivity Jacob Keller
2015-08-17  8:46 ` [PATCH v8 7/8] notes: add notes.mergeStrategy option to select default strategy Jacob Keller
2015-08-17  8:46 ` [PATCH v8 8/8] notes: teach git-notes about notes.<ref>.mergeStrategy option Jacob Keller
2015-08-17 13:21   ` Johan Herland
2015-08-17 18:25     ` Jacob Keller

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).