patchchange Perl syntax to support Perl 5.6

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(-)

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

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".

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".

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

Jeff King <peff@peff.net> writes:

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)?

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

I did just try, and it is indeed broken.

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

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

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

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

I think it is as simple as:

On Sat, 30 August 2008, Jeff King wrote:

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

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.

On Sat, Aug 30, 2008 at 10:37:15PM +0200, Jakub Narebski wrote:

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

On Sat, Aug 30, 2008 at 4:37 PM, Jakub Narebski <jnareb@gmail.com> wrote:

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

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.

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

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'" } @_ ));
    }

On Sun, Aug 31, 2008 at 12:27 PM, Junio C Hamano <gitster@pobox.com> wrote:

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

On Sub, 31 August 2008, Avery Pennarun wrote:

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

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).

On Sun, Aug 31, 2008 at 10:23:36PM +0200, Jakub Narebski 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?

Petr Baudis wrote:

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

	-hpa

On Sun, Aug 31, 2008 at 08:57:51PM -0700, H. Peter Anvin wrote:

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

Robert

Robert Schiele wrote:

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

Tom G. Christensen wrote:

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

Brandon Casey wrote:

Exactly.

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

Jakub Narebski wrote:

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).

On Sun, Aug 31, 2008 at 1:35 AM, Avery Pennarun <apenwarr@gmail.com> wrote:

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

j.

Avery Pennarun, Sun, Aug 31, 2008 07:35:31 +0200:

except on Windows, where it is impossible to implement.

On Mon, Sep 1, 2008 at 5:42 PM, Alex Riesen <raa.lkml@gmail.com> wrote:

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

Avery

On Mon, Sep 1, 2008 at 8:23 PM, Avery Pennarun <apenwarr@gmail.com> wrote:

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

Robert Schiele <rschiele@gmail.com> writes:

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".

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, '-|').

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

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.

+1 to that one.

  - ask

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

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?

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

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.