threads / patch / 14567

patchbuiltin-merge: give a proper error message for invalid strategies in config

Subject: [PATCH] builtin-merge: give a proper error message for invalid strategies in config

## tl;dr

6 messages between Jul 21, 2008 and Jul 22, 2008. Diffs are folded; open one to read it.

replies: 5people: 2as markdown or json

Miklos Vajna· Jul 21, 2008, 16:10 UTC · lore

Till now 'git merge -s foobar' bailed out with an error message, but foobar in pull.twohead or pull.octopus was just silently ignored. It's better to inform the user then just silently doing nothing.

Signed-off-by: Miklos Vajna <vmiklos@frugalware.org>
---
 builtin-merge.c              |   35 +++++++++++++++--------------------
 t/t7601-merge-pull-config.sh |    6 ++++++
 2 files changed, 21 insertions(+), 20 deletions(-)
Show changes to 2 files +21 −20

builtin-merge.c, t/t7601-merge-pull-config.sh

diff --git a/builtin-merge.c b/builtin-merge.c
index e97c79e..5037acf 100644
--- a/builtin-merge.c
+++ b/builtin-merge.c
@@ -93,28 +93,13 @@ static void append_strategy(struct strategy *s)
 	use_strategies[use_strategies_nr++] = s;
 }
 
+static void add_strategies(const char *string, unsigned attr);
+
 static int option_parse_strategy(const struct option *opt,
 				 const char *name, int unset)
 {
-	int i;
-	struct strategy *s;
-
-	if (unset)
-		return 0;
-
-	s = get_strategy(name);
-
-	if (s)
-		append_strategy(s);
-	else {
-		struct strbuf err;
-		strbuf_init(&err, 0);
-		for (i = 0; i < ARRAY_SIZE(all_strategy); i++)
-			strbuf_addf(&err, " %s", all_strategy[i].name);
-		fprintf(stderr, "Could not find merge strategy '%s'.\n", name);
-		fprintf(stderr, "Available strategies are:%s.\n", err.buf);
-		exit(1);
-	}
+	if (!unset)
+		add_strategies(name, 0);
 	return 0;
 }
 
@@ -639,7 +624,7 @@ static void split_merge_strategies(const char *string, struct strategy **list,
 static void add_strategies(const char *string, unsigned attr)
 {
 	struct strategy *list = NULL;
-	int list_alloc = 0, list_nr = 0, i;
+	int list_alloc = 0, list_nr = 0, i, j;
 
 	memset(&list, 0, sizeof(list));
 	split_merge_strategies(string, &list, &list_nr, &list_alloc);
@@ -650,6 +635,16 @@ static void add_strategies(const char *string, unsigned attr)
 			s = get_strategy(list[i].name);
 			if (s)
 				append_strategy(s);
+			else {
+				struct strbuf err;
+
+				strbuf_init(&err, 0);
+				for (j = 0; j < ARRAY_SIZE(all_strategy); j++)
+					strbuf_addf(&err, " %s", all_strategy[j].name);
+				fprintf(stderr, "Could not find merge strategy '%s'.\n", list[i].name);
+				fprintf(stderr, "Available strategies are:%s.\n", err.buf);
+				exit(1);
+			}
 		}
 		return;
 	}
diff --git a/t/t7601-merge-pull-config.sh b/t/t7601-merge-pull-config.sh
index 95b4d71..ca63451 100755
--- a/t/t7601-merge-pull-config.sh
+++ b/t/t7601-merge-pull-config.sh
@@ -126,4 +126,10 @@ test_expect_success 'merge picks up the best result' '
 	test $auto_count != $resolve_count
 '
 
+test_expect_success 'merge errors out on invalid strategy' '
+	git config pull.twohead "foobar" &&
+	git reset --hard c5 &&
+	test_must_fail git merge c6
+'
+
 test_done

-- 
1.5.6.4.433.g09651.dirty
Junio C Hamano· Jul 22, 2008, 05:01 UTC · re: Miklos Vajna · lore

Re: [PATCH] builtin-merge: give a proper error message for invalid strategies in config

Miklos Vajna <vmiklos@frugalware.org> writes:
Show 5 quoted lines
> Till now 'git merge -s foobar' bailed out with an error message, but
> foobar in pull.twohead or pull.octopus was just silently ignored. It's
> better to inform the user then just silently doing nothing.
>
> Signed-off-by: Miklos Vajna <vmiklos@frugalware.org>
Doesn't this make "git merge -s 'recursive resolve'" to misbehave?
Junio C Hamano· Jul 22, 2008, 05:22 UTC · re: Junio C Hamano · lore

