git.cmd eats ^, introducing incompatibility

[kohsuke]

git.cmd does "git.exe %@", which treats ^ in the argument as a special character. This processing breaks the command line interface compatibility with git.exe (or with git on Unix platforms.)

I'm a developer of Jenkins, a continuous integration server. At some point my program executes git rev-parse sometag^{commit} — if this finds git.exe (or other flavors of git, such as git from Cygwin), then this will work as expected. But if someone has git.cmd from msysgit in PATH before git.exe, then cmd.exe will end up executing git.exe rev-parse sometag{commit}, which obviously doesn't work.

If I change my git invocation to git rev-parse sometag^^{commit} then now it'll work with git.cmd, but it will not work when git.exe is found in PATH. And needless to say, this is incompatible with how I'd invoke git on Unix.

I consider this to be a bug on msysgit. It shouldn't let cmd.exe process its arguments.

Unfortunately I don't have enough Windows-fu to be able to send in a patch, so I'm just filing this as an issue instead.

See JENKINS-13007 where this is biting us.

[dscho]

@kohsuke wow. You are not only "a developer" of Jenkins. You are the inventor. I use Jenkins every single day and love it. Thanks!

I'll see tomorrow what I can do for you!

[patthoyts]

It's not git.cmd that eats the caret -- this is used by Windows command prompt as an escape character. See http://msdn.microsoft.com/en-us/library/kcc7tke7.aspx (section Escape characters). Here is an example:

C:\src\msysgit>echo One&Two
One
'Two' is not recognized as an internal or external command, operable program or batch file.

C:\src\msysgit>echo One^&Two
One&Two

[sschuberth]

This SO post sums up quite nicely what the problem is:

"The reason for this is that the shell special characters ^ and > are interpreted by the command shell at two levels, first on the command line and second inside the CMD script."

While we can fix the second issue using delayed expansion inside of the batch file (see the top voted answer on SO), that would not get us around the need to quote the arguments on the command line in the call to git.cmd. The top answer also suggests another approach that is supposed to work around the quoting need, but it is rather complex and involves writing to a temporary file.

[dscho]

@patthoyts @sschuberth how about working the other angle? I.e. getting rid of git.cmd altogether? All it does is to set a couple of environment variables and make sure that the codepage is set, and that git gui is started with wish.exe. We should be able to do all that in Windows-specific startup code.

The bigger problem would be to get this to users in a backwards-compatible manner, as nobody except @sschuberth reads release notes ;-)

[sschuberth]

I was also thinking about replacing git.cmd / gitk.cmd with native executables. This would be the cleanest solution IMHO, but also the one involving the most work.

[patthoyts]

I thought about this but I don't think its going to help much. Currently if you want to do "git log HEAD^" you have to give two carets because the command shell has taken the first to use as a quote. This will not change if we make a git exe wrapper for the cmd\ folder -- the command shell is still going to perform some command line handling and eat the caret. The only advantage I can see would be to make it simple in calling from other batch scripts where we currently have to do 'call git args...' which might be nice. I suspect a little wrapper making use of mingw_spawnve_fd could be fairly simple to do.

Here's a real quick check of the command line handling:

#define STRICT
#define WIN32_LEAN_AND_MEAN
#define UNICODE
#define _UNICODE
#include <windows.h>
int main(void)
{
    LPCWSTR wsz = GetCommandLineW();
    WriteConsole(GetStdHandle(STD_OUTPUT_HANDLE),
             wsz, wcslen(wsz), NULL, NULL);
    return 0;
}

C:\src\msysgit>cl -nologo -W3 -Od z.c
z.c

C:\src\msysgit>z log --oneline HEAD^^
z  log --oneline HEAD^

With just one caret, it asks "More?" which is the command shell escaping the newline at the end of the line

[sschuberth]

@patthoyts You're right, adding a native executable wrapper would only get us around the escaping in the CMD file, not around the one done by CMD. So if we can only solve the one issue anyway, I'd vote for using delayed expansion inside of the batch file instead of the native wrapper, just for simplicity.

@kohsuke To work around the CMD escaping issue, would it be OK for you to quote strings containing ^ in the call to Git on Windows on the Jenkins side?

[kohsuke]

@sschuberth OK, I didn't realize that surrounding an arugment with a double-quote would prevent git.cmd from eating ^. That is something I can do on my side to work around the problem. So I'm no longer blocked by this.

I still do recommend that git.cmd not to let its cmd.exe eats ^.

I was careful in my experiments to differentiate the command prompt that someone might be using interactively vs cmd.exe that executes git.cmd. The first comment from @patthoyts is only partially right, in that both cmd.exe eats ^ as escape.

I am not asking to eliminate ^ handling all together. I'm only asking the double escape handling to be reduced to one (by having git.cmd not letting its cmd.exe do the escape handling.) I ask this because I think this is the right contract — when someone does CreateProcess("git rev-parse tag^{commit}"), it should work regardless of whether git.cmd is found in PATH first or git.exe (from msysgit or cygwin) is found.

