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

Subsystems: the rest

DORMANTno replies

3 messages, 1 author, 2016-06-15 · open the first message on its own page

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

From: Thomas Rast <hidden>
Date: 2016-06-15 22:53:18

From: Johannes Sixt <redacted>

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 <redacted>
Signed-off-by: Thomas Rast <redacted>
---

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

From: Thomas Rast <hidden>
Date: 2016-06-15 22:53:18

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 <redacted>
---
 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

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

From: Thomas Rast <hidden>
Date: 2016-06-15 22:53:18

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 [off-list ref]
Signed-off-by: Thomas Rast <redacted>
---
 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

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help