Re: [PATCH] builtin-merge: give a proper error message for invalid strategies in config
From: Junio C Hamano <hidden>
Date: 2016-06-15 22:44:59
Subsystem:
the rest · Maintainer:
Linus Torvalds
Possibly related (same subject, not in this thread)
- 2016-06-15 · [PATCH] builtin-merge: give a proper error message for invalid strategies in config · Miklos Vajna <hidden>
- 2016-06-15 · Re: [PATCH] builtin-merge: give a proper error message for invalid strategies in config · Junio C Hamano <hidden>
- 2016-06-15 · Re: [PATCH] builtin-merge: give a proper error message for invalid strategies in config · Miklos Vajna <hidden>
- 2016-06-15 · Re: [PATCH] builtin-merge: give a proper error message for invalid strategies in config · Junio C Hamano <hidden>
Junio C Hamano [off-list ref] writes:
Miklos Vajna [off-list ref] writes:quoted
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 <redacted>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(-)
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