{"thread":{"id":"35030","subject":"[PATCH] merge-recursive: fix parsing of \"diff-algorithm\" option","startedAt":"2013-09-26T20:02:48Z","lastAt":"2013-09-26T21:13:31Z","messageCount":3,"participants":["John Keeping","Jonathan Nieder"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"228286","messageId":"689bf88b6f1d33e123cc786042cc6dba23464351.1380225743.git.john@keeping.me.uk","threadId":"35030","inReplyTo":null,"subject":"[PATCH] merge-recursive: fix parsing of \"diff-algorithm\" option","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2013-09-26T20:02:48Z","receivedAt":"2013-09-26T20:02:48Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"The \"diff-algorithm\" option to the recursive merge strategy takes the\nname of the algorithm as an option, but it uses strcmp on the option\nstring to check if it starts with \"diff-algorithm=\", meaning that this\noptions cannot actually be used.\n\nFix this by switching to prefixcmp. At the same time, clarify the\nfollowing line by using strlen instead of a hard-coded length, which\nalso makes it consistent with nearby code.\n\nReported-by: Luke Noel-Storr \nSigned-off-by: John Keeping \n---\n merge-recursive.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/merge-recursive.c b/merge-recursive.c\nindex 40eb840..dbb7104 100644\n--- a/merge-recursive.c\n+++ b/merge-recursive.c\n@@ -2069,8 +2069,8 @@ int parse_merge_opt(struct merge_options *o, const char *s)\n \t\to->xdl_opts = DIFF_WITH_ALG(o, PATIENCE_DIFF);\n \telse if (!strcmp(s, \"histogram\"))\n \t\to->xdl_opts = DIFF_WITH_ALG(o, HISTOGRAM_DIFF);\n-\telse if (!strcmp(s, \"diff-algorithm=\")) {\n-\t\tlong value = parse_algorithm_value(s+15);\n+\telse if (!prefixcmp(s, \"diff-algorithm=\")) {\n+\t\tlong value = parse_algorithm_value(s + strlen(\"diff-algorithm=\"));\n \t\tif (value < 0)\n \t\t\treturn -1;\n \t\t/* clear out previous settings */\n-- \n1.8.4.566.g73d370b\n"},{"id":"228291","messageId":"20130926204720.GH9464@google.com","threadId":"35030","inReplyTo":"689bf88b6f1d33e123cc786042cc6dba23464351.1380225743.git.john@keeping.me.uk","subject":"Re: [PATCH] merge-recursive: fix parsing of \"diff-algorithm\" option","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-09-26T20:47:20Z","receivedAt":"2013-09-26T20:47:20Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"John Keeping wrote:\n\n> The \"diff-algorithm\" option to the recursive merge strategy takes the\n> name of the algorithm as an option, but it uses strcmp on the option\n> string to check if it starts with \"diff-algorithm=\", meaning that this\n> options cannot actually be used.\n>\n> Fix this by switching to prefixcmp. At the same time, clarify the\n> following line by using strlen instead of a hard-coded length, which\n> also makes it consistent with nearby code.\n>\n> Reported-by: Luke Noel-Storr \n> Signed-off-by: John Keeping \n\nThanks, both.\n\n[...]\n> --- a/merge-recursive.c\n> +++ b/merge-recursive.c\n> @@ -2069,8 +2069,8 @@ int parse_merge_opt(struct merge_options *o, const char *s)\n> \t\to->xdl_opts = DIFF_WITH_ALG(o, PATIENCE_DIFF);\n> \telse if (!strcmp(s, \"histogram\"))\n> \t\to->xdl_opts = DIFF_WITH_ALG(o, HISTOGRAM_DIFF);\n> -\telse if (!strcmp(s, \"diff-algorithm=\")) {\n> -\t\tlong value = parse_algorithm_value(s+15);\n> +\telse if (!prefixcmp(s, \"diff-algorithm=\")) {\n> +\t\tlong value = parse_algorithm_value(s + strlen(\"diff-algorithm=\"));\n> \t\tif (value < 0)\n> \t\t\treturn -1;\n\nWhile we're here:\n\nPart of the problem is that there are no tests for this option (or for\n'git diff --diff-algorithm', or for the '[diff] algorithm'\nconfiguration), so we didn't notice it didn't work until someone\nactually tried it.\n\nDo you have any examples of a diff or merge that works better with\nsome particular diff algorithm, that could be used in tests?\n\nCiao,\nJonathan\n"},{"id":"228293","messageId":"20130926211331.GD27238@serenity.lan","threadId":"35030","inReplyTo":"20130926204720.GH9464@google.com","subject":"Re: [PATCH] merge-recursive: fix parsing of \"diff-algorithm\" option","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2013-09-26T21:13:31Z","receivedAt":"2013-09-26T21:13:31Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"On Thu, Sep 26, 2013 at 01:47:20PM -0700, Jonathan Nieder wrote:\n> John Keeping wrote:\n> \n> > The \"diff-algorithm\" option to the recursive merge strategy takes the\n> > name of the algorithm as an option, but it uses strcmp on the option\n> > string to check if it starts with \"diff-algorithm=\", meaning that this\n> > options cannot actually be used.\n> >\n> > Fix this by switching to prefixcmp. At the same time, clarify the\n> > following line by using strlen instead of a hard-coded length, which\n> > also makes it consistent with nearby code.\n> >\n> > Reported-by: Luke Noel-Storr \n> > Signed-off-by: John Keeping \n> \n> Thanks, both.\n> \n> [...]\n> > --- a/merge-recursive.c\n> > +++ b/merge-recursive.c\n> > @@ -2069,8 +2069,8 @@ int parse_merge_opt(struct merge_options *o, const char *s)\n> > \t\to->xdl_opts = DIFF_WITH_ALG(o, PATIENCE_DIFF);\n> > \telse if (!strcmp(s, \"histogram\"))\n> > \t\to->xdl_opts = DIFF_WITH_ALG(o, HISTOGRAM_DIFF);\n> > -\telse if (!strcmp(s, \"diff-algorithm=\")) {\n> > -\t\tlong value = parse_algorithm_value(s+15);\n> > +\telse if (!prefixcmp(s, \"diff-algorithm=\")) {\n> > +\t\tlong value = parse_algorithm_value(s + strlen(\"diff-algorithm=\"));\n> > \t\tif (value < 0)\n> > \t\t\treturn -1;\n> \n> While we're here:\n> \n> Part of the problem is that there are no tests for this option (or for\n> 'git diff --diff-algorithm', or for the '[diff] algorithm'\n> configuration), so we didn't notice it didn't work until someone\n> actually tried it.\n> \n> Do you have any examples of a diff or merge that works better with\n> some particular diff algorithm, that could be used in tests?\n\nThe following is the same diff, but the first time using histogram and\nthen with myers.\n\nI'm not sure whether we should encode specific diff output into general\ntests though - I assume there's a reason why there aren't already tests\nfor this, but perhaps it's just that no one's written them yet.\n\n-- >8 --\ndiff --git a/test.c b/test.c\nindex f3eae9e..55f0c37 100644\n--- a/test.c\n+++ b/test.c\n@@ -1,9 +1,9 @@\n-void bye(void)\n-{\n-\tprintf(\"goodbye\\n\");\n-}\n-\n void hello(void)\n {\n \tprintf(\"hello\\n\");\n }\n+\n+void bye(void)\n+{\n+\tprintf(\"goodbye\\n\");\n+}\n\n\ndiff --git a/test.c b/test.c\nindex f3eae9e..55f0c37 100644\n--- a/test.c\n+++ b/test.c\n@@ -1,9 +1,9 @@\n-void bye(void)\n+void hello(void)\n {\n-\tprintf(\"goodbye\\n\");\n+\tprintf(\"hello\\n\");\n }\n \n-void hello(void)\n+void bye(void)\n {\n-\tprintf(\"hello\\n\");\n+\tprintf(\"goodbye\\n\");\n }\n"}]}