Improve handling of commands in Git.pm on native Windows

[ColMelvin]

Improve the Git.pm module so native Windows users can make use of it without overbearing restrictions on what can be sent.

This change allows users to use:

• multiple command pipes simultaneously,
• to use input pipes,
• and to use arguments with embedded spaces.

This will enable users to run even more Git commands in a Windows environment (like cmd or PowerShell) through Perl.

[ColMelvin]

@bk2204

[PhilipOakley]

@ColMelvin Looks sensible. The patches will need the 'sign off' added (can be force pushed to update).

[bk2204]

This looks sane enough. I'm not a Windows user, so I can't speak to the patches' functionality, but I don't see anything obviously wrong.

Note that Git expects things to run on Perl 5.8. I'm not sure about why the ActiveState pipe wrapper was needed in the first place, but assuming the native pipe implementation can run on 5.8 on Windows, that should be fine.

[ColMelvin]

Is there anyone from @ActiveState in the community? I tried searching for ActiveState Perl 5.8 but was unable to find any Community Editions that I could test with.

Failing that, I'd like to know why an untested Git::activestate_pipe object was created in the first place [a6065b5]. It must have been deemed necessary at the time (2006), but there was no rush to get it functional. It took nearly 7 more months before the feature was made usable (and, I presume, tested) [d3b1785].

It is my sincere hope that Perl 5.8 solved the problem that Git::activestate_pipe was trying to work around. Since the minimum requirement wasn't upped to 5.8 until 4 years later, in 2010 [d48b284], there is a reasonable chance.

Maintaining compatibility with older versions is important to me. Indeed, it's part of why I separated the fixes into two different commits, in case the TIEHANDLE was important for older versions. If it is, I'd like to know which version no longer requires it.

However, this all requires access to older Perl versions on Windows that I'm having difficulty obtaining.

[dscho]

@ColMelvin Looks sensible. The patches will need the 'sign off' added (can be force pushed to update).

@ColMelvin thank you for your contribution. Could you please sign off on the patches, too? See https://github.com/probot/dco#how-it-works for details.

[dscho]

Also, I am too little of a Perl expert to review these patches. So after you add your Signed-off-by lines, I would like to take these upstream. Please note that core Git requires your real name in those Signed-off-by lines, to allow for future conversations about your patches.

As to ActiveState Perl: I have not heard in years of anybody using Git.pm with it...

[ColMelvin]

In light of the compatibility comments, I'm re-thinking my testing strategy. I'm going to take a stab at testing older versions on Windows, which may take a while. When I'm done, I'll force push with signed off commits.

[dscho]

@ColMelvin thanks!

[ColMelvin]

