threads / patch / 35030

patchmerge-recursive: fix parsing of "diff-algorithm" option

Subject: [PATCH] merge-recursive: fix parsing of "diff-algorithm" option

## tl;dr

3 messages between Sep 26, 2013 and Sep 26, 2013. Diffs are folded; open one to read it.

replies: 2people: 2as markdown or json

John Keeping· Sep 26, 2013, 20:02 UTC · lore

The "diff-algorithm" option to the recursive merge strategy takes the name of the algorithm as an option, but it uses strcmp on the option string to check if it starts with "diff-algorithm=", meaning that this options cannot actually be used.

Fix this by switching to prefixcmp. At the same time, clarify the following line by using strlen instead of a hard-coded length, which also makes it consistent with nearby code.

Reported-by: Luke Noel-Storr <luke.noel-storr@integrate.co.uk>
Signed-off-by: John Keeping <john@keeping.me.uk>
---
 merge-recursive.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Show changes to merge-recursive.c +2 −2

diff --git a/merge-recursive.c b/merge-recursive.c
index 40eb840..dbb7104 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -2069,8 +2069,8 @@ int parse_merge_opt(struct merge_options *o, const char *s)
 		o->xdl_opts = DIFF_WITH_ALG(o, PATIENCE_DIFF);
 	else if (!strcmp(s, "histogram"))
 		o->xdl_opts = DIFF_WITH_ALG(o, HISTOGRAM_DIFF);
-	else if (!strcmp(s, "diff-algorithm=")) {
-		long value = parse_algorithm_value(s+15);
+	else if (!prefixcmp(s, "diff-algorithm=")) {
+		long value = parse_algorithm_value(s + strlen("diff-algorithm="));
 		if (value < 0)
 			return -1;
 		/* clear out previous settings */

-- 
1.8.4.566.g73d370b

Jonathan Nieder· Sep 26, 2013, 20:47 UTC · re: John Keeping · lore

Re: [PATCH] merge-recursive: fix parsing of "diff-algorithm" option

John Keeping wrote:

Show 11 quoted lines

> The "diff-algorithm" option to the recursive merge strategy takes the
> name of the algorithm as an option, but it uses strcmp on the option
> string to check if it starts with "diff-algorithm=", meaning that this
> options cannot actually be used.
>
> Fix this by switching to prefixcmp.  At the same time, clarify the
> following line by using strlen instead of a hard-coded length, which
> also makes it consistent with nearby code.
>
> Reported-by: Luke Noel-Storr <luke.noel-storr@integrate.co.uk>
> Signed-off-by: John Keeping <john@keeping.me.uk>

Thanks, both.

[...]

Show 12 quoted lines

> --- a/merge-recursive.c
> +++ b/merge-recursive.c
> @@ -2069,8 +2069,8 @@ int parse_merge_opt(struct merge_options *o, const char *s)
>  		o->xdl_opts = DIFF_WITH_ALG(o, PATIENCE_DIFF);
>  	else if (!strcmp(s, "histogram"))
>  		o->xdl_opts = DIFF_WITH_ALG(o, HISTOGRAM_DIFF);
> -	else if (!strcmp(s, "diff-algorithm=")) {
> -		long value = parse_algorithm_value(s+15);
> +	else if (!prefixcmp(s, "diff-algorithm=")) {
> +		long value = parse_algorithm_value(s + strlen("diff-algorithm="));
>  		if (value < 0)
>  			return -1;

While we're here:

Part of the problem is that there are no tests for this option (or for 'git diff --diff-algorithm', or for the '[diff] algorithm' configuration), so we didn't notice it didn't work until someone actually tried it.

Do you have any examples of a diff or merge that works better with some particular diff algorithm, that could be used in tests?

Ciao, Jonathan

John Keeping· Sep 26, 2013, 21:13 UTC · re: Jonathan Nieder · lore

Re: [PATCH] merge-recursive: fix parsing of "diff-algorithm" option

On Thu, Sep 26, 2013 at 01:47:20PM -0700, Jonathan Nieder wrote:

Show 39 quoted lines

> John Keeping wrote:
> 
> > The "diff-algorithm" option to the recursive merge strategy takes the
> > name of the algorithm as an option, but it uses strcmp on the option
> > string to check if it starts with "diff-algorithm=", meaning that this
> > options cannot actually be used.
> >
> > Fix this by switching to prefixcmp.  At the same time, clarify the
> > following line by using strlen instead of a hard-coded length, which
> > also makes it consistent with nearby code.
> >
> > Reported-by: Luke Noel-Storr <luke.noel-storr@integrate.co.uk>
> > Signed-off-by: John Keeping <john@keeping.me.uk>
> 
> Thanks, both.
> 
> [...]
> > --- a/merge-recursive.c
> > +++ b/merge-recursive.c
> > @@ -2069,8 +2069,8 @@ int parse_merge_opt(struct merge_options *o, const char *s)
> >  		o->xdl_opts = DIFF_WITH_ALG(o, PATIENCE_DIFF);
> >  	else if (!strcmp(s, "histogram"))
> >  		o->xdl_opts = DIFF_WITH_ALG(o, HISTOGRAM_DIFF);
> > -	else if (!strcmp(s, "diff-algorithm=")) {
> > -		long value = parse_algorithm_value(s+15);
> > +	else if (!prefixcmp(s, "diff-algorithm=")) {
> > +		long value = parse_algorithm_value(s + strlen("diff-algorithm="));
> >  		if (value < 0)
> >  			return -1;
> 
> While we're here:
> 
> Part of the problem is that there are no tests for this option (or for
> 'git diff --diff-algorithm', or for the '[diff] algorithm'
> configuration), so we didn't notice it didn't work until someone
> actually tried it.
> 
> Do you have any examples of a diff or merge that works better with
> some particular diff algorithm, that could be used in tests?

The following is the same diff, but the first time using histogram and then with myers.

I'm not sure whether we should encode specific diff output into general tests though - I assume there's a reason why there aren't already tests for this, but perhaps it's just that no one's written them yet.

-- >8 --

Show changes to test.c +9 −9

diff --git a/test.c b/test.c
index f3eae9e..55f0c37 100644
--- a/test.c
+++ b/test.c
@@ -1,9 +1,9 @@
-void bye(void)
-{
-	printf("goodbye\n");
-}
-
 void hello(void)
 {
 	printf("hello\n");
 }
+
+void bye(void)
+{
+	printf("goodbye\n");
+}


diff --git a/test.c b/test.c
index f3eae9e..55f0c37 100644
--- a/test.c
+++ b/test.c
@@ -1,9 +1,9 @@
-void bye(void)
+void hello(void)
 {
-	printf("goodbye\n");
+	printf("hello\n");
 }
 
-void hello(void)
+void bye(void)
 {
-	printf("hello\n");
+	printf("goodbye\n");
 }