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

From: Jeff King <peff@peff.net>
Date: Oct 17, 2022, 17:34 UTC
Message-ID: <Y02SHlW1rNQdfCHI@coredump.intra.peff.net>
In-Reply-To: <Y0y7UdLf3qd7RgVQ@newk>

[+cc people who worked on safe-directory stuff; please check out the included test and final comments]

On Sun, Oct 16, 2022 at 10:17:53PM -0400, Michael McClimon wrote:

Show 6 quoted lines

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

Doh, sorry to mislead you; I hadn't noticed this was in the bare repository code path until you pointed it out. I get the same outcome as you and the OP once that is fixed (both in t9700 and in my manual testing).

Show 5 quoted lines

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

Right, I imagined it was something like that. Your fix is definitely the right thing, then.

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

Yeah, this test is particularly confusing because unlike most of our suite, it drives the test harness using a separate perl script. So you have setup in one file and tests in another.

You'd want something like:

diff --git a/t/t9700-perl-git.sh b/t/t9700-perl-git.sh
index 4aa5d90d32..53a838a8e8 100755
--- a/t/t9700-perl-git.sh
+++ b/t/t9700-perl-git.sh
@@ -45,6 +45,10 @@ test_expect_success \
      git config --add test.pathmulti bar
      '
 
+test_expect_success 'set up bare repository' '
+        git init --bare bare.git
+'
+
 test_expect_success 'use t9700/test.pl to test Git.pm' '
 	"$PERL_PATH" "$TEST_DIRECTORY"/t9700/test.pl 2>stderr &&
 	test_must_be_empty stderr
diff --git a/t/t9700/test.pl b/t/t9700/test.pl
index e046f7db76..917b09cdf9 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 => "$abs_repo_dir/bare.git") };
+	ok(!$failed, "reject unsafe bare repository");
+	like($@, qr/not a git repository/i, "unsafe error message");
+}
 
 # config
 is($r->config("test.string"), "value", "config scalar: string");

But curiously this still does not pass after your patch, because we seem
to actually open the repository! I think this is because the C code
allows an explicit GIT_DIR to override the safe-directory checks. But in
this case that GIT_DIR is set by Git.pm searching for the directory, not
because the user desires it.

(I know that I used a "Directory" in the example above, but that is only
to avoid extra chdir-ing around in the test script; calling a bare
Git->repository() triggers the same behavior in the right directory).

So your patch is definitely still the right thing to do, but it feels
like a hole in the safe-directory mechanism, at least when called via
Git.pm. +cc folks who worked on that.

-Peff