[PATCH][resend] git-svn: Respect GIT_SSH setting

[PATCH][resend] git-svn: Respect GIT_SSH setting

[Karthik R]

Setting GIT_SSH when using "git svn clone svn+ssh://..." does not
override the default ssh; SVN_SSH needed to be set instead. Fixed
this.

Also, on Windows, SVN_SSH needs to be set with \ escaped
  e.g., "C:\\PuTTY\\plink.exe"

See http://code.google.com/p/msysgit/issues/detail?id=305

Signed-off-by: Karthik R <karthikr@fastmail.fm>
---

Originally sent as [PATCH] GIT_SSH does not override ssh in git-svn

 git-svn.perl |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/git-svn.perl b/git-svn.perl
index b0bfb74..9bc1e71 100755
--- a/git-svn.perl
+++ b/git-svn.perl
@@ -21,6 +21,13 @@ $Git::SVN::default_ref_id = $ENV{GIT_SVN_ID} || 
'git-svn';
 $Git::SVN::Ra::_log_window_size = 100;
 $Git::SVN::_minimize_url = 'unset';
 
+# If GIT_SSH is set, also set SVN_SSH...
+$ENV{SVN_SSH} = $ENV{GIT_SSH} if defined $ENV{GIT_SSH};
+# ... and escape \s in shell-variable on Windows
+if ($^O eq 'MSWin32' || $^O eq 'msys') {
+       $ENV{SVN_SSH} =~ s/\\/\\\\/g if defined $ENV{SVN_SSH};
+}
+
 $Git::SVN::Log::TZ = $ENV{TZ};
 $ENV{TZ} = 'UTC';
 $| = 1; # unbuffer STDOUT
-- 1.5.4.3

Re: [PATCH][resend] git-svn: Respect GIT_SSH setting

[Johannes Schindelin]

Hi,

On Mon, 17 Aug 2009, Karthik R wrote:

> Setting GIT_SSH when using "git svn clone svn+ssh://..." does not
> override the default ssh; SVN_SSH needed to be set instead.

This is now in past tense, no?

> diff --git a/git-svn.perl b/git-svn.perl
> index b0bfb74..9bc1e71 100755
> --- a/git-svn.perl
> +++ b/git-svn.perl
> @@ -21,6 +21,13 @@ $Git::SVN::default_ref_id = $ENV{GIT_SVN_ID} || 'git-svn';
> $Git::SVN::Ra::_log_window_size = 100;
> $Git::SVN::_minimize_url = 'unset';
> 
> +# If GIT_SSH is set, also set SVN_SSH...
> +$ENV{SVN_SSH} = $ENV{GIT_SSH} if defined $ENV{GIT_SSH};
> +# ... and escape \s in shell-variable on Windows
> +if ($^O eq 'MSWin32' || $^O eq 'msys') {
> +       $ENV{SVN_SSH} =~ s/\\/\\\\/g if defined $ENV{SVN_SSH};
> +}

This is a change from before... I do not know if it is a good one, as 
SVN_SSH could be defined differently by the user, no?  In that case, the 
user was most likely using the correct amount of backslashes...

So maybe it was correct to make this dependent on "if defined 
$ENV{GIT_SSH}", and maybe it should be dependent on "&& !defined 
$ENV{SVN_SSH}" as well...

Ciao,
Dscho

Re: [PATCH][resend] git-svn: Respect GIT_SSH setting

[Junio C Hamano]

Karthik R <karthikr@fastmail.fm> writes:

> +# If GIT_SSH is set, also set SVN_SSH...
> +$ENV{SVN_SSH} = $ENV{GIT_SSH} if defined $ENV{GIT_SSH};
> +# ... and escape \s in shell-variable on Windows
> +if ($^O eq 'MSWin32' || $^O eq 'msys') {
> +       $ENV{SVN_SSH} =~ s/\\/\\\\/g if defined $ENV{SVN_SSH};
> +}
> +

