Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Not possible to set GIT_SSH to a path with spaces #692

Closed
klikh opened this issue Mar 12, 2016 · 12 comments
Closed

Not possible to set GIT_SSH to a path with spaces #692

klikh opened this issue Mar 12, 2016 · 12 comments

Comments

@klikh
Copy link

klikh commented Mar 12, 2016

Windows 7 64-bit
git version 2.7.2.windows.1
Git Bash

I have an executable script located in a folder with spaces, and what to use it for git over SSH. However, it seems that it is simply not possible to define GIT_SSH so that it points to a path with spaces.

Neither of the following works:

$ export GIT_SSH=C:\\Users\\John\ Smith\\git-ssh-.bat
$ export GIT_SSH=/c/Users/John\ Smith/git-ssh-.bat

$ git fetch
'C:\Users\John' is not recognized as an internal or external command,
operable program or batch file.
fatal: Could not read from remote repository.

Quoting gives another error but still doesn't work:

$ export GIT_SSH="/c/Users/John\ Smith/git-ssh-.bat"
$ export GIT_SSH=\"/c/Users/John\ Smith/git-ssh-.bat\"
$ export GIT_SSH="C:\\Users\\John\ Smith\\git-ssh-.bat"
$ export GIT_SSH="C:\Users\John Smith\git-ssh-.bat"

$ git fetch
error: cannot spawn C:/Users/John/ Smith/git-ssh-.bat: No such file or directory
fatal: unable to fork

I would appreciate if you give me a workaround (if there is any workaround except "use path without spaces") until the issue is fixed.

@klikh
Copy link
Author

klikh commented Mar 12, 2016

Googling shows that the issue exists for a long time, and was already reported but was not ported to this bugtracker.

@dscho
Copy link
Member

dscho commented Mar 14, 2016

@klikh the quoted approach is the correct one, but I suspect that we never supported calling .bat scripts, only .sh ones.

@klikh
Copy link
Author

klikh commented Mar 14, 2016

the quoted approach is the correct one

Which one exactly?

I suspect that we never supported calling .bat scripts, only .sh ones.

.bat scripts are perfectly supported (at least they work - intentionally or not :) ) unless the path to the script contains a space.

However, I'll check sh as well, thanks.

@dscho
Copy link
Member

dscho commented Apr 1, 2016

the quoted approach is the correct one

Which one exactly?

Sorry. I meant this one:

$ export GIT_SSH="C:\\Users\\John\ Smith\\git-ssh-.bat"

However, you are correct:

.bat scripts are perfectly supported

Whoa, you're right! I did not know...

And I did verify that it works without spaces in the path and does not work with spaces. The easiest workaround for you is to use the short names (for backwards-compatibility, every file name has an alternate 8.3 compliant name, in your case it might be C:\Users\JOHNSM~1\GIT-SS~1.BAT).

It appears that something spooky is going on in the CreateProcessW() call: it simply does not work for .bat files if the the lpApplicationName parameter contains spaces, and even if it is replaced by the short path (using the GetShortPathName() function), if the lpCommandLine does not match the short path, it still will fail.

It does work, however, if quoting the first argument in lpCommandLine and passing NULL as lpApplicationName.

@dscho
Copy link
Member

dscho commented Apr 2, 2016

Fixed via 8086988

