Skip to content

Improve handling of commands in Git.pm on native Windows #1604

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.

Sign up for GitHub

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

Closed
wants to merge 3 commits into from
Closed

Improve handling of commands in Git.pm on native Windows #1604

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
fff0040
Perl: Support spaces in command arguments on MSWin32
ColMelvin Mar 31, 2018
5d4934a
Perl: Support piping commands on MSWin32
ColMelvin Mar 31, 2018
f6033be
Make MSWin32 Perl available to all tests
ColMelvin Oct 13, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2475,6 +2475,19 @@ LIB_PERL_GEN := $(patsubst perl/%.pm,perl/build/lib/%.pm,$(LIB_PERL))
LIB_CPAN := $(wildcard perl/FromCPAN/*.pm perl/FromCPAN/*/*.pm)
LIB_CPAN_GEN := $(patsubst perl/%.pm,perl/build/lib/%.pm,$(LIB_CPAN))

# Set MSWIN32_PERL_PATH=false to disable search
ifndef MSWIN32_PERL_PATH
MSWIN32_PERL_PATH = $(shell \
for perl in $$(type -ap perl); do \
if "$$perl" -e 'exit 1 if $$^O ne q{MSWin32}'; then \
echo "$$perl"; \
exit 0; \
fi; \
done \
)
endif
MSWIN32_PERL_PATH_SQ = $(subst ','\'',$(MSWIN32_PERL_PATH))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


