[PATCH] change Perl syntax to support Perl 5.6

[PATCH] change Perl syntax to support Perl 5.6

[Robert Schiele]

git-add--interactive has one Perl command that was not yet present in
Perl 5.6.  Changing this single command makes it compatible again.

Signed-off-by: Robert Schiele <rschiele@gmail.com>
---
This is an alternative to my previous patch that just declared Perl 5.8 to
be the required version.

 git-add--interactive.perl |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index da768ee..777ec5f 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -61,7 +61,7 @@ sub run_cmd_pipe {
 		return qx{@args};
 	} else {
 		my $fh = undef;
-		open($fh, '-|', @_) or die;
+		open($fh, '-|', join(' ', @_)) or die;
 		return <$fh>;
 	}
 }
-- 
1.5.4.5

Re: [PATCH] change Perl syntax to support Perl 5.6

[Jeff King]

On Sat, Aug 30, 2008 at 07:39:47PM +0200, Robert Schiele wrote:

> git-add--interactive has one Perl command that was not yet present in
> Perl 5.6.  Changing this single command makes it compatible again.

Having read your other message, I know what it is about this command
that does not work with Perl 5.6. But probably it would be good to
mention "list form of three argument open with pipe".

>  git-add--interactive.perl |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)

What about the similar uses in cvsimport, cvsserver, and gitweb? How
about the scripts in contrib (a quick grep reveals some fast-import
scripts, blameview, cidaemon, continuous, and hooks/update-paranoid.

Most of those things are not as "core" as add--interactive, so I am not
opposed to changing just this one spot and documenting "core can use
5.6, but these other things need 5.8".

> -		open($fh, '-|', @_) or die;
> +		open($fh, '-|', join(' ', @_)) or die;

Won't this execute the command using the shell, which means that
metacharacters need to be escaped? I didn't try, but I'm pretty sure
this would break

  git add -i "file with space"

-Peff

Re: [PATCH] change Perl syntax to support Perl 5.6

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Sat, Aug 30, 2008 at 07:39:47PM +0200, Robert Schiele wrote:
>
>> git-add--interactive has one Perl command that was not yet present in
>> Perl 5.6.  Changing this single command makes it compatible again.
>
> Having read your other message, I know what it is about this command
> that does not work with Perl 5.6. But probably it would be good to
> mention "list form of three argument open with pipe".
>
>>  git-add--interactive.perl |    2 +-
>>  1 files changed, 1 insertions(+), 1 deletions(-)
>
> What about the similar uses in cvsimport, cvsserver, and gitweb? How
> about the scripts in contrib (a quick grep reveals some fast-import
> scripts, blameview, cidaemon, continuous, and hooks/update-paranoid.
>
> Most of those things are not as "core" as add--interactive, so I am not
> opposed to changing just this one spot and documenting "core can use
> 5.6, but these other things need 5.8".
>
>> -		open($fh, '-|', @_) or die;
>> +		open($fh, '-|', join(' ', @_)) or die;
>
> Won't this execute the command using the shell, which means that
> metacharacters need to be escaped? I didn't try, but I'm pretty sure
> this would break
>
>   git add -i "file with space"

I didn't try either but I think you are right.  And I agree we should say
we rely on 5.6 or newer.

I thought gitweb folks are targetting 5.6.1 as the minimum for unicode
support (Lea and Jakub Cc'ed)?

Re: [PATCH] change Perl syntax to support Perl 5.6

[Jeff King]

On Sat, Aug 30, 2008 at 11:06:06AM -0700, Junio C Hamano wrote:

> > Won't this execute the command using the shell, which means that
> > metacharacters need to be escaped? I didn't try, but I'm pretty sure
> > this would break
> >
> >   git add -i "file with space"
> 
> I didn't try either but I think you are right.  And I agree we should say
> we rely on 5.6 or newer.

I did just try, and it is indeed broken.

> I thought gitweb folks are targetting 5.6.1 as the minimum for unicode
> support (Lea and Jakub Cc'ed)?

Has it been tested? I don't have perl 5.6 handy, but:

  $ man perl58delta | grep -A8 'list form of'
       ·   If your platform supports fork(), you can use the list form of
           "open" for pipes.  For example:

               open KID_PS, "-|", "ps", "aux" or die $!;

           forks the ps(1) command (without spawning a shell, as there are
           more than three arguments to open()), and reads its standard output
           via the "KID_PS" filehandle.  See perlipc.

  $ grep -- '-|' gitweb/gitweb.perl | head
        if (open my $fd, "-|", git_cmd(), "rev-parse", "--verify", "HEAD") {
        open my $fd, "-|", git_cmd(), "cat-file", '-t', $hash or return;
        open my $fh, "-|", git_cmd(), "config", '-z', '-l',
        open my $fd, "-|", git_cmd(), "ls-tree", $base, "--", $path
        open my $fd, "-|", git_cmd(), "ls-tree", '-r', '-t', '-z', $base
        open($fd, "-|", git_cmd(), 'for-each-ref',
        open my $fd, "-|", git_cmd(), "show-ref", "--dereference",
        open my $fd, "-|", git_cmd(), "name-rev", "--tags", $hash
        open my $fd, "-|", git_cmd(), "cat-file", "tag", $tag_id or return;
        open my $fd, "-|", git_cmd(), "rev-list",

So either I am misunderstanding something, or gitweb uses a construct
that needs perl 5.8.

-Peff

Re: [PATCH] change Perl syntax to support Perl 5.6

[Robert Schiele]

[-- Attachment #1: Type: text/plain, Size: 719 bytes --]

On Sat, Aug 30, 2008 at 11:06:06AM -0700, Junio C Hamano wrote:
> I didn't try either but I think you are right.  And I agree we should say
> we rely on 5.6 or newer.

If we don't change it we need to rely on 5.8 or newer as my initial patch
suggested.

If there are problems with that change I recommend just using my initial patch
changing the documentation to require Perl 5.8 since my interest in Perl 5.6
support is not big enough to mess around with quoting all that stuff.  If
someone really needs this he or she can still do it --- it should be not too
difficult.

Robert

-- 
Robert Schiele
Dipl.-Wirtsch.informatiker	mailto:rschiele@gmail.com

"Quidquid latine dictum sit, altum sonatur."

[-- Attachment #2: Type: application/pgp-signature, Size: 194 bytes --]

Re: [PATCH] change Perl syntax to support Perl 5.6

[Jeff King]

On Sat, Aug 30, 2008 at 08:34:13PM +0200, Robert Schiele wrote:

> If there are problems with that change I recommend just using my
> initial patch changing the documentation to require Perl 5.8 since my
> interest in Perl 5.6 support is not big enough to mess around with
> quoting all that stuff.  If someone really needs this he or she can
> still do it --- it should be not too difficult.

I think it is as simple as:

diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index da768ee..4ee6f89 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -61,7 +61,7 @@ sub run_cmd_pipe {
 		return qx{@args};
 	} else {
 		my $fh = undef;
-		open($fh, '-|', @_) or die;
+		open($fh, '-|', join(' ', map { quotemeta($_) } @_)) or die;
 		return <$fh>;
 	}
 }

But I didn't do any testing beyond checking that "git add -i 'file with
spaces'" which was broken by your patch now works at all.

-Peff

Re: [PATCH] change Perl syntax to support Perl 5.6

[Jakub Narebski]

On Sat, 30 August 2008, Jeff King wrote:
> On Sat, Aug 30, 2008 at 08:34:13PM +0200, Robert Schiele wrote:
> 
> > If there are problems with that change I recommend just using my
> > initial patch changing the documentation to require Perl 5.8 since my
> > interest in Perl 5.6 support is not big enough to mess around with
> > quoting all that stuff.  If someone really needs this he or she can
> > still do it --- it should be not too difficult.

First, IIRC gitweb requires Perl 5.8 _anyway_ because of unicode stuff.

> I think it is as simple as:
> 
> diff --git a/git-add--interactive.perl b/git-add--interactive.perl
> index da768ee..4ee6f89 100755
> --- a/git-add--interactive.perl
> +++ b/git-add--interactive.perl
> @@ -61,7 +61,7 @@ sub run_cmd_pipe {
>  		return qx{@args};
>  	} else {
>  		my $fh = undef;
> -		open($fh, '-|', @_) or die;
> +		open($fh, '-|', join(' ', map { quotemeta($_) } @_)) or die;
>  		return <$fh>;
>  	}
>  }
> 
> But I didn't do any testing beyond checking that "git add -i 'file with
> spaces'" which was broken by your patch now works at all.

No, you would need something like this code from gitweb.perl (used in
very rare cases where list form of open cannot be used, which means
when we need pipeline like for compressed snapshot, or redirecting
stdout and/or stderr to /dev/null like when getting type of possibly
not existing object in git_object)

 # quote the given arguments for passing them to the shell
 # quote_command("command", "arg 1", "arg with ' and ! characters")
 # => "'command' 'arg 1' 'arg with '\'' and '\!' characters'"
 # Try to avoid using this function wherever possible.
 sub quote_command {
 	return join(' ',
 		    map( { my $a = $_; $a =~ s/(['!])/'\\$1'/g; "'$a'" } @_ ));
 }


Or you can use "open $fd, '-|'" to fork, an "manually" exec/system.
-- 
Jakub Narebski
Poland

Re: [PATCH] change Perl syntax to support Perl 5.6

[Robert Schiele]

[-- Attachment #1: Type: text/plain, Size: 902 bytes --]

On Sat, Aug 30, 2008 at 10:37:15PM +0200, Jakub Narebski wrote:
>  # quote the given arguments for passing them to the shell
>  # quote_command("command", "arg 1", "arg with ' and ! characters")
>  # => "'command' 'arg 1' 'arg with '\'' and '\!' characters'"
>  # Try to avoid using this function wherever possible.
>  sub quote_command {
>  	return join(' ',
>  		    map( { my $a = $_; $a =~ s/(['!])/'\\$1'/g; "'$a'" } @_ ));
>  }

Well, I guess you know why I did not consider it worth from my side to go into
that business.  ;-)

If someone has a concrete implementation and a list of (concrete) tests I
should do I can do so.

Otherwise I just consider Perl 5.6 as unsupported since our main development
systems have Perl 5.8 or later anyway.

Robert

-- 
Robert Schiele
Dipl.-Wirtsch.informatiker	mailto:rschiele@gmail.com

"Quidquid latine dictum sit, altum sonatur."

[-- Attachment #2: Type: application/pgp-signature, Size: 194 bytes --]

Re: [PATCH] change Perl syntax to support Perl 5.6

[Avery Pennarun]

On Sat, Aug 30, 2008 at 4:37 PM, Jakub Narebski <jnareb@gmail.com> wrote:
> Or you can use "open $fd, '-|'" to fork, an "manually" exec/system.

Shell quoting is a disaster (including security holes, where relevant)
waiting to happen.  The above is the only sane way to do it, and it
isn't very hard to implement.  (Instead of system() in the subprocess,
you can use exec().)

Have fun,

Avery

Re: [PATCH] change Perl syntax to support Perl 5.6

[Randal L. Schwartz]

>>>>> "Avery" == Avery Pennarun <apenwarr@gmail.com> writes:

Avery> Shell quoting is a disaster (including security holes, where relevant)
Avery> waiting to happen.  The above is the only sane way to do it, and it
Avery> isn't very hard to implement.  (Instead of system() in the subprocess,
Avery> you can use exec().)

quotemeta() is about regex quoting.  This is not precisely the same as shell
quoting, and is both misleading, and potentially broken.

-- 
Randal L. Schwartz - Stonehenge Consulting Services, Inc. - +1 503 777 0095
<merlyn@stonehenge.com> <URL:http://www.stonehenge.com/merlyn/>
Smalltalk/Perl/Unix consulting, Technical writing, Comedy, etc. etc.
See http://methodsandmessages.vox.com/ for Smalltalk and Seaside discussion

Re: [PATCH] change Perl syntax to support Perl 5.6

[Junio C Hamano]

merlyn@stonehenge.com (Randal L. Schwartz) writes:

>>>>>> "Avery" == Avery Pennarun <apenwarr@gmail.com> writes:
>
> Avery> Shell quoting is a disaster (including security holes, where relevant)
> Avery> waiting to happen.  The above is the only sane way to do it, and it
> Avery> isn't very hard to implement.  (Instead of system() in the subprocess,
> Avery> you can use exec().)
>
> quotemeta() is about regex quoting.  This is not precisely the same as shell
> quoting, and is both misleading, and potentially broken.

Agreed to, and grateful for, both of your comments.

Do you like the one Jakub quoted from how gitweb does it?  It looks like
this:

    # quote the given arguments for passing them to the shell
    # quote_command("command", "arg 1", "arg with ' and ! characters")
    # => "'command' 'arg 1' 'arg with '\'' and '\!' characters'"
    # Try to avoid using this function wherever possible.
    sub quote_command {
           return join(' ',
                       map( { my $a = $_; $a =~ s/(['!])/'\\$1'/g; "'$a'" } @_ ));
    }

Re: [PATCH] change Perl syntax to support Perl 5.6

[Avery Pennarun]

On Sun, Aug 31, 2008 at 12:27 PM, Junio C Hamano <gitster@pobox.com> wrote:
> merlyn@stonehenge.com (Randal L. Schwartz) writes:
>
>>>>>>> "Avery" == Avery Pennarun <apenwarr@gmail.com> writes:
>>
>> Avery> Shell quoting is a disaster (including security holes, where relevant)
>> Avery> waiting to happen.  The above is the only sane way to do it, and it
>> Avery> isn't very hard to implement.  (Instead of system() in the subprocess,
>> Avery> you can use exec().)
>>
>> quotemeta() is about regex quoting.  This is not precisely the same as shell
>> quoting, and is both misleading, and potentially broken.
>
> Agreed to, and grateful for, both of your comments.
>
> Do you like the one Jakub quoted from how gitweb does it?  It looks like
> this:
>
>    # quote the given arguments for passing them to the shell
>    # quote_command("command", "arg 1", "arg with ' and ! characters")
>    # => "'command' 'arg 1' 'arg with '\'' and '\!' characters'"
>    # Try to avoid using this function wherever possible.
>    sub quote_command {
>           return join(' ',
>                       map( { my $a = $_; $a =~ s/(['!])/'\\$1'/g; "'$a'" } @_ ));
>    }

No, that's just another feeble attempt at quoting, which may or may
not be correct.  I'm not smart enough to tell.

I have a proper implementation in the 'runlock' script in gitbuilder:

    http://github.com/apenwarr/gitbuilder/tree/master/runlock

In that particular case, I wanted to handle signals carefully, so I
needed the manual fork thing even in perl 5.8.  You can safely remove
the signal handling stuff (and of course the lockfile stuff) if you
just want a minimal safe fork-exec-wait implementation in perl.

Have fun,

Avery

Re: [PATCH] change Perl syntax to support Perl 5.6

[Jakub Narebski]

On Sub, 31 August 2008, Avery Pennarun wrote:
> On Sun, Aug 31, 2008 at 12:27 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> merlyn@stonehenge.com (Randal L. Schwartz) writes:
>>
>>>>>>>> "Avery" == Avery Pennarun <apenwarr@gmail.com> writes:
>>>
>>> Avery> Shell quoting is a disaster (including security holes, where relevant)
>>> Avery> waiting to happen.  The above is the only sane way to do it, and it
>>> Avery> isn't very hard to implement.  (Instead of system() in the subprocess,
>>> Avery> you can use exec().)
>>>
>>> quotemeta() is about regex quoting.  This is not precisely the same as shell
>>> quoting, and is both misleading, and potentially broken.
>>
>> Agreed to, and grateful for, both of your comments.
>>
>> Do you like the one Jakub quoted from how gitweb does it?  It looks like
>> this:
>>
>>    # quote the given arguments for passing them to the shell
>>    # quote_command("command", "arg 1", "arg with ' and ! characters")
>>    # => "'command' 'arg 1' 'arg with '\'' and '\!' characters'"
>>    # Try to avoid using this function wherever possible.
>>    sub quote_command {
>>           return join(' ',
>>                       map( { my $a = $_; $a =~ s/(['!])/'\\$1'/g; "'$a'" } @_ ));
>>    }
> 
> No, that's just another feeble attempt at quoting, which may or may
> not be correct.  I'm not smart enough to tell.

First, according to POSIX, for POSIX-compatibile shells we should have:

 2. Shell Command Language
 2.2 Quoting
 2.2.2 Single-Quotes

 Enclosing characters in single-quotes ( '' ) shall preserve the literal
 value of each character within the single-quotes. A single-quote cannot
 occur within single-quotes.

http://www.opengroup.org/onlinepubs/009695399/utilities/xcu_chap02.html#tag_02_02_02

So that is why single quote "'" must be escaped as "I'am" -> 'I'\''am'
(' -> '\'', i.e. close quote, escaped ' = \', (re-)open quote).


Second, as Lea Wrote in commit message for 516381d5:

    gitweb: quote commands properly when calling the shell
    
    This eliminates the function git_cmd_str, which was used for composing
    command lines, and adds a quote_command function, which quotes all of
    its arguments (as in quote.c).

We have to go to quote.c to get to know why "!" is a special case too,
in addition to "'".  The commit message for 77d604c3 (by H. Peter Anvin,
which is CC-ed) states:

    Create function to sq_quote into a buffer
    Handle !'s for csh-based shells

> I have a proper implementation in the 'runlock' script in gitbuilder:
> 
>     http://github.com/apenwarr/gitbuilder/tree/master/runlock
> 
> In that particular case, I wanted to handle signals carefully, so I
> needed the manual fork thing even in perl 5.8.  You can safely remove
> the signal handling stuff (and of course the lockfile stuff) if you
> just want a minimal safe fork-exec-wait implementation in perl.

But if we go this way, i.e. fork+exec (perhaps implicit fork), why do
not simply use appropriate commands from Git.pm (Git::Repo doesn't
have it yet, IIRC).  As far as I remember Git.pm was created initially
to unify all different "safe_pipe" and "safe_cmd" implementations among
different Perl scripts in Git (Petr "Pasky" Baudis CC-ed).

-- 
Jakub Narebski
Poland

Re: [PATCH] change Perl syntax to support Perl 5.6

[Petr Baudis]

On Sun, Aug 31, 2008 at 10:23:36PM +0200, Jakub Narebski wrote:
> On Sub, 31 August 2008, Avery Pennarun wrote:
> > I have a proper implementation in the 'runlock' script in gitbuilder:
> > 
> >     http://github.com/apenwarr/gitbuilder/tree/master/runlock
> > 
> > In that particular case, I wanted to handle signals carefully, so I
> > needed the manual fork thing even in perl 5.8.  You can safely remove
> > the signal handling stuff (and of course the lockfile stuff) if you
> > just want a minimal safe fork-exec-wait implementation in perl.
> 
> But if we go this way, i.e. fork+exec (perhaps implicit fork), why do
> not simply use appropriate commands from Git.pm (Git::Repo doesn't
> have it yet, IIRC).  As far as I remember Git.pm was created initially
> to unify all different "safe_pipe" and "safe_cmd" implementations among
> different Perl scripts in Git (Petr "Pasky" Baudis CC-ed).

Can anyone give a concrete justification for Perl 5.6 support? Who is
needing it and why do they have to use Perl 5.6? Does it offset the time
spent discussing and reviewing this and the maintenance burden of extra
complicated code?

-- 
				Petr "Pasky" Baudis
The next generation of interesting software will be done
on the Macintosh, not the IBM PC.  -- Bill Gates

Re: [PATCH] change Perl syntax to support Perl 5.6

[H. Peter Anvin]

Petr Baudis wrote:
> 
> Can anyone give a concrete justification for Perl 5.6 support? Who is
> needing it and why do they have to use Perl 5.6? Does it offset the time
> spent discussing and reviewing this and the maintenance burden of extra
> complicated code?
> 

I believe RHEL4 is Perl 5.6.1, but I could be wrong.

	-hpa

Re: [PATCH] change Perl syntax to support Perl 5.6

[Robert Schiele]

[-- Attachment #1: Type: text/plain, Size: 332 bytes --]

On Sun, Aug 31, 2008 at 08:57:51PM -0700, H. Peter Anvin wrote:
> I believe RHEL4 is Perl 5.6.1, but I could be wrong.

No, that's 5.8.5.  RHEL3 is 5.8.0.  RHEL2 is what you are talking about.

Robert

-- 
Robert Schiele
Dipl.-Wirtsch.informatiker	mailto:rschiele@gmail.com

"Quidquid latine dictum sit, altum sonatur."

[-- Attachment #2: Type: application/pgp-signature, Size: 194 bytes --]

Re: [PATCH] change Perl syntax to support Perl 5.6

[Tom G. Christensen]

Robert Schiele wrote:
> On Sun, Aug 31, 2008 at 08:57:51PM -0700, H. Peter Anvin wrote:
>> I believe RHEL4 is Perl 5.6.1, but I could be wrong.
> 
> No, that's 5.8.5.  RHEL3 is 5.8.0.  RHEL2 is what you are talking about.
> 
Indeed. RHEL 2.1 will be supported by Redhat until May 31, 2009 so has
about 9 months of support life left.
It is probably also the last still supported Linux dist with perl 5.6.x.

On RHEL 2.1 git 1.6.0.1 currently builds fine and it passes the
testsuite except for the tests using --interactive.

-tgc

Re: [PATCH] change Perl syntax to support Perl 5.6

[Brandon Casey]

Tom G. Christensen wrote:

> On RHEL 2.1 git 1.6.0.1 currently builds fine and it passes the
> testsuite except for the tests using --interactive.

Even t9700-perl-git.sh? Or perhaps it is skipped since Test::More
is missing?

t9700-perl-git.sh doesn't run successfully for me on perl 5.8.0 without
changes since test.pl uses

    'File::Temp->new()'

object form, and

    'open our $tmpstderr, ">&", STDERR'

bare STDERR as 3rd argument which this perl version complains about.

-brandon

Re: [PATCH] change Perl syntax to support Perl 5.6

[Tom G. Christensen]

Brandon Casey wrote:
> Tom G. Christensen wrote:
> 
>> On RHEL 2.1 git 1.6.0.1 currently builds fine and it passes the
>> testsuite except for the tests using --interactive.
> 
> Even t9700-perl-git.sh? Or perhaps it is skipped since Test::More
> is missing?
> 
Exactly.

> t9700-perl-git.sh doesn't run successfully for me on perl 5.8.0 without
> changes since test.pl uses
> 
>     'File::Temp->new()'
> 
> object form, and
> 
>     'open our $tmpstderr, ">&", STDERR'
> 
> bare STDERR as 3rd argument which this perl version complains about.
> 
I saw your patch for this.
I haven't got this far with 5.8.0 since t9100-git-svn-basic.sh kills the 
build because of File::Temp->new() in Git.pm.

-tgc

Re: [PATCH] change Perl syntax to support Perl 5.6

[Jakub Narebski]

Jakub Narebski wrote:

> We have to go to quote.c to get to know why "!" is a special case too,
> in addition to "'".  The commit message for 77d604c3 (by H. Peter Anvin,
> which is CC-ed) states:
> 
>     Create function to sq_quote into a buffer
>     Handle !'s for csh-based shells

Although according to POSIX 

  Enclosing characters in single-quotes ( '' ) shall preserve the literal
  value of each character within the single-quotes. A single-quote cannot
  occur within single-quotes.

so "!" should be not treated as special character in POSIX shell,
csh unfortunately does not follow this.  Try

  $ echo '!!'

in csh, tcsh (now that's funny) and bash (or dash).

-- 
Jakub Narebski
Poland

Re: [PATCH] change Perl syntax to support Perl 5.6

[Jay Soffian]

On Sun, Aug 31, 2008 at 1:35 AM, Avery Pennarun <apenwarr@gmail.com> wrote:
> On Sat, Aug 30, 2008 at 4:37 PM, Jakub Narebski <jnareb@gmail.com> wrote:
>> Or you can use "open $fd, '-|'" to fork, an "manually" exec/system.
>
> Shell quoting is a disaster (including security holes, where relevant)
> waiting to happen.  The above is the only sane way to do it, and it
> isn't very hard to implement.  (Instead of system() in the subprocess,
> you can use exec().)

The perlipc man page has example routines. Search for "safe backtick"
and/or "safe pipe".

j.

Re: [PATCH] change Perl syntax to support Perl 5.6

[Alex Riesen]

Avery Pennarun, Sun, Aug 31, 2008 07:35:31 +0200:
> On Sat, Aug 30, 2008 at 4:37 PM, Jakub Narebski <jnareb@gmail.com> wrote:
> > Or you can use "open $fd, '-|'" to fork, an "manually" exec/system.
> 
> Shell quoting is a disaster (including security holes, where relevant)
> waiting to happen.  The above is the only sane way to do it, and it
> isn't very hard to implement. ...

except on Windows, where it is impossible to implement.

Re: [PATCH] change Perl syntax to support Perl 5.6

[Avery Pennarun]

On Mon, Sep 1, 2008 at 5:42 PM, Alex Riesen <raa.lkml@gmail.com> wrote:
> Avery Pennarun, Sun, Aug 31, 2008 07:35:31 +0200:
>> On Sat, Aug 30, 2008 at 4:37 PM, Jakub Narebski <jnareb@gmail.com> wrote:
>> > Or you can use "open $fd, '-|'" to fork, an "manually" exec/system.
>>
>> Shell quoting is a disaster (including security holes, where relevant)
>> waiting to happen.  The above is the only sane way to do it, and it
>> isn't very hard to implement. ...
>
> except on Windows, where it is impossible to implement.

True.  Although every program parses its own options on Windows, so
proper, safe quoting is *also* impossible to implement.

Avery

Re: [PATCH] change Perl syntax to support Perl 5.6

[Avery Pennarun]

On Mon, Sep 1, 2008 at 8:23 PM, Avery Pennarun <apenwarr@gmail.com> wrote:
> On Mon, Sep 1, 2008 at 5:42 PM, Alex Riesen <raa.lkml@gmail.com> wrote:
>> Avery Pennarun, Sun, Aug 31, 2008 07:35:31 +0200:
>>> On Sat, Aug 30, 2008 at 4:37 PM, Jakub Narebski <jnareb@gmail.com> wrote:
>>> > Or you can use "open $fd, '-|'" to fork, an "manually" exec/system.
>>>
>>> Shell quoting is a disaster (including security holes, where relevant)
>>> waiting to happen.  The above is the only sane way to do it, and it
>>> isn't very hard to implement. ...
>>
>> except on Windows, where it is impossible to implement.
>
> True.  Although every program parses its own options on Windows, so
> proper, safe quoting is *also* impossible to implement.

Hmm, furthermore, perl seems to implement the "-|" operation just fine
on Windows.  I'm not really sure what it does, but it works.  Try the
attached perl script with the following command sequence.  (It works
on cygwin bash, but that might be magic; try it from cmd.exe instead
if you really want to reassure yourself.)

    echo >"foo blah"
    dir foo blah
         # above gives an error
    dir "foo blah"
         # above works
    c:\perl\bin\perl test.pl cmd /c dir "foo blah"
         # above works

Tested with ActiveState perl 5.6.1 (fails as "-|" is apparently not
supported *at all*, but maybe that's only on Windows), Cygwin perl
5.8.8 (works fine), and msysgit's perl 5.8.8 (works fine).

Now, as you can see above, the only copy of perl 5.6.x that I have
*didn't* work with this test, but it's on Windows, where I suspect
that version is just broken.  It says:

   '-' is not recognized as an internal or external command, operable
program or batch file.

You might then suspect that perhaps perl 5.6.1 didn't support the
open($fh, "-|") syntax at all, but 'perldoc perlipc' even on my
ActiveState perl 5.6.1 documents the feature.  Thus, I think it's
*just* the (obsolete) ActiveState version on Windows that has a buggy
implementation.  I would appreciate if someone with perl 5.6 on Linux
could try the program below with the commands above and see if it
works.

As another side note, the ActiveState perl *does* work if you call
fork() instead of open($fh, "-|"), but of course that doesn't redirect
stdin/stdout of the called process.  So perl on Windows *does*
correctly fake the fork/exec part.

test.pl follows.

Have fun,

Avery

P.S. Congratulations to the msysgit people for providing the only
version of msys perl that I could figure out how to install.


#!/usr/bin/perl -w
use strict;

if (@ARGV < 1) {
    print STDERR "Usage: $0 <command line...>\n";
    exit 127;
}

print "Arguments:\n{", join("}\n{", @ARGV), "}\n\n";

my $pid = open my $fh, "-|";
if ($pid) {
    # parent
    while (<$fh>) {
        s/\r?\n$//;
        chomp;
        print "[$_]\n";
    }
    my $newpid = waitpid($pid, 0);
    if ($newpid != $pid) {
        die("waitpid returned '$newpid', expected '$pid'\n");
    }	
    my $ret = $?;
    exit $? >> 8;
} else {
    # child
    exec(@ARGV);
}

# NOTREACHED

Re: [PATCH] change Perl syntax to support Perl 5.6

[Junio C Hamano]

Robert Schiele <rschiele@gmail.com> writes:

> On Sat, Aug 30, 2008 at 11:06:06AM -0700, Junio C Hamano wrote:
>> I didn't try either but I think you are right.  And I agree we should say
>> we rely on 5.6 or newer.
>
> If we don't change it we need to rely on 5.8 or newer as my initial patch
> suggested.

I think it should be as simple as Jeff's patch, and I think it is
moderately preferable to make sure the core part runs with earlier version
than punting.

I however do not have 5.6 (or 5.6.1) installation handy, so I cannot test
to make sure that Jeff's patch is the only necessary fix to the "add -i".

Re: [PATCH] change Perl syntax to support Perl 5.6

[Randal L. Schwartz]

>>>>> "Robert" == Robert Schiele <rschiele@gmail.com> writes:
Robert> -		open($fh, '-|', @_) or die;
Robert> +		open($fh, '-|', join(' ', @_)) or die;

No.  This is dangerous.

If you want 5.6 equivalent code, you'll have to use something
that deals with the explicit fork() of open($fh, '-|').

-- 
Randal L. Schwartz - Stonehenge Consulting Services, Inc. - +1 503 777 0095
<merlyn@stonehenge.com> <URL:http://www.stonehenge.com/merlyn/>
Smalltalk/Perl/Unix consulting, Technical writing, Comedy, etc. etc.
See http://methodsandmessages.vox.com/ for Smalltalk and Seaside discussion

Re: [PATCH] change Perl syntax to support Perl 5.6

[Ask Bjørn Hansen]

On Aug 30, 2008, at 10:39, Robert Schiele wrote:

> git-add--interactive has one Perl command that was not yet present in
> Perl 5.6.  Changing this single command makes it compatible again.

Perl 5.8.0 was released six years ago.  Perl 5.6.0 was released in  
*2000*.  I think we can safely tell people to get with the program and  
use Perl 5.8.

> This is an alternative to my previous patch that just declared Perl  
> 5.8 to
> be the required version.


+1 to that one.


  - ask

-- 
http://develooper.com/ - http://askask.com/

Re: [PATCH] change Perl syntax to support Perl 5.6

[Junio C Hamano]

Ask Bjørn Hansen <ask@develooper.com> writes:

> On Aug 30, 2008, at 10:39, Robert Schiele wrote:
>
>> git-add--interactive has one Perl command that was not yet present in
>> Perl 5.6.  Changing this single command makes it compatible again.
>
> Perl 5.8.0 was released six years ago.  Perl 5.6.0 was released in
> *2000*.  I think we can safely tell people to get with the program and
> use Perl 5.8.
>
>> This is an alternative to my previous patch that just declared Perl
>> 5.8 to
>> be the required version.
>
> +1 to that one.

Now, would somebody volunteer to go everywhere our user base (who no
longer read Release Notes) hang around, post a message saying that some
people on git development list are proposing to drop Perl 5.6 support, and
if the proposal goes ahead, it is possible that the next release may force
upgrading Perl for them, to make sure they won't complain saying they've
never heard about the "incompatible change" beforehand?

Re: [PATCH] change Perl syntax to support Perl 5.6

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Ask Bjørn Hansen <ask@develooper.com> writes:
> ...
>>> This is an alternative to my previous patch that just declared Perl
>>> 5.8 to
>>> be the required version.
>>
>> +1 to that one.
>
> Now, would somebody volunteer to go everywhere our user base (who no
> longer read Release Notes) hang around, post a message saying that some
> people on git development list are proposing to drop Perl 5.6 support, and
> if the proposal goes ahead, it is possible that the next release may force
> upgrading Perl for them, to make sure they won't complain saying they've
> never heard about the "incompatible change" beforehand?

Just in case before anybody overreacts without reading the mailing list
backlog; the above is meant to be a moderately bitter joke ;-)

I tend to think that everybody who needs to run git in their development
environment (the deployment site could be running with ancient Perl for
all we care --- after all, git or any SCM is primarily to be run in the
development environment, and even if people use git to transfer the end
result from the development side, all they need is the really core part of
history tranfer tools like fetch, push and checkout on the deployed site;
these tools do not depend on Perl) would be running 5.8 by now.  Granted,
some development machine need to have the exact same version of everything
as the intended deployment environment for testing purposes, and some
people may be developing a piece of software that is meant to run on Perl
that is no more recent than 5.6, but with virtual machines and all, I
think it would be rare to have such a setup --- it is a lot easier to
develop on a platform with more reasonably recent Perl that your
development tools (not limited to git itself) may depend on, and test on a
virtual bochs with an ancient software configuration as the intended
deployment site.

So I personally think it is probably Ok to declare that we do depend on
5.8.