@dscho dscho closed this as completed Apr 2, 2016
dscho added a commit to git-for-windows/build-extra that referenced this issue Apr 2, 2016
GIT_SSH (and other executable paths that Git wants to spawn) [can
now contain spaces](git-for-windows/git#692).

Signed-off-by: Johannes Schindelin <[email protected]>
@klikh
Copy link
Author

klikh commented Apr 2, 2016

Wow. Thanks for the quick fix!

The easiest workaround for you is to use the short names (for backwards-compatibility, every file name has an alternate 8.3 compliant nam

Yep, the workaround is clear, but the problem is how to identify this short name. E.g. in my Java program there is no straightforward method to do it, only by making a native system call.

@avih
Copy link

avih commented Apr 7, 2016

every file name has an alternate 8.3 compliant name

AFAIK, on windows 8 and later, 8.3 names are only enabled by default on drive C (try dir /X)

@dscho
Copy link
Member

dscho commented Apr 7, 2016

on windows 8 and later, 8.3 names are only enabled by default on drive C

Would you have a reference for this?

@avih
Copy link

avih commented Apr 7, 2016

Would you have a reference for this?

Nothing too concrete, but that's my recollection from some time ago when I checked the subject, and when I search now I also see people who don't have 8.3 names and how to enable them (fsutil apparently).

Also, on few win7 systems which I checked just now they have short names on non C drives, and on win8/10 which I checked there are no 8.3 names on non-C drives.

So so far, I can't find evidence to contradict this.

@dscho
Copy link
Member

dscho commented Apr 7, 2016

So so far, I can't find evidence to contradict this.

Hmm. I hoped for some link to an official statement that positively verifies this, not for the absence of contradicting evidence.

@avih
Copy link

avih commented Apr 7, 2016

Same, but I couldn't find one.

dscho added a commit to dscho/git that referenced this issue Feb 7, 2019
On some older Windows versions (e.g. Windows 7), the CreateProcessW()
function does not really support spaces in its first argument,
lpApplicationName. But it supports passing NULL as lpApplicationName,
which makes it figure out the application from the (possibly quoted)
first argument of lpCommandLine.

Let's use that trick (if we are certain that the first argument matches
the executable's path) to support launching programs whose path contains
spaces.

We will abuse the test-fake-ssh.exe helper to verify that this works and
does not regress.

This fixes git-for-windows#692

Signed-off-by: Johannes Schindelin <[email protected]>
dscho added a commit to dscho/git that referenced this issue Feb 7, 2019
On some older Windows versions (e.g. Windows 7), the CreateProcessW()
function does not really support spaces in its first argument,
lpApplicationName. But it supports passing NULL as lpApplicationName,
which makes it figure out the application from the (possibly quoted)
first argument of lpCommandLine.

Let's use that trick (if we are certain that the first argument matches
the executable's path) to support launching programs whose path contains
spaces.

We will abuse the test-fake-ssh.exe helper to verify that this works and
does not regress.

This fixes git-for-windows#692

Signed-off-by: Johannes Schindelin <[email protected]>
dscho added a commit that referenced this issue Feb 12, 2019
On some older Windows versions (e.g. Windows 7), the CreateProcessW()
function does not really support spaces in its first argument,
lpApplicationName. But it supports passing NULL as lpApplicationName,
which makes it figure out the application from the (possibly quoted)
first argument of lpCommandLine.

Let's use that trick (if we are certain that the first argument matches
the executable's path) to support launching programs whose path contains
spaces.

We will abuse the test-fake-ssh.exe helper to verify that this works and
does not regress.

This fixes #692

Signed-off-by: Johannes Schindelin <[email protected]>
git-for-windows-ci pushed a commit that referenced this issue Feb 12, 2019
On some older Windows versions (e.g. Windows 7), the CreateProcessW()
function does not really support spaces in its first argument,
lpApplicationName. But it supports passing NULL as lpApplicationName,
which makes it figure out the application from the (possibly quoted)
first argument of lpCommandLine.

Let's use that trick (if we are certain that the first argument matches
the executable's path) to support launching programs whose path contains
spaces.

We will abuse the test-fake-ssh.exe helper to verify that this works and
does not regress.

This fixes #692

Signed-off-by: Johannes Schindelin <[email protected]>
git-for-windows-ci pushed a commit that referenced this issue Feb 12, 2019
On some older Windows versions (e.g. Windows 7), the CreateProcessW()
function does not really support spaces in its first argument,
lpApplicationName. But it supports passing NULL as lpApplicationName,
which makes it figure out the application from the (possibly quoted)
first argument of lpCommandLine.

Let's use that trick (if we are certain that the first argument matches
the executable's path) to support launching programs whose path contains
spaces.

We will abuse the test-fake-ssh.exe helper to verify that this works and
does not regress.

This fixes #692

Signed-off-by: Johannes Schindelin <[email protected]>
dscho added a commit to dscho/git that referenced this issue Feb 14, 2019
On some older Windows versions (e.g. Windows 7), the CreateProcessW()
function does not really support spaces in its first argument,
lpApplicationName. But it supports passing NULL as lpApplicationName,
which makes it figure out the application from the (possibly quoted)
first argument of lpCommandLine.

Let's use that trick (if we are certain that the first argument matches
the executable's path) to support launching programs whose path contains
spaces.

We will abuse the test-fake-ssh.exe helper to verify that this works and
does not regress.

This fixes git-for-windows#692

Signed-off-by: Johannes Schindelin <[email protected]>
dscho added a commit that referenced this issue Feb 14, 2019
On some older Windows versions (e.g. Windows 7), the CreateProcessW()
function does not really support spaces in its first argument,
lpApplicationName. But it supports passing NULL as lpApplicationName,
which makes it figure out the application from the (possibly quoted)
first argument of lpCommandLine.

Let's use that trick (if we are certain that the first argument matches
the executable's path) to support launching programs whose path contains
spaces.

We will abuse the test-fake-ssh.exe helper to verify that this works and
does not regress.

This fixes #692

Signed-off-by: Johannes Schindelin <[email protected]>
dscho added a commit to dscho/git that referenced this issue Feb 14, 2019
On some older Windows versions (e.g. Windows 7), the CreateProcessW()
function does not really support spaces in its first argument,
lpApplicationName. But it supports passing NULL as lpApplicationName,
which makes it figure out the application from the (possibly quoted)
first argument of lpCommandLine.

Let's use that trick (if we are certain that the first argument matches
the executable's path) to support launching programs whose path contains
spaces.

We will abuse the test-fake-ssh.exe helper to verify that this works and
does not regress.

This fixes git-for-windows#692

Signed-off-by: Johannes Schindelin <[email protected]>
git-for-windows-ci pushed a commit that referenced this issue Jul 11, 2019
On some older Windows versions (e.g. Windows 7), the CreateProcessW()
function does not really support spaces in its first argument,
lpApplicationName. But it supports passing NULL as lpApplicationName,
which makes it figure out the application from the (possibly quoted)
first argument of lpCommandLine.

Let's use that trick (if we are certain that the first argument matches
the executable's path) to support launching programs whose path contains
spaces.

We will abuse the test-fake-ssh.exe helper to verify that this works and
does not regress.

This fixes #692

Signed-off-by: Johannes Schindelin <[email protected]>
dscho added a commit that referenced this issue Jul 11, 2019
On some older Windows versions (e.g. Windows 7), the CreateProcessW()
function does not really support spaces in its first argument,
lpApplicationName. But it supports passing NULL as lpApplicationName,
which makes it figure out the application from the (possibly quoted)
first argument of lpCommandLine.

Let's use that trick (if we are certain that the first argument matches
the executable's path) to support launching programs whose path contains
spaces.

We will abuse the test-fake-ssh.exe helper to verify that this works and
does not regress.

This fixes #692

Signed-off-by: Johannes Schindelin <[email protected]>
git-for-windows-ci pushed a commit that referenced this issue Jul 11, 2019
On some older Windows versions (e.g. Windows 7), the CreateProcessW()
function does not really support spaces in its first argument,
lpApplicationName. But it supports passing NULL as lpApplicationName,
which makes it figure out the application from the (possibly quoted)
first argument of lpCommandLine.

Let's use that trick (if we are certain that the first argument matches
the executable's path) to support launching programs whose path contains
spaces.

We will abuse the test-fake-ssh.exe helper to verify that this works and
does not regress.

This fixes #692

Signed-off-by: Johannes Schindelin <[email protected]>
dscho added a commit that referenced this issue Jul 12, 2019
On some older Windows versions (e.g. Windows 7), the CreateProcessW()
function does not really support spaces in its first argument,
lpApplicationName. But it supports passing NULL as lpApplicationName,
which makes it figure out the application from the (possibly quoted)
first argument of lpCommandLine.

Let's use that trick (if we are certain that the first argument matches
the executable's path) to support launching programs whose path contains
spaces.

We will abuse the test-fake-ssh.exe helper to verify that this works and
does not regress.

This fixes #692

Signed-off-by: Johannes Schindelin <[email protected]>
git-for-windows-ci pushed a commit that referenced this issue Jul 12, 2019
On some older Windows versions (e.g. Windows 7), the CreateProcessW()
function does not really support spaces in its first argument,
lpApplicationName. But it supports passing NULL as lpApplicationName,
which makes it figure out the application from the (possibly quoted)
first argument of lpCommandLine.

Let's use that trick (if we are certain that the first argument matches
the executable's path) to support launching programs whose path contains
spaces.

We will abuse the test-fake-ssh.exe helper to verify that this works and
does not regress.

This fixes #692

Signed-off-by: Johannes Schindelin <[email protected]>
git-for-windows-ci pushed a commit that referenced this issue Jul 12, 2019
On some older Windows versions (e.g. Windows 7), the CreateProcessW()
function does not really support spaces in its first argument,
lpApplicationName. But it supports passing NULL as lpApplicationName,
which makes it figure out the application from the (possibly quoted)
first argument of lpCommandLine.

Let's use that trick (if we are certain that the first argument matches
the executable's path) to support launching programs whose path contains
spaces.

We will abuse the test-fake-ssh.exe helper to verify that this works and
does not regress.

This fixes #692

Signed-off-by: Johannes Schindelin <[email protected]>
dscho added a commit that referenced this issue Jul 15, 2019
On some older Windows versions (e.g. Windows 7), the CreateProcessW()
function does not really support spaces in its first argument,
lpApplicationName. But it supports passing NULL as lpApplicationName,
which makes it figure out the application from the (possibly quoted)
first argument of lpCommandLine.

Let's use that trick (if we are certain that the first argument matches
the executable's path) to support launching programs whose path contains
spaces.

We will abuse the test-fake-ssh.exe helper to verify that this works and
does not regress.

This fixes #692

Signed-off-by: Johannes Schindelin <[email protected]>
git-for-windows-ci pushed a commit that referenced this issue Jul 15, 2019
On some older Windows versions (e.g. Windows 7), the CreateProcessW()
function does not really support spaces in its first argument,
lpApplicationName. But it supports passing NULL as lpApplicationName,
which makes it figure out the application from the (possibly quoted)
first argument of lpCommandLine.

Let's use that trick (if we are certain that the first argument matches
the executable's path) to support launching programs whose path contains
spaces.

We will abuse the test-fake-ssh.exe helper to verify that this works and
does not regress.

This fixes #692

Signed-off-by: Johannes Schindelin <[email protected]>
dscho added a commit that referenced this issue Jul 16, 2019
On some older Windows versions (e.g. Windows 7), the CreateProcessW()
function does not really support spaces in its first argument,
lpApplicationName. But it supports passing NULL as lpApplicationName,
which makes it figure out the application from the (possibly quoted)
first argument of lpCommandLine.

Let's use that trick (if we are certain that the first argument matches
the executable's path) to support launching programs whose path contains
spaces.

We will abuse the test-fake-ssh.exe helper to verify that this works and
does not regress.

This fixes #692

Signed-off-by: Johannes Schindelin <[email protected]>
gitster pushed a commit to gitster/git that referenced this issue Jul 16, 2019
On some older Windows versions (e.g. Windows 7), the CreateProcessW()
function does not really support spaces in its first argument,
lpApplicationName. But it supports passing NULL as lpApplicationName,
which makes it figure out the application from the (possibly quoted)
first argument of lpCommandLine.

Let's use that trick (if we are certain that the first argument matches
the executable's path) to support launching programs whose path contains
spaces.

We will abuse the test-fake-ssh.exe helper to verify that this works and
does not regress.

This fixes git-for-windows#692

Signed-off-by: Johannes Schindelin <[email protected]>
Signed-off-by: Junio C Hamano <[email protected]>
git-for-windows-ci pushed a commit that referenced this issue Jul 23, 2019
On some older Windows versions (e.g. Windows 7), the CreateProcessW()
function does not really support spaces in its first argument,
lpApplicationName. But it supports passing NULL as lpApplicationName,
which makes it figure out the application from the (possibly quoted)
first argument of lpCommandLine.

Let's use that trick (if we are certain that the first argument matches
the executable's path) to support launching programs whose path contains
spaces.

We will abuse the test-fake-ssh.exe helper to verify that this works and
does not regress.

This fixes #692

Signed-off-by: Johannes Schindelin <[email protected]>
git-for-windows-ci pushed a commit that referenced this issue Jul 23, 2019
On some older Windows versions (e.g. Windows 7), the CreateProcessW()
function does not really support spaces in its first argument,
lpApplicationName. But it supports passing NULL as lpApplicationName,
which makes it figure out the application from the (possibly quoted)
first argument of lpCommandLine.

Let's use that trick (if we are certain that the first argument matches
the executable's path) to support launching programs whose path contains
spaces.

We will abuse the test-fake-ssh.exe helper to verify that this works and
does not regress.

This fixes #692

Signed-off-by: Johannes Schindelin <[email protected]>
git-for-windows-ci pushed a commit that referenced this issue Jul 25, 2019
On some older Windows versions (e.g. Windows 7), the CreateProcessW()
function does not really support spaces in its first argument,
lpApplicationName. But it supports passing NULL as lpApplicationName,
which makes it figure out the application from the (possibly quoted)
first argument of lpCommandLine.

Let's use that trick (if we are certain that the first argument matches
the executable's path) to support launching programs whose path contains
spaces.

We will abuse the test-fake-ssh.exe helper to verify that this works and
does not regress.

This fixes #692

Signed-off-by: Johannes Schindelin <[email protected]>
@SyntevoAlex
Copy link

Came across this issue by accident. We also had problems with it.

The problem here is not with CreateProcessW and also not related to Windows 7.

  1. In OP's case, git is trying to run git-ssh-.bat

  2. But .bat file is not an executable, so it can't be started directly.

  3. CreateProcessW seems to have a dedicated code path for that, which eventually starts cmd.exe /c instead.

  4. cmd.exe has non-standard commandline parsing rules.
    The most interesting part to us is that it won't accept two quoted arguments.

    Try it yourself:
    cmd.exe /c "notepad.exe" 1.txt
    OK

    cmd.exe /c "notepad.exe" "1.txt"
    'notepad.exe" "1.txt' is not recognized as an internal or external command, operable program or batch file.

  5. So if both SSH helper has spaces and also one of its arguments has spaces, then:
    a. CreateProcessW will succeed.
    b. cmd.exe will exit with error is not recognized....
    c. Notice that in OP's sample output we actually see is not recognized....

Now regarding the fix applied. This is also rather curious.

  1. If you use CreateProcessW with both arguments, it ends up starting like
    C:\WINDOWS\system32\cmd.exe /c "C:\Temp\ssh helper.bat" [email protected] "git-upload-pack '/repo 1'"
  2. If you only use lpCommandLine
    C:\WINDOWS\system32\cmd.exe /c ""C:\Temp\ssh helper.bat" [email protected] "git-upload-pack '/repo 1'""

You can see it using Process Monitor.

Notice additional surrounding quotes. That's the "correct" way in terms of cmd.exe special parsing rules.

SyntevoAlex added a commit to SyntevoAlex/git that referenced this issue Sep 26, 2019
The test was originally designed for the case where user reported
that setting GIT_SSH to a .bat file with spaces in path fails on
Windows: git-for-windows#692

The test has two different problems:

1. It succeeds with AND without fix eb7c786 that addressed user's
   problem. This happens because the core problem was misunderstood,
   leading to conclusion that git is unable to start any programs with
   spaces in path on Win7. But in fact
     a) Bug only affected cmd.exe scripts, such as .bat scripts
     b) Bug only happened when cmd.exe received at least two quoted args
     c) Bug happened on any Windows (verified on Win10).
   Therefore, correct test must involve .bat script and two quoted args.
