Problems with Git's "perl" userdiff driver

Problems with Git's "perl" userdiff driver

[Ævar Arnfjörð Bjarmason]

On Wed, Dec 29, 2010 at 06:22, Junio C Hamano <gitster@pobox.com> wrote:

[better late than never]

> * jn/perl-funcname (2010-12-27) 2 commits
>  - userdiff/perl: catch BEGIN/END/... and POD as headers
>  - diff: funcname and word patterns for perl

The POD rule doesn't work properly. I suspect it has to be:

    "^=head[0-9] .*",

Instead of the current:

    "^=head[0-9] ",

Since e.g.:

    =head1 WHATEVER

Will just be shown as:

    =head1

In the diff context.

And actually it applies very badly to POD in general, since the "sub"
rule will be tried first, so e.g. in Perldoc we'll often end up
finding some "sub" example halfway up the file, instead of the =head1*
or =item* section a few lines up.

And it looks like the regex only catches:

    sub foo {
    }

Not:

    sub foo
    {
    }

Re: Problems with Git's "perl" userdiff driver

[Junio C Hamano]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> On Wed, Dec 29, 2010 at 06:22, Junio C Hamano <gitster@pobox.com> wrote:

Heh; without in-reply-to or references, this is useless, isn't it?

> [better late than never]
>
>> * jn/perl-funcname (2010-12-27) 2 commits
>>  - userdiff/perl: catch BEGIN/END/... and POD as headers
>>  - diff: funcname and word patterns for perl

For others, the above are these two:

 a25e473 (userdiff/perl: catch BEGIN/END/... and POD as headers, 2010-12-27)
 71a5d4b (diff: funcname and word patterns for perl, 2010-12-26)

> The POD rule doesn't work properly. I suspect it has to be:
>
>     "^=head[0-9] .*",
>
> Instead of the current:
>
>     "^=head[0-9] ",
>
> Since e.g.:
>
>     =head1 WHATEVER
>
> Will just be shown as:
>
>     =head1
>
> In the diff context.
>
> And actually it applies very badly to POD in general, since the "sub"
> rule will be tried first, so e.g. in Perldoc we'll often end up
> finding some "sub" example halfway up the file, instead of the =head1*
> or =item* section a few lines up.
>
> And it looks like the regex only catches:
>
>     sub foo {
>     }
>
> Not:
>
>     sub foo
>     {
>     }

Re: Problems with Git's "perl" userdiff driver

[Ævar Arnfjörð Bjarmason]

On Sun, May 15, 2011 at 22:02, Junio C Hamano <gitster@pobox.com> wrote:
> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>
>> On Wed, Dec 29, 2010 at 06:22, Junio C Hamano <gitster@pobox.com> wrote:
>
> Heh; without in-reply-to or references, this is useless, isn't it?

As far as I could tell the patch you appended to that series was never
submitted to the list, so I couldn't reply to it. Thanks for providing
the sha1's.

[PATCH 0/7] Re: Problems with Git's "perl" userdiff driver

[Jonathan Nieder]

Hi Ævar,

Ævar Arnfjörð Bjarmason wrote:

> The POD rule doesn't work properly. I suspect it has to be:

Thanks.  Some context is here (thanks to Thomas for his notes/full
branch).

 http://thread.gmane.org/gmane.comp.version-control.git/164184

Quick thoughts before fixing them:

>     "^=head[0-9] .*",
>
> Instead of the current:
>
>     "^=head[0-9] ",

Good catch.

> And actually it applies very badly to POD in general, since the "sub"
> rule will be tried first, so e.g. in Perldoc we'll often end up
> finding some "sub" example halfway up the file, instead of the =head1*
> or =item* section a few lines up.

I have another worry of the same kind.  If my source file consists of
multiple packages (like git-svn.perl does), then the "package" rule
can be useful for getting bearings in a diff:

	diff --git a/git-svn.perl b/git-svn.perl
	index dc66803..74d8612 100755
	--- a/git-svn.perl
	+++ b/git-svn.perl
	@@ -4000,7 +4000,6 @@ package SVN::Git::Fetcher;
	 use strict;
	 use warnings;
	 use Carp qw/croak/;
	-use File::Temp qw/tempfile/;
	 use IO::File qw//;
	 use vars qw/$_ignore_regex/;