Two questions.

 - What if a user has SVN_SSH exported _and_ wants to use a different one
   from the one s/he uses for git?  Naturally such a user would set both
   environment variables and differently, but this seems to override the
   value in SVN_SSH;

 - Can a user have SVN_SSH exported, on MSWin32 or msys, and use svn
   outside git?  If so, what does the value of SVN_SSH look like?  Does it
   typically have necessary doubling of backslashes already?

What I am getting at is, if the patch should look something like this
instead:

	if (! exists $ENV{SVN_SSH}) {
		if (exists $ENV{GIT_SSH}) {
			$ENV{SVN_SSH} = $ENV{GIT_SSH};
			if ($^O eq 'MSWin32' || $^O eq 'msys') {
                               $ENV{SVN_SSH} =~ s/\\/\\\\/g;
			}
		}
	}

Re: [PATCH][resend] git-svn: Respect GIT_SSH setting

[Karthik R]

Junio C Hamano wrote:
> Karthik R <karthikr@fastmail.fm> writes:
>
>   
>> +# If GIT_SSH is set, also set SVN_SSH...
>> +$ENV{SVN_SSH} = $ENV{GIT_SSH} if defined $ENV{GIT_SSH};
>> +# ... and escape \s in shell-variable on Windows
>> +if ($^O eq 'MSWin32' || $^O eq 'msys') {
>> +       $ENV{SVN_SSH} =~ s/\\/\\\\/g if defined $ENV{SVN_SSH};
>> +}
>> +
>>     
>
> Two questions.
>
>  - What if a user has SVN_SSH exported _and_ wants to use a different one
>    from the one s/he uses for git?  Naturally such a user would set both
>    environment variables and differently, but this seems to override the
>    value in SVN_SSH;
>   
Do you mean user wants to use a different one with "git svn ... 
svn+ssh://" (than the one with "git clone ssh://") ?
In this case
- defining SVN_SSH, but not GIT_SSH will still work (with this patch, 
GIT_SSH overrides)
- but SVN_SSH needs to have \\s.
So unless the user already knew of this quirk, we'll only see unescaped 
\s - so it *does* make sense to escape the \s (if the user knew, then 
too many escaped \s still work).
>  - Can a user have SVN_SSH exported, on MSWin32 or msys, and use svn
>    outside git?  If so, what does the value of SVN_SSH look like?  Does it
>    typically have necessary doubling of backslashes already?
>   
With subversion for Windows, these \\s are not needed (but doesn't cause 
any break). The doubling is something to do with the bash (in msys) I think.
> What I am getting at is, if the patch should look something like this
> instead:
>
> 	if (! exists $ENV{SVN_SSH}) {
> 		if (exists $ENV{GIT_SSH}) {
> 			$ENV{SVN_SSH} = $ENV{GIT_SSH};
> 			if ($^O eq 'MSWin32' || $^O eq 'msys') {
>                                $ENV{SVN_SSH} =~ s/\\/\\\\/g;
> 			}
> 		}
> 	}
>
>   

Re: [PATCH][resend] git-svn: Respect GIT_SSH setting

[Karthik R]

Johannes Schindelin wrote:
> Hi,
>
> On Mon, 17 Aug 2009, Karthik R wrote:
>
>   
>> Setting GIT_SSH when using "git svn clone svn+ssh://..." does not
>> override the default ssh; SVN_SSH needed to be set instead.
>>     
>
> This is now in past tense, no?
>
>   
Yes... this is all in the past tense now :) ... should be "did not 
override the default ssh". I'll fix it if I have to resend the patch for 
a different reason.
>> diff --git a/git-svn.perl b/git-svn.perl
>> index b0bfb74..9bc1e71 100755
>> --- a/git-svn.perl
>> +++ b/git-svn.perl
>> @@ -21,6 +21,13 @@ $Git::SVN::default_ref_id = $ENV{GIT_SVN_ID} || 'git-svn';
>> $Git::SVN::Ra::_log_window_size = 100;
>> $Git::SVN::_minimize_url = 'unset';
>>
>> +# If GIT_SSH is set, also set SVN_SSH...
>> +$ENV{SVN_SSH} = $ENV{GIT_SSH} if defined $ENV{GIT_SSH};
>> +# ... and escape \s in shell-variable on Windows
>> +if ($^O eq 'MSWin32' || $^O eq 'msys') {
>> +       $ENV{SVN_SSH} =~ s/\\/\\\\/g if defined $ENV{SVN_SSH};
>> +}
>>     
>
> This is a change from before... I do not know if it is a good one, as 
> SVN_SSH could be defined differently by the user, no?  In that case, the 
> user was most likely using the correct amount of backslashes...
>   
Dscho, The *correct* amount of backslashes is 1 (per dir) - same as used 
with GIT_SSH. If the user has set SVN_SSH but not GIT_SSH (most likely 
without escaping \), then fixing up SVN_SSH for use with git-svn is not 
a bad thing.

