[PATCH 0/2] add notes.merge configuration variable

[PATCH 0/2] add notes.merge configuration variable

[Jacob Keller]

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

This small series is a precursor to some thoughts I have for enabling
easier notes collaboration. First, make it so that users can configure
the default notes merge strategy. Mostly useful if you always want to
use cat_sort_uniq.

I also found that the rewriteMode supports cat_sort_uniq but does not
document this fact.

Jacob Keller (2):
  notes: document cat_sort_uniq rewriteMode
  notes: add notes.merge option to select default strategy

 Documentation/config.txt              | 11 ++++-
 Documentation/git-notes.txt           | 33 +++++++++------
 builtin/notes.c                       | 75 +++++++++++++++++++++++++----------
 notes-merge.h                         | 16 ++++----
 t/t3309-notes-merge-auto-resolve.sh   | 29 ++++++++++++++
 t/t3310-notes-merge-manual-resolve.sh | 12 ++++++
 6 files changed, 133 insertions(+), 43 deletions(-)

-- 
2.5.0.482.gfcd5645

[PATCH 1/2] 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>
---
 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 6562c6ab09b9..3c1e4df09beb 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1965,8 +1965,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 2/2] 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 the configuration option. Ensure that command line
--strategy option overrides the configured value. Ensure that -s can't
be passed with --commit or --abort. Ensure that --abort and --commit
can't be used together.

Signed-off-by: Jacob Keller <jacob.keller@gmail.com>
---
 Documentation/config.txt              |  7 ++++
 Documentation/git-notes.txt           | 30 +++++++++-----
 builtin/notes.c                       | 75 +++++++++++++++++++++++++----------
 notes-merge.h                         | 16 ++++----
 t/t3309-notes-merge-auto-resolve.sh   | 29 ++++++++++++++
 t/t3310-notes-merge-manual-resolve.sh | 12 ++++++
 6 files changed, 129 insertions(+), 40 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 3c1e4df09beb..85c15126e4ce 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1937,6 +1937,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..d8944f5aec60 100644
--- a/Documentation/git-notes.txt
+++ b/Documentation/git-notes.txt
@@ -101,13 +101,13 @@ 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,
-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.
-When done, the user can either finalize the merge with
-'git notes merge --commit', or abort the merge with
-'git notes merge --abort'.
+conflicting notes (see the -s/--strategy option or notes.merge
+config option) 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. When done, the user
+can either finalize the merge with 'git notes merge --commit',
+or abort the merge with 'git notes merge --abort'.
 
 remove::
 	Remove the notes for given objects (defaults to HEAD). When
@@ -181,10 +181,10 @@ OPTIONS
 -s <strategy>::
 --strategy=<strategy>::
 	When merging notes, resolve notes conflicts using the given
-	strategy. The following strategies are recognized: "manual"
-	(default), "ours", "theirs", "union" and "cat_sort_uniq".
-	See the "NOTES MERGE STRATEGIES" section below for more
-	information on each notes merge strategy.
+	strategy. Overrides "notes.merge". The following strategies are
+	recognized: `manual`, `ours`, `theirs`, `union` and
+	`cat_sort_uniq`. See the `NOTES MERGE STRATEGIES` section below
+	for more information on each notes merge strategy.
 
 --commit::
 	Finalize an in-progress 'git notes merge'. Use this option
