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

From: "Alejandro R. Sedeño" <[email-masked]>
Date: Wed, 06 Apr 2011 11:34:27 -0400

On 04/06/2011 11:22 AM, Michael J Gruber wrote:
> Alejandro R. Sedeño venit, vidit, dixit 06.04.2011 17:05:
>> Signed-off-by: Alejandro R. Sedeño <[email-masked]>
>> Reviewed-off-by: James Y Knight <[email-masked]>
> 
> :) So, if that review is off, that means...

Um, s/-off//. Oops :)

I can send a follow-up, or let Eric deal with that change, however he prefers.

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

Yeah, I just went with full_pushurl returning full_url in cases where
pushurl is not set.

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

Because otherwise it would die with:

  $ git svn fetch
  config --get svn-remote.svn.pushurl: command returned error: 1

when pushurl wasn't defined. If that happens with 'url' too, well, that's
a mis-configured git-svn remote.

>> +sub full_pushurl {
>> +	my ($self) = @_;
> Isn't that a noop?

I'm just copying the style of sub full_url here.

-Alejandro