[PATCH 1/4] diff.c: call regfree to free memory allocated by regcomp when necessary

From: Brandon Casey <redacted>


Signed-off-by: Brandon Casey <redacted>
---
 diff.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/diff.c b/diff.c
index 144f2aa..9a5c77c 100644
--- a/diff.c
+++ b/diff.c

@@ -919,7 +919,10 @@ static void free_diff_words_data(struct emit_callback *ecbdata)
 		free (ecbdata->diff_words->minus.orig);
 		free (ecbdata->diff_words->plus.text.ptr);
 		free (ecbdata->diff_words->plus.orig);
-		free(ecbdata->diff_words->word_regex);
+		if (ecbdata->diff_words->word_regex) {
+			regfree(ecbdata->diff_words->word_regex);
+			free(ecbdata->diff_words->word_regex);
+		}
 		free(ecbdata->diff_words);
 		ecbdata->diff_words = NULL;
 	}

-- 
1.7.2.1

From: Brandon Casey <redacted>

This adds fortran xfuncname and wordRegex patterns to the list of builtin
patterns.  The intention is for the patterns to be appropriate for all
versions of fortran including 77, 90, 95.  The patterns can be enabled by
adding the diff=fortran attribute to the .gitattributes file for the
desired file glob.

This also adds a new macro named iPATTERN which is just like the PATTERNS
macro except it sets the REG_ICASE flag so that case will be ignored.

The test code in t4018 and the docs were updated as appropriate.

Signed-off-by: Brandon Casey <redacted>
---
 Documentation/gitattributes.txt |    2 ++
 t/t4018-diff-funcname.sh        |    2 +-
 userdiff.c                      |   17 +++++++++++++++++
 3 files changed, 20 insertions(+), 1 deletions(-)

diff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt
index e5a27d8..fbf507a 100644
--- a/Documentation/gitattributes.txt
+++ b/Documentation/gitattributes.txt

@@ -477,6 +477,8 @@ patterns are available:
 
 - `csharp` suitable for source code in the C# language.
 
+- `fortran` suitable for source code in the Fortran language.
+
 - `html` suitable for HTML/XHTML documents.
 
 - `java` suitable for source code in the Java language.

diff --git a/t/t4018-diff-funcname.sh b/t/t4018-diff-funcname.sh
index 620cd02..9a57898 100755
--- a/t/t4018-diff-funcname.sh
+++ b/t/t4018-diff-funcname.sh

@@ -32,7 +32,7 @@ EOF
 
 sed 's/beer\\/beer,\\/' < Beer.java > Beer-correct.java
 
-builtin_patterns="bibtex cpp csharp html java objc pascal php python ruby tex"
+builtin_patterns="bibtex cpp csharp fortran html java objc pascal php python ruby tex"
 for p in $builtin_patterns
 do
 	test_expect_success "builtin $p pattern compiles" '

diff --git a/userdiff.c b/userdiff.c
index e552215..953bb5e 100644
--- a/userdiff.c
+++ b/userdiff.c

@@ -9,7 +9,23 @@ static int drivers_alloc;
 
 #define PATTERNS(name, pattern, word_regex)			\
 	{ name, NULL, -1, { pattern, REG_EXTENDED }, word_regex }