2. In Visual Studio build, it fails to run, because 'test-fake-ssh.exe'
   is copied away from its dependencies 'libiconv.dll' and 'zlib1.dll'.

Fix both problems by using .bat script instead of 'test-fake-ssh.exe'.
NOTE: With this change, the test now correctly fails without eb7c786.

Signed-off-by: Alexandr Miloslavskiy <[email protected]>
SyntevoAlex added a commit to SyntevoAlex/git that referenced this issue Sep 30, 2019
The test was originally designed for the case where user reported
that setting GIT_SSH to a .bat file with spaces in path fails on
Windows: git-for-windows#692

The test has two different problems:

1. It succeeds with AND without fix eb7c786 that addressed user's
   problem. This happens because the core problem was misunderstood,
   leading to conclusion that git is unable to start any programs with
   spaces in path on Win7. But in fact
     a) Bug only affected cmd.exe scripts, such as .bat scripts
     b) Bug only happened when cmd.exe received at least two quoted args
     c) Bug happened on any Windows (verified on Win10).
   Therefore, correct test must involve .bat script and two quoted args.
2. In Visual Studio build, it fails to run, because 'test-fake-ssh.exe'
   is copied away from its dependencies 'libiconv.dll' and 'zlib1.dll'.

Fix both problems by using .bat script instead of 'test-fake-ssh.exe'.
NOTE: With this change, the test now correctly fails without eb7c786.

