git-svn: Respect GIT_SSH setting

PATCH RESEND In review Karthik R 3 participants 8 messages Jun 15, 2016

Share & tools

Karthik R

author

2016-06-15 22:47 UTC #0 lore raw

PATCH RESEND 1/1 git-svn: Respect GIT_SSH setting

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 <[email protected]>

---

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

git-svn.perl | 7 +++++++

1 files changed, 7 insertions(+), 0 deletions(-)

git-svn.perl +0 -0

index b0bfb74..9bc1e71 100755
@@ -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

Junio C Hamano

2016-06-15 22:47 UTC #1 lore raw

Review

Quoted 7 lines · Karthik R

(quoted diff) +7 -0

+# 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

author

2016-06-15 22:47 UTC #2 lore raw

Quoted 14 lines · Junio C Hamano

Review

Quoted 8 lines · Karthik R

(quoted diff) +7 -0

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

Quoted 5 lines

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

Quoted 4 lines

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

Quoted 13 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

author

2016-06-15 22:47 UTC #3 lore raw

Quoted 27 lines · Karthik R

Quoted 14 lines · Junio C Hamano
Review
Quoted 8 lines · Karthik R
(quoted diff) +7 -0
+# 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.
Quoted 6 lines
- 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).

Quoted 6 lines

Quoted 5 lines
- 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

2016-06-15 22:47 UTC #4 lore raw

In reply to Karthik R

Quoted 10 lines · Karthik R

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

Quoted 3 lines

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.

Quoted 6 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).

Quoted 8 lines

Quoted 4 lines
- 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.

Quoted 13 lines

Quoted 13 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;
			}
		}
	}

Johannes Schindelin

2016-06-15 22:47 UTC #5 lore raw

In reply to Karthik R

Hi,

Quoted 2 lines · 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.

This is now in past tense, no?

Quoted 5 lines

git-svn.perl +0 -0
index b0bfb74..9bc1e71 100755

git-svn.perl $Git::SVN::default_ref_id = $ENV{GIT_SVN_ID} || 'git-svn'; · line 29

Quoted 9 lines

git-svn.perl +6 -0

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

author

2016-06-15 22:47 UTC #6 lore raw

Quoted 12 lines · Johannes Schindelin

Hi,
Quoted 3 lines · 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.
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.

Quoted 8 lines

git-svn.perl +0 -0

index b0bfb74..9bc1e71 100755
@@ -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';

Quoted 10 lines

git-svn.perl $Git::SVN::default_ref_id = $ENV{GIT_SVN_ID} || 'git-svn'; · line 30
Quoted 7 lines
git-svn.perl +6 -0
+# 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.

Quoted 6 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

2016-06-15 22:47 UTC #7 lore raw

Hi,

Quoted 34 lines · Karthik R

Quoted 6 lines · Johannes Schindelin
Quoted 2 lines · 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.
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.
Quoted 9 lines
Quoted 9 lines
git-svn.perl +0 -0
index b0bfb74..9bc1e71 100755
@@ -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';
 
Quoted 11 lines
git-svn.perl $Git::SVN::default_ref_id = $ENV{GIT_SVN_ID} || · line 31
Quoted 7 lines
git-svn.perl +6 -0
+# 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...

Quoted 8 lines

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