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

From: Jonathan Nieder <jrnieder@gmail.com>
Date: May 21, 2011, 18:53 UTC
Message-ID: <20110521185314.GA10530@elie>
In-Reply-To: <BANLkTi=OXznTspN-CJjM0YXfqARxL=J+Ow@mail.gmail.com>

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:

Show 5 quoted lines

>     "^=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 ...

Show 10 quoted lines

> 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(-)