mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Jeff King <>
To: Michael McClimon <>
Cc: "Carlo Marcelo Arenas Belón" <>,
	"Glen Choo" <>,
	"Johannes Schindelin" <>,
Subject: Re: [PATCH 1/1] add semicolon after catch statement
Date: Mon, 17 Oct 2022 13:34:22 -0400	[thread overview]
Message-ID: <> (raw)
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:

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

> 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/ b/t/
index 4aa5d90d32..53a838a8e8 100755
--- a/t/
+++ b/t/
@@ -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/ to test' '
 	"$PERL_PATH" "$TEST_DIRECTORY"/t9700/ 2>stderr &&
 	test_must_be_empty stderr
diff --git a/t/t9700/ b/t/t9700/
index e046f7db76..917b09cdf9 100755
--- a/t/t9700/
+++ b/t/t9700/
@@ -30,6 +30,12 @@ sub adjust_dirsep {
 # set up
 our $abs_repo_dir = cwd();
 ok(our $r = Git->repository(Directory => "."), "open repository");
+	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 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 +cc folks who worked on that.


  reply	other threads:[~2022-10-17 17:34 UTC|newest]

Thread overview: 23+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-10-16 21:22 [PATCH 0/1] add semicolon after catch statement Michael McClimon
2022-10-16 21:22 ` [PATCH 1/1] " Michael McClimon
2022-10-16 23:18   ` Jeff King
2022-10-17  2:17     ` Michael McClimon
2022-10-17 17:34       ` Jeff King [this message]
2022-10-18  1:39         ` Michael McClimon
2022-11-10 15:10         ` Johannes Schindelin
2022-11-10 21:41           ` Jeff King
2022-10-22  1:19 ` [PATCH v2 0/2] Fix behavior of in unsafe bare repositories Michael McClimon
2022-10-22  1:19   ` [PATCH v2 1/2] add semicolon after catch statement Michael McClimon
2022-10-22  1:19   ` [PATCH v2 2/2] setup: allow to do unsafe repo checking Michael McClimon
2022-10-22  5:29     ` Junio C Hamano
2022-10-22 21:18       ` Jeff King
2022-10-22 23:17         ` Junio C Hamano
2022-10-22 19:45     ` Ævar Arnfjörð Bjarmason
2022-10-22 20:55       ` Jeff King
2022-10-24 10:57         ` Ævar Arnfjörð Bjarmason
2022-10-24 23:38           ` Jeff King
2022-10-22 21:16     ` Jeff King
2022-10-22 22:08       ` Jeff King
2022-10-22 23:19         ` Michael McClimon
2022-10-24 23:33           ` Jeff King
2022-10-22 23:14       ` Junio C Hamano

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:

  List information:

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \ \ \ \ \ \ \ \

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).