[PATCH v2 1/3] Demonstrate a bug in --word-diff where diff.*.wordregex is "sticky"

[PATCH v2 1/3] Demonstrate a bug in --word-diff where diff.*.wordregex is "sticky"

[Thomas Rast]

From: Johannes Sixt <j6t@kdbg.org>

The test case applies a custom wordRegex to one file in a diff, and expects
that the default word splitting applies to the second file in the diff.
But the custom wordRegex is also incorrectly used for the second file.

[tr: unset the diff.wordRegex variable to make the test meaningful]

Signed-off-by: Johannes Sixt <j6t@kdbg.org>
Signed-off-by: Thomas Rast <trast@student.ethz.ch>
---

Compared to your version, I added the first hunk.  Otherwise the
diff.wordRegex=[[:alnum:]]+ setting carries over and makes the test
fail even with the bug fixed.

I deliberately put it as early as possible, rather than into the setup
for your test, to avoid confusion next time someone patches that file.

 t/t4034-diff-words.sh |   37 +++++++++++++++++++++++++++++++++++++
 1 file changed, 37 insertions(+)

diff --git a/t/t4034-diff-words.sh b/t/t4034-diff-words.sh
index 5c20121..69e81f3 100755
--- a/t/t4034-diff-words.sh
+++ b/t/t4034-diff-words.sh
@@ -293,6 +293,10 @@ test_expect_success '--word-diff=none' '
 	word_diff --word-diff=plain --word-diff=none
 '
 
+test_expect_success 'unset default driver' '
+	git config --unset diff.wordregex
+'
+
 test_language_driver bibtex
 test_language_driver cpp
 test_language_driver csharp
@@ -348,4 +352,37 @@ test_expect_success 'word-diff with no newline at EOF' '
 	word_diff --word-diff=plain
 '
 
+test_expect_success 'setup history with two files' '
+	echo "a b; c" >a &&
+	echo "a b; c" >z &&
+	git add a z &&
+	git commit -minitial &&
+
+	# modify both
+	echo "a bx; c" >a &&
+	echo "a bx; c" >z &&
+	git commit -mmodified -a
+'
+
+test_expect_failure 'wordRegex for the first file does not apply to the second' '
+	echo "a diff=tex" >.gitattributes &&
+	git config diff.tex.wordRegex "[a-z]+|." &&
+	cat >expect <<-\EOF &&
+		diff --git a/a b/a
+		index 9823d38..b09f967 100644
+		--- a/a
+		+++ b/a
+		@@ -1 +1 @@
+		a [-b-]{+bx+}; c
+		diff --git a/z b/z
+		index 9823d38..b09f967 100644
+		--- a/z
+		+++ b/z
+		@@ -1 +1 @@
+		a [-b;-]{+bx;+} c
+	EOF
+	git diff --word-diff HEAD~ >actual
+	test_cmp expect actual
+'
+
 test_done
-- 
1.7.10.rc0.286.gd2cb29

[PATCH 2/3] diff: refactor the word-diff setup from builtin_diff_cmd

[Thomas Rast]

Quite a chunk of builtin_diff_cmd deals with word-diff setup, defaults
and such.  This makes the function a bit hard to read, but is also
asymmetric because the corresponding teardown lives in free_diff_words_data
already.

Refactor into a new function init_diff_words_data.  For simplicity,
also shuffle around some functions it depends on.

Signed-off-by: Thomas Rast <trast@student.ethz.ch>
---
 diff.c |  119 ++++++++++++++++++++++++++++++++++------------------------------
 1 file changed, 63 insertions(+), 56 deletions(-)

diff --git a/diff.c b/diff.c
index a698a3f..aac86d3 100644
--- a/diff.c
+++ b/diff.c
@@ -989,6 +989,67 @@ static void diff_words_flush(struct emit_callback *ecbdata)
 		diff_words_show(ecbdata->diff_words);
 }
 
