Re: [PATCH v4] Add default merge options for all branches

[Michael Grubb <devel@dailyvoid.com>]

Well, it certainly serves *my* immediate needs and addresses the specific use case that I was originally working on.  Though I think that what we've come up with would benefit the codebase in general if for no other reason than it lays some ground work for future features and is a bit more generic.  I also wouldn't mind going a step further and working on the globbing feature in a separate series.

Here are some corrections to the previous patch:

This patch makes the branch specific options more flexible by laying
the ground work for supporting globs for the branch names.
At this time only the value '*' is supported which will set default
values for all branches.  In the future this may be expanded to support
true globbing.

Signed-off-by: Michael Grubb <devel@dailyvoid.com>
---
 builtin/merge.c  |  120 ++++++++++++++++++++++++++++++++++-------------------
 t/t7600-merge.sh |   27 ++++++++----
 2 files changed, 95 insertions(+), 52 deletions(-)

diff --git a/builtin/merge.c b/builtin/merge.c
index fa56205..9e6ec5f 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -31,20 +31,12 @@
 #define DEFAULT_OCTOPUS (1<<1)
 #define NO_FAST_FORWARD (1<<2)
 #define NO_TRIVIAL      (1<<3)
+#define MERGE_OPTIONS_INIT {NULL, NULL, NULL}
 
-/* This is for branch.<foo>. blocks
- * the vote member holds a value between
- * 0.0 and 1.0 which measures how closely
- * a branch name matches the key member.
- * where branch.*.mergeoptions would be 0.1 and
- * branch.<name>.mergeoptions would be 1.0
- * Also it is called vote because I couldn't come
- * up with a better name.
- */
 struct merge_options_cb {
-	char *key;
-	char *value;
-	float vote;
+	char *option;
+	char *spec;
+	char *name;
 };
 
 struct strategy {
@@ -518,30 +510,11 @@ cleanup:
 	strbuf_release(&bname);
 }
 
-static void parse_git_merge_options(const char *k, const char *v,
-			void *cb)
+static void free_merge_options_cb(struct merge_options_cb *data)
 {
-	struct merge_options_cb *merge_options = cb;
-	int changed = 0;
-
-	/* We only handle mergeoptions for now */
-	if (suffixcmp(k, ".mergeoptions"))
-		return;
-
-	if (!prefixcmp(k, "branch.*") && merge_options->vote <= 0.1 ) {
-		merge_options->vote = 0.1;
-		changed = 1;
-	} else if (branch && !prefixcmp(k, "branch.") &&
-				!prefixcmp(k + 7, branch) &&
-				merge_options->vote < 1.0) {
-		merge_options->vote = 1.0;
-		changed = 1;
-	}
-
-	if (changed) {
-		merge_options->key = (char *)k;
-		merge_options->value = (char *)v;
-	}
+	free(data->option);
+	free(data->spec);
+	free(data->name);
 }
 
 static void apply_merge_options(struct merge_options_cb *opts)
@@ -550,26 +523,85 @@ static void apply_merge_options(struct merge_options_cb *opts)
 	int argc;
 	char *buf;
 
-	if ( opts == NULL )
+	if (!opts)
 		return;
 
-	buf = xstrdup(opts->value);
+	buf = xstrdup(opts->option);
 	argc = split_cmdline(buf, &argv);
 	if (argc < 0)
-		die(_("Bad %s string: %s"),
-			opts->key, split_cmdline_strerror(argc));
+		die(_("Bad branch.%s.%s string: %s"),
+			opts->spec, opts->name, split_cmdline_strerror(argc));
 
 	argv = xrealloc(argv, sizeof(*argv) * (argc + 2));
 	memmove(argv + 1, argv, sizeof(*argv) * (argc + 1));
 	argc++;
 	parse_options(argc, argv, NULL, builtin_merge_options,
-			  builtin_merge_usage, 0);
+		builtin_merge_usage, 0);
 	free(buf);
 }
 
