{"thread":{"id":"14567","subject":"[PATCH] builtin-merge: give a proper error message for invalid strategies in config","startedAt":"2008-07-21T16:10:47Z","lastAt":"2008-07-22T17:05:59Z","messageCount":6,"participants":["Miklos Vajna","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"84199","messageId":"1216656647-16897-1-git-send-email-vmiklos@frugalware.org","threadId":"14567","inReplyTo":null,"subject":"[PATCH] builtin-merge: give a proper error message for invalid strategies in config","fromName":"Miklos Vajna","fromEmail":"vmiklos@frugalware.org","sentAt":"2008-07-21T16:10:47Z","receivedAt":"2008-07-21T16:10:47Z","isPatch":true,"sender":{"key":"vmiklos@frugalware.org","avatar":"https://gravatar.com/avatar/401c1cbbb3a5d13e650c691a2c71d6fd0b80df1a01bc74d9f1972675dd58f2bd?d=mp&s=160"},"body":"Till now 'git merge -s foobar' bailed out with an error message, but\nfoobar in pull.twohead or pull.octopus was just silently ignored. It's\nbetter to inform the user then just silently doing nothing.\n\nSigned-off-by: Miklos Vajna <vmiklos@frugalware.org>\n---\n builtin-merge.c              |   35 +++++++++++++++--------------------\n t/t7601-merge-pull-config.sh |    6 ++++++\n 2 files changed, 21 insertions(+), 20 deletions(-)\n\ndiff --git a/builtin-merge.c b/builtin-merge.c\nindex e97c79e..5037acf 100644\n--- a/builtin-merge.c\n+++ b/builtin-merge.c\n@@ -93,28 +93,13 @@ static void append_strategy(struct strategy *s)\n \tuse_strategies[use_strategies_nr++] = s;\n }\n \n+static void add_strategies(const char *string, unsigned attr);\n+\n static int option_parse_strategy(const struct option *opt,\n \t\t\t\t const char *name, int unset)\n {\n-\tint i;\n-\tstruct strategy *s;\n-\n-\tif (unset)\n-\t\treturn 0;\n-\n-\ts = get_strategy(name);\n-\n-\tif (s)\n-\t\tappend_strategy(s);\n-\telse {\n-\t\tstruct strbuf err;\n-\t\tstrbuf_init(&err, 0);\n-\t\tfor (i = 0; i < ARRAY_SIZE(all_strategy); i++)\n-\t\t\tstrbuf_addf(&err, \" %s\", all_strategy[i].name);\n-\t\tfprintf(stderr, \"Could not find merge strategy '%s'.\\n\", name);\n-\t\tfprintf(stderr, \"Available strategies are:%s.\\n\", err.buf);\n-\t\texit(1);\n-\t}\n+\tif (!unset)\n+\t\tadd_strategies(name, 0);\n \treturn 0;\n }\n \n@@ -639,7 +624,7 @@ static void split_merge_strategies(const char *string, struct strategy **list,\n static void add_strategies(const char *string, unsigned attr)\n {\n \tstruct strategy *list = NULL;\n-\tint list_alloc = 0, list_nr = 0, i;\n+\tint list_alloc = 0, list_nr = 0, i, j;\n \n \tmemset(&list, 0, sizeof(list));\n \tsplit_merge_strategies(string, &list, &list_nr, &list_alloc);\n@@ -650,6 +635,16 @@ static void add_strategies(const char *string, unsigned attr)\n \t\t\ts = get_strategy(list[i].name);\n \t\t\tif (s)\n \t\t\t\tappend_strategy(s);\n+\t\t\telse {\n+\t\t\t\tstruct strbuf err;\n+\n+\t\t\t\tstrbuf_init(&err, 0);\n+\t\t\t\tfor (j = 0; j < ARRAY_SIZE(all_strategy); j++)\n+\t\t\t\t\tstrbuf_addf(&err, \" %s\", all_strategy[j].name);\n+\t\t\t\tfprintf(stderr, \"Could not find merge strategy '%s'.\\n\", list[i].name);\n+\t\t\t\tfprintf(stderr, \"Available strategies are:%s.\\n\", err.buf);\n+\t\t\t\texit(1);\n+\t\t\t}\n \t\t}\n \t\treturn;\n \t}\ndiff --git a/t/t7601-merge-pull-config.sh b/t/t7601-merge-pull-config.sh\nindex 95b4d71..ca63451 100755\n--- a/t/t7601-merge-pull-config.sh\n+++ b/t/t7601-merge-pull-config.sh\n@@ -126,4 +126,10 @@ test_expect_success 'merge picks up the best result' '\n \ttest $auto_count != $resolve_count\n '\n \n+test_expect_success 'merge errors out on invalid strategy' '\n+\tgit config pull.twohead \"foobar\" &&\n+\tgit reset --hard c5 &&\n+\ttest_must_fail git merge c6\n+'\n+\n test_done\n-- \n1.5.6.4.433.g09651.dirty\n"},{"id":"84273","messageId":"7vr69mpl5v.fsf@gitster.siamese.dyndns.org","threadId":"14567","inReplyTo":"1216656647-16897-1-git-send-email-vmiklos@frugalware.org","subject":"Re: [PATCH] builtin-merge: give a proper error message for invalid strategies in config","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-07-22T05:01:16Z","receivedAt":"2008-07-22T05:01:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Miklos Vajna <vmiklos@frugalware.org> writes:\n\n> Till now 'git merge -s foobar' bailed out with an error message, but\n> foobar in pull.twohead or pull.octopus was just silently ignored. It's\n> better to inform the user then just silently doing nothing.\n>\n> Signed-off-by: Miklos Vajna <vmiklos@frugalware.org>\n\nDoesn't this make \"git merge -s 'recursive resolve'\" to misbehave?\n"},{"id":"84274","messageId":"7vljzupk6h.fsf@gitster.siamese.dyndns.org","threadId":"14567","inReplyTo":"7vr69mpl5v.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] builtin-merge: give a proper error message for invalid strategies in config","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-07-22T05:22:30Z","receivedAt":"2008-07-22T05:22:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Miklos Vajna <vmiklos@frugalware.org> writes:\n>\n>> Till now 'git merge -s foobar' bailed out with an error message, but\n>> foobar in pull.twohead or pull.octopus was just silently ignored. It's\n>> better to inform the user then just silently doing nothing.\n>>\n>> Signed-off-by: Miklos Vajna <vmiklos@frugalware.org>\n>\n> Doesn't this make \"git merge -s 'recursive resolve'\" to misbehave?\n\nPerhaps this would be a better replacement.  It makes get_strategy() to\nvalidate and barf on unknown one.  It is slightly smaller and more\ncontained.  If/when we add user-defined ones, that logic will also be\ncontained in the function.\n\n builtin-merge.c              |   37 ++++++++++++-------------------------\n t/t7601-merge-pull-config.sh |   12 ++++++++++++\n 2 files changed, 24 insertions(+), 25 deletions(-)\n\ndiff --git a/builtin-merge.c b/builtin-merge.c\nindex e97c79e..0fd7985 100644\n--- a/builtin-merge.c\n+++ b/builtin-merge.c\n@@ -77,6 +77,7 @@ static int option_parse_message(const struct option *opt,\n static struct strategy *get_strategy(const char *name)\n {\n \tint i;\n+\tstruct strbuf err;\n \n \tif (!name)\n \t\treturn NULL;\n@@ -84,7 +85,13 @@ static struct strategy *get_strategy(const char *name)\n \tfor (i = 0; i < ARRAY_SIZE(all_strategy); i++)\n \t\tif (!strcmp(name, all_strategy[i].name))\n \t\t\treturn &all_strategy[i];\n-\treturn NULL;\n+\n+\tstrbuf_init(&err, 0);\n+\tfor (i = 0; i < ARRAY_SIZE(all_strategy); i++)\n+\t\tstrbuf_addf(&err, \" %s\", all_strategy[i].name);\n+\tfprintf(stderr, \"Could not find merge strategy '%s'.\\n\", name);\n+\tfprintf(stderr, \"Available strategies are:%s.\\n\", err.buf);\n+\texit(1);\n }\n \n static void append_strategy(struct strategy *s)\n@@ -96,25 +103,10 @@ static void append_strategy(struct strategy *s)\n static int option_parse_strategy(const struct option *opt,\n \t\t\t\t const char *name, int unset)\n {\n-\tint i;\n-\tstruct strategy *s;\n-\n \tif (unset)\n \t\treturn 0;\n \n-\ts = get_strategy(name);\n-\n-\tif (s)\n-\t\tappend_strategy(s);\n-\telse {\n-\t\tstruct strbuf err;\n-\t\tstrbuf_init(&err, 0);\n-\t\tfor (i = 0; i < ARRAY_SIZE(all_strategy); i++)\n-\t\t\tstrbuf_addf(&err, \" %s\", all_strategy[i].name);\n-\t\tfprintf(stderr, \"Could not find merge strategy '%s'.\\n\", name);\n-\t\tfprintf(stderr, \"Available strategies are:%s.\\n\", err.buf);\n-\t\texit(1);\n-\t}\n+\tappend_strategy(get_strategy(name));\n \treturn 0;\n }\n \n@@ -643,14 +635,9 @@ static void add_strategies(const char *string, unsigned attr)\n \n \tmemset(&list, 0, sizeof(list));\n \tsplit_merge_strategies(string, &list, &list_nr, &list_alloc);\n-\tif (list != NULL) {\n-\t\tfor (i = 0; i < list_nr; i++) {\n-\t\t\tstruct strategy *s;\n-\n-\t\t\ts = get_strategy(list[i].name);\n-\t\t\tif (s)\n-\t\t\t\tappend_strategy(s);\n-\t\t}\n+\tif (list) {\n+\t\tfor (i = 0; i < list_nr; i++)\n+\t\t\tappend_strategy(get_strategy(list[i].name));\n \t\treturn;\n \t}\n \tfor (i = 0; i < ARRAY_SIZE(all_strategy); i++)\ndiff --git a/t/t7601-merge-pull-config.sh b/t/t7601-merge-pull-config.sh\nindex 95b4d71..6b9f638 100755\n--- a/t/t7601-merge-pull-config.sh\n+++ b/t/t7601-merge-pull-config.sh\n@@ -126,4 +126,16 @@ test_expect_success 'merge picks up the best result' '\n \ttest $auto_count != $resolve_count\n '\n \n+test_expect_success 'merge errors out on invalid strategy' '\n+\tgit config pull.twohead \"foobar\" &&\n+\tgit reset --hard c5 &&\n+\ttest_must_fail git merge c6\n+'\n+\n+test_expect_success 'merge errors out on invalid strategy' '\n+\tgit config --unset-all pull.twohead &&\n+\tgit reset --hard c5 &&\n+\ttest_must_fail git merge -s \"resolve recursive\" c6\n+'\n+\n test_done\n"},{"id":"84297","messageId":"20080722073912.GN32057@genesis.frugalware.org","threadId":"14567","inReplyTo":"7vr69mpl5v.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] builtin-merge: give a proper error message for invalid strategies in config","fromName":"Miklos Vajna","fromEmail":"vmiklos@frugalware.org","sentAt":"2008-07-22T07:39:12Z","receivedAt":"2008-07-22T07:39:12Z","isPatch":true,"sender":{"key":"vmiklos@frugalware.org","avatar":"https://gravatar.com/avatar/401c1cbbb3a5d13e650c691a2c71d6fd0b80df1a01bc74d9f1972675dd58f2bd?d=mp&s=160"},"body":"On Mon, Jul 21, 2008 at 10:01:16PM -0700, Junio C Hamano <gitster@pobox.com> wrote:\n> Doesn't this make \"git merge -s 'recursive resolve'\" to misbehave?\n\nDepends on what do we expect it to do. ;-)\n\nMy patch unified the handling of pull.twohead / -s option, so it\n(actually unintentionally) made -s accepting multiple strategies as a\nspace separated list. Given that we already accept multiple strategies\nas a space separated list in pull.twohead (and we do _not_ use multiple\npull.twohead entries) I think my patch is more logical.\n\nThough, there was already a thread about how should we specify multiple\nstrategies on the commandline; and you suggested in\n\n        http://article.gmane.org/gmane.comp.version-control.git/89208\n\nto use -s strategy1 -s strategy2.\n\nIn that case, I think your patch is better, and once it hits git.git, I\nwould like to send a patch that changes the config parsing as well, so\nthat pull.twohead \"foo bar\" would be invalid, and the user would have to\nhave two pull.twohead entries: one for foo and one for bar.\n\nDoes this sound reasonable?\n\nThanks.\n"},{"id":"84306","messageId":"7v7ibenx75.fsf@gitster.siamese.dyndns.org","threadId":"14567","inReplyTo":"20080722073912.GN32057@genesis.frugalware.org","subject":"Re: [PATCH] builtin-merge: give a proper error message for invalid strategies in config","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-07-22T08:24:14Z","receivedAt":"2008-07-22T08:24:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Miklos Vajna <vmiklos@frugalware.org> writes:\n\n> ... I\n> would like to send a patch that changes the config parsing as well, so\n> that pull.twohead \"foo bar\" would be invalid, and the user would have to\n> have two pull.twohead entries: one for foo and one for bar.\n\nDon't.  pull.* has always been defined as \"list of strategies\", and -s has\nalways been defined to take \"a\" strategy.\n\nIOW, you don't need to break anything further.\n"},{"id":"84339","messageId":"1216746359-21170-1-git-send-email-vmiklos@frugalware.org","threadId":"14567","inReplyTo":"7v7ibenx75.fsf@gitster.siamese.dyndns.org","subject":"[PATCH] t7601: extend the 'merge picks up the best result' test","fromName":"Miklos Vajna","fromEmail":"vmiklos@frugalware.org","sentAt":"2008-07-22T17:05:59Z","receivedAt":"2008-07-22T17:05:59Z","isPatch":true,"sender":{"key":"vmiklos@frugalware.org","avatar":"https://gravatar.com/avatar/401c1cbbb3a5d13e650c691a2c71d6fd0b80df1a01bc74d9f1972675dd58f2bd?d=mp&s=160"},"body":"The test only checked if the best result picking code works if there are\nmultiple strategies set in the config. Add a similar one that tests if\nthe same true if the -s option of git merge was used multiple times.\n\nSigned-off-by: Miklos Vajna <vmiklos@frugalware.org>\n---\n\nOn Tue, Jul 22, 2008 at 01:24:14AM -0700, Junio C Hamano <gitster@pobox.com> wrote:\n> Don't.  pull.* has always been defined as \"list of strategies\", and -s\n> has\n> always been defined to take \"a\" strategy.\n\nOK. Here is a testcase for the later. As far as I see the behaviour of\nmultiple -s was not checked till now.\n\n t/t7601-merge-pull-config.sh |   15 +++++++++++++++\n 1 files changed, 15 insertions(+), 0 deletions(-)\n\ndiff --git a/t/t7601-merge-pull-config.sh b/t/t7601-merge-pull-config.sh\nindex 6b9f638..55aa6b5 100755\n--- a/t/t7601-merge-pull-config.sh\n+++ b/t/t7601-merge-pull-config.sh\n@@ -112,6 +112,21 @@ test_expect_success 'setup conflicted merge' '\n # recusive is choosen.\n \n test_expect_success 'merge picks up the best result' '\n+\tgit config --unset-all pull.twohead &&\n+\tgit reset --hard c5 &&\n+\tgit merge -s resolve c6\n+\tresolve_count=$(conflict_count) &&\n+\tgit reset --hard c5 &&\n+\tgit merge -s recursive c6\n+\trecursive_count=$(conflict_count) &&\n+\tgit reset --hard c5 &&\n+\tgit merge -s recursive -s resolve c6\n+\tauto_count=$(conflict_count) &&\n+\ttest $auto_count = $recursive_count &&\n+\ttest $auto_count != $resolve_count\n+'\n+\n+test_expect_success 'merge picks up the best result (from config)' '\n \tgit config pull.twohead \"recursive resolve\" &&\n \tgit reset --hard c5 &&\n \tgit merge -s resolve c6\n-- \n1.5.6.4.433.g09651.dirty\n"}]}