+#define iPATTERN(name, pattern, word_regex)			\
+	{ name, NULL, -1, { pattern, REG_EXTENDED | REG_ICASE }, word_regex }
 static struct userdiff_driver builtin_drivers[] = {
+iPATTERN("fortran",
+	 "!^([C*]|[ \t]*!)\n"
+	 "!^[ \t]*MODULE[ \t]+PROCEDURE[ \t]\n"
+	 "^[ \t]*((END[ \t]+)?(PROGRAM|MODULE|BLOCK[ \t]+DATA"
+		"|([^'\" \t]+[ \t]+)*(SUBROUTINE|FUNCTION))[ \t]+[A-Z].*)$",
+	 /* -- */
+	 "[a-zA-Z][a-zA-Z0-9_]*"
+	 "|\\.([Ee][Qq]|[Nn][Ee]|[Gg][TtEe]|[Ll][TtEe]|[Tt][Rr][Uu][Ee]|[Ff][Aa][Ll][Ss][Ee]|[Aa][Nn][Dd]|[Oo][Rr]|[Nn]?[Ee][Qq][Vv]|[Nn][Oo][Tt])\\."
+	 /* numbers and format statements like 2E14.4, or ES12.6, 9X.
+	  * Don't worry about format statements without leading digits since
+	  * they would have been matched above as a variable anyway. */
+	 "|[-+]?[0-9.]+([AaIiDdEeFfLlTtXx][Ss]?[-+]?[0-9.]*)?(_[a-zA-Z0-9][a-zA-Z0-9_]*)?"
+	 "|//|\\*\\*|::|[/<>=]="
+	 "|[^[:space:]]|[\x80-\xff]+"),
 PATTERNS("html", "^[ \t]*(<[Hh][1-6][ \t].*>.*)$",
 	 "[^<>= \t]+|[^[:space:]]|[\x80-\xff]+"),
 PATTERNS("java",

@@ -101,6 +117,7 @@ PATTERNS("csharp",
 { "default", NULL, -1, { NULL, 0 } },
 };
 #undef PATTERNS
+#undef iPATTERN
 
 static struct userdiff_driver driver_true = {
 	"diff=true",

-- 
1.7.2.1

From: Brandon Casey <redacted>

Generally, trailing space is removed from the string matched by the
xfuncname patterns.  The exception is when the matched string exceeds the
length of the fixed-size buffer that it will be copied in to.  But, a
string that exceeds the buffer can still contain trailing space in the
portion of the string that will be copied into the buffer.  So, simplify
this code slightly, and just perform the trailing space removal always.

Signed-off-by: Brandon Casey <redacted>
---
 xdiff-interface.c |    5 ++---
 1 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/xdiff-interface.c b/xdiff-interface.c
index cd2285d..e1e054e 100644
--- a/xdiff-interface.c
+++ b/xdiff-interface.c

@@ -286,9 +286,8 @@ static long ff_regexp(const char *line, long len,
 	result = pmatch[i].rm_eo - pmatch[i].rm_so;
 	if (result > buffer_size)
 		result = buffer_size;
-	else
-		while (result > 0 && (isspace(line[result - 1])))
-			result--;
+	while (result > 0 && (isspace(line[result - 1])))
+		result--;
 	memcpy(buffer, line, result);
  fail:
 	free(line_buffer);

-- 
1.7.2.1

From: Brandon Casey <redacted>

Previously (e3bf5e43), a test was added to test whether the builtin
xfuncname regular expressions could be compiled without error by regcomp.
Let's do the same for the word_regex patterns.  This should help catch any
cross-platform incompatibilities that exist between the pattern creator's
system and the various platforms that the test suite is commonly run on.

Signed-off-by: Brandon Casey <redacted>
---
 t/t4018-diff-funcname.sh |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/t/t4018-diff-funcname.sh b/t/t4018-diff-funcname.sh
index 61de8a2..620cd02 100755
--- a/t/t4018-diff-funcname.sh
+++ b/t/t4018-diff-funcname.sh

@@ -40,6 +40,11 @@ do
 		! ( git diff --no-index Beer.java Beer-correct.java 2>&1 |
 			grep "fatal" > /dev/null )
 	'
+	test_expect_success "builtin $p wordRegex pattern compiles" '
+		! ( git diff --no-index --word-diff \
+			Beer.java Beer-correct.java 2>&1 |
+			grep "fatal" > /dev/null )
+	'
 done
 
 test_expect_success 'default behaviour' '

-- 
1.7.2.1

On Thu, Sep 09, 2010 at 02:02:45PM -0500, Brandon Casey wrote:

Makes sense, and closes a leak. But I wonder why word_regex is a pointer
to malloc'd memory at all? Couldn't it just be an actual regex_t inside
the diff_words struct, and we regcomp and regfree it?

I guess maybe having it be NULL is useful. You could check
diff_words->o->word_regex, but I suppose that would mean carrying an
extra variable around through the callchain.

-Peff

On Thu, Sep 09, 2010 at 02:02:46PM -0500, Brandon Casey wrote:

From: Brandon Casey <redacted>

Generally, trailing space is removed from the string matched by the
xfuncname patterns.  The exception is when the matched string exceeds the
length of the fixed-size buffer that it will be copied in to.  But, a
string that exceeds the buffer can still contain trailing space in the
portion of the string that will be copied into the buffer.  So, simplify
this code slightly, and just perform the trailing space removal always.

Hrm. So we are cutting off trailing space that might have been
non-trailing space in their original string? It is hard to argue that is
much worse than truncating the original string in the first place. But I
really wonder whether we should be silently truncating anything, and not
just dying or somehow handling this better?

If I understand what is going on (and I'm not sure that I do), are we
silently producing bogus word-diffs in the face of really long lines?

-Peff

On Thu, Sep 09, 2010 at 02:02:47PM -0500, Brandon Casey wrote:

From: Brandon Casey <redacted>

Previously (e3bf5e43), a test was added to test whether the builtin
xfuncname regular expressions could be compiled without error by regcomp.
Let's do the same for the word_regex patterns.  This should help catch any
cross-platform incompatibilities that exist between the pattern creator's
system and the various platforms that the test suite is commonly run on.

Definitely something we should be doing, but one nit:

Why the subshell? Shouldn't just testing the pipeline outcome work?

-Peff

On Thu, Sep 09, 2010 at 02:02:48PM -0500, Brandon Casey wrote:

This adds fortran xfuncname and wordRegex patterns to the list of builtin
patterns.  The intention is for the patterns to be appropriate for all
versions of fortran including 77, 90, 95.  The patterns can be enabled by
adding the diff=fortran attribute to the .gitattributes file for the
desired file glob.

The rest of your series looks reasonable, modulo the comments I posted
to individual patches. But I won't torture my eyes by trying to actually
see how well this matches fortran code, and just assume this is a good
change. :)

This also adds a new macro named iPATTERN which is just like the PATTERNS
macro except it sets the REG_ICASE flag so that case will be ignored.

Style nit: please keep macros all uppercase.

-Peff

On 09/09/2010 02:21 PM, Jeff King wrote:

On Thu, Sep 09, 2010 at 02:02:46PM -0500, Brandon Casey wrote:

quoted

From: Brandon Casey <redacted>

Generally, trailing space is removed from the string matched by the
xfuncname patterns.  The exception is when the matched string exceeds the
length of the fixed-size buffer that it will be copied in to.  But, a
string that exceeds the buffer can still contain trailing space in the
portion of the string that will be copied into the buffer.  So, simplify
this code slightly, and just perform the trailing space removal always.

Hrm. So we are cutting off trailing space that might have been
non-trailing space in their original string? It is hard to argue that is
much worse than truncating the original string in the first place. But I
really wonder whether we should be silently truncating anything, and not
just dying or somehow handling this better?

If I understand what is going on (and I'm not sure that I do), are we
silently producing bogus word-diffs in the face of really long lines?

I don't think this function is used to do the word-diffs.  Unless I'm
missing something, ff_regexp is only used to do the funcname matching to
produce the hunk header string.  An 80-byte buffer is used to hold
that string.  So, the trimming is performed on what is effectively a
comment.

-Brandon

On Thu, Sep 09, 2010 at 02:33:15PM -0500, Brandon Casey wrote:

On 09/09/2010 02:21 PM, Jeff King wrote:

quoted

On Thu, Sep 09, 2010 at 02:02:46PM -0500, Brandon Casey wrote:

quoted

From: Brandon Casey <redacted>

Generally, trailing space is removed from the string matched by the
xfuncname patterns.  The exception is when the matched string exceeds the
length of the fixed-size buffer that it will be copied in to.  But, a
string that exceeds the buffer can still contain trailing space in the
portion of the string that will be copied into the buffer.  So, simplify
this code slightly, and just perform the trailing space removal always.

Hrm. So we are cutting off trailing space that might have been
non-trailing space in their original string? It is hard to argue that is
much worse than truncating the original string in the first place. But I
really wonder whether we should be silently truncating anything, and not
just dying or somehow handling this better?

If I understand what is going on (and I'm not sure that I do), are we
silently producing bogus word-diffs in the face of really long lines?

I don't think this function is used to do the word-diffs.  Unless I'm
missing something, ff_regexp is only used to do the funcname matching to
produce the hunk header string.  An 80-byte buffer is used to hold
that string.  So, the trimming is performed on what is effectively a
comment.

Ah, OK, that makes a lot more sense. Then yeah, we should always be
trimming trailing whitespace from the result, so your patch is good.

-Peff

On 09/09/2010 02:23 PM, Jeff King wrote:

On Thu, Sep 09, 2010 at 02:02:47PM -0500, Brandon Casey wrote:

quoted

From: Brandon Casey <redacted>

Previously (e3bf5e43), a test was added to test whether the builtin
xfuncname regular expressions could be compiled without error by regcomp.
Let's do the same for the word_regex patterns.  This should help catch any
cross-platform incompatibilities that exist between the pattern creator's
system and the various platforms that the test suite is commonly run on.

Definitely something we should be doing, but one nit:

quoted

diff --git a/t/t4018-diff-funcname.sh b/t/t4018-diff-funcname.sh
index 61de8a2..620cd02 100755
--- a/t/t4018-diff-funcname.sh
+++ b/t/t4018-diff-funcname.sh

@@ -40,6 +40,11 @@ do
 		! ( git diff --no-index Beer.java Beer-correct.java 2>&1 |
 			grep "fatal" > /dev/null )
 	'
+	test_expect_success "builtin $p wordRegex pattern compiles" '
+		! ( git diff --no-index --word-diff \
+			Beer.java Beer-correct.java 2>&1 |
+			grep "fatal" > /dev/null )
+	'

Why the subshell? Shouldn't just testing the pipeline outcome work?

Notice the similarity between the added lines and the lines just
above them?  Where were you when I submitted those? :)

Thanks,
-Brandon

On 09/09/2010 02:25 PM, Jeff King wrote:

On Thu, Sep 09, 2010 at 02:02:48PM -0500, Brandon Casey wrote:

quoted

This adds fortran xfuncname and wordRegex patterns to the list of builtin
patterns.  The intention is for the patterns to be appropriate for all
versions of fortran including 77, 90, 95.  The patterns can be enabled by
adding the diff=fortran attribute to the .gitattributes file for the
desired file glob.

The rest of your series looks reasonable, modulo the comments I posted
to individual patches. But I won't torture my eyes by trying to actually
see how well this matches fortran code, and just assume this is a good
change. :)

I can send you some. :)

quoted

This also adds a new macro named iPATTERN which is just like the PATTERNS
macro except it sets the REG_ICASE flag so that case will be ignored.

Style nit: please keep macros all uppercase.

Ah, yeah, I guess you're right.

  IPATTERN

Thanks,
-Brandon

On Thu, Sep 09, 2010 at 02:39:32PM -0500, Brandon Casey wrote:

quoted

Definitely something we should be doing, but one nit:

quoted

diff --git a/t/t4018-diff-funcname.sh b/t/t4018-diff-funcname.sh
index 61de8a2..620cd02 100755
--- a/t/t4018-diff-funcname.sh
+++ b/t/t4018-diff-funcname.sh

@@ -40,6 +40,11 @@ do
 		! ( git diff --no-index Beer.java Beer-correct.java 2>&1 |
 			grep "fatal" > /dev/null )
 	'
+	test_expect_success "builtin $p wordRegex pattern compiles" '
+		! ( git diff --no-index --word-diff \
+			Beer.java Beer-correct.java 2>&1 |
+			grep "fatal" > /dev/null )
+	'

Why the subshell? Shouldn't just testing the pipeline outcome work?

Notice the similarity between the added lines and the lines just
above them?  Where were you when I submitted those? :)

I did. I almost said "obviously you copied from above, but that is no
excuse". So let me say it now. :)

-Peff

From: Brandon Casey <redacted>


Signed-off-by: Brandon Casey <redacted>
---


Thanks again Jeff for your, as always, very valuable review.


 t/t4018-diff-funcname.sh |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/t/t4018-diff-funcname.sh b/t/t4018-diff-funcname.sh
index 620cd02..c8e1937 100755
--- a/t/t4018-diff-funcname.sh
+++ b/t/t4018-diff-funcname.sh

@@ -37,13 +37,13 @@ for p in $builtin_patterns
 do
 	test_expect_success "builtin $p pattern compiles" '
 		echo "*.java diff=$p" > .gitattributes &&
-		! ( git diff --no-index Beer.java Beer-correct.java 2>&1 |
-			grep "fatal" > /dev/null )
+		! { git diff --no-index Beer.java Beer-correct.java 2>&1 |
+			grep "fatal" > /dev/null; }
 	'
 	test_expect_success "builtin $p wordRegex pattern compiles" '
-		! ( git diff --no-index --word-diff \
+		! { git diff --no-index --word-diff \
 			Beer.java Beer-correct.java 2>&1 |
-			grep "fatal" > /dev/null )
+			grep "fatal" > /dev/null; }
 	'
 done
 

-- 
1.7.2.1

From: Brandon Casey <redacted>

This adds fortran xfuncname and wordRegex patterns to the list of builtin
patterns.  The intention is for the patterns to be appropriate for all
versions of fortran including 77, 90, 95.  The patterns can be enabled by
adding the diff=fortran attribute to the .gitattributes file for the
desired file glob.

This also adds a new macro named IPATTERN which is just like the PATTERNS
macro except it sets the REG_ICASE flag so that case will be ignored.

The test code in t4018 and the docs were updated as appropriate.

Signed-off-by: Brandon Casey <redacted>
---


This version renames the iPATTERN macro to the all uppercase IPATTERN, and
is a replacement for the version in pu.


 Documentation/gitattributes.txt |    2 ++
 t/t4018-diff-funcname.sh        |    2 +-
 userdiff.c                      |   17 +++++++++++++++++
 3 files changed, 20 insertions(+), 1 deletions(-)

diff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt
index e5a27d8..fbf507a 100644
--- a/Documentation/gitattributes.txt
+++ b/Documentation/gitattributes.txt

@@ -477,6 +477,8 @@ patterns are available:
 
 - `csharp` suitable for source code in the C# language.
 
+- `fortran` suitable for source code in the Fortran language.
+
 - `html` suitable for HTML/XHTML documents.
 
 - `java` suitable for source code in the Java language.

diff --git a/t/t4018-diff-funcname.sh b/t/t4018-diff-funcname.sh
index c8e1937..0a61b57 100755
--- a/t/t4018-diff-funcname.sh
+++ b/t/t4018-diff-funcname.sh

@@ -32,7 +32,7 @@ EOF
 
 sed 's/beer\\/beer,\\/' < Beer.java > Beer-correct.java
 
-builtin_patterns="bibtex cpp csharp html java objc pascal php python ruby tex"
+builtin_patterns="bibtex cpp csharp fortran html java objc pascal php python ruby tex"
 for p in $builtin_patterns
 do
 	test_expect_success "builtin $p pattern compiles" '

diff --git a/userdiff.c b/userdiff.c
index e552215..f9e05b5 100644
--- a/userdiff.c
+++ b/userdiff.c

@@ -9,7 +9,23 @@ static int drivers_alloc;
 
 #define PATTERNS(name, pattern, word_regex)			\
 	{ name, NULL, -1, { pattern, REG_EXTENDED }, word_regex }
+#define IPATTERN(name, pattern, word_regex)			\
+	{ name, NULL, -1, { pattern, REG_EXTENDED | REG_ICASE }, word_regex }
 static struct userdiff_driver builtin_drivers[] = {
+IPATTERN("fortran",
+	 "!^([C*]|[ \t]*!)\n"
+	 "!^[ \t]*MODULE[ \t]+PROCEDURE[ \t]\n"
+	 "^[ \t]*((END[ \t]+)?(PROGRAM|MODULE|BLOCK[ \t]+DATA"
+		"|([^'\" \t]+[ \t]+)*(SUBROUTINE|FUNCTION))[ \t]+[A-Z].*)$",
+	 /* -- */
+	 "[a-zA-Z][a-zA-Z0-9_]*"
+	 "|\\.([Ee][Qq]|[Nn][Ee]|[Gg][TtEe]|[Ll][TtEe]|[Tt][Rr][Uu][Ee]|[Ff][Aa][Ll][Ss][Ee]|[Aa][Nn][Dd]|[Oo][Rr]|[Nn]?[Ee][Qq][Vv]|[Nn][Oo][Tt])\\."
+	 /* numbers and format statements like 2E14.4, or ES12.6, 9X.
+	  * Don't worry about format statements without leading digits since
+	  * they would have been matched above as a variable anyway. */
+	 "|[-+]?[0-9.]+([AaIiDdEeFfLlTtXx][Ss]?[-+]?[0-9.]*)?(_[a-zA-Z0-9][a-zA-Z0-9_]*)?"
+	 "|//|\\*\\*|::|[/<>=]="
+	 "|[^[:space:]]|[\x80-\xff]+"),
 PATTERNS("html", "^[ \t]*(<[Hh][1-6][ \t].*>.*)$",
 	 "[^<>= \t]+|[^[:space:]]|[\x80-\xff]+"),
 PATTERNS("java",

@@ -101,6 +117,7 @@ PATTERNS("csharp",
 { "default", NULL, -1, { NULL, 0 } },
 };
 #undef PATTERNS
+#undef IPATTERN
 
 static struct userdiff_driver driver_true = {
 	"diff=true",

-- 
1.7.2.1

On Fri, Sep 10, 2010 at 16:13, Brandon Casey [off-list ref] wrote:

FWIW (but don't think you need to change 'em) I'd do these as
(untested):

test_expect_success "builtin $p wordRegex pattern compiles" '
    git diff --no-index --word-diff >out 2>err &&
    # Or whatever out should be..
    ! test -s out &&
    grep fatal err
'

It's much easier to debug tests that use intermediate files with
--immediate --debug when they break, because you can just check out
what out and err contain. You can't do that if the output was
forgotten in some pipe.

Maybe I should add a bit into t/README about this ...