I did this change to retain existing behavior (using SVN_SSH to 
override) even when user doesn't know the \\ quirk - or if the user has 
set it for some other non-msys version of svn.
> So maybe it was correct to make this dependent on "if defined 
> $ENV{GIT_SSH}", and maybe it should be dependent on "&& !defined 
> $ENV{SVN_SSH}" as well...
>
> Ciao,
> Dscho
>
>   

Re: [PATCH][resend] git-svn: Respect GIT_SSH setting

[Karthik R]

Karthik R wrote:
> Junio C Hamano wrote:
>> Karthik R <karthikr@fastmail.fm> writes:
>>
>>  
>>> +# If GIT_SSH is set, also set SVN_SSH...
>>> +$ENV{SVN_SSH} = $ENV{GIT_SSH} if defined $ENV{GIT_SSH};
>>> +# ... and escape \s in shell-variable on Windows
>>> +if ($^O eq 'MSWin32' || $^O eq 'msys') {
>>> +       $ENV{SVN_SSH} =~ s/\\/\\\\/g if defined $ENV{SVN_SSH};
>>> +}
>>> +
>>>     
>>
>> Two questions.
>>
>>  - What if a user has SVN_SSH exported _and_ wants to use a different 
>> one
>>    from the one s/he uses for git?  Naturally such a user would set both
>>    environment variables and differently, but this seems to override the
>>    value in SVN_SSH;
>>   
> Do you mean user wants to use a different one with "git svn ... 
> svn+ssh://" (than the one with "git clone ssh://") ?
> In this case
> - defining SVN_SSH, but not GIT_SSH will still work (with this patch, 
> GIT_SSH overrides)
> - but SVN_SSH needs to have \\s.
> So unless the user already knew of this quirk, we'll only see 
> unescaped \s - so it *does* make sense to escape the \s (if the user 
> knew, then too many escaped \s still work).
>>  - Can a user have SVN_SSH exported, on MSWin32 or msys, and use svn
>>    outside git?  If so, what does the value of SVN_SSH look like?  
>> Does it
>>    typically have necessary doubling of backslashes already?
>>   
> With subversion for Windows, these \\s are not needed (but doesn't 
> cause any break). The doubling is something to do with the bash (in 
> msys) I think.
I was wrong... the \\ seems to be a subversion issue. This s/\\/ line in 
this patch would at best be a work-around (necessary because GIT_SSH 
doesn't have this bug).
http://subversion.tigris.org/issues/show_bug.cgi?id=3454 (GIT_SSH would 
look like the last one in the list - and with this patch, SVN_SSH can 
also look the same)

Re: [PATCH][resend] git-svn: Respect GIT_SSH setting

[Johannes Schindelin]

Hi,

On Mon, 17 Aug 2009, Karthik R wrote:

> Johannes Schindelin wrote:
>
> > On Mon, 17 Aug 2009, Karthik R wrote:
> >
> > > Setting GIT_SSH when using "git svn clone svn+ssh://..." does not 
> > > override the default ssh; SVN_SSH needed to be set instead.
> >
> > This is now in past tense, no?
>
> Yes... this is all in the past tense now :) ... should be "did not 
> override the default ssh". I'll fix it if I have to resend the patch for 
> a different reason.