The second comment from @patthoyts seems to argue that "it's not going to help much" because the user of an interactive command prompt would still have to do one escaping. With all due respect, I think that's missing the point. I'm not raising it an issue that the user of an interactive command prompt has to do the escaping (on that matter, even Unix users often have to escape interactive invocation of git rev-parse tag^{commit} from shell), I'm raising an issue that the same argument to CreateProcess work differently between different git implementations on Windows.

[zyv]

@dscho i lold when realized that this is exactly the problem why we have fiji and msysgit builds failing on Windows ;-) ...

[patthoyts]

I can reproduce the double quoting issue and there seems to be a simple fix for the cmd script. If we do want to try for a compiled wrapper I uploaded an attempt that seems to work ok to https://gist.github.com/3002978

Showing the problem:

C:\src\msysgit>cmd\git rev-parse Git-1.7.11-preview20120620^^{commit}
Git-1.7.11-preview20120620{commit}
fatal: ambiguous argument 'Git-1.7.11-preview20120620{commit}': unknown revision or path not in the working tree.
Use '--' to separate paths from revisions

C:\src\msysgit>cmd\git rev-parse Git-1.7.11-preview20120620^^^^{commit}
8b7eb7e08b282344298cb05c6d14c47971a22383

If we apply the following patch:

diff --git a/cmd/git.cmd b/cmd/git.cmd
index e697a23..5fdcc95 100644
--- a/cmd/git.cmd
+++ b/cmd/git.cmd
@@ -21,7 +21,7 @@
 @for /f %%i in ('getcp -ansi') do @set cp_ansi=%%i
 @rem Set the console codepage to match the GUI codepage.
 @chcp %cp_ansi% > nul < nul
-@git.exe %*
+@for /f "delims=" %%I in ("git.exe %*") do @%%I
 @set ErrorLevel=%ErrorLevel%
 @rem Restore the original console codepage.
 @chcp %cp_oem% > nul < nul

Then it avoids the requirement for double-quoting. The setlocal EnabledDelayedExpansion did not seem to help when I played around with this but the use of for as above seems to be sufficient.

C:\src\msysgit>cmd\git rev-parse Git-1.7.11-preview20120620^^{commit}
8b7eb7e08b282344298cb05c6d14c47971a22383

[sschuberth]

@patthoyts Great stuff about the git.cmd patch! So my understanding is that with your patched git.cmd also this should work:

C:\src\msysgit>cmd\git rev-parse "Git-1.7.11-preview20120620^{commit}"

Is that correct?

[patthoyts]

Sigh. No. :(

[patthoyts]

I've tried a few more versions with Enable/DisableDelayedExpansion in various forms. It seems I can have either quotes or caret quoting but not deal with both however I modify the cmd script. The %* seems to present a problem within the script when it comes to expanding this.

However if we use the C code wrapper as a replacement (copied to cmd\git.exe) - no problem:

C:\src\msysgit>git rev-parse Git-1.7.11-preview20120620^^{commit}
8b7eb7e08b282344298cb05c6d14c47971a22383

C:\src\msysgit>git rev-parse "Git-1.7.11-preview20120620^{commit}"
8b7eb7e08b282344298cb05c6d14c47971a22383

So unless anyone comes up with a bright idea for the script it looks like putting a wrapper exe in place as cmd\git.exe will be the way to sort this out. I think the wrapper is covering everything. 'git gui' launches detached, 'git gui citool' stays attached and the codepage handling is replicated along with the HOME environment setup.

[sschuberth]

@patthoyts Well, now that you have the C-wrapper already done I'd say we should save our time thinking about a hack for git.cmd and simply use your git.exe! :-)

[kblees]

I believe setting the code page is no longer necessary with Unicode support (git, tcl and all msys-progs (including bash and perl) use WriteConsoleW to write to the console, so the console code page has no effect).

[dscho]

@patthoyts well done! Let's use your wrapper for now, how about putting it into /src/wrapper/? In the long run, we might want to think of merging this into compat/mingw.c's mingw_startup() function, no?

@kblees I'd like to avoid conflating the move to a C-based wrapper with the removal of the code page setting. We could do that as a follow-up patch, but I am not sure whether our test suite would have any chance to catch any breakages that might be incurred by not setting the code page. So it'd need extensive manual testing.

[dscho]

@patthoyts I merged your work into a topic branch, please see https://github.com/msysgit/msysgit/commits/git-wrapper; If you're okay with it, please merge into devel!

[patthoyts]

heh - we collided somewhat - see pt/git-wrapper. If you copy the git.exe into your installed version (Program Files\Git\cmd) and remove/rename the .cmd script there it can get a good workout. I had some issue with 'git gui blame filename' which I've since fixed. The program does expect to be in either msysGit/cmd or the Git for Windows installed location (Git\cmd) as in each case it will look for ..\bin\git.exe and add ..\bin to the path.

[dscho]

@patthoyts no sweat, it did not take me long (and your Makefile is much neater, anyway). I deleted my branch.

[dscho]

@patthoyts do you want to leave this issue open until a new installer is released?