Re: [PATCH 1/2] Add per-svn-remote ignore-paths config

From: Eric Wong <normalperson@yhbt.net>
Date: Apr 11, 2009, 01:22 UTC
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:

Show 10 quoted lines

> 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.

Show 9 quoted lines

> --- 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.

Show 8 quoted lines

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