Re: [PATCH] git-svn: Add a svn-remote.<name>.pushurl config key

[Michael J Gruber <git@drmicha.warpmail.net>]

Alejandro R. Sedeño venit, vidit, dixit 06.04.2011 17:05:
> Similar to the 'remote.<name>.pushurl' config key for git remotes,
> 'pushurl' is designed to be used in cases where 'url' points to an SVN
> repository via a read-only transport, to provide an alternate
> read/write transport. It is assumed that both keys point to the same
> repository.
> 
> The 'pushurl' key is distinct from the 'commiturl' key in that
> 'commiturl' is a full svn path while 'pushurl' (like 'url') is a base
> path. 'commiturl' takes precendece over 'pushurl' in cases where
> either might be used.
> 
> The 'pushurl' is used by git-svn's dcommit and branch commands.
> 

Thanks, very clear now.

> Signed-off-by: Alejandro R. Sedeño <asedeno@mit.edu>
> Reviewed-off-by: James Y Knight <jknight@itasoftware.com>

:) So, if that review is off, that means...

> ---
>  Documentation/git-svn.txt |   10 ++++++++++
>  git-svn.perl              |   18 +++++++++++++++---
>  2 files changed, 25 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/git-svn.txt b/Documentation/git-svn.txt
> index ea8fafd..4aa6404 100644
> --- a/Documentation/git-svn.txt
> +++ b/Documentation/git-svn.txt
> @@ -648,6 +648,16 @@ svn-remote.<name>.rewriteUUID::
>  	where the original UUID is not available via either useSvmProps
>  	or useSvnsyncProps.
>  
> +svn-remote.<name>.pushurl::
> +
> +	Similar to git's 'remote.<name>.pushurl', this key is designed
> +	to be used in cases where 'url' points to an SVN repository
> +	via a read-only transport, to provide an alternate read/write
> +	transport. It is assumed that both keys point to the same
> +	repository. Unlike 'commiturl', 'pushurl' is a base path. If
> +	either 'commiturl' or 'pushurl' could be used, 'commiturl'
> +	takes precedence.
> +
>  svn.brokenSymlinkWorkaround::
>  	This disables potentially expensive checks to workaround
>  	broken symlinks checked into SVN by broken clients.  Set this
> diff --git a/git-svn.perl b/git-svn.perl
> index fa8cd07..184442a 100755
> --- a/git-svn.perl
> +++ b/git-svn.perl
> @@ -531,7 +531,7 @@ sub cmd_dcommit {
>  		$url = eval { command_oneline('config', '--get',
>  			      "svn-remote.$gs->{repo_id}.commiturl") };
>  		if (!$url) {
> -			$url = $gs->full_url
> +			$url = $gs->full_pushurl

Wouldn't we want to do the same $gs->full_pushurl || $gs->full_url fall
back here as below, or is fullpush_url always set? OK, I see it always is.

>  		}
>  	}
>  
> @@ -679,7 +679,7 @@ sub cmd_branch {
>  	$head ||= 'HEAD';
>  
>  	my (undef, $rev, undef, $gs) = working_head_info($head);
> -	my $src = $gs->full_url;
> +	my $src = $gs->full_pushurl;

Same here.

>  
>  	my $remote = Git::SVN::read_all_remotes()->{$gs->{repo_id}};
>  	my $allglobs = $remote->{ $_tag ? 'tags' : 'branches' };
> @@ -730,7 +730,7 @@ sub cmd_branch {
>  		$url = eval { command_oneline('config', '--get',
>  			"svn-remote.$gs->{repo_id}.commiturl") };
>  		if (!$url) {
> -			$url = $remote->{url};
> +			$url = $remote->{pushurl} || $remote->{url};
>  		}
>  	}
>  	my $dst = join '/', $url, $lft, $branch_name, ($rgt || ());
> @@ -1834,6 +1834,8 @@ sub read_all_remotes {
>  			$r->{$1}->{svm} = {};
>  		} elsif (m!^(.+)\.url=\s*(.*)\s*$!) {
>  			$r->{$1}->{url} = $2;
> +		} elsif (m!^(.+)\.pushurl=\s*(.*)\s*$!) {
> +			$r->{$1}->{pushurl} = $2;
>  		} elsif (m!^(.+)\.(branches|tags)=$svn_refspec$!) {
>  			my ($remote, $t, $local_ref, $remote_ref) =
>  			                                     ($1, $2, $3, $4);
> @@ -2071,6 +2073,8 @@ sub new {
>  	$self->{url} = command_oneline('config', '--get',
>  	                               "svn-remote.$repo_id.url") or
>                    die "Failed to read \"svn-remote.$repo_id.url\" in config\n";
> +	$self->{pushurl} = eval { command_oneline('config', '--get',
> +	                          "svn-remote.$repo_id.pushurl") };

Why eval? We don't do it for url either.

>  	$self->rebuild;
>  	$self;
>  }
> @@ -2548,6 +2552,14 @@ sub full_url {
>  	$self->{url} . (length $self->{path} ? '/' . $self->{path} : '');
>  }
>  
> +sub full_pushurl {
> +	my ($self) = @_;

Isn't that a noop?

> +	if ($self->{pushurl}) {
> +		return $self->{pushurl} . (length $self->{path} ? '/' . $self->{path} : '');
> +	} else {
> +		return $self->full_url;
> +	}
> +}
>  
>  sub set_commit_header_env {
>  	my ($log_entry) = @_;