Re: [PATCH 1/1] Git.pm: add semicolon after catch statement

[Michael McClimon <michael@mcclimon.org>]

> Curiously this works as expected for me, both before and after your
> patch. I wonder if it depends on perl version. Mine is 5.34.

Hm, curious indeed! It reliably fails without my patch and passes with it on
all the versions I had lying around (5.8, 5.18, 5.24, 5.26, 5.28, 5.30, 5.34,
and 5.36).

> I've never used Error.pm's try/catch before, so I don't know what's
> normal. Regular if/unless doesn't need it, but certainly an earlier
> catch uses a semicolon. So it seems like a reasonable fix.

Ha, well Perl is...let's say special. try/catch is not a language construct
(until 5.34, where it is experimental), and so it's always implemented by
subroutines. One upshot of this is that try/catch needs a semicolon, because
it's sugar for try(sub { ... }), and statements need semicolons separating
them. 

Compare these two examples: -MO=Deparse,-p tells perl to deparse the program
and print it with parentheses:

    $ perl -MError -MO=Deparse,-p -e 'try { die "bad" } catch Pkg with { warn "caught" }; print "after"'
    do {
        die('bad')
    }->try('Pkg'->catch(do {
        warn('caught')
    }->with));
    print('after');
    -e syntax OK

    $ perl -MError -MO=Deparse,-p -e 'try { die "bad" } catch Pkg with { warn "caught" } print "after"'
    do {
        die('bad')
    }->try('Pkg'->catch(do {
        warn('caught')
    }->with(print('after'))));
    -e syntax OK

The first here is the good case case (with semicolon), and you can see that
print('after') is its own statement. The bad case, with no semicolon, passes
it as an argument to with(), which is what's causing the error here: something
called by with() is trying to use it as a hash reference, and it's a string.

> I'd assume t9700 passes for you, since I don't think we cover this case.
> Maybe it's worth squashing this in:
> 
> diff --git a/t/t9700/test.pl b/t/t9700/test.pl
> index e046f7db76..5bd3687f37 100755
> --- a/t/t9700/test.pl
> +++ b/t/t9700/test.pl
> @@ -30,6 +30,12 @@ sub adjust_dirsep {
>  # set up
>  our $abs_repo_dir = cwd();
>  ok(our $r = Git->repository(Directory => "."), "open repository");
> +{
> +	local $ENV{GIT_TEST_ASSUME_DIFFERENT_OWNER} = 1;
> +	my $failed = eval { Git->repository(Directory => ".") };
> +	ok(!$failed, "reject unsafe repository");
> +	like($@, qr/not a git repository/i, "unsafe error message");
> +}
>  
>  # config
>  is($r->config("test.string"), "value", "config scalar: string");

Curiously, t9700 passes for me with this suggestion both with and without my
patch. You'd only see this bug in bare repositories, though, and the one set
up in t9700 is not bare. I can see about trying to make it do so, but I'll
need to do a bit more reading of how even the tests are set up and run first.
Thanks!

-- 
Michael McClimon
michael@mcclimon.org