But as soon as "git diff" encounters a subroutine, that replaces the
context.  It seems that the --show-function mechanism works best for
files structured such that every line has a scope, with no nesting:

	sub foo {
		... lines of foo come here ...
	}

	sub bar {
		... lines of bar come here ...
	}

and does not cope as well with nested scopes or lines that do not
fall in any scope.

The upshot is that all these patterns should be anchored at the left
margin for now, and it might be worth considering teaching userdiff to
push and pop scopes so that lines outside the innermost scope are
correctly attributed, as in the last two lines below:

	package Foo::Bar::Baz

	=head1 NAME

	Foo::Bar::Baz - well-documented frobber

	=cut

	use strict;
	use Qux::Quux;

That would also take care of your example of

	=item B<something>

	Use it like so:

		sub example {
			something else;
		}

	... more documentation that should be attributed to the =item,
	not the sub ...

> And it looks like the regex only catches:
>
>     sub foo {
>     }
>
> Not:
>
>     sub foo
>     {
>     }

Yep, that seems worth fixing, too.  Patches follow, with a few unrelated
cleanups thrown in.  These patches are based against "maint" for no
particular reason.  Bugs and improvements welcome, of course. :)

Jonathan Nieder (7):
  t4018 (diff funcname patterns): make .gitattributes state easier to
    track
  t4018 (diff funcname patterns): make configuration easier to track
  t4018 (diff funcname patterns): minor tweaks
  userdiff/perl: anchor "sub" and "package" patterns on the left
  userdiff/perl: match full line of POD headers
  userdiff/perl: catch subs with brace on second line
  tests: make test_expect_code quieter on success

 t/t4018-diff-funcname.sh |  148 ++++++++++++++++++++++++++++++++++++---------
 t/test-lib.sh            |    7 +-
 userdiff.c               |   22 ++++++-
 3 files changed, 139 insertions(+), 38 deletions(-)

[PATCH 1/7] t4018 (funcname patterns): make .gitattributes state easier to track

[Jonathan Nieder]

Most, but not all, tests in this script rely on attributes declaring
that files with a .java extension should use the "java" driver:

	*.java diff=java

Split out a "set up" test to put such a .gitattributes in place after
the tests that do not want it have run, to make it more likely that
individual tests other than this setup test can be safely modified,
rearranged, or skipped.  Presumably this setup code will learn to
request other drivers for other extensions in the same place when the
test suite learns to exercise other diff drivers.

Similarly, make sure that early test assertions that do not use these
default attributes set up .gitattributes appropriately for themselves,
so tests that run before can be modified with less risk of breaking
something.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 t/t4018-diff-funcname.sh |    8 ++++++--
 1 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/t/t4018-diff-funcname.sh b/t/t4018-diff-funcname.sh
index 3646930..24eb1a3 100755
--- a/t/t4018-diff-funcname.sh
+++ b/t/t4018-diff-funcname.sh
@@ -36,11 +36,12 @@ builtin_patterns="bibtex cpp csharp fortran html java objc pascal perl php pytho
 for p in $builtin_patterns
 do
 	test_expect_success "builtin $p pattern compiles" '
-		echo "*.java diff=$p" > .gitattributes &&
+		echo "*.java diff=$p" >.gitattributes &&
 		! { git diff --no-index Beer.java Beer-correct.java 2>&1 |
 			grep "fatal" > /dev/null; }
 	'
 	test_expect_success "builtin $p wordRegex pattern compiles" '
+		echo "*.java diff=$p" >.gitattributes &&
 		! { git diff --no-index --word-diff \
 			Beer.java Beer-correct.java 2>&1 |
 			grep "fatal" > /dev/null; }
@@ -53,8 +54,11 @@ test_expect_success 'default behaviour' '
 	grep "^@@.*@@ public class Beer"
 '
 
+test_expect_success 'set up .gitattributes declaring drivers to test' '
+	echo "*.java diff=java" >.gitattributes
+'
+
 test_expect_success 'preset java pattern' '
-	echo "*.java diff=java" >.gitattributes &&
 	git diff --no-index Beer.java Beer-correct.java |
 	grep "^@@.*@@ public static void main("
 '
-- 
1.7.5.1