Re: [PATCH] builtin-merge: give a proper error message for invalid strategies in config

Junio C Hamano <gitster@pobox.com> writes:
Show 9 quoted lines
> Miklos Vajna <vmiklos@frugalware.org> writes:
>
>> Till now 'git merge -s foobar' bailed out with an error message, but
>> foobar in pull.twohead or pull.octopus was just silently ignored. It's
>> better to inform the user then just silently doing nothing.
>>
>> Signed-off-by: Miklos Vajna <vmiklos@frugalware.org>
>
> Doesn't this make "git merge -s 'recursive resolve'" to misbehave?

Perhaps this would be a better replacement. It makes get_strategy() to validate and barf on unknown one. It is slightly smaller and more contained. If/when we add user-defined ones, that logic will also be contained in the function.

 builtin-merge.c              |   37 ++++++++++++-------------------------
 t/t7601-merge-pull-config.sh |   12 ++++++++++++
 2 files changed, 24 insertions(+), 25 deletions(-)
Show changes to 2 files +24 −25

builtin-merge.c, t/t7601-merge-pull-config.sh

diff --git a/builtin-merge.c b/builtin-merge.c
index e97c79e..0fd7985 100644
--- a/builtin-merge.c
+++ b/builtin-merge.c
@@ -77,6 +77,7 @@ static int option_parse_message(const struct option *opt,
 static struct strategy *get_strategy(const char *name)
 {
 	int i;
+	struct strbuf err;
 
 	if (!name)
 		return NULL;
@@ -84,7 +85,13 @@ static struct strategy *get_strategy(const char *name)
 	for (i = 0; i < ARRAY_SIZE(all_strategy); i++)
 		if (!strcmp(name, all_strategy[i].name))
 			return &all_strategy[i];
-	return NULL;
+
+	strbuf_init(&err, 0);
+	for (i = 0; i < ARRAY_SIZE(all_strategy); i++)
+		strbuf_addf(&err, " %s", all_strategy[i].name);
+	fprintf(stderr, "Could not find merge strategy '%s'.\n", name);
+	fprintf(stderr, "Available strategies are:%s.\n", err.buf);
+	exit(1);
 }
 
 static void append_strategy(struct strategy *s)
@@ -96,25 +103,10 @@ static void append_strategy(struct strategy *s)
 static int option_parse_strategy(const struct option *opt,
 				 const char *name, int unset)
 {
-	int i;
-	struct strategy *s;
-
 	if (unset)
 		return 0;
 
-	s = get_strategy(name);
-
-	if (s)
-		append_strategy(s);
-	else {
-		struct strbuf err;
-		strbuf_init(&err, 0);
-		for (i = 0; i < ARRAY_SIZE(all_strategy); i++)
-			strbuf_addf(&err, " %s", all_strategy[i].name);
-		fprintf(stderr, "Could not find merge strategy '%s'.\n", name);
-		fprintf(stderr, "Available strategies are:%s.\n", err.buf);
-		exit(1);
-	}
+	append_strategy(get_strategy(name));
 	return 0;
 }
 
@@ -643,14 +635,9 @@ static void add_strategies(const char *string, unsigned attr)
 
 	memset(&list, 0, sizeof(list));
 	split_merge_strategies(string, &list, &list_nr, &list_alloc);
-	if (list != NULL) {
-		for (i = 0; i < list_nr; i++) {
-			struct strategy *s;
-
-			s = get_strategy(list[i].name);
-			if (s)
-				append_strategy(s);
-		}
+	if (list) {
+		for (i = 0; i < list_nr; i++)
+			append_strategy(get_strategy(list[i].name));
 		return;
 	}
 	for (i = 0; i < ARRAY_SIZE(all_strategy); i++)
diff --git a/t/t7601-merge-pull-config.sh b/t/t7601-merge-pull-config.sh
index 95b4d71..6b9f638 100755
--- a/t/t7601-merge-pull-config.sh
+++ b/t/t7601-merge-pull-config.sh
@@ -126,4 +126,16 @@ test_expect_success 'merge picks up the best result' '
 	test $auto_count != $resolve_count
 '
 
+test_expect_success 'merge errors out on invalid strategy' '
+	git config pull.twohead "foobar" &&
+	git reset --hard c5 &&
+	test_must_fail git merge c6
+'
+
+test_expect_success 'merge errors out on invalid strategy' '
+	git config --unset-all pull.twohead &&
+	git reset --hard c5 &&
+	test_must_fail git merge -s "resolve recursive" c6
+'
+
 test_done
Miklos Vajna· Jul 22, 2008, 07:39 UTC · re: Junio C Hamano · lore

Re: [PATCH] builtin-merge: give a proper error message for invalid strategies in config

On Mon, Jul 21, 2008 at 10:01:16PM -0700, Junio C Hamano <gitster@pobox.com> wrote:
> Doesn't this make "git merge -s 'recursive resolve'" to misbehave?
Depends on what do we expect it to do. ;-)

My patch unified the handling of pull.twohead / -s option, so it (actually unintentionally) made -s accepting multiple strategies as a space separated list. Given that we already accept multiple strategies as a space separated list in pull.twohead (and we do _not_ use multiple pull.twohead entries) I think my patch is more logical.

Though, there was already a thread about how should we specify multiple strategies on the commandline; and you suggested in

        http://article.gmane.org/gmane.comp.version-control.git/89208
to use -s strategy1 -s strategy2.

In that case, I think your patch is better, and once it hits git.git, I would like to send a patch that changes the config parsing as well, so that pull.twohead "foo bar" would be invalid, and the user would have to have two pull.twohead entries: one for foo and one for bar.

Does this sound reasonable?
Thanks.
Junio C Hamano· Jul 22, 2008, 08:24 UTC · re: Miklos Vajna · lore

Re: [PATCH] builtin-merge: give a proper error message for invalid strategies in config

Miklos Vajna <vmiklos@frugalware.org> writes:
> ... I
> would like to send a patch that changes the config parsing as well, so
> that pull.twohead "foo bar" would be invalid, and the user would have to
> have two pull.twohead entries: one for foo and one for bar.

Don't. pull.* has always been defined as "list of strategies", and -s has always been defined to take "a" strategy.

IOW, you don't need to break anything further.
Miklos Vajna· Jul 22, 2008, 17:05 UTC · re: Junio C Hamano · lore

[PATCH] t7601: extend the 'merge picks up the best result' test

The test only checked if the best result picking code works if there are multiple strategies set in the config. Add a similar one that tests if the same true if the -s option of git merge was used multiple times.

Signed-off-by: Miklos Vajna <vmiklos@frugalware.org>
---
On Tue, Jul 22, 2008 at 01:24:14AM -0700, Junio C Hamano <gitster@pobox.com> wrote:
> Don't.  pull.* has always been defined as "list of strategies", and -s
> has
> always been defined to take "a" strategy.

OK. Here is a testcase for the later. As far as I see the behaviour of multiple -s was not checked till now.

 t/t7601-merge-pull-config.sh |   15 +++++++++++++++
 1 files changed, 15 insertions(+), 0 deletions(-)
Show changes to t/t7601-merge-pull-config.sh +15 −0
diff --git a/t/t7601-merge-pull-config.sh b/t/t7601-merge-pull-config.sh
index 6b9f638..55aa6b5 100755
--- a/t/t7601-merge-pull-config.sh
+++ b/t/t7601-merge-pull-config.sh
@@ -112,6 +112,21 @@ test_expect_success 'setup conflicted merge' '
 # recusive is choosen.
 
 test_expect_success 'merge picks up the best result' '
+	git config --unset-all pull.twohead &&
+	git reset --hard c5 &&
+	git merge -s resolve c6
+	resolve_count=$(conflict_count) &&
+	git reset --hard c5 &&
+	git merge -s recursive c6
+	recursive_count=$(conflict_count) &&
+	git reset --hard c5 &&
+	git merge -s recursive -s resolve c6
+	auto_count=$(conflict_count) &&
+	test $auto_count = $recursive_count &&
+	test $auto_count != $resolve_count
+'
+
+test_expect_success 'merge picks up the best result (from config)' '
 	git config pull.twohead "recursive resolve" &&
 	git reset --hard c5 &&
 	git merge -s resolve c6

-- 
1.5.6.4.433.g09651.dirty

← back to recent threads