> > > diff --git a/git-svn.perl b/git-svn.perl
> > > index b0bfb74..9bc1e71 100755
> > > --- a/git-svn.perl
> > > +++ b/git-svn.perl
> > > @@ -21,6 +21,13 @@ $Git::SVN::default_ref_id = $ENV{GIT_SVN_ID} ||
> > > 'git-svn';
> > > $Git::SVN::Ra::_log_window_size = 100;
> > > $Git::SVN::_minimize_url = 'unset';
> > >
> > > +# If GIT_SSH is set, also set SVN_SSH...
> > > +$ENV{SVN_SSH} = $ENV{GIT_SSH} if defined $ENV{GIT_SSH};
> > > +# ... and escape \s in shell-variable on Windows
> > > +if ($^O eq 'MSWin32' || $^O eq 'msys') {
> > > +       $ENV{SVN_SSH} =~ s/\\/\\\\/g if defined $ENV{SVN_SSH};
> > > +}
> > >     
> >
> > This is a change from before... I do not know if it is a good one, as 
> > SVN_SSH could be defined differently by the user, no?  In that case, 
> > the user was most likely using the correct amount of backslashes...
>
> Dscho, The *correct* amount of backslashes is 1 (per dir) - same as used 
> with GIT_SSH. If the user has set SVN_SSH but not GIT_SSH (most likely 
> without escaping \), then fixing up SVN_SSH for use with git-svn is not 
> a bad thing.
> 
> I did this change to retain existing behavior (using SVN_SSH to 
> override) even when user doesn't know the \\ quirk - or if the user has 
> set it for some other non-msys version of svn.

Two things: you want to say that much in the commit message, lest people 
as stupid as me fall into the same trap.  And you might want to avoid 
doing that for MSWin32: we may have a MinGW Perl in the near future (Bosko 
was working really hard on that), and then we do _not_ want that behavior, 
right?

Ciao,
Dscho

Re: [PATCH][resend] git-svn: Respect GIT_SSH setting

[Junio C Hamano]

Karthik R <karthikr@fastmail.fm> writes:

>> Two questions.
>>
>>  - What if a user has SVN_SSH exported _and_ wants to use a different one
>>    from the one s/he uses for git?  Naturally such a user would set both
>>    environment variables and differently, but this seems to override the
>>    value in SVN_SSH;
>>   

> Do you mean user wants to use a different one with "git svn
> ... svn+ssh://" (than the one with "git clone ssh://") ?

Yes.

> In this case
> - defining SVN_SSH, but not GIT_SSH will still work (with this patch,
> GIT_SSH overrides)

Which means if you need to use GIT_SSH to specify one and SVN_SSH to
specify another, you have trouble.  IOW, you cannot use anything but
whatever the default is for native git access over ssh:// protocol.

> - but SVN_SSH needs to have \\s.
>
> So unless the user already knew of this quirk, we'll only see
> unescaped \s - so it *does* make sense to escape the \s (if the user
> knew, then too many escaped \s still work).
>
>>  - Can a user have SVN_SSH exported, on MSWin32 or msys, and use svn
>>    outside git?  If so, what does the value of SVN_SSH look like?  Does it
>>    typically have necessary doubling of backslashes already?
>>   
> With subversion for Windows, these \\s are not needed (but doesn't
> cause any break). The doubling is something to do with the bash (in
> msys) I think.
>

Ok, so does that mean the logic should look more like the one you quoted
below without saying yes/no/anything?  The points are:

 (1) do not muck with SVN_SSH if already given by the user.

 (2) when and only when we reuse value from GIT_SSH for SVN_SSH, double
     the backslashes.

>> What I am getting at is, if the patch should look something like this
>> instead:
>>
>> 	if (! exists $ENV{SVN_SSH}) {
>> 		if (exists $ENV{GIT_SSH}) {
>> 			$ENV{SVN_SSH} = $ENV{GIT_SSH};
>> 			if ($^O eq 'MSWin32' || $^O eq 'msys') {
>>                                $ENV{SVN_SSH} =~ s/\\/\\\\/g;
>> 			}
>> 		}
>> 	}
>>
>>