ifndef NO_PERL
all:: $(LIB_PERL_GEN)
ifndef NO_PERL_CPAN_FALLBACKS
Expand Down Expand Up @@ -2573,6 +2586,7 @@ GIT-BUILD-OPTIONS: FORCE
@echo PAGER_ENV=\''$(subst ','\'',$(subst ','\'',$(PAGER_ENV)))'\' >>$@+
@echo DC_SHA1=\''$(subst ','\'',$(subst ','\'',$(DC_SHA1)))'\' >>$@+
@echo X=\'$(X)\' >>$@+
@echo MSWIN32_PERL_PATH=\''$(subst ','\'',$(MSWIN32_PERL_PATH_SQ))'\' >>$@+
ifdef TEST_OUTPUT_DIRECTORY
@echo TEST_OUTPUT_DIRECTORY=\''$(subst ','\'',$(subst ','\'',$(TEST_OUTPUT_DIRECTORY)))'\' >>$@+
endif
Expand Down
68 changes: 27 additions & 41 deletions perl/Git.pm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1607,15 +1607,11 @@ sub _command_common_pipe {
# ActiveState Perl
#defined $opts{STDERR} and
# warn 'ignoring STDERR option - running w/ ActiveState';
$direction eq '-|' or
die 'input pipe for ActiveState not implemented';
# the strange construction with *ACPIPE is just to
# explain the tie below that we want to bind to
# a handle class, not scalar. It is not known if
# it is something specific to ActiveState Perl or
# just a Perl quirk.
tie (*ACPIPE, 'Git::activestate_pipe', $cmd, @args);
$fh = *ACPIPE;
my $cmdline = make_windows_commandline('git', $cmd, @args);
my $pid = open($fh, $direction, $cmdline);
if (not defined $pid) {
throw Error::Simple("open failed: $!");
}

} else {
my $pid = open($fh, $direction);
Expand Down Expand Up @@ -1686,44 +1682,34 @@ sub DESTROY {
}


# Pipe implementation for ActiveState Perl.

package Git::activestate_pipe;
sub make_windows_commandline {
my $cmdline = join ' ', unescape_windows_commandline_args(@_);

sub TIEHANDLE {
my ($class, @params) = @_;
# FIXME: This is probably horrible idea and the thing will explode
# at the moment you give it arguments that require some quoting,
# but I have no ActiveState clue... --pasky
# Let's just hope ActiveState Perl does at least the quoting
# correctly.
my @data = qx{git @params};
bless { i => 0, data => \@data }, $class;
# The set of meta-characters, as determined by Perl, was found in
# has_shell_metachars() in perl5/win32/win32.c
return unescape_cmd_commandline($cmdline) if $cmdline =~ m/[<>|%]/;
return $cmdline;
}

sub READLINE {
my $self = shift;
if ($self->{i} >= scalar @{$self->{data}}) {
return undef;
}
my $i = $self->{i};
if (wantarray) {
$self->{i} = $#{$self->{'data'}} + 1;
return splice(@{$self->{'data'}}, $i);
# See https://blogs.msdn.microsoft.com/twistylittlepassagesallalike/2011/04/23/everyone-quotes-command-line-arguments-the-wrong-way/
sub unescape_windows_commandline_args {
my @out;
foreach (@_) {
my $value = $_;
$value =~ s{(\\*)"}{$1$1\\"}g;
if ($value =~ m/\W/) {
$value =~ s{(\\+)$}{$1$1};
$value = "\"$value\"";
}
push @out, $value;
}
$self->{i} = $i + 1;
return $self->{'data'}->[ $i ];
}

sub CLOSE {
my $self = shift;
delete $self->{data};
delete $self->{i};
return @out;
}

sub EOF {
my $self = shift;
return ($self->{i} >= scalar @{$self->{data}});
sub unescape_cmd_commandline {
my ($cmdline) = @_;
$cmdline =~ s{([()%!^"<>&|])}{^$1}g;
return $cmdline;
}


Expand Down
32 changes: 32 additions & 0 deletions t/t9701-perl-git-MSWin32.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
#!/bin/sh
#
# Copyright (c) 2018 Chris Lindee
#

test_description='perl interface (Git.pm) on MSWin32'
. ./test-lib.sh

if ! test_have_prereq MSWIN32_PERL; then
skip_all='skipping perl on MSWin32 interface tests, MSWin32 perl not available'
test_done
fi

mswin32_perl -MTest::More -e 0 2>/dev/null || {
skip_all="MSWin32 Perl Test::More unavailable, skipping test"
test_done
}

# The external test will outputs its own plan
test_external_has_tap=1

# Ensure paths are recognized by Windows executables.
perl_test_path="$TEST_DIRECTORY"/t9701/test.pl
if test_have_prereq CYGWIN || test_have_prereq MINGW; then
perl_test_path="$(cygpath -w "$perl_test_path")"
fi

test_external_without_stderr \
'Windows command line (Perl API)' \
mswin32_perl "$perl_test_path"

test_done
38 changes: 38 additions & 0 deletions t/t9701/test.pl
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
#!/usr/bin/perl
use lib (split(/:/, $ENV{GITPERLLIB}));

use 5.008;
use strict;
use warnings;

use Test::More qw(no_plan);

BEGIN {
# t9701-perl-git-MSWin32.sh kicks off our testing, so we have to go from
# there.
Test::More->builder->current_test(1);
Test::More->builder->no_ending(1);
}

my @tests = (
'\\\\',
'()%!^"<>&|',
'C:\\Windows\\system32\\',
map { chr } 32 .. 126, 160 .. 255 # printable ISO-8859-1, the base for Windows-1252
);

# Just in case
plan skip_all => 'Test will not run on MSYS Perl (distributed with Git for Windows)' if $^O eq 'msys';
plan skip_all => 'Test requires Microsoft Windows' if $^O ne 'MSWin32';

require_ok('Git');

foreach (@tests) {
my $cmdline = Git::make_windows_commandline($^X, '-e', 'print $ARGV[0]', $_);
is qx{$cmdline}, $_, $cmdline;
}

printf "1..%d\n", Test::More->builder->current_test;

my $is_passing = eval { Test::More->is_passing };
exit($is_passing ? 0 : 1) unless $@ =~ /Can't locate object method/;
8 changes: 8 additions & 0 deletions t/test-lib-functions.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -933,6 +933,14 @@ perl () {
command "$PERL_PATH" "$@" 2>&7
} 7>&2 2>&4

mswin32_perl () {
local GITPERLLIB="$GITPERLLIB"
if test_have_prereq CYGWIN || test_have_prereq MINGW; then
GITPERLLIB="$(cygpath -w -p "$GITPERLLIB")"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

fi
command "$MSWIN32_PERL_PATH" "$@" 2>&7
} 7>&2 2>&4

# Is the value one of the various ways to spell a boolean true/false?
test_normalize_bool () {
git -c magic.variable="$1" config --bool magic.variable 2>/dev/null
Expand Down
10 changes: 9 additions & 1 deletion t/test-lib.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ then
exit 1
fi
. "$GIT_BUILD_DIR"/GIT-BUILD-OPTIONS
export PERL_PATH SHELL_PATH
export MSWIN32_PERL_PATH PERL_PATH SHELL_PATH

################################################################
# It appears that people try to run tests without building...
Expand Down Expand Up @@ -1268,3 +1268,11 @@ test_lazy_prereq CURL '
test_lazy_prereq SHA1 '
test $(git hash-object /dev/null) = e69de29bb2d1d6434b8b29ae775ad8c2e48c5391
'

# Some tests require a native Win32 Perl interpreter, such as Strawberry Perl
# or ActiveState Perl, which is not distributed with Git for Windows. These
# tests only run if an appropriate Perl is specified (via MSWIN32_PERL_PATH).
test_lazy_prereq MSWIN32_PERL '
test -n "$MSWIN32_PERL_PATH" &&
$MSWIN32_PERL_PATH -e "exit 1 if \$^O ne q{MSWin32}"
'