+/*
+ * This function returns -1 if the first argument is less specific
+ * than the second, and 1 vice versa. Returns -1 if both arguments
+ * are the same.
+ */
+static int cmp_specificity(const char *a, const char *b)
+{
+	switch((!strcmp(a, "*") ? 2 : 0) |
+		(!strcmp(b, "*") ? 1 : 0)) {
+	case 3:
+	case 0:
+		return -1; /* later one wins if they are the same */
+	case 1:
+		return 1;
+	case 2:
+		return -1;
+	}
+	return 0; /* should never happen */
+}
+
+static void parse_git_merge_options(const char *k, const char *v,
+			void *cb)
+{
+	struct merge_options_cb *merge_options = cb;
+	char *spec, *eon; /* end-of-name */
+
+	k += 7; /* not interested in the leading "branch." */
+	eon = strrchr(k, '.');
+	if (!eon || strcmp(eon, ".mergeoptions"))
+		return;
+
+	/* k through eon is name or glob */
+	spec = xmemdupz(k, eon - k);
+	/*
+	 * NEEDSWORK: for now we say "*" matches; we would need
+	 * to turn the following into something like:
+	 *	if (has_wildcard(spec)
+	 *		? !glob_matches(spec, branch)
+	 *		: strcmp(spec, branch)) {
+	 *		free(spec);
+	 *		return;
+	 *	}
+	 */
+	if (strcmp(spec, "*") && strcmp(spec, branch)) {
+		free(spec);
+		return;
+	}
+
+	if (!merge_options->option ||
+			cmp_specificity(merge_options->spec, spec) < 0) {
+		free_merge_options_cb(merge_options);
+		merge_options->option = xstrdup(v);
+		merge_options->name = xstrdup(eon + 1);
+		merge_options->spec = spec;
+		return;
+	}
+	free(spec);
+}
+
 static int git_merge_config(const char *k, const char *v, void *cb)
 {
-	if (cb != NULL && branch && !prefixcmp(k, "branch."))
+	if (branch && !prefixcmp(k, "branch."))
 		parse_git_merge_options(k, v, cb);
 
 	if (!strcmp(k, "merge.diffstat") || !strcmp(k, "merge.stat"))
@@ -1034,7 +1066,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 	const char *head_arg;
 	int flag, head_invalid = 0, i;
 	int best_cnt = -1, merge_was_ok = 0, automerge_was_ok = 0;
-	struct merge_options_cb merge_options = {NULL, NULL, 0.0};
+	struct merge_options_cb merge_options = MERGE_OPTIONS_INIT;
 	struct commit_list *common = NULL;
 	const char *best_strategy = NULL, *wt_strategy = NULL;
 	struct commit_list **remotes = &remoteheads;
@@ -1053,8 +1085,10 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 		head_invalid = 1;
 
 	git_config(git_merge_config, &merge_options);
-	if (merge_options.key != NULL && merge_options.value != NULL)
+	if (merge_options.option) {
 		apply_merge_options(&merge_options);
+		free_merge_options_cb(&merge_options);
+	}
 
 	/* for color.ui */
 	if (diff_use_color_default == -1)
diff --git a/t/t7600-merge.sh b/t/t7600-merge.sh
index 5b1f8e1..d7a60b2 100755
--- a/t/t7600-merge.sh
+++ b/t/t7600-merge.sh
@@ -417,37 +417,46 @@ test_debug 'git log --graph --decorate --oneline --all'
 
 test_expect_success 'merge c0 with c1 (default no-ff)' '
 	git reset --hard c0 &&
-	test_might_fail git config --unset branch.master.mergeoptions &&
-	git config "branch.*.mergeoptions" "--no-ff" &&
+	cat >.git/config <<-\EOF &&
+	[branch "*"]
+		mergeoptions = --no-ff
+	EOF
 	test_tick &&
 	git merge c1 &&
 	git config --remove-section "branch.*" &&
 	verify_merge file result.1 &&
 	verify_parents $c0 $c1
 '
-
 test_debug 'git log --graph --decorate --oneline --all'
 
 test_expect_success 'combine branch.*.mergeoptions with branch.x.mergeoptions' '
 	git reset --hard c0 &&
-	test_might_fail git config --remove-section branch.master &&
-	git config "branch.*.mergeoptions" "--no-ff" &&
-	git config branch.master.mergeoptions "--ff" &&
+	cat >.git/config <<-\EOF &&
+	[branch "*"]
+		mergeoptions = --no-ff
+	[branch "master"]
+		mergeoptions = --ff
+	EOF
 	test_tick &&
 	git merge c1 &&
 	git config --remove-section "branch.*" &&
+	git config --remove-section "branch.master" &&
 	verify_merge file result.1 &&
 	verify_parents "$c0"
 '
 
 test_expect_success 'reverse branch.x.mergeoptions with branch.*.mergeoptions' '
 	git reset --hard c0 &&
-	test_might_fail git config --remove-section branch.master &&
-	git config branch.master.mergeoptions "--ff" &&
-	git config "branch.*.mergeoptions" "--no-ff" &&
+	cat >.git/config <<-\EOF &&
+	[branch "master"]
+		mergeoptions = --ff
+	[branch "*"]
+		mergeoptions = --no-ff
+	EOF
 	test_tick &&
 	git merge c1 &&
 	git config --remove-section "branch.*" &&
+	git config --remove-section "branch.master" &&
 	verify_merge file result.1 &&
 	verify_parents "$c0"
 '
-- 
1.7.5



On 5/3/11 11:58 PM, Jonathan Nieder wrote:
> Michael Grubb wrote:
> 
>> I take this to mean that my patch is no longer needed/wanted?
> 
> No, as usual it means that Junio was thinking and sent us his
> thoughts.
> 
> Do you like it?  If so, the way to move it forward would be to add
> tests and write a commit log message.  If not, the way to move things
> forward would be to explain what use case it misses and work on a
> better patch that takes care of them.
> 
> Hope that helps,
> Jonathan
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>