"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@xxxxxxx>
>>> Reviewed-off-by: James Y Knight <jknight@xxxxxxxxxxxxxxx>
>>
>> :) 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html