Let's bump the minimum Perl version to 5.8

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Fri, Sep 24, 2010 at 11:22, Tor Arntsen <tor@spacetec.no> wrote:
> On Fri, Sep 24, 2010 at 13:05, Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
>> On Fri, Sep 24, 2010 at 10:27, Tor Arntsen <tor@spacetec.no> wrote:
> [..]
>>> I've found that for add -p you'll need 5.8.x or newer, due to stuff like
>>>
>>>                my $fh = undef;
>>>                open($fh, '-|', @_) or die;
>>>
>>> which fails in e.g. perl 5.6.
>>> There could be some other stuff (in addition to add -p) that also does
>>> this kind of thing.
>>
>> If that's the case (I don't have a 5.6 here to do archeology on) then
>> git add -p never worked in 5.6. That was added in 5cde71d6 when it was
>> introduced in 2006:
>>
>>    +sub run_cmd_pipe {
>>    +       my $fh = undef;
>>    +       open($fh, '-|', @_) or die;
>>    +       return <$fh>;
>>    +}
>>
>> Can you show us the specific error you're getting, and the output of
>> your `perl -V` ?
>
> I don't have that particular installation anymore (I installed perl
> 5.8 on the machine I had trouble with), and the only other system I
> have left with perl 5.6 only has an old Git 1.5 version. But it's easy
> enough to reproduce, Perl 5.6 simply doesn't support that notation.
> Put the code above in a perl script and execute it:
>
> Can't use an undefined value as filehandle reference at test-pl.pl line 5.
> (that's the 'open' line)
>
> I can provide the output of -V if you wish, but I don't think it
> matters really, except for the version:

It matters because you keep saying "5.6", which is ambiguous since the
syntax in question was introduced in 5.6.1. The error you may be
having on 5.6.0 is very different from 5.6.1, and 5.6.2 has additional
bugfixes:

    http://search.cpan.org/~jesse/perl-5.12.2/pod/perl561delta.pod#open()_with_more_than_two_arguments

Anyway, I compiled maint-5.6 from perl.git and confirmed this:

    $ /home/avar/perl5/installed/bin/perl5.6.2 -Mdiagnostics -le
'print $]; sub run_cmd_pipe { my $fh = undef; open($fh, "-|", @_) or
die;  return <$fh>; } print run_cmd_pipe(qw(git --help));'
    5.006002
    Can't use an undefined value as filehandle reference at -e line 1 (#1)
        (F) A value used as either a hard reference or a symbolic reference must
        be a defined value.  This helps to delurk some insidious errors.

    Uncaught exception from user code:
            Can't use an undefined value as filehandle reference at -e line 1.
            main::run_cmd_pipe('git', '--help') called at -e line 1

As compared to a more recent perl:

    $ perl -Mdiagnostics -le 'print $]; sub run_cmd_pipe { my $fh =
undef; open($fh, "-|", @_) or die;  return <$fh>; } print
run_cmd_pipe(qw(git --help));'
    5.013004
    usage: git [--version] [--exec-path[=GIT_EXEC_PATH]] [--html-path]
    [...]

And to reply to Alex Riesen I can't get this to work with `my $fh`
(which would be undef too) or glob open either. Although it should be
possible with other types of pipe open, or even using temporary files.

    $ /home/avar/perl5/installed/bin/perl5.6.2 -Mdiagnostics -le
'print $]; sub run_cmd_pipe { local *TMP; open(TMP, "-|", @_) or die;
return <TMP>; } print run_cmd_pipe(qw(git --help));'
    5.006002
    Can't use an undefined value as filehandle reference at -e line 1 (#1)
        (F) A value used as either a hard reference or a symbolic reference must
        be a defined value.  This helps to delurk some insidious errors.

    Uncaught exception from user code:
            Can't use an undefined value as filehandle reference at -e line 1.
            main::run_cmd_pipe('git', '--help') called at -e line 1

However, I'd like to shift the discussion a bit: Do we want to support
the 5.6 line *at all* anymore? I don't think so. As you point out
yourself you can just compile 5.8 or later on these machines.

I just tried running our test suite with maint-5.6 after compiling with:

    NO_PERL_MAKEMAKER=1
PERL_PATH=/home/avar/perl5/installed/bin/perl5.6.2
prefix=/opt/git/perl-56 all install