It took a while, but I eventually setup a Windows VM and found a copy of ActiveState Perl 5.8 (see http://www.perlmonks.org/?node_id=1188770). I ran a test using the list form of an open pipe; it failed and dashed my hopes for an optimal fix (where Windows isn't a special snowflake).

List form of pipe open not implemented at \test.pl line 19.

The good news is: I was able to run the unit test on 5.8 and the current fix (treating Windows specially) passes the tests. Once the merge conflict is resolved, this change should be ready for merging.

[dscho]

Thanks for keeping at it!

[ColMelvin]

Checked ActiveState Perl 5.8.0 (build 806) on Windows 7 32-bit:

• Verified perl -Iperl -MGit -e "my $fh = Git::command_input_pipe(qw{hash-object --stdin}); print {$fh} q{testing}; Git::command_close_pipe($fh);" supported input pipes
• Verified perl -Iperl t\t9701\test.pl passed

---

RE: Reviews

Also, I am too little of a Perl expert to review these patches. So after you add your Signed-off-by lines, I would like to take these upstream. Please note that core Git requires your real name in those Signed-off-by lines, to allow for future conversations about your patches.

By all means. I am personally satisfied with @bk2204's approval; he is one of the most knowledgeable Perl developers (on Linux) I know.

[bk2204]

A little bit of context for those involved: @ColMelvin and I are co-workers, and we've discussed this issue in person.

If this goes to the git list, I'm happy to be CC'd for review and comments.

[dscho]

For the record, I have serious troubles with the test case that was added in this PR.

First of all, it uses a .bat file (probably to be able to run the test using a custom Perl?).

Then, it uses the wrong paths to the Perl modules (there was a change recently where things are generated into build/lib/ instead of blib/lib/.

Then, it is skipped by default in the Git for Windows SDK because it expects $^O to be MSWin32, when it is msys here...

Even if I fix all that, things don't work for me, as now every single test seems to be failing (and I interrupted it at test case 161...).

Will keep you posted on my progress.

[dscho]

For the record, this is the first failing test case:

not ok 2 - perl "-e" "print $ARGV[0]" "\\\\"
#   Failed test 'perl "-e" "print $ARGV[0]" "\\\\"'
#   at C:\git-sdk-64\usr\src\git\wip4\t/t9701/test.pl line 23.
#          got: 'ARRAY(0x600003720)'
#     expected: '\\'
sh: -c: line 0: unexpected EOF while looking for matching `"'
sh: -c: line 1: syntax error: unexpected end of file

[dscho]

Even launching this from a CMD window does not help:

C:\git-sdk-64\usr\src\git\wip4\t>set PATH=C:\git-sdk-64\mingw64\bin;C:\git-sdk-64\usr\bin;%PATH%

C:\git-sdk-64\usr\src\git\wip4\t>t9701-perl-git-windows.bat
1..195
ok 1 - require Git;
not ok 2 - perl "-e" "print $ARGV[0]" "\\\\"
#   Failed test 'perl "-e" "print $ARGV[0]" "\\\\"'
#   at C:\git-sdk-64\usr\src\git\wip4\t/t9701/test.pl line 23.
#          got: 'ARRAY(0x600003640)'
#     expected: '\\'
sh: -c: line 0: unexpected EOF while looking for matching `"'
sh: -c: line 1: syntax error: unexpected end of file
not ok 3 - perl ^"-e^" ^"print $ARGV[0]^" ^"^(^)^%^!^^\^"^<^>^&^|^"
#   Failed test 'perl ^"-e^" ^"print $ARGV[0]^" ^"^(^)^%^!^^\^"^<^>^&^|^"'
#   at C:\git-sdk-64\usr\src\git\wip4\t/t9701/test.pl line 23.
#          got: ''
#     expected: '()%!^"<>&|'
[...]

[dscho]

Hmm. I seem to be unable to wrap my head around the problem. It does seem as if $ARGV is expanded improperly before running the child Perl process.

@bk2204 any ideas?

[dscho]

Okay, I give up for now. I cannot make that test work with MSYS2 Perl as included in Git for Windows' SDK...

[ColMelvin]

I reproduced the error you're seeing and it stems from the command line used by Perl. On *nix-like systems, such as MSYS2, system & qx{} calls are directed to the exec system call (or sh, if it contains a special character). However, on native Windows systems they are directed to the Windows (kernel-level) command line (or CMD, if it contains a special character).

As you correctly surmized, the problem comes from interpolation. The Windows command line does not perform variable interpolation (just like exec). And in CMD, variable interpolation only occurs with % characters. Thus, the double quotes are safe to use with $ARGV[0].

In a Microsoft quirk, both the Windows command line and CMD will process double quotes and use them to group arguments, yet neither supports single quotes. Thus, double quotes, which break test runs on MSYS Perl, are the only option for a native Windows Perl. As a quick demonstration of this (which can be seen with MSYS Perl), the following code will work correctly on CMD but not in bash. If you change the double quotes to single quotes, it switches: CMD produces bad output but bash works fine.

perl -e "print $ARGV[0]" testing

---

The change I made only applies to MSWin32 systems (the conditional is hidden just above the first hunk). As such, the change can only be verified on an MSWin32 system, like ActiveState Perl, which is why the test is written using a *.bat file. This is clearly an aberration in Git for Windows, so it may be prudent to add additional messaging to the test, clarifying the rationale.

That said, I must apologize -- I did not test the *.bat file after rebasing my work. That was a mistake and I would have caught the pathing issue had I done so. I will update the paths accordingly.

[dscho]

@ColMelvin how do you want to proceed from here? I am still eager to take fixes ;-)

[dscho]

@ColMelvin ping?

[ColMelvin]

Thanks for pinging me; it's about time I pick up a personal project again.

I made several changes, mostly to add documentation to the tests. I haven't had a chance to test the batch script as my current machine lacks a compiler. I hope to get one setup tomorrow for validation.

---

As an additional improvement, I spent some time trying to run t9701-perl-git-windows.bat during make test but I can't get the environment to use the default system PATH -- the Git Bash PATH is used, so the MSYS Perl is called instead of a MSWin32 Perl. If you have any ideas, I'd be interested; I tried using start, but the /I switch didn't reset the environment like I thought it would. That said, running the test regularly should not be necessary, as the test really only validates that Perl hasn't changed (it doesn't test Git much). Therefore, unless you have a suggestion, I'm resigned to omitting this feature.

start //B //I /WAIT cmd //C t9701-perl-git-windows.bat

[dscho]

Welcome back @ColMelvin!

As an additional improvement, I spent some time trying to run t9701-perl-git-windows.bat during make test but I can't get the environment to use the default system PATH -- the Git Bash PATH is used, so the MSYS Perl is called instead of a MSWin32 Perl.

You could, in theory, adjust the PATH in that test script. But I fear I am missing something... So let me ask a couple of dumb questions:

• does your fix affect the Perl shipped with Git for Windows at all? As in: is it supposed to change anything there?
• from the commit messages, I got the impression that this is a problem with any Perl interpreter on Windows, is this incorrect? (If my understanding is incorrect, maybe those commit messages could talk a bit about the specific requirements to trigger the bugs fixed by them?)
• if there is indeed only a problem with a particular Perl interpreter, why not simply make it a test prerequisite (using the test_lazy_prereq construct, see the JGIT prerequisite for an example to follow; you could hard-code the path to that Perl interpreter for starters)?

[ColMelvin]

The problem is, I don't know where the user installed ActiveState or Strawberry Perl (or another 'MSWin32' Perl). I can make some educated guesses (the installer's defaults, for 32-bit and 64-bit, for global and user-only installs), but a consistent (and, theoretically, simpler) way to detect the binary location is to use the PATH as set by Windows when CMD is first started. If the user installed one of these Perls, but did not setup the PATH, then they likely aren't using that Perl from CMD and they won't run into this issue. Therefore, the PATH seems - to me - to be the most authoritative source.

Unfortunately, since Git Bash modifies the PATH to put the Git for Windows Perl first, I can't use that mechanism. Thus, I was hoping to reset the values to the system default with start, but that failed for me.

To answer your questions:

• No; the Perl shipped with Git for Windows is completely unaffected by all of these changes.
• This problem only exists on Perl's where $^O eq 'MSWin32', like ActiveState and Strawberry; since the Perl distributed with Git for Windows sets $^O eq 'MSYS', it is unaffected. I don't recall the value of $^O under Cygwin, but if (and only if) it is 'MSWin32' would it be affected. I'll add 'MSWin32' to the commit messages.
• If I can address the problem of finding the appropriate Perl, then this would be a viable mechanism for limiting testing to affected systems. That said, the test.pl file already does skip_all to avoid running on invalid systems.

[ColMelvin]

@dscho I've validated the batch test works correctly on Windows and I updated the commit messages to hopefully be more helpful. I would say this is ready for review.

[dscho]

The problem is, I don't know where the user installed ActiveState or Strawberry Perl (or another 'MSWin32' Perl).

How about making this an environment variable, then, that is tested in that lazy prerequisite? That way, if you want to run that test case, you simply have to specify that environment variable (e.g. GIT_ACTIVESTATE_PERL_PATH=C:\Program Files\ActiveState\Perl5\bin\perl.exe or some such), and then the test case will be executed, using that perl.exe. Otherwise, the test case will be skipped.

As you can guess from my harping on this issue, I find it highly important to integrate this test into the test suite, to make it automatable, rather than forcing the user to run it manually, and to figure out in the first place how to run this correctly.

Running on Cygwin, for example, resulted in a failure when it should have passed, as the path to t/t9701/perl.pl was not a valid Windows path.

Requiring that environment variable as a prerequisite would cirumvent this in a very elegant way: when running in Cygwin, you are simply expected to specify the path to the Perl executable in a way understood by Cygwin's Bash.

[ColMelvin]

I think we both agree: if a test needs a user to take action so it can run, it will (almost) never run. From this principle, I don't think the environment variable will suffice. The underlying problem with an environment variable is the same as with the batch script: a user needs to know it exists and then take action to get the test running. My original submission assumed as much -- the "test" was primarily designed for manual debugging.

However, if automation is to be our goal, I would want tests to work (pass or skip) without user configuration and on all platforms. After much trial and error (on Linux, Cygwin, & MSYS2 builds), I think I have a solution that meets this ideal. The latest commits have this change (and were rebased onto v2.18.0, to stay up to date).

However, to truly ensure the test always runs, we still need to add an MSWin32 build of Perl to VSTS on the GitHub project. This is something I cannot do (AFAIK).

[dscho]

if a test needs a user to take action so it can run, it will (almost) never run.

I agree.

From this principle, I don't think the environment variable will suffice.

The difference there is that I can integrate that into CI, while I cannot integrate your .bat into CI easily. And since I have a Strawberry Perl lying around here, thanks to building a cURL package in vcpkg. So if there was support for that environment variable, I could very easily run the test here. (Which I have not managed so far, with the .bat, because I really did not understand what I need to do to make this .bat run, while a prerequisite "you need this environment variable pointing to a Strawberry or ActiveState perl" is rather easy to understand, even for me).

The underlying problem with an environment variable is the same as with the batch script: a user needs to know it exists and then take action to get the test running.

Right now, there is a .bat file hidden in t/. Nobody knows about this.

With the prerequisite I propose, the user running the test script would see that some test was skipped because of an unmet prerequisite. Reading the documentation above that prerequisite would then inform the user that a specific environment variable needs to be set, to point to an appropriate perl.exe.

However, to truly ensure the test always runs

This is not my goal. I do not want that test to run always, in particular not when there is no Strawberry nor ActiveState Perl to test against.

What I want is to make it easy and convenient for people who are not called @ColMelvin to discover and to run this test ;-)

