From: Eric Wong <normalperson@yhbt.net>
Date: Sat, 11 Apr 2009 01:22:57 GMT
Subject: Re: [PATCH 1/2] Add per-svn-remote ignore-paths config
Message-ID: <20090411012256.GA2945@dcvr.yhbt.net>
In-Reply-To: <1239314289-36149-1-git-send-email-ben@ben.com>

Ben Jackson <ben@ben.com> wrote:
> The --ignore-paths option to fetch is very useful for working on a subset
> of a SVN repository.  For proper operation, every command that causes a
> fetch (explicit or implied) must include a matching --ignore-paths option.
> 
> This patch adds a persistent svn-remote.$repo_id.ignore-paths config by
> promoting Fetcher::is_path_ignored to a member function and initializing
> $self->{ignore_regex} in Fetcher::new.  Command line --ignore-paths is
> still recognized and acts in addition to the config value.
> 
> Signed-off-by: Ben Jackson <ben@ben.com>

Hi Ben, the patch looks useful, but there are some minor issues
with comments inline.

> --- a/git-svn.perl
> +++ b/git-svn.perl
>  # return value: 0 -- don't ignore, 1 -- ignore
>  sub is_path_ignored {
> -	my ($path) = @_;
> +	my ($self, $path) = @_;
>  	return 1 if in_dot_git($path);
> +	return 1 if defined($self->{ignore_regex}) &&
> +	            $path =~ m!$self->{ignore_regex}!o;

Since we're making the regex per-remote, I'd remove the "o" operator. It
can break things if Git::SVN::Fetcher ever gets reused by multiple SVN
remotes within the same process.

> --- a/t/t9134-git-svn-ignore-paths.sh
> +++ b/t/t9134-git-svn-ignore-paths.sh
> @@ -44,7 +51,7 @@ test_expect_success 'SVN-side change outside of www' '
>  test_expect_success 'update git svn-cloned repo' '
>  	(
>  		cd g &&
> -		git svn rebase --ignore-paths="^www" &&
> +		git svn rebase &&

I'd rather not change the existing tests since existing behavior
can break.  Instead, augment the existing tests so we test both
the .git/config and command-line cases.

Thanks.

-- 
Eric Wong