BooK

For the last five years, the version comparison code in Git::Repository started with this:

my ( $r, $v, @o ) = ( shift, ( grep !ref, @_ )[0], grep ref, @_ );

The above line looks clever (that's probably why I wrote it that way in the first place), but it also has a subtle bug.

The variables are:

• $r: the Git::Repository object (or class name)
• $v: the version to compare to
• @o: whatever additional options passed to the function call, meant to be passed on to the call to $r->version

This would allow one to write things like the following and test the version number of a very specific Git binary:

Git::Repository->version_ge( '1.6.5', { git => '/opt/git-collection/mygit/bin/git' } );

The options are expected in a hash ref, and it's allowed to pass more than one option hash. The order of arguments is irrelevant: Git::Repository expects to receive at least one string, and will pick the first one and ignore the rest. Whatever references are left are assumed to be option hashes.

Now, what if the version number we pass is an object with overloaded stringification? Well, it won't pass the !ref check, and $v will be... the first reference picked up by the grep ref that follows!

Note: $v is not undef in the above, because ( grep !ref, @_ )[0] is the first element of an empty list (not an empty array); and taken in list context, that ends up being an empty list. The following one-liner makes that clear:

$ perl -MData::Dumper -le '$Data::Dumper::Indent=0;print Dumper( [ (@a)[0], "<- nil", $a[0], "<- undef" ] )'
$VAR1 = ['<- nil',undef,'<- undef'];

With the advent of Git::Version, I began testing Git::Repository with version objects, and got weird errors because of the above bug:

HASH(0x174dfa8) does not look like a Git version at ...

My first fix was to be more explicit about what's expected:

my $r   = shift;
my ($v) = grep !ref || ref ne 'HASH', @_;
my @o   = grep ref eq 'HASH', @_;

Then I realized it would give unnecessary warnings (or at least, warnings at the wrong stage) if any parameter was undef.

I also realized that what I wanted to be explicit about was the definition of the options (zero or more simple hash references) and not the version itself, which could be a string or anything that ends up looking close enough to a Git version version number.

The fixed version looks like this, and each element of @_ is only checked once (to assess if it's an option hash or not):

my $r = shift;
my @o;
my ($v) = grep !( ref && ref eq 'HASH' ? push @o, $_ : 0 ), @_;

(Also note the use of De Morgan's laws to turn an || into an &&.)

The fixed version is in Git-Repository v1.318, which is on its way to CPAN (and my seventh Git-related CPAN release in a row!).