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:34:
> 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 <asedeno@mit.edu>
>>> Reviewed-off-by: James Y Knight <jknight@itasoftware.com>
>>
>> :) 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.
> 

OK, I finally understood the reason for "eval" and "my ($self) = @_;". I
simply shouldn't look at perl code ;)

Thanks!

Michael