[PATCH 0/7] perl/Git.pm: a bunch of fixes for Windows

[PATCH 0/7] perl/Git.pm: a bunch of fixes for Windows

[Gustavo L. de M. Chaves]

From: "Gustavo L. de M. Chaves" <gnustavo@cpan.org>

I'm working on Git::Hooks, a Perl module to facilitate the
implementation of git hooks. (http://search.cpan.org/dist/Git-Hooks/)

Git::Hooks uses the Git module implemented in perl/Git.pm and
distributed with git.

While working on porting Git::Hooks to Windows I stumbled upon a few
problems in the Git module, problems specific to the Windows
environment. In the following sequence of patches I try to fix them.

For the record, I'm using Strawberry Perl on Windows.

This is my first patch submission to git. I tried to follow all the
project conventions but I may have done it wrong. If so, please, help
me learn it.

Thanks!

Gustavo L. de M. Chaves (7):
  perl/Git.pm: test portably if a path is absolute
  perl/Git.pm: set up command environment on Windows
  perl/Git.pm: fix _cmd_close on Windows
  perl/Git.pm: escape external command's arguments on Windows
  perl/Git.pm: simplify Git::activestate_pipe
  perl/Git.pm: make command pipe work in slurp-mode on Windows
  perl/Git.pm: rename 'ActiveState' to 'Windows'

 perl/Git.pm | 63 +++++++++++++++++++++++++++++++++++--------------------------
 1 file changed, 36 insertions(+), 27 deletions(-)

-- 
1.7.12.464.g83379df.dirty

[PATCH 1/7] perl/Git.pm: test portably if a path is absolute

[Gustavo L. de M. Chaves]

From: "Gustavo L. de M. Chaves" <gnustavo@cpan.org>

The code was testing if a path was absolute by checking if its first
character was a '/'. This does not work on Windows.

The portable way to do it is to use File::Spec::file_name_is_absolute.

Signed-off-by: Gustavo L. de M. Chaves <gnustavo@cpan.org>
---
 perl/Git.pm | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/perl/Git.pm b/perl/Git.pm
index 931047c..658b602 100644
--- a/perl/Git.pm
+++ b/perl/Git.pm
@@ -185,7 +185,7 @@ sub repository {
 		};
 
 		if ($dir) {
-			$dir =~ m#^/# or $dir = $opts{Directory} . '/' . $dir;
+			$dir = $opts{Directory} . '/' . $dir unless File::Spec->file_name_is_absolute($dir);
 			$opts{Repository} = abs_path($dir);
 
 			# If --git-dir went ok, this shouldn't die either.
-- 
1.7.12.464.g83379df.dirty

[PATCH 2/7] perl/Git.pm: set up command environment on Windows

[Gustavo L. de M. Chaves]

From: "Gustavo L. de M. Chaves" <gnustavo@cpan.org>

Routine _cmd_exec invokes _setup_git_cmd_env inside the child process
before invoking an external git command to set up the environment
variables GIT_DIR and GIT_WORK_TREE and, also, to chdir to the
repository. But _cmd_exec is only used on Unix. On Windows, it's not
used and the main code path is in _command_common_pipe, which didn't
prepare the environment like _cmd_exec.

Without this environment preparation some git commands, such as "git
clone", don't work.

We can't use _setup_git_cmd_env in this case because we don't use a
forking open like _cmd_exec does and don't get a chance to make such
preparations on the child process.

So, the preparation is done on _command_common_pipe by setting up
localized environment variables and by chdir temporarily just before
invoking the external command.

Signed-off-by: Gustavo L. de M. Chaves <gnustavo@cpan.org>
---
 perl/Git.pm | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/perl/Git.pm b/perl/Git.pm
index 658b602..e14b41a 100644
--- a/perl/Git.pm
+++ b/perl/Git.pm
@@ -1302,6 +1302,19 @@ sub _command_common_pipe {
 		#	warn 'ignoring STDERR option - running w/ ActiveState';
 		$direction eq '-|' or
 			die 'input pipe for ActiveState not implemented';
+
+		# Set up repo environment
+		local $ENV{GIT_DIR}       = $self->repo_path() if defined $self && $self->repo_path();
+		local $ENV{GIT_WORK_TREE} = $self->wc_path()   if defined $self && $self->repo_path() && $self->wc_path();
+
+		my $cwd = cwd;
+
+		if (defined $self) {
+			chdir $self->repo_path() if $self->repo_path();
+			chdir $self->wc_path()	 if $self->wc_path();
+			chdir $self->wc_subdir() if $self->wc_subdir();
+		}
+
 		# 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
@@ -1310,6 +1323,7 @@ sub _command_common_pipe {
 		tie (*ACPIPE, 'Git::activestate_pipe', $cmd, @args);
 		$fh = *ACPIPE;
 
+		chdir $cwd;
 	} else {
 		my $pid = open($fh, $direction);
 		if (not defined $pid) {
-- 
1.7.12.464.g83379df.dirty

[PATCH 3/7] perl/Git.pm: fix _cmd_close on Windows

[Gustavo L. de M. Chaves]

From: "Gustavo L. de M. Chaves" <gnustavo@cpan.org>

The Git::activestate_pipe::CLOSE routine wasn't explicitly returning
anything. This means that on Windows the routine _cmd_close wasn't
checking correctly the external command's exit code.

Now we store the command's exit code on the object created by
Git::activestate_pipe::TIEHANDLE and return a sensible value on
Git::activestate_pipe::CLOSE to _cmd_close.

Signed-off-by: Gustavo L. de M. Chaves <gnustavo@cpan.org>
---
 perl/Git.pm | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/perl/Git.pm b/perl/Git.pm
index e14b41a..ef3134b 100644
--- a/perl/Git.pm
+++ b/perl/Git.pm
@@ -1404,7 +1404,7 @@ sub TIEHANDLE {
 	# Let's just hope ActiveState Perl does at least the quoting
 	# correctly.
 	my @data = qx{git @params};
-	bless { i => 0, data => \@data }, $class;
+	bless { i => 0, data => \@data, exit => $? }, $class;
 }
 
 sub READLINE {
@@ -1425,6 +1425,7 @@ sub CLOSE {
 	my $self = shift;
 	delete $self->{data};
 	delete $self->{i};
+	return $self->{exit} == 0;
 }
 
 sub EOF {
-- 
1.7.12.464.g83379df.dirty

[PATCH 4/7] perl/Git.pm: escape external command's arguments on Windows

[Gustavo L. de M. Chaves]

From: "Gustavo L. de M. Chaves" <gnustavo@cpan.org>

On Windows, the external git commands are invoked using backticks by
Git::activestate_pipe::TIEHANDLE, but there was no attempt to properly
quote their arguments. This caused problems with all but the simplest
command invokations.

The arguments are now surrounded by quotes and internal quotes are
doubled. This is not a complete quoting solution but takes care of
some of the most common problems on Windows.

Signed-off-by: Gustavo L. de M. Chaves <gnustavo@cpan.org>
---
 perl/Git.pm | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/perl/Git.pm b/perl/Git.pm
index ef3134b..42c3971 100644
--- a/perl/Git.pm
+++ b/perl/Git.pm
@@ -1398,12 +1398,10 @@ use strict;
 
 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};
+	# FIXME: The quoting done below is not completely right but it
+	# should take care of the most common cases.
+	my @escaped_params = map { "\"$_\"" } map { s/"/""/g; $_ } @params;
+	my @data = qx{git @escaped_params};
 	bless { i => 0, data => \@data, exit => $? }, $class;
 }
 
-- 
1.7.12.464.g83379df.dirty

[PATCH 5/7] perl/Git.pm: simplify Git::activestate_pipe

[Gustavo L. de M. Chaves]

From: "Gustavo L. de M. Chaves" <gnustavo@cpan.org>

Git::activestate_pipe::TIEHANDLE creates an object to keep the
external command's output as an array of lines. The object also kept
an index into the array to know up to which line had already been read
via Git::activestate_pipe::READLINE.

We don't really need that index because lines already read don't need
to be kept. So, we simply unshift lines as they're being read and use
the array's size to know when we have read all lines.

This implementation uses more idiomatic Perl, which makes it more
readable and probably a little bit faster.

Signed-off-by: Gustavo L. de M. Chaves <gnustavo@cpan.org>
---
 perl/Git.pm | 17 ++++++-----------
 1 file changed, 6 insertions(+), 11 deletions(-)

diff --git a/perl/Git.pm b/perl/Git.pm
index 42c3971..2d88b89 100644
--- a/perl/Git.pm
+++ b/perl/Git.pm
@@ -1402,33 +1402,28 @@ sub TIEHANDLE {
 	# should take care of the most common cases.
 	my @escaped_params = map { "\"$_\"" } map { s/"/""/g; $_ } @params;
 	my @data = qx{git @escaped_params};
-	bless { i => 0, data => \@data, exit => $? }, $class;
+	bless { data => \@data, exit => $? }, $class;
 }
 
 sub READLINE {
 	my $self = shift;
-	if ($self->{i} >= scalar @{$self->{data}}) {
-		return undef;
-	}
-	my $i = $self->{i};
+	return unless @{$self->{data}};
 	if (wantarray) {
-		$self->{i} = $#{$self->{'data'}} + 1;
-		return splice(@{$self->{'data'}}, $i);
+		return splice @{$self->{data}};
+	} else {
+		return shift @{$self->{data}};
 	}
-	$self->{i} = $i + 1;
-	return $self->{'data'}->[ $i ];
 }
 
 sub CLOSE {
 	my $self = shift;
 	delete $self->{data};
-	delete $self->{i};
 	return $self->{exit} == 0;
 }
 
 sub EOF {
 	my $self = shift;
-	return ($self->{i} >= scalar @{$self->{data}});
+	return @{$self->{data}} == 0;
 }
 
 
-- 
1.7.12.464.g83379df.dirty

[PATCH 6/7] perl/Git.pm: make command pipe work in slurp-mode on Windows

[Gustavo L. de M. Chaves]

From: "Gustavo L. de M. Chaves" <gnustavo@cpan.org>

Git::activestate_pipe::READLINE implementation wasn't working when the
pipe was read in slurp-mode (i.e., with $/ undefined).

Now, when in slurp-mode it correctly returns the remaining lines
joined together.

Signed-off-by: Gustavo L. de M. Chaves <gnustavo@cpan.org>
---
 perl/Git.pm | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/perl/Git.pm b/perl/Git.pm
index 2d88b89..fdef024 100644
--- a/perl/Git.pm
+++ b/perl/Git.pm
@@ -1410,8 +1410,10 @@ sub READLINE {
 	return unless @{$self->{data}};
 	if (wantarray) {
 		return splice @{$self->{data}};
-	} else {
+	} elsif (defined $/) {
 		return shift @{$self->{data}};
+	} else {
+		return join('', splice @{$self->{data}});
 	}
 }
 
-- 
1.7.12.464.g83379df.dirty

[PATCH 7/7] perl/Git.pm: rename 'ActiveState' to 'Windows'

[Gustavo L. de M. Chaves]

From: "Gustavo L. de M. Chaves" <gnustavo@cpan.org>

Windows specific code was mentioning ActiveState Perl specifically,
but that code works with Strawberry Perl too.

Hence, we rename every instance of 'ActivePerl' to 'Windows' to convey
the more general idea.

Signed-off-by: Gustavo L. de M. Chaves <gnustavo@cpan.org>
---
 perl/Git.pm | 17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)

diff --git a/perl/Git.pm b/perl/Git.pm
index fdef024..e03b82f 100644
--- a/perl/Git.pm
+++ b/perl/Git.pm
@@ -1297,11 +1297,10 @@ sub _command_common_pipe {
 
 	my $fh;
 	if ($^O eq 'MSWin32') {
-		# ActiveState Perl
 		#defined $opts{STDERR} and
-		#	warn 'ignoring STDERR option - running w/ ActiveState';
+		#	warn 'ignoring STDERR option - running on Windows;
 		$direction eq '-|' or
-			die 'input pipe for ActiveState not implemented';
+			die 'input pipe for Windows not implemented';
 
 		# Set up repo environment
 		local $ENV{GIT_DIR}       = $self->repo_path() if defined $self && $self->repo_path();
@@ -1315,13 +1314,13 @@ sub _command_common_pipe {
 			chdir $self->wc_subdir() if $self->wc_subdir();
 		}
 
-		# the strange construction with *ACPIPE is just to
+		# the strange construction with *WINPIPE 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
+		# it is something specific to Perl on Windows or
 		# just a Perl quirk.
-		tie (*ACPIPE, 'Git::activestate_pipe', $cmd, @args);
-		$fh = *ACPIPE;
+		tie (*WINPIPE, 'Git::windows_pipe', $cmd, @args);
+		$fh = *WINPIPE;
 
 		chdir $cwd;
 	} else {
@@ -1391,9 +1390,9 @@ sub DESTROY {
 }
 
 
-# Pipe implementation for ActiveState Perl.
+# Pipe implementation for Perl on Windows.
 
-package Git::activestate_pipe;
+package Git::windows_pipe;
 use strict;
 
 sub TIEHANDLE {
-- 
1.7.12.464.g83379df.dirty

Re: [PATCH 0/7] perl/Git.pm: a bunch of fixes for Windows

[Junio C Hamano]

"Gustavo L. de M. Chaves" <gnustavo@cpan.org> writes:

> ...
> While working on porting Git::Hooks to Windows I stumbled upon a few
> problems in the Git module, problems specific to the Windows
> environment. In the following sequence of patches I try to fix them.

Any comment on this from Windows folks?