[dscho]

On Thu, 5 Jul 2018, Beat Bolli wrote: Can't we use `which -a perl | grep -v ^/usr | grep -v /^mingw` to check if a MSWin32 perl is found in some other path?

I would rather have the environment variable, but you're right, we could probably have both. The problem with your version is that it is not portable IIRC (Git tries to avoid `which` for some reason that I seem to vaguely recall being portability). To help with that, you could first use ```sh { test_have_prereq MINGW || test_have_prereq CYGWIN; } && ``` in that lazy prerequisite, but I think at this stage it is just too much effort for too little gain. The main benefit for me will be that I can easily reuse my vcpkg's portable Strawberry Perl to test this.

[bbolli]

On Thu, 5 Jul 2018, Beat Bolli wrote...

I have deleted this comment because I saw after writing it that this was exactly the method used in the commit, so my comment was redundant.

[dscho]

@ColMelvin are you still interested in this PR?

[dscho]

@ColMelvin ping?

[ColMelvin]

Oh, I thought this was resolved. Apparently, I misread the comments and conflated the authors. Adding an optional environmental variable should be easy to add.

[ColMelvin]

The problem with your version is that it is not portable IIRC (Git tries to avoid which for some reason that I seem to vaguely recall being portability).

Ah, I see what you mean, the t/check-non-portable-shell.pl script does not like which. Though, that script doesn't detect my usage. That may be a separate issue.

[dscho]

@ColMelvin what's the status on this PR?

[ColMelvin]

I am under the impression that all concerns raised were addressed and that this change is awaiting merge. Other than the merge conflicts that have appeared, are there any concerns preventing this from merging?

[dscho]

I am really not sure that I like the third patch. I'd rather have this an opt-in than an automatic default that adds (typically useless) churn to every invocation of make.

[dscho]

I am not sure that this is a good change. By default, the Git for Windows SDK comes with a perl.exe and we know exactly which one it is: the one that is shipped with (and therefore, supported by) Git for Windows.

In other words, if you really want this, I would want to make it an opt-in rather than an opt-out.

Besides, would it not make things substantially cleaner if you simply prepended the path to the known MSWin32 Perl (if configured via MSWIN32_PERL_PATH explicitly) to PATH? That way, perl would already find the Perl interpreter you want, no need to play extra games with any mswin32_perl shell function.

[dscho]

Does that really work? There are native .dll files that some of the Perl modules want to load, and I am rather certain that they can be used only by /usr/bin/perl. It has to be an MSYS Perl. It even has to match the precise version, otherwise it won't load.

[dscho]

Let's close this as stale.