NO_PERL_MAKEMAKER=1 is needed because the ExtUtils::MakeMaker that
comes with 5.6.2 doesn't know how to move perl/private-Error.pm to
perl/blib/*/Error.pm. So if you don't provide it you'll get:

    $ /opt/git/perl-56/bin/git add -p
    Can't locate Error.pm

That would also break e.g. git-send-email.

Here are the test results *without* NO_PERL_MAKEMAKER=1, for
reference:

    Test Summary Report
    -------------------
    t2016-checkout-patch.sh                          (Wstat: 256
Tests: 14 Failed: 12)
      Failed tests:  2-13
      Non-zero exit status: 1
    t3701-add-interactive.sh                         (Wstat: 256
Tests: 33 Failed: 17)
      Failed tests:  2, 4-5, 7, 9-10, 13, 16, 18, 21-25, 29
                    31, 33
      Non-zero exit status: 1
    t3904-stash-patch.sh                             (Wstat: 256
Tests: 5 Failed: 2)
      Failed tests:  3-4
      Non-zero exit status: 1
    t7105-reset-patch.sh                             (Wstat: 256
Tests: 8 Failed: 6)
      Failed tests:  2-7
      Non-zero exit status: 1
    t7501-commit.sh                                  (Wstat: 256
Tests: 42 Failed: 1)
      Failed test:  21
      Non-zero exit status: 1
    t7800-difftool.sh                                (Wstat: 256
Tests: 22 Failed: 21)
      Failed tests:  2-22
      Non-zero exit status: 1
    t9700-perl-git.sh                                (Wstat: 256
Tests: 2 Failed: 1)
      Failed test:  2
      Non-zero exit status: 1
      Parse errors: No plan found in TAP output
    t9001-send-email.sh                              (Wstat: 256
Tests: 85 Failed: 59)
      Failed tests:  4-7, 9-10, 12-13, 15, 17-20, 22, 24, 26
                    28-30, 32, 34, 36, 38, 40, 42, 44, 46, 48-55
                    58-77, 80-82, 85
      Non-zero exit status: 1

And with NO_PERL_MAKEMAKER=1:

    Test Summary Report
    -------------------
    t2016-checkout-patch.sh                          (Wstat: 256
Tests: 14 Failed: 12)
      Failed tests:  2-13
      Non-zero exit status: 1
    t3904-stash-patch.sh                             (Wstat: 256
Tests: 5 Failed: 2)
      Failed tests:  3-4
      Non-zero exit status: 1
    t3701-add-interactive.sh                         (Wstat: 256
Tests: 33 Failed: 17)
      Failed tests:  2, 4-5, 7, 9-10, 13, 16, 18, 21-25, 29
                    31, 33
      Non-zero exit status: 1
    t7105-reset-patch.sh                             (Wstat: 256
Tests: 8 Failed: 6)
      Failed tests:  2-7
      Non-zero exit status: 1
    t7501-commit.sh                                  (Wstat: 256
Tests: 42 Failed: 1)
      Failed test:  21
      Non-zero exit status: 1
    t9700-perl-git.sh                                (Wstat: 256
Tests: 14 Failed: 0)
      Non-zero exit status: 1
      Parse errors: No plan found in TAP output

All but the last error are due to this bug in
git-add--interactive.perl. I didn't track down what was wrong with
t9700-perl-git.sh.

git-svn would have failed too if I had SVN libraries:

    $ /opt/git/perl-56/bin/git svn
    Can't locate Digest/MD5.pm in @INC [...]

Getting a working Digest::* (and anything else git-svn needs) for 5.6
at this point is a *lot* harder than just compiling 5.8 (or even
better, 5.12).

Since we're not getting patches for common things that have been
broken on 5.6 for years and bumping the requirenment to an 8 year old
perl (5.8) instead of a 10 year old one (5.6) would make things much
easier, including:

 * Fixing the perl/ Makefile mess

 * Being able to use 5.8 features

 * Being able to honestly support the 5.8 release, 5.6 doesn't even
   compile on modern systems without undocumented monkeypatches, and
   few people use it so we don't get fixes for it.

I'd like to propose dropping 5.6 support, and move to say 5.008. I can
do the work required to add appropriate docs / use statements and
fixes to bugs that we can't fix on 5.6.