Signed-off-by: Alexandr Miloslavskiy <[email protected]>
SyntevoAlex added a commit to SyntevoAlex/git that referenced this issue Sep 30, 2019
The test was originally designed for the case where user reported
that setting GIT_SSH to a .bat file with spaces in path fails on
Windows: git-for-windows#692

The test has two different problems:

1. It succeeds with AND without fix eb7c786 that addressed user's
   problem. This happens because the core problem was misunderstood,
   leading to conclusion that git is unable to start any programs with
   spaces in path on Win7. But in fact
     a) Bug only affected cmd.exe scripts, such as .bat scripts
     b) Bug only happened when cmd.exe received at least two quoted args
     c) Bug happened on any Windows (verified on Win10).
   Therefore, correct test must involve .bat script and two quoted args.
2. In Visual Studio build, it fails to run, because 'test-fake-ssh.exe'
   is copied away from its dependencies 'libiconv.dll' and 'zlib1.dll'.

Fix both problems by using .bat script instead of 'test-fake-ssh.exe'.
NOTE: With this change, the test now correctly fails without eb7c786.

Signed-off-by: Alexandr Miloslavskiy <[email protected]>
gitster pushed a commit to git/git that referenced this issue Oct 2, 2019
The test was originally designed for the case where user reported
that setting GIT_SSH to a .bat file with spaces in path fails on
Windows: git-for-windows#692

The test has two different problems:

1. It succeeds with AND without fix eb7c786 that addressed user's
   problem. This happens because the core problem was misunderstood,
   leading to conclusion that git is unable to start any programs with
   spaces in path on Win7. But in fact
     a) Bug only affected cmd.exe scripts, such as .bat scripts
     b) Bug only happened when cmd.exe received at least two quoted args
     c) Bug happened on any Windows (verified on Win10).
   Therefore, correct test must involve .bat script and two quoted args.
2. In Visual Studio build, it fails to run, because 'test-fake-ssh.exe'
   is copied away from its dependencies 'libiconv.dll' and 'zlib1.dll'.

Fix both problems by using .bat script instead of 'test-fake-ssh.exe'.
NOTE: With this change, the test now correctly fails without eb7c786.

Signed-off-by: Alexandr Miloslavskiy <[email protected]>
Signed-off-by: Junio C Hamano <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants