Re: [PATCH 1/5] Add a generic tree traversal to fetch SVN properties.

From: Eric Wong <normalperson@yhbt.net>
Date: Oct 16, 2007, 07:43 UTC
Message-ID: <20071016074310.GA32254@soma>
In-Reply-To: <1192462506-3783-1-git-send-email-tsuna@lrde.epita.fr>

Benoit Sigoure <tsuna@lrde.epita.fr> wrote:

Show 5 quoted lines

> 	* git-svn.perl (&traverse_ignore): Remove.
> 	(&prop_walk): New.
> 	(&cmd_show_ignore): Use prop_walk.
> 
> Signed-off-by: Benoit Sigoure <tsuna@lrde.epita.fr>

Although I myself have never needed this functionality, this series looks pretty good in general.

Thanks.

One comment below about property selection (whitelist vs blacklist).

It would be possible to get identical information out of unhandled.log, but older repositories may not have complete information... Maybe some local option would be good for people with complete unhandled.log files; but it could be really incomplete/insufficient.

I'm not sure about 5/5, it's purely a style issue, however I don't really feel strongly about a trailing "\n" either way... Nevertheless, it is definitely not part of this series and should be treated independently.

Coding style

Other than that, I prefer to keep braces on the same line as foreach, if, else statements. I generally follow the git and Linux coding style for C in my Perl code.

One exception that I make for Perl (but not C) is that I keep the "{" for subs on the same line (since subs can be nested and anonymous ones passed as arguments and such); unlike their C counterparts[1]

[1] - well, nesting functions is allowed in C99 or GNU C, I can't
      remember which or both...

Show 70 quoted lines

> ---
>  git-svn.perl |   66 ++++++++++++++++++++++++++++++++++++++++-----------------
>  1 files changed, 46 insertions(+), 20 deletions(-)
> 
> diff --git a/git-svn.perl b/git-svn.perl
> index 777e436..abc83ec 100755
> --- a/git-svn.perl
> +++ b/git-svn.perl
> @@ -488,7 +488,15 @@ sub cmd_show_ignore {
>  	my ($url, $rev, $uuid, $gs) = working_head_info('HEAD');
>  	$gs ||= Git::SVN->new;
>  	my $r = (defined $_revision ? $_revision : $gs->ra->get_latest_revnum);
> -	$gs->traverse_ignore(\*STDOUT, $gs->{path}, $r);
> +	$gs->prop_walk($gs->{path}, $r, sub {
> +		my ($gs, $path, $props) = @_;
> +		print STDOUT "\n# $path\n";
> +		my $s = $props->{'svn:ignore'} or return;
> +		$s =~ s/[\r\n]+/\n/g;
> +		chomp $s;
> +		$s =~ s#^#$path#gm;
> +		print STDOUT "$s\n";
> +	});
>  }
>  
>  sub cmd_multi_init {
> @@ -1480,28 +1488,46 @@ sub rel_path {
>  	$url;
>  }
>  
> -sub traverse_ignore {
> -	my ($self, $fh, $path, $r) = @_;
> -	$path =~ s#^/+##g;
> -	my $ra = $self->ra;
> -	my ($dirent, undef, $props) = $ra->get_dir($path, $r);
> +# prop_walk(PATH, REV, SUB)
> +# -------------------------
> +# Recursively traverse PATH at revision REV and invoke SUB for each
> +# directory that contains a SVN property.  SUB will be invoked as
> +# follows:  &SUB(gs, path, props);  where `gs' is this instance of
> +# Git::SVN, `path' the path to the directory where the properties
> +# `props' were found.  The `path' will be relative to point of checkout,
> +# that is, if url://repo/trunk is the current Git branch, and that
> +# directory contains a sub-directory `d', SUB will be invoked with `/d/'
> +# as `path' (note the trailing `/').
> +sub prop_walk {
> +	my ($self, $path, $rev, $sub) = @_;
> +
> +	my ($dirent, undef, $props) = $self->ra->get_dir($path, $rev);
> +	$path =~ s#^/*#/#g;
>  	my $p = $path;
> -	$p =~ s#^\Q$self->{path}\E(/|$)##;
> -	print $fh length $p ? "\n# $p\n" : "\n# /\n";
> -	if (my $s = $props->{'svn:ignore'}) {
> -		$s =~ s/[\r\n]+/\n/g;
> -		chomp $s;
> -		if (length $p == 0) {
> -			$s =~ s#\n#\n/$p#g;
> -			print $fh "/$s\n";
> -		} else {
> -			$s =~ s#\n#\n/$p/#g;
> -			print $fh "/$p/$s\n";
> -		}
> -	}
> +	# Strip the irrelevant part of the path.
> +	$p =~ s#^/+\Q$self->{path}\E(/|$)#/#;
> +	# Ensure the path is terminated by a `/'.
> +	$p =~ s#/*$#/#;
> +
> +	# The properties contain all the internal SVN stuff nobody
> +	# (usually) cares about.

How about having a blacklist (for the author, date, log, uuid?) instead
of a whitelist?  I can't remember all of them that should be blacklisted,
 but maybe it's just author, date and log)..

Show 20 quoted lines

> +	my $interesting_props = 0;
> +	foreach(keys %{$props})
> +	{
> +		# If it doesn't start with `svn:', it must be a
> +		# user-defined property.
> +		++$interesting_props and next if $_ !~ /^svn:/;
> +		# FIXME: Fragile, if SVN adds new public properties,
> +		# this needs to be updated.
> +		++$interesting_props if /^svn:(?:ignore|keywords|executable
> +		                                 |eol-style|mime-type
> +						 |externals|needs-lock)$/x;
> +	}
> +	&$sub($self, $p, $props) if $interesting_props;
> +
>  	foreach (sort keys %$dirent) {
>  		next if $dirent->{$_}->{kind} != $SVN::Node::dir;
> -		$self->traverse_ignore($fh, "$path/$_", $r);
> +		$self->prop_walk($path . '/' . $_, $rev, $sub);
>  	}
>  }

-- 
Eric Wong