[PATCH 2/7] t4018 (funcname patterns): make configuration easier to track

[Jonathan Nieder]

Introduce a "test_config" function to set a configuration variable
for use by a single test (automatically unsetting it when the
assertion finishes).  If this function is used consistently, the
configuration used in a test_expect_success block can be read at the
beginning of that block instead of requiring reading all the tests
that come before.  So it becomes a little easier to add new tests or
rearrange existing ones without fear of breaking configuration.

In particular, the test of alternation in xfuncname patterns also
checks that xfuncname takes precedence over funcname variable as a
sort of side-effect, since the latter leaks in from previous tests.
In the new syntax, the test has to say explicitly what variables it is
using, making the test clearer and a future regression in coverage
from carelessly editing the script less likely.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 t/t4018-diff-funcname.sh |   19 ++++++++++++-------
 1 files changed, 12 insertions(+), 7 deletions(-)

diff --git a/t/t4018-diff-funcname.sh b/t/t4018-diff-funcname.sh
index 24eb1a3..ce0a0e3 100755
--- a/t/t4018-diff-funcname.sh
+++ b/t/t4018-diff-funcname.sh
@@ -32,6 +32,11 @@ EOF
 
 sed 's/beer\\/beer,\\/' < Beer.java > Beer-correct.java
 
+test_config () {
+	git config "$1" "$2" &&
+	test_when_finished "git config --unset $1"
+}
+
 builtin_patterns="bibtex cpp csharp fortran html java objc pascal perl php python ruby tex"
 for p in $builtin_patterns
 do
@@ -63,29 +68,29 @@ test_expect_success 'preset java pattern' '
 	grep "^@@.*@@ public static void main("
 '
 
-git config diff.java.funcname '!static
-!String
-[^ 	].*s.*'
-
 test_expect_success 'custom pattern' '
+	test_config diff.java.funcname "!static
+!String
+[^ 	].*s.*" &&
 	git diff --no-index Beer.java Beer-correct.java |
 	grep "^@@.*@@ int special;$"
 '
 
 test_expect_success 'last regexp must not be negated' '
-	git config diff.java.funcname "!static" &&
+	test_config diff.java.funcname "!static" &&
 	git diff --no-index Beer.java Beer-correct.java 2>&1 |
 	grep "fatal: Last expression must not be negated:"
 '
 
 test_expect_success 'pattern which matches to end of line' '
-	git config diff.java.funcname "Beer$" &&
+	test_config diff.java.funcname "Beer$" &&
 	git diff --no-index Beer.java Beer-correct.java |
 	grep "^@@.*@@ Beer"
 '
 
 test_expect_success 'alternation in pattern' '
-	git config diff.java.xfuncname "^[ 	]*((public|static).*)$" &&
+	test_config diff.java.funcname "Beer$" &&
+	test_config diff.java.xfuncname "^[ 	]*((public|static).*)$" &&
 	git diff --no-index Beer.java Beer-correct.java |
 	grep "^@@.*@@ public static void main("
 '
-- 
1.7.5.1

[PATCH 3/7] t4018 (funcname patterns): minor cleanups

[Jonathan Nieder]

Introduce a test_expect_funcname function to make a diff and apply a
regexp anchored on the left to the function name it writes, avoiding
some repetition.

Omit the space after >, <<, and < operators for consistency with
other scripts.  Quote the <<here document delimiter and $ signs in
quotes so readers don't have to worry about the effect of shell
metacharacters.

Remove some unnecessary blank lines.

Run "git diff" as a separate command instead of as upstream of a pipe
that checks its output, so the exit status can be tested.  In
particular, this way if "git diff" starts segfaulting the test harness
will notice.

Allow "error:" as a synonym for "fatal:" when checking error messages,
since whether a command uses die() or "return error()" is a small
implementation detail.

Anchor some more regexes on the right.

None of the above is very important on its own; the point is just to
make the script a little easier to read and the code less scary to
modify.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 t/t4018-diff-funcname.sh |   49 +++++++++++++++++++++++----------------------
 1 files changed, 25 insertions(+), 24 deletions(-)

diff --git a/t/t4018-diff-funcname.sh b/t/t4018-diff-funcname.sh
index ce0a0e3..ad74c60 100755
--- a/t/t4018-diff-funcname.sh
+++ b/t/t4018-diff-funcname.sh
@@ -9,8 +9,7 @@ test_description='Test custom diff function name patterns'
 
 LF='
 '
