{"thread":{"id":"32777","subject":"[PATCH 0/7] perl/Git.pm: a bunch of fixes for Windows","startedAt":"2013-01-30T17:22:56Z","lastAt":"2013-02-25T06:54:24Z","messageCount":9,"participants":["Gustavo L. de M. Chaves","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":7},"messages":[{"id":"208278","messageId":"1359566583-19654-1-git-send-email-gnustavo@cpan.org","threadId":"32777","inReplyTo":null,"subject":"[PATCH 0/7] perl/Git.pm: a bunch of fixes for Windows","fromName":"Gustavo L. de M. Chaves","fromEmail":"gnustavo@cpan.org","sentAt":"2013-01-30T17:22:56Z","receivedAt":"2013-01-30T17:22:56Z","isPatch":true,"sender":{"key":"gnustavo@cpan.org","avatar":"https://gravatar.com/avatar/f49379a63251fa4d6c0038b8b834d06d82fe0f8c92b72d8c72088b21628e51db?d=mp&s=160"},"body":"From: \"Gustavo L. de M. Chaves\" \n\nI'm working on Git::Hooks, a Perl module to facilitate the\nimplementation of git hooks. (http://search.cpan.org/dist/Git-Hooks/)\n\nGit::Hooks uses the Git module implemented in perl/Git.pm and\ndistributed with git.\n\nWhile working on porting Git::Hooks to Windows I stumbled upon a few\nproblems in the Git module, problems specific to the Windows\nenvironment. In the following sequence of patches I try to fix them.\n\nFor the record, I'm using Strawberry Perl on Windows.\n\nThis is my first patch submission to git. I tried to follow all the\nproject conventions but I may have done it wrong. If so, please, help\nme learn it.\n\nThanks!\n\nGustavo L. de M. Chaves (7):\n perl/Git.pm: test portably if a path is absolute\n perl/Git.pm: set up command environment on Windows\n perl/Git.pm: fix _cmd_close on Windows\n perl/Git.pm: escape external command's arguments on Windows\n perl/Git.pm: simplify Git::activestate_pipe\n perl/Git.pm: make command pipe work in slurp-mode on Windows\n perl/Git.pm: rename 'ActiveState' to 'Windows'\n\n perl/Git.pm | 63 +++++++++++++++++++++++++++++++++++--------------------------\n 1 file changed, 36 insertions(+), 27 deletions(-)\n\n-- \n1.7.12.464.g83379df.dirty\n"},{"id":"208284","messageId":"1359566583-19654-2-git-send-email-gnustavo@cpan.org","threadId":"32777","inReplyTo":"1359566583-19654-1-git-send-email-gnustavo@cpan.org","subject":"[PATCH 1/7] perl/Git.pm: test portably if a path is absolute","fromName":"Gustavo L. de M. Chaves","fromEmail":"gnustavo@cpan.org","sentAt":"2013-01-30T17:22:57Z","receivedAt":"2013-01-30T17:22:57Z","isPatch":true,"sender":{"key":"gnustavo@cpan.org","avatar":"https://gravatar.com/avatar/f49379a63251fa4d6c0038b8b834d06d82fe0f8c92b72d8c72088b21628e51db?d=mp&s=160"},"body":"From: \"Gustavo L. de M. Chaves\" \n\nThe code was testing if a path was absolute by checking if its first\ncharacter was a '/'. This does not work on Windows.\n\nThe portable way to do it is to use File::Spec::file_name_is_absolute.\n\nSigned-off-by: Gustavo L. de M. Chaves \n---\n perl/Git.pm | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/perl/Git.pm b/perl/Git.pm\nindex 931047c..658b602 100644\n--- a/perl/Git.pm\n+++ b/perl/Git.pm\n@@ -185,7 +185,7 @@ sub repository {\n \t\t};\n \n \t\tif ($dir) {\n-\t\t\t$dir =~ m#^/# or $dir = $opts{Directory} . '/' . $dir;\n+\t\t\t$dir = $opts{Directory} . '/' . $dir unless File::Spec->file_name_is_absolute($dir);\n \t\t\t$opts{Repository} = abs_path($dir);\n \n \t\t\t# If --git-dir went ok, this shouldn't die either.\n-- \n1.7.12.464.g83379df.dirty\n"},{"id":"208279","messageId":"1359566583-19654-3-git-send-email-gnustavo@cpan.org","threadId":"32777","inReplyTo":"1359566583-19654-1-git-send-email-gnustavo@cpan.org","subject":"[PATCH 2/7] perl/Git.pm: set up command environment on Windows","fromName":"Gustavo L. de M. Chaves","fromEmail":"gnustavo@cpan.org","sentAt":"2013-01-30T17:22:58Z","receivedAt":"2013-01-30T17:22:58Z","isPatch":true,"sender":{"key":"gnustavo@cpan.org","avatar":"https://gravatar.com/avatar/f49379a63251fa4d6c0038b8b834d06d82fe0f8c92b72d8c72088b21628e51db?d=mp&s=160"},"body":"From: \"Gustavo L. de M. Chaves\" \n\nRoutine _cmd_exec invokes _setup_git_cmd_env inside the child process\nbefore invoking an external git command to set up the environment\nvariables GIT_DIR and GIT_WORK_TREE and, also, to chdir to the\nrepository. But _cmd_exec is only used on Unix. On Windows, it's not\nused and the main code path is in _command_common_pipe, which didn't\nprepare the environment like _cmd_exec.\n\nWithout this environment preparation some git commands, such as \"git\nclone\", don't work.\n\nWe can't use _setup_git_cmd_env in this case because we don't use a\nforking open like _cmd_exec does and don't get a chance to make such\npreparations on the child process.\n\nSo, the preparation is done on _command_common_pipe by setting up\nlocalized environment variables and by chdir temporarily just before\ninvoking the external command.\n\nSigned-off-by: Gustavo L. de M. Chaves \n---\n perl/Git.pm | 14 ++++++++++++++\n 1 file changed, 14 insertions(+)\n\ndiff --git a/perl/Git.pm b/perl/Git.pm\nindex 658b602..e14b41a 100644\n--- a/perl/Git.pm\n+++ b/perl/Git.pm\n@@ -1302,6 +1302,19 @@ sub _command_common_pipe {\n \t\t#\twarn 'ignoring STDERR option - running w/ ActiveState';\n \t\t$direction eq '-|' or\n \t\t\tdie 'input pipe for ActiveState not implemented';\n+\n+\t\t# Set up repo environment\n+\t\tlocal $ENV{GIT_DIR} = $self->repo_path() if defined $self && $self->repo_path();\n+\t\tlocal $ENV{GIT_WORK_TREE} = $self->wc_path() if defined $self && $self->repo_path() && $self->wc_path();\n+\n+\t\tmy $cwd = cwd;\n+\n+\t\tif (defined $self) {\n+\t\t\tchdir $self->repo_path() if $self->repo_path();\n+\t\t\tchdir $self->wc_path()\t if $self->wc_path();\n+\t\t\tchdir $self->wc_subdir() if $self->wc_subdir();\n+\t\t}\n+\n \t\t# the strange construction with *ACPIPE is just to\n \t\t# explain the tie below that we want to bind to\n \t\t# a handle class, not scalar. It is not known if\n@@ -1310,6 +1323,7 @@ sub _command_common_pipe {\n \t\ttie (*ACPIPE, 'Git::activestate_pipe', $cmd, @args);\n \t\t$fh = *ACPIPE;\n \n+\t\tchdir $cwd;\n \t} else {\n \t\tmy $pid = open($fh, $direction);\n \t\tif (not defined $pid) {\n-- \n1.7.12.464.g83379df.dirty\n"},{"id":"208283","messageId":"1359566583-19654-4-git-send-email-gnustavo@cpan.org","threadId":"32777","inReplyTo":"1359566583-19654-1-git-send-email-gnustavo@cpan.org","subject":"[PATCH 3/7] perl/Git.pm: fix _cmd_close on Windows","fromName":"Gustavo L. de M. Chaves","fromEmail":"gnustavo@cpan.org","sentAt":"2013-01-30T17:22:59Z","receivedAt":"2013-01-30T17:22:59Z","isPatch":true,"sender":{"key":"gnustavo@cpan.org","avatar":"https://gravatar.com/avatar/f49379a63251fa4d6c0038b8b834d06d82fe0f8c92b72d8c72088b21628e51db?d=mp&s=160"},"body":"From: \"Gustavo L. de M. Chaves\" \n\nThe Git::activestate_pipe::CLOSE routine wasn't explicitly returning\nanything. This means that on Windows the routine _cmd_close wasn't\nchecking correctly the external command's exit code.\n\nNow we store the command's exit code on the object created by\nGit::activestate_pipe::TIEHANDLE and return a sensible value on\nGit::activestate_pipe::CLOSE to _cmd_close.\n\nSigned-off-by: Gustavo L. de M. Chaves \n---\n perl/Git.pm | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/perl/Git.pm b/perl/Git.pm\nindex e14b41a..ef3134b 100644\n--- a/perl/Git.pm\n+++ b/perl/Git.pm\n@@ -1404,7 +1404,7 @@ sub TIEHANDLE {\n \t# Let's just hope ActiveState Perl does at least the quoting\n \t# correctly.\n \tmy @data = qx{git @params};\n-\tbless { i => 0, data => \\@data }, $class;\n+\tbless { i => 0, data => \\@data, exit => $? }, $class;\n }\n \n sub READLINE {\n@@ -1425,6 +1425,7 @@ sub CLOSE {\n \tmy $self = shift;\n \tdelete $self->{data};\n \tdelete $self->{i};\n+\treturn $self->{exit} == 0;\n }\n \n sub EOF {\n-- \n1.7.12.464.g83379df.dirty\n"},{"id":"208281","messageId":"1359566583-19654-5-git-send-email-gnustavo@cpan.org","threadId":"32777","inReplyTo":"1359566583-19654-1-git-send-email-gnustavo@cpan.org","subject":"[PATCH 4/7] perl/Git.pm: escape external command's arguments on Windows","fromName":"Gustavo L. de M. Chaves","fromEmail":"gnustavo@cpan.org","sentAt":"2013-01-30T17:23:00Z","receivedAt":"2013-01-30T17:23:00Z","isPatch":true,"sender":{"key":"gnustavo@cpan.org","avatar":"https://gravatar.com/avatar/f49379a63251fa4d6c0038b8b834d06d82fe0f8c92b72d8c72088b21628e51db?d=mp&s=160"},"body":"From: \"Gustavo L. de M. Chaves\" \n\nOn Windows, the external git commands are invoked using backticks by\nGit::activestate_pipe::TIEHANDLE, but there was no attempt to properly\nquote their arguments. This caused problems with all but the simplest\ncommand invokations.\n\nThe arguments are now surrounded by quotes and internal quotes are\ndoubled. This is not a complete quoting solution but takes care of\nsome of the most common problems on Windows.\n\nSigned-off-by: Gustavo L. de M. Chaves \n---\n perl/Git.pm | 10 ++++------\n 1 file changed, 4 insertions(+), 6 deletions(-)\n\ndiff --git a/perl/Git.pm b/perl/Git.pm\nindex ef3134b..42c3971 100644\n--- a/perl/Git.pm\n+++ b/perl/Git.pm\n@@ -1398,12 +1398,10 @@ use strict;\n \n sub TIEHANDLE {\n \tmy ($class, @params) = @_;\n-\t# FIXME: This is probably horrible idea and the thing will explode\n-\t# at the moment you give it arguments that require some quoting,\n-\t# but I have no ActiveState clue... --pasky\n-\t# Let's just hope ActiveState Perl does at least the quoting\n-\t# correctly.\n-\tmy @data = qx{git @params};\n+\t# FIXME: The quoting done below is not completely right but it\n+\t# should take care of the most common cases.\n+\tmy @escaped_params = map { \"\\\"$_\\\"\" } map { s/\"/\"\"/g; $_ } @params;\n+\tmy @data = qx{git @escaped_params};\n \tbless { i => 0, data => \\@data, exit => $? }, $class;\n }\n \n-- \n1.7.12.464.g83379df.dirty\n"},{"id":"208280","messageId":"1359566583-19654-6-git-send-email-gnustavo@cpan.org","threadId":"32777","inReplyTo":"1359566583-19654-1-git-send-email-gnustavo@cpan.org","subject":"[PATCH 5/7] perl/Git.pm: simplify Git::activestate_pipe","fromName":"Gustavo L. de M. Chaves","fromEmail":"gnustavo@cpan.org","sentAt":"2013-01-30T17:23:01Z","receivedAt":"2013-01-30T17:23:01Z","isPatch":true,"sender":{"key":"gnustavo@cpan.org","avatar":"https://gravatar.com/avatar/f49379a63251fa4d6c0038b8b834d06d82fe0f8c92b72d8c72088b21628e51db?d=mp&s=160"},"body":"From: \"Gustavo L. de M. Chaves\" \n\nGit::activestate_pipe::TIEHANDLE creates an object to keep the\nexternal command's output as an array of lines. The object also kept\nan index into the array to know up to which line had already been read\nvia Git::activestate_pipe::READLINE.\n\nWe don't really need that index because lines already read don't need\nto be kept. So, we simply unshift lines as they're being read and use\nthe array's size to know when we have read all lines.\n\nThis implementation uses more idiomatic Perl, which makes it more\nreadable and probably a little bit faster.\n\nSigned-off-by: Gustavo L. de M. Chaves \n---\n perl/Git.pm | 17 ++++++-----------\n 1 file changed, 6 insertions(+), 11 deletions(-)\n\ndiff --git a/perl/Git.pm b/perl/Git.pm\nindex 42c3971..2d88b89 100644\n--- a/perl/Git.pm\n+++ b/perl/Git.pm\n@@ -1402,33 +1402,28 @@ sub TIEHANDLE {\n \t# should take care of the most common cases.\n \tmy @escaped_params = map { \"\\\"$_\\\"\" } map { s/\"/\"\"/g; $_ } @params;\n \tmy @data = qx{git @escaped_params};\n-\tbless { i => 0, data => \\@data, exit => $? }, $class;\n+\tbless { data => \\@data, exit => $? }, $class;\n }\n \n sub READLINE {\n \tmy $self = shift;\n-\tif ($self->{i} >= scalar @{$self->{data}}) {\n-\t\treturn undef;\n-\t}\n-\tmy $i = $self->{i};\n+\treturn unless @{$self->{data}};\n \tif (wantarray) {\n-\t\t$self->{i} = $#{$self->{'data'}} + 1;\n-\t\treturn splice(@{$self->{'data'}}, $i);\n+\t\treturn splice @{$self->{data}};\n+\t} else {\n+\t\treturn shift @{$self->{data}};\n \t}\n-\t$self->{i} = $i + 1;\n-\treturn $self->{'data'}->[ $i ];\n }\n \n sub CLOSE {\n \tmy $self = shift;\n \tdelete $self->{data};\n-\tdelete $self->{i};\n \treturn $self->{exit} == 0;\n }\n \n sub EOF {\n \tmy $self = shift;\n-\treturn ($self->{i} >= scalar @{$self->{data}});\n+\treturn @{$self->{data}} == 0;\n }\n \n \n-- \n1.7.12.464.g83379df.dirty\n"},{"id":"208282","messageId":"1359566583-19654-7-git-send-email-gnustavo@cpan.org","threadId":"32777","inReplyTo":"1359566583-19654-1-git-send-email-gnustavo@cpan.org","subject":"[PATCH 6/7] perl/Git.pm: make command pipe work in slurp-mode on Windows","fromName":"Gustavo L. de M. Chaves","fromEmail":"gnustavo@cpan.org","sentAt":"2013-01-30T17:23:02Z","receivedAt":"2013-01-30T17:23:02Z","isPatch":true,"sender":{"key":"gnustavo@cpan.org","avatar":"https://gravatar.com/avatar/f49379a63251fa4d6c0038b8b834d06d82fe0f8c92b72d8c72088b21628e51db?d=mp&s=160"},"body":"From: \"Gustavo L. de M. Chaves\" \n\nGit::activestate_pipe::READLINE implementation wasn't working when the\npipe was read in slurp-mode (i.e., with $/ undefined).\n\nNow, when in slurp-mode it correctly returns the remaining lines\njoined together.\n\nSigned-off-by: Gustavo L. de M. Chaves \n---\n perl/Git.pm | 4 +++-\n 1 file changed, 3 insertions(+), 1 deletion(-)\n\ndiff --git a/perl/Git.pm b/perl/Git.pm\nindex 2d88b89..fdef024 100644\n--- a/perl/Git.pm\n+++ b/perl/Git.pm\n@@ -1410,8 +1410,10 @@ sub READLINE {\n \treturn unless @{$self->{data}};\n \tif (wantarray) {\n \t\treturn splice @{$self->{data}};\n-\t} else {\n+\t} elsif (defined $/) {\n \t\treturn shift @{$self->{data}};\n+\t} else {\n+\t\treturn join('', splice @{$self->{data}});\n \t}\n }\n \n-- \n1.7.12.464.g83379df.dirty\n"},{"id":"208285","messageId":"1359566583-19654-8-git-send-email-gnustavo@cpan.org","threadId":"32777","inReplyTo":"1359566583-19654-1-git-send-email-gnustavo@cpan.org","subject":"[PATCH 7/7] perl/Git.pm: rename 'ActiveState' to 'Windows'","fromName":"Gustavo L. de M. Chaves","fromEmail":"gnustavo@cpan.org","sentAt":"2013-01-30T17:23:03Z","receivedAt":"2013-01-30T17:23:03Z","isPatch":true,"sender":{"key":"gnustavo@cpan.org","avatar":"https://gravatar.com/avatar/f49379a63251fa4d6c0038b8b834d06d82fe0f8c92b72d8c72088b21628e51db?d=mp&s=160"},"body":"From: \"Gustavo L. de M. Chaves\" \n\nWindows specific code was mentioning ActiveState Perl specifically,\nbut that code works with Strawberry Perl too.\n\nHence, we rename every instance of 'ActivePerl' to 'Windows' to convey\nthe more general idea.\n\nSigned-off-by: Gustavo L. de M. Chaves \n---\n perl/Git.pm | 17 ++++++++---------\n 1 file changed, 8 insertions(+), 9 deletions(-)\n\ndiff --git a/perl/Git.pm b/perl/Git.pm\nindex fdef024..e03b82f 100644\n--- a/perl/Git.pm\n+++ b/perl/Git.pm\n@@ -1297,11 +1297,10 @@ sub _command_common_pipe {\n \n \tmy $fh;\n \tif ($^O eq 'MSWin32') {\n-\t\t# ActiveState Perl\n \t\t#defined $opts{STDERR} and\n-\t\t#\twarn 'ignoring STDERR option - running w/ ActiveState';\n+\t\t#\twarn 'ignoring STDERR option - running on Windows;\n \t\t$direction eq '-|' or\n-\t\t\tdie 'input pipe for ActiveState not implemented';\n+\t\t\tdie 'input pipe for Windows not implemented';\n \n \t\t# Set up repo environment\n \t\tlocal $ENV{GIT_DIR} = $self->repo_path() if defined $self && $self->repo_path();\n@@ -1315,13 +1314,13 @@ sub _command_common_pipe {\n \t\t\tchdir $self->wc_subdir() if $self->wc_subdir();\n \t\t}\n \n-\t\t# the strange construction with *ACPIPE is just to\n+\t\t# the strange construction with *WINPIPE is just to\n \t\t# explain the tie below that we want to bind to\n \t\t# a handle class, not scalar. It is not known if\n-\t\t# it is something specific to ActiveState Perl or\n+\t\t# it is something specific to Perl on Windows or\n \t\t# just a Perl quirk.\n-\t\ttie (*ACPIPE, 'Git::activestate_pipe', $cmd, @args);\n-\t\t$fh = *ACPIPE;\n+\t\ttie (*WINPIPE, 'Git::windows_pipe', $cmd, @args);\n+\t\t$fh = *WINPIPE;\n \n \t\tchdir $cwd;\n \t} else {\n@@ -1391,9 +1390,9 @@ sub DESTROY {\n }\n \n \n-# Pipe implementation for ActiveState Perl.\n+# Pipe implementation for Perl on Windows.\n \n-package Git::activestate_pipe;\n+package Git::windows_pipe;\n use strict;\n \n sub TIEHANDLE {\n-- \n1.7.12.464.g83379df.dirty\n"},{"id":"210230","messageId":"7vsj4k289r.fsf@alter.siamese.dyndns.org","threadId":"32777","inReplyTo":"1359566583-19654-1-git-send-email-gnustavo@cpan.org","subject":"Re: [PATCH 0/7] perl/Git.pm: a bunch of fixes for Windows","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-02-25T06:54:24Z","receivedAt":"2013-02-25T06:54:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Gustavo L. de M. Chaves\" writes:\n\n> ...\n> While working on porting Git::Hooks to Windows I stumbled upon a few\n> problems in the Git module, problems specific to the Windows\n> environment. In the following sequence of patches I try to fix them.\n\nAny comment on this from Windows folks?\n"}]}