Re: [PATCH] git-svn: add --ignore-paths option for fetching

From: Thomas Rast <trast@student.ethz.ch>
Date: Jan 25, 2009, 14:21 UTC
Message-ID: <200901251521.15591.trast@student.ethz.ch>
In-Reply-To: <1232864842-8841-1-git-send-email-public_vi@tut.by>

Vitaly "_Vi" Shukela wrote:

> 
> Signed-off-by: Vitaly "_Vi" Shukela <public_vi@tut.by>

This would be a good place to explain why this is useful, and (if applicable) why you chose to implement it the way you did.

Show 13 quoted lines

> --- a/Documentation/git-svn.txt
> +++ b/Documentation/git-svn.txt
> @@ -96,6 +96,10 @@ COMMANDS
>  	Store Git commit times in the local timezone instead of UTC.  This
>  	makes 'git-log' (even without --date=local) show the same times
>  	that `svn log` would in the local timezone.
> +--ignore-paths=<regex>;;
> +	This allows one to specify regular expression that will
> +	cause skipping of all matching paths from checkout from SVN.
> +	Example: --ignore-paths='^doc'
>  
>  This doesn't interfere with interoperating with the Subversion
>  repository you cloned from, but if you wish for your local Git

You put the --ignore-paths explanation in the middle of the --localtime documentation (the last paragraph quoted still talks about --localtime).

Show 13 quoted lines

> @@ -3245,6 +3246,15 @@ use warnings;
>  use Carp qw/croak/;
>  use File::Temp qw/tempfile/;
>  use IO::File qw//;
> +use vars qw/ $ignoreRegex/;
> +
> +# 0 -- don't ignore, 1 -- ignore
> +sub isPathIgnored($) {
> +    return 0 unless defined($ignoreRegex);
> +    my $path = shift;
> +    return 1 if $path =~ m!^$ignoreRegex!o;
> +    return 0;
> +}

This is the first function in git-svn.perl using camelCase. Consider sticking to the current style and spelling it is_path_ignored().

Show 15 quoted lines

> @@ -3372,11 +3384,14 @@ sub add_file {
>  	my ($self, $path, $pb, $cp_path, $cp_rev) = @_;
>  	my $mode;
>  
> +	goto out if isPathIgnored($path);
> +
>  	if (!in_dot_git($path)) {
>  		my ($dir, $file) = ($path =~ m#^(.*?)/?([^/]+)$#);
>  		delete $self->{empty}->{$dir};
>  		$mode = '100644';
>  	}
> +out:
>  	{ path => $path, mode_a => $mode, mode_b => $mode,
>  	  pool => SVN::Pool->new, action => 'A' };
>  }

You broke the symmetry here, while all other hunks just add an equivalent check to the existing in_dot_git().

However, the latter makes me wonder if it would be cleaner to move the in_dot_git() test to isPathIgnored (er, is_path_ignored) too?

FWIW, I like the feature; it seems a good way to exclude subtrees with large blobs, and I know several SVN repos that have such a directory.

-- 
Thomas Rast
trast@{inf,student}.ethz.ch