threads / patch / 20646

patch[resend] git-svn: Respect GIT_SSH setting

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

## tl;dr

8 messages between Aug 17, 2009 and Aug 18, 2009. Diffs are folded; open one to read it.

replies: 7people: 3as markdown or json

Karthik R· Aug 17, 2009, 23:02 UTC · lore

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(-)
Show changes to git-svn.perl +7 −1
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
Johannes Schindelin· Aug 17, 2009, 23:20 UTC · re: Karthik R · lore

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

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?
Show 14 quoted lines
> 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

Karthik R· Aug 18, 2009, 04:33 UTC · re: Johannes Schindelin · lore

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

Johannes Schindelin wrote:
Show 12 quoted lines
> 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.

Show 20 quoted lines
>> 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.

Show 8 quoted lines
> 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
>
>
Johannes Schindelin· Aug 18, 2009, 09:52 UTC · re: Karthik R · lore

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

Hi,
On Mon, 17 Aug 2009, Karthik R wrote:
Show 12 quoted lines
> 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.
Show 29 quoted lines
> > > 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

Junio C Hamano· Aug 17, 2009, 23:21 UTC · re: Karthik R · lore

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

Karthik R <karthikr@fastmail.fm> writes:
Show 7 quoted lines
> +# 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;
			}
		}
	}
Karthik R· Aug 17, 2009, 23:47 UTC · re: Junio C Hamano · lore

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

Junio C Hamano wrote:
Show 19 quoted lines
> 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.

Show 13 quoted lines
> 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;
> 			}
> 		}
> 	}
>
>
Karthik R· Aug 18, 2009, 05:48 UTC · re: Karthik R · lore

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

Karthik R wrote:
Show 38 quoted lines
> 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)

Junio C Hamano· Aug 18, 2009, 19:25 UTC · re: Karthik R · lore

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

Karthik R <karthikr@fastmail.fm> writes:
Show 7 quoted lines
>> 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.

Show 14 quoted lines
> - 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.
Show 13 quoted lines
>> 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;
>> 			}
>> 		}
>> 	}
>>
>>

← back to recent threads