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

From: Michael McClimon <michael@mcclimon.org>
Date: Oct 17, 2022, 02:17 UTC
Message-ID: <Y0y7UdLf3qd7RgVQ@newk>
In-Reply-To: <Y0yRStZ6gM+H8/Bf@coredump.intra.peff.net>

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

Show 20 quoted lines

> 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