-
-cat > Beer.java << EOF
+cat >Beer.java <<\EOF
 public class Beer
 {
 	int special;
@@ -29,34 +28,40 @@ public class Beer
 	}
 }
 EOF
-
-sed 's/beer\\/beer,\\/' < Beer.java > Beer-correct.java
+sed 's/beer\\/beer,\\/' <Beer.java >Beer-correct.java
 
 test_config () {
 	git config "$1" "$2" &&
 	test_when_finished "git config --unset $1"
 }
 
-builtin_patterns="bibtex cpp csharp fortran html java objc pascal perl php python ruby tex"
-for p in $builtin_patterns
+test_expect_funcname () {
+	test_expect_code 1 git diff --no-index \
+		Beer.java Beer-correct.java >diff &&
+	grep "^@@.*@@ $1" diff
+}
+
+for p in bibtex cpp csharp fortran html java objc pascal perl php python ruby tex
 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; }
+		test_expect_code 1 git diff --no-index \
+			Beer.java Beer-correct.java 2>msg &&
+		! grep fatal msg &&
+		! grep error msg
 	'
 	test_expect_success "builtin $p wordRegex pattern compiles" '
 		echo "*.java diff=$p" >.gitattributes &&
-		! { git diff --no-index --word-diff \
-			Beer.java Beer-correct.java 2>&1 |
-			grep "fatal" > /dev/null; }
+		test_expect_code 1 git diff --no-index --word-diff \
+			Beer.java Beer-correct.java 2>msg &&
+		! grep fatal msg &&
+		! grep error msg
 	'
 done
 
 test_expect_success 'default behaviour' '
 	rm -f .gitattributes &&
-	git diff --no-index Beer.java Beer-correct.java |
-	grep "^@@.*@@ public class Beer"
+	test_expect_funcname "public class Beer\$"
 '
 
 test_expect_success 'set up .gitattributes declaring drivers to test' '
@@ -64,35 +69,31 @@ test_expect_success 'set up .gitattributes declaring drivers to test' '
 '
 
 test_expect_success 'preset java pattern' '
-	git diff --no-index Beer.java Beer-correct.java |
-	grep "^@@.*@@ public static void main("
+	test_expect_funcname "public static void main("
 '
 
 test_expect_success 'custom pattern' '
 	test_config diff.java.funcname "!static
 !String
 [^ 	].*s.*" &&
-	git diff --no-index Beer.java Beer-correct.java |
-	grep "^@@.*@@ int special;$"
+	test_expect_funcname "int special;\$"
 '
 
 test_expect_success 'last regexp must not be negated' '
 	test_config diff.java.funcname "!static" &&
-	git diff --no-index Beer.java Beer-correct.java 2>&1 |
-	grep "fatal: Last expression must not be negated:"
+	test_expect_code 128 git diff --no-index Beer.java Beer-correct.java 2>msg &&
+	grep ": Last expression must not be negated:" msg
 '
 
 test_expect_success 'pattern which matches to end of line' '
-	test_config diff.java.funcname "Beer$" &&
-	git diff --no-index Beer.java Beer-correct.java |
-	grep "^@@.*@@ Beer"
+	test_config diff.java.funcname "Beer\$" &&
+	test_expect_funcname "Beer\$"
 '
 
 test_expect_success 'alternation in pattern' '
 	test_config diff.java.funcname "Beer$" &&
 	test_config diff.java.xfuncname "^[ 	]*((public|static).*)$" &&
-	git diff --no-index Beer.java Beer-correct.java |
-	grep "^@@.*@@ public static void main("
+	test_expect_funcname "public static void main("
 '
 
 test_done
-- 
1.7.5.1

[PATCH 4/7] userdiff/perl: anchor "sub" and "package" patterns on the left

[Jonathan Nieder]

The userdiff funcname mechanism has no concept of nested scopes ---
instead, "git diff" and "git grep --show-function" simply label the
diff header with the most recent matching line.  Unfortunately that
means text following a subroutine in a POD section:

	=head1 DESCRIPTION

	You might use this facility like so:

		sub example {
			foo;
		}

	Now, having said that, let's say more about the facility.
	Blah blah blah ... etc etc.