+static void diff_filespec_load_driver(struct diff_filespec *one)
+{
+	/* Use already-loaded driver */
+	if (one->driver)
+		return;
+
+	if (S_ISREG(one->mode))
+		one->driver = userdiff_find_by_path(one->path);
+
+	/* Fallback to default settings */
+	if (!one->driver)
+		one->driver = userdiff_find_by_name("default");
+}
+
+static const char *userdiff_word_regex(struct diff_filespec *one)
+{
+	diff_filespec_load_driver(one);
+	return one->driver->word_regex;
+}
+
+static void init_diff_words_data(struct emit_callback *ecbdata,
+				 struct diff_options *o,
+				 struct diff_filespec *one,
+				 struct diff_filespec *two)
+{
+	int i;
+
+	ecbdata->diff_words =
+		xcalloc(1, sizeof(struct diff_words_data));
+	ecbdata->diff_words->type = o->word_diff;
+	ecbdata->diff_words->opt = o;
+	if (!o->word_regex)
+		o->word_regex = userdiff_word_regex(one);
+	if (!o->word_regex)
+		o->word_regex = userdiff_word_regex(two);
+	if (!o->word_regex)
+		o->word_regex = diff_word_regex_cfg;
+	if (o->word_regex) {
+		ecbdata->diff_words->word_regex = (regex_t *)
+			xmalloc(sizeof(regex_t));
+		if (regcomp(ecbdata->diff_words->word_regex,
+			    o->word_regex,
+			    REG_EXTENDED | REG_NEWLINE))
+			die ("Invalid regular expression: %s",
+			     o->word_regex);
+	}
+	for (i = 0; i < ARRAY_SIZE(diff_words_styles); i++) {
+		if (o->word_diff == diff_words_styles[i].type) {
+			ecbdata->diff_words->style =
+				&diff_words_styles[i];
+			break;
+		}
+	}
+	if (want_color(o->use_color)) {
+		struct diff_words_style *st = ecbdata->diff_words->style;
+		st->old.color = diff_get_color_opt(o, DIFF_FILE_OLD);
+		st->new.color = diff_get_color_opt(o, DIFF_FILE_NEW);
+		st->ctx.color = diff_get_color_opt(o, DIFF_PLAIN);
+	}
+}
+
 static void free_diff_words_data(struct emit_callback *ecbdata)
 {
 	if (ecbdata->diff_words) {
@@ -2061,20 +2122,6 @@ static void emit_binary_diff(FILE *file, mmfile_t *one, mmfile_t *two, char *pre
 	emit_binary_diff_body(file, two, one, prefix);
 }
 
-static void diff_filespec_load_driver(struct diff_filespec *one)
-{
-	/* Use already-loaded driver */
-	if (one->driver)
-		return;
-
-	if (S_ISREG(one->mode))
-		one->driver = userdiff_find_by_path(one->path);
-
-	/* Fallback to default settings */
-	if (!one->driver)
-		one->driver = userdiff_find_by_name("default");
-}
-
 int diff_filespec_is_binary(struct diff_filespec *one)
 {
 	if (one->is_binary == -1) {
@@ -2100,12 +2147,6 @@ static const struct userdiff_funcname *diff_funcname_pattern(struct diff_filespe
 	return one->driver->funcname.pattern ? &one->driver->funcname : NULL;
 }
 
-static const char *userdiff_word_regex(struct diff_filespec *one)
-{
-	diff_filespec_load_driver(one);
-	return one->driver->word_regex;
-}
-
 void diff_set_mnemonic_prefix(struct diff_options *options, const char *a, const char *b)
 {
 	if (!options->a_prefix)
@@ -2292,42 +2333,8 @@ static void builtin_diff(const char *name_a,
 			xecfg.ctxlen = strtoul(diffopts + 10, NULL, 10);
 		else if (!prefixcmp(diffopts, "-u"))
 			xecfg.ctxlen = strtoul(diffopts + 2, NULL, 10);
-		if (o->word_diff) {
-			int i;
-
-			ecbdata.diff_words =
-				xcalloc(1, sizeof(struct diff_words_data));
-			ecbdata.diff_words->type = o->word_diff;
-			ecbdata.diff_words->opt = o;
-			if (!o->word_regex)
-				o->word_regex = userdiff_word_regex(one);
-			if (!o->word_regex)
-				o->word_regex = userdiff_word_regex(two);
-			if (!o->word_regex)
-				o->word_regex = diff_word_regex_cfg;
-			if (o->word_regex) {
-				ecbdata.diff_words->word_regex = (regex_t *)
-					xmalloc(sizeof(regex_t));
-				if (regcomp(ecbdata.diff_words->word_regex,
-						o->word_regex,
-						REG_EXTENDED | REG_NEWLINE))
-					die ("Invalid regular expression: %s",
-							o->word_regex);
-			}
-			for (i = 0; i < ARRAY_SIZE(diff_words_styles); i++) {
-				if (o->word_diff == diff_words_styles[i].type) {
-					ecbdata.diff_words->style =
-						&diff_words_styles[i];
-					break;
-				}
-			}
-			if (want_color(o->use_color)) {
-				struct diff_words_style *st = ecbdata.diff_words->style;
-				st->old.color = diff_get_color_opt(o, DIFF_FILE_OLD);
-				st->new.color = diff_get_color_opt(o, DIFF_FILE_NEW);
-				st->ctx.color = diff_get_color_opt(o, DIFF_PLAIN);
-			}
-		}
+		if (o->word_diff)
+			init_diff_words_data(&ecbdata, o, one, two);
 		xdi_diff_outf(&mf1, &mf2, fn_out_consume, &ecbdata,
 			      &xpp, &xecfg);
 		if (o->word_diff)
-- 
1.7.10.rc0.286.gd2cb29

Re: [PATCH 2/3] diff: refactor the word-diff setup from builtin_diff_cmd

[Junio C Hamano]

Thomas Rast <trast@student.ethz.ch> writes:

> Quite a chunk of builtin_diff_cmd deals with word-diff setup, defaults
> and such.  This makes the function a bit hard to read, but is also
> asymmetric because the corresponding teardown lives in free_diff_words_data
> already.

Yay!  "A bit hard to read" is a gross understatement ;-)

Whether the series fixes a bug of not, this alone makes the patch
worthwhile to look.

Thanks.

[PATCH 3/3] diff: tweak a _copy_ of diff_options with word-diff

[Thomas Rast]

When using word diff, the code sets the word_regex from various
defaults if it was not set already.  The problem is that it does this
on the original diff_options, which will also be used in subsequent
diffs.

This means that when the word_regex is not given on the command line,
only the first diff for which a setting for word_regex (either from
attributes or diff.wordRegex) ever takes effect.  This value then
propagates to the rest of the diff runs and in particular prevents
further attribute lookups.

Fix the problem of changing diff state once and for all, by working
with a _copy_ of the diff_options.

Noticed-by: Johannes Sixt <j6t@kdbg.org>
Signed-off-by: Thomas Rast <trast@student.ethz.ch>
---
 diff.c                |    5 ++++-
 t/t4034-diff-words.sh |    2 +-
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/diff.c b/diff.c
index aac86d3..c6104b9 100644
--- a/diff.c
+++ b/diff.c
@@ -1010,11 +1010,13 @@ static const char *userdiff_word_regex(struct diff_filespec *one)
 }
 
 static void init_diff_words_data(struct emit_callback *ecbdata,
-				 struct diff_options *o,
+				 struct diff_options *orig_opts,
 				 struct diff_filespec *one,
 				 struct diff_filespec *two)
 {
 	int i;
+	struct diff_options *o = xmalloc(sizeof(struct diff_options));
+	memcpy(o, orig_opts, sizeof(struct diff_options));
 
 	ecbdata->diff_words =
 		xcalloc(1, sizeof(struct diff_words_data));
@@ -1054,6 +1056,7 @@ static void free_diff_words_data(struct emit_callback *ecbdata)
 {
 	if (ecbdata->diff_words) {
 		diff_words_flush(ecbdata);
+		free (ecbdata->diff_words->opt);
 		free (ecbdata->diff_words->minus.text.ptr);
 		free (ecbdata->diff_words->minus.orig);
 		free (ecbdata->diff_words->plus.text.ptr);
diff --git a/t/t4034-diff-words.sh b/t/t4034-diff-words.sh
index 69e81f3..c04756e 100755
--- a/t/t4034-diff-words.sh
+++ b/t/t4034-diff-words.sh
@@ -364,7 +364,7 @@ test_expect_success 'setup history with two files' '
 	git commit -mmodified -a
 '
 
-test_expect_failure 'wordRegex for the first file does not apply to the second' '
+test_expect_success 'wordRegex for the first file does not apply to the second' '
 	echo "a diff=tex" >.gitattributes &&
 	git config diff.tex.wordRegex "[a-z]+|." &&
 	cat >expect <<-\EOF &&
-- 
1.7.10.rc0.286.gd2cb29

Re: [PATCH 3/3] diff: tweak a _copy_ of diff_options with word-diff

[Junio C Hamano]

Makes sense.  Thanks.

Re: [PATCH v2 1/3] Demonstrate a bug in --word-diff where diff.*.wordregex is "sticky"

[Junio C Hamano]

Thomas Rast <trast@student.ethz.ch> writes:

> From: Johannes Sixt <j6t@kdbg.org>
>
> The test case applies a custom wordRegex to one file in a diff, and expects
> that the default word splitting applies to the second file in the diff.
> But the custom wordRegex is also incorrectly used for the second file.
>
> [tr: unset the diff.wordRegex variable to make the test meaningful]
>
> Signed-off-by: Johannes Sixt <j6t@kdbg.org>
> Signed-off-by: Thomas Rast <trast@student.ethz.ch>
> ---
>
> Compared to your version, I added the first hunk.  Otherwise the
> diff.wordRegex=[[:alnum:]]+ setting carries over and makes the test
> fail even with the bug fixed.
>
> I deliberately put it as early as possible, rather than into the setup
> for your test, to avoid confusion next time someone patches that file.
>
>  t/t4034-diff-words.sh |   37 +++++++++++++++++++++++++++++++++++++
>  1 file changed, 37 insertions(+)
>
> diff --git a/t/t4034-diff-words.sh b/t/t4034-diff-words.sh
> index 5c20121..69e81f3 100755
> --- a/t/t4034-diff-words.sh
> +++ b/t/t4034-diff-words.sh
> @@ -293,6 +293,10 @@ test_expect_success '--word-diff=none' '
>  	word_diff --word-diff=plain --word-diff=none
>  '
>  
> +test_expect_success 'unset default driver' '
> +	git config --unset diff.wordregex
> +'

Isn't this unsafe if some of the tests before this one failed?

	test_unconfig diff.wordregex

By the way, I really loathe the change that gutted major parts out of
t/test-lib.sh and moved them to another file; now I have to eyeball two
files to write a response like this instead of one.

> +test_expect_failure 'wordRegex for the first file does not apply to the second' '
> +	echo "a diff=tex" >.gitattributes &&
> +	git config diff.tex.wordRegex "[a-z]+|." &&

The use of files "a" and "z" as an example of pair of a tex and non tex
input makes the test look overly artificial (why not a.tex vs z.txt or
something?), but other than that, looks cleanly done.

Thanks.

> +	cat >expect <<-\EOF &&
> +		diff --git a/a b/a
> +		index 9823d38..b09f967 100644
> +		--- a/a
> +		+++ b/a
> +		@@ -1 +1 @@
> +		a [-b-]{+bx+}; c
> +		diff --git a/z b/z
> +		index 9823d38..b09f967 100644
> +		--- a/z
> +		+++ b/z
> +		@@ -1 +1 @@
> +		a [-b;-]{+bx;+} c
> +	EOF
> +	git diff --word-diff HEAD~ >actual
> +	test_cmp expect actual
> +'
> +
>  test_done

[PATCH v3 1/3] Demonstrate a bug in --word-diff where diff.*.wordregex is "sticky"

[Johannes Sixt]

The test case applies a custom wordRegex to one file in a diff, and expects
that the default word splitting applies to the second file in the diff.
But the custom wordRegex is also incorrectly used for the second file.

Helped-by: Thomas Rast <trast@student.ethz.ch>
Signed-off-by: Johannes Sixt <j6t@kdbg.org>
---
Changes in this round, where I do not resend 2/3 and 3/3:

- Use test_unconfig.
- Use compare_diff_patch to check the result.
- Fix a broken && chain at the end of the last test.
- Name the files a.tex and z.tex as per your suggestion.

 t/t4034-diff-words.sh |   36 ++++++++++++++++++++++++++++++++++++
 1 files changed, 36 insertions(+), 0 deletions(-)

diff --git a/t/t4034-diff-words.sh b/t/t4034-diff-words.sh
index 5c20121..9e9643e 100755
--- a/t/t4034-diff-words.sh
+++ b/t/t4034-diff-words.sh
@@ -3,6 +3,7 @@
 test_description='word diff colors'
 
 . ./test-lib.sh
+. "$TEST_DIRECTORY"/diff-lib.sh
 
 cat >pre.simple <<-\EOF
 	h(4)
@@ -293,6 +294,10 @@ test_expect_success '--word-diff=none' '
 	word_diff --word-diff=plain --word-diff=none
 '
 
+test_expect_success 'unset default driver' '
+	test_unconfig diff.wordregex
+'
+
 test_language_driver bibtex
 test_language_driver cpp
 test_language_driver csharp
@@ -348,4 +353,35 @@ test_expect_success 'word-diff with no newline at EOF' '
 	word_diff --word-diff=plain
 '
 
+test_expect_success 'setup history with two files' '
+	echo "a b; c" >a.tex &&
+	echo "a b; c" >z.tex &&
+	git add a.tex z.tex &&
+	git commit -minitial &&
+
+	# modify both
+	echo "a bx; c" >a.tex &&
+	echo "a bx; c" >z.tex &&
+	git commit -mmodified -a
+'
+
+test_expect_failure 'wordRegex for the first file does not apply to the second' '
+	echo "a.tex diff=tex" >.gitattributes &&
+	git config diff.tex.wordRegex "[a-z]+|." &&
+	cat >expect <<-\EOF &&
+		diff --git a/a.tex b/a.tex
+		--- a/a.tex
+		+++ b/a.tex
+		@@ -1 +1 @@
+		a [-b-]{+bx+}; c
+		diff --git a/z.tex b/z.tex
+		--- a/z.tex
+		+++ b/z.tex
+		@@ -1 +1 @@
+		a [-b;-]{+bx;+} c
+	EOF
+	git diff --word-diff HEAD~ >actual &&
+	compare_diff_patch expect actual
+'
+
 test_done
-- 
1.7.8.216.g2e426

Re: [PATCH v3 1/3] Demonstrate a bug in --word-diff where diff.*.wordregex is "sticky"

[Stefano Lattarini]

On 03/14/2012 08:50 PM, Johannes Sixt wrote:
> The test case applies a custom wordRegex to one file in a diff, and expects
> that the default word splitting applies to the second file in the diff.
> But the custom wordRegex is also incorrectly used for the second file.
> 
> Helped-by: Thomas Rast <trast@student.ethz.ch>
> Signed-off-by: Johannes Sixt <j6t@kdbg.org>
> ---
> Changes in this round, where I do not resend 2/3 and 3/3:
> 
> - Use test_unconfig.
> - Use compare_diff_patch to check the result.
> - Fix a broken && chain at the end of the last test.
> - Name the files a.tex and z.tex as per your suggestion.
>
But Junio suggested using 'z.txt' (not 'z.tex'); quoting his last message:

  The use of files "a" and "z" as an example of pair of a tex and non tex
  input makes the test look overly artificial (why not a.tex vs z.txt or
  something?), but other than that, looks cleanly done.

Regards,
  Stefano

Re: [PATCH v3 1/3] Demonstrate a bug in --word-diff where diff.*.wordregex is "sticky"

[Junio C Hamano]

Johannes Sixt <j6t@kdbg.org> writes:

> The test case applies a custom wordRegex to one file in a diff, and expects
> that the default word splitting applies to the second file in the diff.
> But the custom wordRegex is also incorrectly used for the second file.
>
> Helped-by: Thomas Rast <trast@student.ethz.ch>
> Signed-off-by: Johannes Sixt <j6t@kdbg.org>
> ---
> Changes in this round, where I do not resend 2/3 and 3/3:
>
> - Use test_unconfig.
> - Use compare_diff_patch to check the result.
> - Fix a broken && chain at the end of the last test.
> - Name the files a.tex and z.tex as per your suggestion.

Thanks, but using a.tex and z.tex and marking only the former as tex
does not change anything in the puzzlement I mentioned in my response.

Perhaps this on top of your patch?

 t/t4034-diff-words.sh |   14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/t/t4034-diff-words.sh b/t/t4034-diff-words.sh
index 9e9643e..310ace1 100755
--- a/t/t4034-diff-words.sh
+++ b/t/t4034-diff-words.sh
@@ -355,18 +355,18 @@ test_expect_success 'word-diff with no newline at EOF' '
 
 test_expect_success 'setup history with two files' '
 	echo "a b; c" >a.tex &&
-	echo "a b; c" >z.tex &&
-	git add a.tex z.tex &&
+	echo "a b; c" >z.txt &&
+	git add a.tex z.txt &&
 	git commit -minitial &&
 
 	# modify both
 	echo "a bx; c" >a.tex &&
-	echo "a bx; c" >z.tex &&
+	echo "a bx; c" >z.txt &&
 	git commit -mmodified -a
 '
 
 test_expect_failure 'wordRegex for the first file does not apply to the second' '
-	echo "a.tex diff=tex" >.gitattributes &&
+	echo "*.tex diff=tex" >.gitattributes &&
 	git config diff.tex.wordRegex "[a-z]+|." &&
 	cat >expect <<-\EOF &&
 		diff --git a/a.tex b/a.tex
@@ -374,9 +374,9 @@ test_expect_failure 'wordRegex for the first file does not apply to the second'
 		+++ b/a.tex
 		@@ -1 +1 @@
 		a [-b-]{+bx+}; c
-		diff --git a/z.tex b/z.tex
-		--- a/z.tex
-		+++ b/z.tex
+		diff --git a/z.txt b/z.txt
+		--- a/z.txt
+		+++ b/z.txt
 		@@ -1 +1 @@
 		a [-b;-]{+bx;+} c
 	EOF
-- 
1.7.10.rc0.65.g3445e

Re: [PATCH v3 1/3] Demonstrate a bug in --word-diff where diff.*.wordregex is "sticky"

[Johannes Sixt]

Am 14.03.2012 22:38, schrieb Junio C Hamano:
> Thanks, but using a.tex and z.tex and marking only the former as tex
> does not change anything in the puzzlement I mentioned in my response.

Oh, I see. I didn't notice this detail. Please squash in your proposed
fixup that uses z.txt.

Thanks,
-- Hannes