@@ -310,6 +310,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 88fdafb32bc0..7123311b563e 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -93,6 +93,9 @@ static const char * const git_notes_get_ref_usage[] = {
 static const char note_template[] =
 	"\nWrite/edit the notes for the following object:\n";
 
+static int override_strategy = 0;
+static enum notes_merge_strategy strategy;
+
 struct note_data {
 	int given;
 	int use_editor;
@@ -741,6 +744,36 @@ static int merge_commit(struct notes_merge_options *o)
 	return ret;
 }
 
+static int parse_notes_strategy(const char *var, 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 error("Unknown notes merge strategy: %s", arg);
+	}
+
+	return 0;
+}
+
+static int parse_opt_strategy(const struct option *opt, const char *arg, int unset)
+{
+	enum notes_merge_strategy *strategy = opt->value;
+
+	/* let merge() know we overrode the configured strategy */
+	override_strategy = 1;
+
+	return parse_notes_strategy(NULL, arg, strategy);
+}
+
 static int merge(int argc, const char **argv, const char *prefix)
 {
 	struct strbuf remote_ref = STRBUF_INIT, msg = STRBUF_INIT;
@@ -749,14 +782,16 @@ static int merge(int argc, const char **argv, const char *prefix)
 	struct notes_merge_options o;
 	int do_merge = 0, do_commit = 0, do_abort = 0;
 	int verbosity = 0, result;
-	const char *strategy = NULL;
 	struct option options[] = {
 		OPT_GROUP(N_("General options")),
 		OPT__VERBOSITY(&verbosity),
 		OPT_GROUP(N_("Merge options")),
-		OPT_STRING('s', "strategy", &strategy, N_("strategy"),
-			   N_("resolve notes conflicts using the given strategy "
-			      "(manual/ours/theirs/union/cat_sort_uniq)")),
+		{
+			OPTION_CALLBACK, 's', "strategy", &strategy, N_("strategy"),
+			N_("resolve notes conflicts using the given strategy "
+			   "(manual/ours/theirs/union/cat_sort_uniq)"),
+			PARSE_OPT_NONEG, parse_opt_strategy
+		},
 		OPT_GROUP(N_("Committing unmerged notes")),
 		{ OPTION_SET_INT, 0, "commit", &do_commit, NULL,
 			N_("finalize notes merge by committing unmerged notes"),
@@ -771,7 +806,7 @@ static int merge(int argc, const char **argv, const char *prefix)
 	argc = parse_options(argc, argv, prefix, options,
 			     git_notes_merge_usage, 0);
 
-	if (strategy || do_commit + do_abort == 0)
+	if (override_strategy || do_commit + do_abort == 0)
 		do_merge = 1;
 	if (do_merge + do_commit + do_abort != 1) {
 		error("cannot mix --commit, --abort or -s/--strategy");
@@ -799,22 +834,7 @@ 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);
-		}
-	}
+	o.strategy = strategy;
 
 	t = init_notes_check("merge", NOTES_INIT_WRITABLE);
 
@@ -950,6 +970,17 @@ 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);
+		return parse_notes_strategy(var, value, &strategy);
+	}
+
+	return git_default_config(var, value, cb);
+}
+
 int cmd_notes(int argc, const char **argv, const char *prefix)
 {
 	int result;
@@ -960,7 +991,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
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

Re: [PATCH 0/2] add notes.merge configuration variable

[Junio C Hamano]

CC'ing yourself is ok, but please do not forget to ask reviews from
people who have touched the area of the code you are touching (they
are hopefully the ones who are more knowledgeable and can help you).

Old timers like I know Johan is the go-to person on things around
notes, but new people can ask their copy of Git:

$ git shortlog -n -s --no-merges master builtin/notes.c notes.c | head -n 5
    46	Johan Herland
    11	Junio C Hamano
    11	Michael Haggerty
     5	Jeff King
     5	Nguyễn Thái Ngọc Duy

The changes to the code looked reasonable from a cursory read,
though.

Thanks.

Re: [PATCH 2/2] notes: add notes.merge option to select default strategy

[Eric Sunshine]

On Fri, Jul 31, 2015 at 3:21 PM, 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 the configuration option. Ensure that command line
> --strategy option overrides the configured value. Ensure that -s can't
> be passed with --commit or --abort. Ensure that --abort and --commit
> can't be used together.
>
> Signed-off-by: Jacob Keller <jacob.keller@gmail.com>
> ---
> diff --git a/builtin/notes.c b/builtin/notes.c
> index 88fdafb32bc0..7123311b563e 100644
> --- a/builtin/notes.c
> +++ b/builtin/notes.c
> @@ -741,6 +744,36 @@ static int merge_commit(struct notes_merge_options *o)
> +static int parse_notes_strategy(const char *var, const char *arg,
> +                               enum notes_merge_strategy *strategy)

What is the 'var' argument for? It's not used by the function.

> +{
> +       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 error("Unknown notes merge strategy: %s", arg);

Perhaps reporting of the error should be left to the callers so they
can tailor the message ("--strategy" vs. "notes.merge")? Doing so
would make it easier for the user to trace the source of a bogus
value.

> +       }
> +
> +       return 0;
> +}
> +
> +static int parse_opt_strategy(const struct option *opt, const char *arg, int unset)
> +{
> +       enum notes_merge_strategy *strategy = opt->value;
> +
> +       /* let merge() know we overrode the configured strategy */
> +       override_strategy = 1;
> +
> +       return parse_notes_strategy(NULL, arg, strategy);
> +}
> +
>  static int merge(int argc, const char **argv, const char *prefix)
>  {
>         struct strbuf remote_ref = STRBUF_INIT, msg = STRBUF_INIT;
> @@ -749,14 +782,16 @@ static int merge(int argc, const char **argv, const char *prefix)
> -       const char *strategy = NULL;
>         struct option options[] = {
>                 OPT_GROUP(N_("General options")),
>                 OPT__VERBOSITY(&verbosity),
>                 OPT_GROUP(N_("Merge options")),
> -               OPT_STRING('s', "strategy", &strategy, N_("strategy"),
> -                          N_("resolve notes conflicts using the given strategy "
> -                             "(manual/ours/theirs/union/cat_sort_uniq)")),
> +               {
> +                       OPTION_CALLBACK, 's', "strategy", &strategy, N_("strategy"),
> +                       N_("resolve notes conflicts using the given strategy "
> +                          "(manual/ours/theirs/union/cat_sort_uniq)"),
> +                       PARSE_OPT_NONEG, parse_opt_strategy
> +               },

Why change this from an OPT_STRING to an OPTION_CALLBACK? Couldn't you
just as easily have called parse_notes_strategy() after
parse_options()?

>                 OPT_GROUP(N_("Committing unmerged notes")),
>                 { OPTION_SET_INT, 0, "commit", &do_commit, NULL,
>                         N_("finalize notes merge by committing unmerged notes"),
> @@ -771,7 +806,7 @@ static int merge(int argc, const char **argv, const char *prefix)
>         argc = parse_options(argc, argv, prefix, options,
>                              git_notes_merge_usage, 0);
>
> -       if (strategy || do_commit + do_abort == 0)
> +       if (override_strategy || do_commit + do_abort == 0)
>                 do_merge = 1;
>         if (do_merge + do_commit + do_abort != 1) {
>                 error("cannot mix --commit, --abort or -s/--strategy");
> @@ -799,22 +834,7 @@ 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);
> -               }
> -       }
> +       o.strategy = strategy;