gets the subroutine name instead of the POD header in its diff/grep
funcname header, making it harder to get oriented when reading a
diff without enough context.

The fix is simple: anchor the funcname syntax to the left margin so
nested subroutines and packages like this won't get picked up.  (The
builtin C++ funcname pattern already does the same thing.)  This means
the userdiff driver will misparse the idiom

	{
		my $static;
		sub foo {
			... use $static ...
		}
	}

but I think that's worth it; we can revisit this later if the userdiff
mechanism learns to keep track of the beginning and end of nested
scopes.

Reported-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 t/t4018-diff-funcname.sh |   59 +++++++++++++++++++++++++++++++++++++++++++--
 userdiff.c               |    4 +-
 2 files changed, 58 insertions(+), 5 deletions(-)

diff --git a/t/t4018-diff-funcname.sh b/t/t4018-diff-funcname.sh
index ad74c60..f071a8f 100755
--- a/t/t4018-diff-funcname.sh
+++ b/t/t4018-diff-funcname.sh
@@ -29,6 +29,47 @@ public class Beer
 }
 EOF
 sed 's/beer\\/beer,\\/' <Beer.java >Beer-correct.java
+cat >Beer.perl <<\EOF
+package Beer;
+
+use strict;
+use warnings;
+use parent qw(Exporter);
+our @EXPORT_OK = qw(round);
+
+sub round {
+	my ($n) = @_;
+	print "$n bottles of beer on the wall ";
+	print "$n bottles of beer\n";
+	print "Take one down, pass it around, ";
+	$n = $n - 1;
+	print "$n bottles of beer on the wall.\n";
+}
+
+__END__
+
+=head1 NAME
+
+Beer - subroutine to output fragment of a drinking song
+
+=head1 SYNOPSIS
+
+	use Beer qw(round);
+
+	sub song {
+		for (my $i = 99; $i > 0; $i--) {
+			round $i;
+		}
+	}
+
+	song;
+
+=cut
+EOF
+sed -e '
+	s/beer\\/beer,\\/
+	s/song;/song();/
+' <Beer.perl >Beer-correct.perl
 
 test_config () {
 	git config "$1" "$2" &&
@@ -36,8 +77,9 @@ test_config () {
 }
 
 test_expect_funcname () {
-	test_expect_code 1 git diff --no-index \
-		Beer.java Beer-correct.java >diff &&
+	lang=${2-java}
+	test_expect_code 1 git diff --no-index -U1 \
+		"Beer.$lang" "Beer-correct.$lang" >diff &&
 	grep "^@@.*@@ $1" diff
 }
 
@@ -65,13 +107,24 @@ test_expect_success 'default behaviour' '
 '
 
 test_expect_success 'set up .gitattributes declaring drivers to test' '
-	echo "*.java diff=java" >.gitattributes
+	cat >.gitattributes <<-\EOF
+	*.java diff=java
+	*.perl diff=perl
+	EOF
 '
 
 test_expect_success 'preset java pattern' '
 	test_expect_funcname "public static void main("
 '
 
+test_expect_success 'preset perl pattern' '
+	test_expect_funcname "sub round {\$" perl
+'
+
+test_expect_success 'perl pattern is not distracted by sub within POD' '
+	test_expect_funcname "=head" perl
+'
+
 test_expect_success 'custom pattern' '
 	test_config diff.java.funcname "!static
 !String
diff --git a/userdiff.c b/userdiff.c
index 1ff4797..2cca0af 100644
--- a/userdiff.c
+++ b/userdiff.c
@@ -60,8 +60,8 @@ PATTERNS("pascal",
 	 "|[-+0-9.e]+|0[xXbB]?[0-9a-fA-F]+"
 	 "|<>|<=|>=|:=|\\.\\."),
 PATTERNS("perl",
-	 "^[ \t]*package .*;\n"
-	 "^[ \t]*sub .* \\{\n"
+	 "^package .*;\n"
+	 "^sub .* \\{\n"
 	 "^[A-Z]+ \\{\n"	/* BEGIN, END, ... */
 	 "^=head[0-9] ",	/* POD */
 	 /* -- */
-- 
1.7.5.1

[PATCH 5/7] userdiff/perl: match full line of POD headers

[Jonathan Nieder]

The builtin perl userdiff driver is not greedy enough about catching
POD header lines.  Capture the whole line, so instead of just
declaring that we are in some "@@ =head1" section, diff/grep output
can explain that the enclosing section is about "@@ =head1 OPTIONS".

Reported-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 t/t4018-diff-funcname.sh |    4 ++++
 userdiff.c               |    2 +-
 2 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/t/t4018-diff-funcname.sh b/t/t4018-diff-funcname.sh
index f071a8f..8a57149 100755
--- a/t/t4018-diff-funcname.sh
+++ b/t/t4018-diff-funcname.sh
@@ -125,6 +125,10 @@ test_expect_success 'perl pattern is not distracted by sub within POD' '
 	test_expect_funcname "=head" perl
 '
 
+test_expect_success 'perl pattern gets full line of POD header' '
+	test_expect_funcname "=head1 SYNOPSIS\$" perl
+'
+
 test_expect_success 'custom pattern' '
 	test_config diff.java.funcname "!static
 !String
diff --git a/userdiff.c b/userdiff.c
index 2cca0af..32ead96 100644
--- a/userdiff.c
+++ b/userdiff.c
@@ -63,7 +63,7 @@ PATTERNS("perl",
 	 "^package .*;\n"
 	 "^sub .* \\{\n"
 	 "^[A-Z]+ \\{\n"	/* BEGIN, END, ... */
-	 "^=head[0-9] ",	/* POD */
+	 "^=head[0-9] .*",	/* POD */
 	 /* -- */
 	 "[[:alpha:]_'][[:alnum:]_']*"
 	 "|0[xb]?[0-9a-fA-F_]*"
-- 
1.7.5.1

[PATCH 6/7] userdiff/perl: catch sub with brace on second line

[Jonathan Nieder]

Accept

	sub foo
	{
	}

as an alternative to a more common style that introduces perl
functions with a brace on the first line (and likewise for BEGIN/END
blocks).  The new regex is a little hairy to avoid matching

	# forward declaration
	sub foo;

while continuing to match "sub foo($;@) {" and

	sub foo { # This routine is interesting;
		# in fact, the lines below explain how...

While at it, pay attention to Perl 5.14's "package foo {" syntax as an
alternative to the traditional "package foo;".

Requested-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 t/t4018-diff-funcname.sh |   25 +++++++++++++++++++++++--
 userdiff.c               |   20 +++++++++++++++++---
 2 files changed, 40 insertions(+), 5 deletions(-)

diff --git a/t/t4018-diff-funcname.sh b/t/t4018-diff-funcname.sh
index 8a57149..b2fd1a9 100755
--- a/t/t4018-diff-funcname.sh
+++ b/t/t4018-diff-funcname.sh
@@ -35,7 +35,11 @@ package Beer;
 use strict;
 use warnings;
 use parent qw(Exporter);
-our @EXPORT_OK = qw(round);
+our @EXPORT_OK = qw(round finalround);
+
+sub other; # forward declaration
+
+# hello
 
 sub round {
 	my ($n) = @_;
@@ -46,6 +50,12 @@ sub round {
 	print "$n bottles of beer on the wall.\n";
 }
 
+sub finalround
+{
+	print "Go to the store, buy some more\n";
+	print "99 bottles of beer on the wall.\n");
+}
+
 __END__
 
 =head1 NAME
@@ -54,12 +64,13 @@ Beer - subroutine to output fragment of a drinking song
 
 =head1 SYNOPSIS
 
-	use Beer qw(round);
+	use Beer qw(round finalround);
 
 	sub song {
 		for (my $i = 99; $i > 0; $i--) {
 			round $i;
 		}
+		finalround;
 	}
 
 	song;
@@ -67,7 +78,9 @@ Beer - subroutine to output fragment of a drinking song
 =cut
 EOF
 sed -e '
+	s/hello/goodbye/
 	s/beer\\/beer,\\/
+	s/more\\/more,\\/
 	s/song;/song();/
 ' <Beer.perl >Beer-correct.perl
 
@@ -121,6 +134,10 @@ test_expect_success 'preset perl pattern' '
 	test_expect_funcname "sub round {\$" perl
 '
 
+test_expect_success 'perl pattern accepts K&R style brace placement, too' '
+	test_expect_funcname "sub finalround\$" perl
+'
+
 test_expect_success 'perl pattern is not distracted by sub within POD' '
 	test_expect_funcname "=head" perl
 '
@@ -129,6 +146,10 @@ test_expect_success 'perl pattern gets full line of POD header' '
 	test_expect_funcname "=head1 SYNOPSIS\$" perl
 '
 
+test_expect_success 'perl pattern is not distracted by forward declaration' '
+	test_expect_funcname "package Beer;\$" perl
+'
+
 test_expect_success 'custom pattern' '
 	test_config diff.java.funcname "!static
 !String
diff --git a/userdiff.c b/userdiff.c
index 32ead96..42b86ac 100644
--- a/userdiff.c
+++ b/userdiff.c
@@ -60,9 +60,23 @@ PATTERNS("pascal",
 	 "|[-+0-9.e]+|0[xXbB]?[0-9a-fA-F]+"
 	 "|<>|<=|>=|:=|\\.\\."),
 PATTERNS("perl",
-	 "^package .*;\n"
-	 "^sub .* \\{\n"
-	 "^[A-Z]+ \\{\n"	/* BEGIN, END, ... */
+	 "^package .*\n"
+	 "^sub [[:alnum:]_':]+[ \t]*"
+		"(\\([^)]*\\)[ \t]*)?" /* prototype */
+		/*
+		 * Attributes.  A regex can't count nested parentheses,
+		 * so just slurp up whatever we see, taking care not
+		 * to accept lines like "sub foo; # defined elsewhere".
+		 *
+		 * An attribute could contain a semicolon, but at that
+		 * point it seems reasonable enough to give up.
+		 */
+		"(:[^;#]*)?"
+		"(\\{[ \t]*)?" /* brace can come here or on the next line */
+		"(#.*)?$\n" /* comment */
+	 "^[A-Z]+[ \t]*"	/* BEGIN, END, ... */
+		"(\\{[ \t]*)?" /* brace can come here or on the next line */
+		"(#.*)?$\n"
 	 "^=head[0-9] .*",	/* POD */
 	 /* -- */
 	 "[[:alpha:]_'][[:alnum:]_']*"
-- 
1.7.5.1

[PATCH 8/7] userdiff/perl: tighten BEGIN/END block pattern to reject here-doc delimiters

[Jonathan Nieder]

A naive method of treating BEGIN/END blocks with a brace on the second
line as diff/grep funcname context involves also matching unrelated
lines that consist of all-caps letters:

	sub foo {
		print <<'EOF'
	text goes here
	...
	EOF
		... rest of foo ...
	}

That's not so great, because it means that "git diff" and "git grep
--show-function" would write "=EOF" or "@@ EOF" as context instead of
a more useful reminder like "@@ sub foo {".

To avoid this, tighten the pattern to only match the special block
names that perl accepts (namely BEGIN, END, INIT, CHECK, UNITCHECK,
AUTOLOAD, and DESTROY).  The list is taken from perl's toke.c.

Suggested-by: Jakub Narebski <jnareb@gmail.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Jonathan Nieder wrote:

> Accept
>
> 	sub foo
> 	{
> 	}
>
> as an alternative to a more common style that introduces perl
> functions with a brace on the first line (and likewise for BEGIN/END
> blocks).

I just remembered that this pattern is overzealous since it will match
here-document terminators, as in:

	sub foo {
		print <<'FINIS'
	here-doc goes here
	FINIS
		... rest of foo, a change to which might be described in
		a diff ...
	}

Fix follows.

 t/t4018-diff-funcname.sh |   17 +++++++++++++++--
 userdiff.c               |    2 +-
 2 files changed, 16 insertions(+), 3 deletions(-)

diff --git a/t/t4018-diff-funcname.sh b/t/t4018-diff-funcname.sh
index b2fd1a9..b68c56b 100755
--- a/t/t4018-diff-funcname.sh
+++ b/t/t4018-diff-funcname.sh
@@ -29,7 +29,7 @@ public class Beer
 }
 EOF
 sed 's/beer\\/beer,\\/' <Beer.java >Beer-correct.java
-cat >Beer.perl <<\EOF
+cat >Beer.perl <<\EOT
 package Beer;
 
 use strict;
@@ -56,6 +56,15 @@ sub finalround
 	print "99 bottles of beer on the wall.\n");
 }
 
+sub withheredocument {
+	print <<"EOF"
+decoy here-doc
+EOF
+	# some lines of context
+	# to pad it out
+	print "hello\n";
+}
+
 __END__
 
 =head1 NAME
@@ -76,7 +85,7 @@ Beer - subroutine to output fragment of a drinking song
 	song;
 
 =cut
-EOF
+EOT
 sed -e '
 	s/hello/goodbye/
 	s/beer\\/beer,\\/
@@ -138,6 +147,10 @@ test_expect_success 'perl pattern accepts K&R style brace placement, too' '
 	test_expect_funcname "sub finalround\$" perl
 '
 
+test_expect_success 'but is not distracted by end of <<here document' '
+	test_expect_funcname "sub withheredocument {\$" perl
+'
+
 test_expect_success 'perl pattern is not distracted by sub within POD' '
 	test_expect_funcname "=head" perl
 '
diff --git a/userdiff.c b/userdiff.c
index 42b86ac..e55310c 100644
--- a/userdiff.c
+++ b/userdiff.c
@@ -74,7 +74,7 @@ PATTERNS("perl",
 		"(:[^;#]*)?"
 		"(\\{[ \t]*)?" /* brace can come here or on the next line */
 		"(#.*)?$\n" /* comment */
-	 "^[A-Z]+[ \t]*"	/* BEGIN, END, ... */
+	 "^(BEGIN|END|INIT|CHECK|UNITCHECK|AUTOLOAD|DESTROY)[ \t]*"
 		"(\\{[ \t]*)?" /* brace can come here or on the next line */
 		"(#.*)?$\n"
 	 "^=head[0-9] .*",	/* POD */
-- 
1.7.5.1

[PATCH 7/7] tests: make test_expect_code quieter on success

[Jonathan Nieder]

A command exiting with the expected status is not particularly
notable.

While the indication of progress might be useful when tracking down
where in a test a failure has happened, the same applies to most other
test helpers, which are quiet about success, so this single helper's
output stands out in an unpleasant way.  An alternative method for
showing progress information might to invent a --progress option that
runs tests with "set -x", or until that is available, to run tests
using commands like

	prove -v -j2 --shuffle --exec='sh -x' t2202-add-addremove.sh

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
That's the end of the series.  This last one should probably have
been called 7/6 or sent separately as an RFC, since it's more a
matter of taste.  It was just something I ran into while trying out
the tests.

Hopefully the series wasn't too boring.  Thanks for reading.

 t/test-lib.sh |    7 +++----
 1 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/t/test-lib.sh b/t/test-lib.sh
index 8a274fb..a174f66 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -731,12 +731,11 @@ test_expect_code () {
 	exit_code=$?
 	if test $exit_code = $want_code
 	then
-		echo >&2 "test_expect_code: command exited with $exit_code: $*"
 		return 0
-	else
-		echo >&2 "test_expect_code: command exited with $exit_code, we wanted $want_code $*"
-		return 1
 	fi
+
+	echo >&2 "test_expect_code: command exited with $exit_code, we wanted $want_code $*"
+	return 1
 }
 
 # test_cmp is a helper function to compare actual and expected output.
-- 
1.7.5.1

Re: [PATCH 0/7] Re: Problems with Git's "perl" userdiff driver

[Ævar Arnfjörð Bjarmason]

On Sat, May 21, 2011 at 20:53, Jonathan Nieder <jrnieder@gmail.com> wrote:

> Jonathan Nieder (7):
>  t4018 (diff funcname patterns): make .gitattributes state easier to
>    track
>  t4018 (diff funcname patterns): make configuration easier to track
>  t4018 (diff funcname patterns): minor tweaks
>  userdiff/perl: anchor "sub" and "package" patterns on the left
>  userdiff/perl: match full line of POD headers
>  userdiff/perl: catch subs with brace on second line
>  tests: make test_expect_code quieter on success

This series gets my acked-by, it's a good improvement.

I agree that we should have some sort of scoring mechanism so that the
root cause of these issues can be solved. But that's something for
another series.