{"thread":{"id":"62100","subject":"Can't use bare repositories with Git.pm","startedAt":"2024-09-12T16:32:28Z","lastAt":"2024-09-13T17:46:45Z","messageCount":7,"participants":["Rodrigo","Jeff King","Patrick Steinhardt","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"502711","messageId":"CAGUZU_JZd_+8y19=kGif6u1+4n_+iOcVWV4p-kC0Uo=8Ev=aBA@mail.gmail.com","threadId":"62100","inReplyTo":null,"subject":"Can't use bare repositories with Git.pm","fromName":"Rodrigo","fromEmail":"rodrigolive@gmail.com","sentAt":"2024-09-12T16:32:00Z","receivedAt":"2024-09-12T16:32:28Z","isPatch":false,"sender":{"key":"rodrigolive@gmail.com","avatar":null},"body":"We're having an issue migrating from 2.31.1 to 2.46.0. The following\nPerl code does not work in 2.46.0 as it did in 2.31.1 (tested linux\nand darwin, did not check Windows):\n\n    # test.pl\n    use Git;\n    my $repo = Git->repository( Directory => '/repo/bare.git' );\n    my ($fh, $ctx) = $repo->command_output_pipe('rev-list', \"2acf3456\");\n    print do { local $/; <$fh> };\n\nRun:\n\n   $ cd /home/rodrigo\n   $ perl test.pl\n\nFails with the error:\n\n    fatal: not a git repository: '/home/rodrigo'\n\nIf the repository it points to is a *bare repository* outside of the\ncurrent working directory, it only works if the directory is a child\ndirectory to the bare (/repo/bare.git/info). The code above does work\nfor non-bare repos, and also works if we set `Repository =>\n\"/repo/bare.git\"` instead of `Directory => ...`, but the Git.pm\ndocumentation states `Directory => ...` should work for both bare and\nnon-bare alike, like it did in 2.31.1 (and other versions).\n\nBug hunting through the Git.pm code and skimming through the Git SCM\nrepo, there's a significant change (commit 20da61f25) that makes the\nrecent Git.pm rely on:\n\n     git rev-parse --is-bare-repository --git-dir\n\nfor locating the correct (maybe a parent) repo directory. The change\nwas understandably done for security (and other many) reasons. It\nstarted using --is-bare-repository to detect if it's a bare repository\nwe're dealing with, instead of relying on the old Git.pm redundant\ncode for bare repo detection, which was a sound decision. But some\ncrucial code was taken out.\n\nNow if the directory path we're passing to `Directory => ...` is a\nbare repo this new code will fail because git rev-parse --git-dir will\nreturn a dot `.` (internally Git.pm will chdir() to the directory\nbefore running git rev-parse, hence the result). The issue is that the\ndot is now is being taken as is by Git.pm as the intended directory,\ntricking it to think my cwd /home/rodrigo is the bare repository,\nleading finally to the fatal error.\n\nThis could even become a security issue if the Perl program runs from\nanother working dir which happens to be a sensitive repo. Git.pm\ncommands would take action on the wrong repo. This hypothetical Perl\nprogram, if run as, ie., a server program, could reveal / change\nsensitive information from/on a server.\n\nSOLUTION: I propose using \"--absolute-git-dir\" instead of \"--git-dir\":\n\n    git rev-parse --is-bare-repository --absolute-git-dir\n\nWhich will replace the `.`  rev-parse response with a full path\nresolved by git itself (and not Perl). This means the change to the\nPerl code is minimal. I don't know if this will resolve all possible\ncases, but it does fix our issue.\n\nHere's a diff on my proposal -- sorry, noob in this neck of the woods,\ndidn't know if this is the correct way to contribute, but the change\nis tiny and better if parsed by seasoned eyes anyway:\n\ndiff --git a/perl/Git.pm b/perl/Git.pm\nindex aebfe0c..63d0f92 100644\n--- a/perl/Git.pm\n+++ b/perl/Git.pm\n@@ -187,7 +187,7 @@ sub repository {\n                try {\n                  # Note that \"--is-bare-repository\" must come first, as\n                  # --git-dir output could contain newlines.\n-                 $out = $search->command([qw(rev-parse\n--is-bare-repository --git-dir)],\n+                 $out = $search->command([qw(rev-parse\n--is-bare-repository --absolute-git-dir)],\n                                          STDERR => 0);\n                } catch Git::Error::Command with {\n                        throw Error::Simple(\"fatal: not a git\nrepository: $opts{Directory}\");\n\n\nthanks and best regards,\nRod\ngihub.com/rodrigolive\n"},{"id":"502723","messageId":"20240912223413.GA649897@coredump.intra.peff.net","threadId":"62100","inReplyTo":"CAGUZU_JZd_+8y19=kGif6u1+4n_+iOcVWV4p-kC0Uo=8Ev=aBA@mail.gmail.com","subject":"[PATCH 0/2] fix bare repositories with Git.pm","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-09-12T22:34:13Z","receivedAt":"2024-09-12T22:34:15Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Sep 12, 2024 at 06:32:00PM +0200, Rodrigo wrote:\n\n> We're having an issue migrating from 2.31.1 to 2.46.0. The following\n> Perl code does not work in 2.46.0 as it did in 2.31.1 (tested linux\n> and darwin, did not check Windows):\n> \n>     # test.pl\n>     use Git;\n>     my $repo = Git->repository( Directory => '/repo/bare.git' );\n>     my ($fh, $ctx) = $repo->command_output_pipe('rev-list', \"2acf3456\");\n>     print do { local $/; <$fh> };\n> \n> Run:\n> \n>    $ cd /home/rodrigo\n>    $ perl test.pl\n> \n> Fails with the error:\n> \n>     fatal: not a git repository: '/home/rodrigo'\n\nYikes, good catch. That's a pretty bad bug. I'm surprised we didn't\ncover this in the tests, but it's specific to bare repositories.\n\n> Bug hunting through the Git.pm code and skimming through the Git SCM\n> repo, there's a significant change (commit 20da61f25) that makes the\n> recent Git.pm rely on:\n> \n>      git rev-parse --is-bare-repository --git-dir\n\nYep, I confirmed via bisection that that commit is the culprit. Your\nanalysis is mostly good, though...\n\n> for locating the correct (maybe a parent) repo directory. The change\n> was understandably done for security (and other many) reasons. It\n> started using --is-bare-repository to detect if it's a bare repository\n> we're dealing with, instead of relying on the old Git.pm redundant\n> code for bare repo detection, which was a sound decision. But some\n> crucial code was taken out.\n\n...the problem is actually that not enough code was taken out. ;) I left\nthe code in the bare-only code to turn the relative path to absolute,\nbut it used the wrong path (the one returned by rev-parse, rather than\nthe original Directory option that was passed in). That bare-only path\nshould just go away entirely, and both should use the same (correct)\ncode to get the absolute path.\n\n> SOLUTION: I propose using \"--absolute-git-dir\" instead of \"--git-dir\":\n> \n>     git rev-parse --is-bare-repository --absolute-git-dir\n> \n> Which will replace the `.`  rev-parse response with a full path\n> resolved by git itself (and not Perl). This means the change to the\n> Perl code is minimal. I don't know if this will resolve all possible\n> cases, but it does fix our issue.\n\nIt does fix all cases, but it leaves some redundant code in place.\nHere are two patches. The first does the minimal fix within the code\n(what 20da61f25 should have done!) and corrects the problem. The second\nswitches to --absolute-git-dir and drops the now-redundant code.\n\nThank you for a very thorough bug report!\n\n  [1/2]: Git.pm: fix bare repository search with Directory option\n  [2/2]: Git.pm: use \"rev-parse --absolute-git-dir\" rather than perl code\n\n perl/Git.pm         | 14 ++++++--------\n t/t9700-perl-git.sh |  3 ++-\n t/t9700/test.pl     |  5 +++++\n 3 files changed, 13 insertions(+), 9 deletions(-)\n\n-Peff\n"},{"id":"502724","messageId":"20240912223604.GA650605@coredump.intra.peff.net","threadId":"62100","inReplyTo":"20240912223413.GA649897@coredump.intra.peff.net","subject":"[PATCH 1/2] Git.pm: fix bare repository search with Directory option","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-09-12T22:36:04Z","receivedAt":"2024-09-12T22:36:05Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When opening a bare repository like:\n\n  Git->repository(Directory => '/path/to/bare.git');\n\nwe will incorrectly point the repository object at the _current_\ndirectory, not the one specified by the option.\n\nThe bug was introduced by 20da61f25f (Git.pm: trust rev-parse to find\nbare repositories, 2022-10-22). Before then, we'd ask \"rev-parse\n--git-dir\" if it was a Git repo, and if it returned anything, we'd\ncorrectly convert that result to an absolute path using File::Spec and\nCwd::abs_path(). If it didn't, we'd guess it might be a bare repository\nand find it ourselves, which was wrong (rev-parse should find even a\nbare repo, and our search circumvented some of its rules).\n\nThat commit dropped most of the custom bare-repo search code in favor of\nusing \"rev-parse --is-bare-repository\" and trusting the \"--git-dir\" it\nreturned. But it mistakenly left some of the bare-repo code path in\nplace, which was now broken. That code calls Cwd::abs_path($dir); prior\nto 20da61f25f $dir contained the \"Directory\" option the user passed in.\nBut afterwards, it contains the output of \"rev-parse --git-dir\". And\nsince our tentative rev-parse command is invoked after changing\ndirectory, it will always be the relative path \".\"! So we'll end up with\nthe absolute path of the process's current directory, not the Directory\noption the caller asked for.\n\nSo the non-bare case is correct, but the bare one is broken. Our tests\nonly check the non-bare one, so we didn't notice. We can fix this by\nrunning the same absolute-path fixup code for both sides.\n\nHelped-by: Rodrigo <rodrigolive@gmail.com>\nSigned-off-by: Jeff King <peff@peff.net>\n---\n perl/Git.pm         | 10 ++++------\n t/t9700-perl-git.sh |  3 ++-\n t/t9700/test.pl     |  5 +++++\n 3 files changed, 11 insertions(+), 7 deletions(-)\n\ndiff --git a/perl/Git.pm b/perl/Git.pm\nindex aebfe0c6e0..cf1ef0b32a 100644\n--- a/perl/Git.pm\n+++ b/perl/Git.pm\n@@ -197,11 +197,11 @@ sub repository {\n \t\tmy ($bare, $dir) = split /\\n/, $out, 2;\n \n \t\trequire Cwd;\n-\t\tif ($bare ne 'true') {\n-\t\t\trequire File::Spec;\n-\t\t\tFile::Spec->file_name_is_absolute($dir) or $dir = $opts{Directory} . '/' . $dir;\n-\t\t\t$opts{Repository} = Cwd::abs_path($dir);\n+\t\trequire File::Spec;\n+\t\tFile::Spec->file_name_is_absolute($dir) or $dir = $opts{Directory} . '/' . $dir;\n+\t\t$opts{Repository} = Cwd::abs_path($dir);\n \n+\t\tif ($bare ne 'true') {\n \t\t\t# If --git-dir went ok, this shouldn't die either.\n \t\t\tmy $prefix = $search->command_oneline('rev-parse', '--show-prefix');\n \t\t\t$dir = Cwd::abs_path($opts{Directory}) . '/';\n@@ -214,8 +214,6 @@ sub repository {\n \t\t\t$opts{WorkingCopy} = $dir;\n \t\t\t$opts{WorkingSubdir} = $prefix;\n \n-\t\t} else {\n-\t\t\t$opts{Repository} = Cwd::abs_path($dir);\n \t\t}\n \n \t\tdelete $opts{Directory};\ndiff --git a/t/t9700-perl-git.sh b/t/t9700-perl-git.sh\nindex ccc8212d73..4431697122 100755\n--- a/t/t9700-perl-git.sh\n+++ b/t/t9700-perl-git.sh\n@@ -45,7 +45,8 @@ test_expect_success 'set up test repository' '\n '\n \n test_expect_success 'set up bare repository' '\n-\tgit init --bare bare.git\n+\tgit init --bare bare.git &&\n+\tgit -C bare.git --work-tree=. commit --allow-empty -m \"bare commit\"\n '\n \n test_expect_success 'use t9700/test.pl to test Git.pm' '\ndiff --git a/t/t9700/test.pl b/t/t9700/test.pl\nindex d8e85482ab..2e1d50d4d1 100755\n--- a/t/t9700/test.pl\n+++ b/t/t9700/test.pl\n@@ -147,6 +147,11 @@ sub adjust_dirsep {\n unlink $tmpfile3;\n chdir($abs_repo_dir);\n \n+# open alternate bare repo\n+my $r4 = Git->repository(Directory => \"$abs_repo_dir/bare.git\");\n+is($r4->command_oneline(qw(log --format=%s)), \"bare commit\",\n+\t\"log of bare repo works\");\n+\n # unquoting paths\n is(Git::unquote_path('abc'), 'abc', 'unquote unquoted path');\n is(Git::unquote_path('\"abc def\"'), 'abc def', 'unquote simple quoted path');\n-- \n2.46.0.918.gab30941bff\n\n"},{"id":"502725","messageId":"20240912223725.GB650605@coredump.intra.peff.net","threadId":"62100","inReplyTo":"20240912223413.GA649897@coredump.intra.peff.net","subject":"[PATCH 2/2] Git.pm: use \"rev-parse --absolute-git-dir\" rather than perl code","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-09-12T22:37:25Z","receivedAt":"2024-09-12T22:37:26Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When we open a repository with the \"Directory\" option, we use \"rev-parse\n--git-dir\" to get the path relative to that directory, and then use\nCwd::abs_path() to make it absolute (since our process working directory\nmay not be the same).\n\nThese days we can just ask for \"--absolute-git-dir\" instead, which saves\nus a little code. That option was added in Git v2.13.0 via a2f5a87626\n(rev-parse: add '--absolute-git-dir' option, 2017-02-03). I don't think\nwe make any promises about running mismatched versions of git and\nGit.pm, but even if somebody tries it, that's sufficiently old that it\nshould be OK.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nI retained the \"require Cwd\" here since we use it in the conditional\n(but moved it closer to the point of use). It's not strictly necessary,\nas earlier code will have required it as a side effect, but it's\nprobably best not to rely on that.\n\n perl/Git.pm | 10 +++++-----\n 1 file changed, 5 insertions(+), 5 deletions(-)\n\ndiff --git a/perl/Git.pm b/perl/Git.pm\nindex cf1ef0b32a..667152c6c6 100644\n--- a/perl/Git.pm\n+++ b/perl/Git.pm\n@@ -187,7 +187,7 @@ sub repository {\n \t\ttry {\n \t\t  # Note that \"--is-bare-repository\" must come first, as\n \t\t  # --git-dir output could contain newlines.\n-\t\t  $out = $search->command([qw(rev-parse --is-bare-repository --git-dir)],\n+\t\t  $out = $search->command([qw(rev-parse --is-bare-repository --absolute-git-dir)],\n \t\t\t                  STDERR => 0);\n \t\t} catch Git::Error::Command with {\n \t\t\tthrow Error::Simple(\"fatal: not a git repository: $opts{Directory}\");\n@@ -196,12 +196,12 @@ sub repository {\n \t\tchomp $out;\n \t\tmy ($bare, $dir) = split /\\n/, $out, 2;\n \n-\t\trequire Cwd;\n-\t\trequire File::Spec;\n-\t\tFile::Spec->file_name_is_absolute($dir) or $dir = $opts{Directory} . '/' . $dir;\n-\t\t$opts{Repository} = Cwd::abs_path($dir);\n+\t\t# We know this is an absolute path, because we used\n+\t\t# --absolute-git-dir above.\n+\t\t$opts{Repository} = $dir;\n \n \t\tif ($bare ne 'true') {\n+\t\t\trequire Cwd;\n \t\t\t# If --git-dir went ok, this shouldn't die either.\n \t\t\tmy $prefix = $search->command_oneline('rev-parse', '--show-prefix');\n \t\t\t$dir = Cwd::abs_path($opts{Directory}) . '/';\n-- \n2.46.0.918.gab30941bff\n"},{"id":"502732","messageId":"ZuPWIzzYda5aVjMa@pks.im","threadId":"62100","inReplyTo":"20240912223604.GA650605@coredump.intra.peff.net","subject":"Re: [PATCH 1/2] Git.pm: fix bare repository search with Directory option","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-13T06:05:23Z","receivedAt":"2024-09-13T06:05:27Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Thu, Sep 12, 2024 at 06:36:04PM -0400, Jeff King wrote:\n> diff --git a/perl/Git.pm b/perl/Git.pm\n> index aebfe0c6e0..cf1ef0b32a 100644\n> --- a/perl/Git.pm\n> +++ b/perl/Git.pm\n> @@ -197,11 +197,11 @@ sub repository {\n>  \t\tmy ($bare, $dir) = split /\\n/, $out, 2;\n>  \n>  \t\trequire Cwd;\n> -\t\tif ($bare ne 'true') {\n> -\t\t\trequire File::Spec;\n> -\t\t\tFile::Spec->file_name_is_absolute($dir) or $dir = $opts{Directory} . '/' . $dir;\n> -\t\t\t$opts{Repository} = Cwd::abs_path($dir);\n> +\t\trequire File::Spec;\n> +\t\tFile::Spec->file_name_is_absolute($dir) or $dir = $opts{Directory} . '/' . $dir;\n> +\t\t$opts{Repository} = Cwd::abs_path($dir);\n>  \n> +\t\tif ($bare ne 'true') {\n>  \t\t\t# If --git-dir went ok, this shouldn't die either.\n>  \t\t\tmy $prefix = $search->command_oneline('rev-parse', '--show-prefix');\n>  \t\t\t$dir = Cwd::abs_path($opts{Directory}) . '/';\n> @@ -214,8 +214,6 @@ sub repository {\n>  \t\t\t$opts{WorkingCopy} = $dir;\n>  \t\t\t$opts{WorkingSubdir} = $prefix;\n>  \n> -\t\t} else {\n> -\t\t\t$opts{Repository} = Cwd::abs_path($dir);\n>  \t\t}\n>  \n>  \t\tdelete $opts{Directory};\n\nMakes sense. We already knew that the $dir was relative, but only\nremembered to handle this in case the repository was non-bare. Now both\ncases use the same code to translate the relative path to an absolute\none.\n\n> diff --git a/t/t9700-perl-git.sh b/t/t9700-perl-git.sh\n> index ccc8212d73..4431697122 100755\n> --- a/t/t9700-perl-git.sh\n> +++ b/t/t9700-perl-git.sh\n> @@ -45,7 +45,8 @@ test_expect_success 'set up test repository' '\n>  '\n>  \n>  test_expect_success 'set up bare repository' '\n> -\tgit init --bare bare.git\n> +\tgit init --bare bare.git &&\n> +\tgit -C bare.git --work-tree=. commit --allow-empty -m \"bare commit\"\n>  '\n>  \n>  test_expect_success 'use t9700/test.pl to test Git.pm' '\n\nI didn't even know that this hack was possible. I guess the alternative\nwould be to use git-commit-tree(1) with the empty tree ID, but that'd\nalso require us to update branches manually via git-update-ref(1). So...\na bit gross, but hey, if it works...\n\nPatrick\n"},{"id":"502733","messageId":"ZuPWJtPo-2f2Mgbl@pks.im","threadId":"62100","inReplyTo":"20240912223725.GB650605@coredump.intra.peff.net","subject":"Re: [PATCH 2/2] Git.pm: use \"rev-parse --absolute-git-dir\" rather than perl code","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-13T06:05:26Z","receivedAt":"2024-09-13T06:05:28Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Thu, Sep 12, 2024 at 06:37:25PM -0400, Jeff King wrote:\n> When we open a repository with the \"Directory\" option, we use \"rev-parse\n> --git-dir\" to get the path relative to that directory, and then use\n> Cwd::abs_path() to make it absolute (since our process working directory\n> may not be the same).\n> \n> These days we can just ask for \"--absolute-git-dir\" instead, which saves\n> us a little code. That option was added in Git v2.13.0 via a2f5a87626\n> (rev-parse: add '--absolute-git-dir' option, 2017-02-03). I don't think\n> we make any promises about running mismatched versions of git and\n> Git.pm, but even if somebody tries it, that's sufficiently old that it\n> should be OK.\n\nAgreed. We should eventually be able to rely on things that we have\nimplemented many years ago.\n\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n> I retained the \"require Cwd\" here since we use it in the conditional\n> (but moved it closer to the point of use). It's not strictly necessary,\n> as earlier code will have required it as a side effect, but it's\n> probably best not to rely on that.\n> \n>  perl/Git.pm | 10 +++++-----\n>  1 file changed, 5 insertions(+), 5 deletions(-)\n> \n> diff --git a/perl/Git.pm b/perl/Git.pm\n> index cf1ef0b32a..667152c6c6 100644\n> --- a/perl/Git.pm\n> +++ b/perl/Git.pm\n> @@ -187,7 +187,7 @@ sub repository {\n>  \t\ttry {\n>  \t\t  # Note that \"--is-bare-repository\" must come first, as\n>  \t\t  # --git-dir output could contain newlines.\n> -\t\t  $out = $search->command([qw(rev-parse --is-bare-repository --git-dir)],\n> +\t\t  $out = $search->command([qw(rev-parse --is-bare-repository --absolute-git-dir)],\n>  \t\t\t                  STDERR => 0);\n>  \t\t} catch Git::Error::Command with {\n>  \t\t\tthrow Error::Simple(\"fatal: not a git repository: $opts{Directory}\");\n> @@ -196,12 +196,12 @@ sub repository {\n>  \t\tchomp $out;\n>  \t\tmy ($bare, $dir) = split /\\n/, $out, 2;\n\nThis line here made me think for a second what happens if the absolute\npath contains newlines. But it should be fine, because we only split at\nthe first newline character we find. And as the first parameter that we\npass to git-rev-parse(1) is `--is-bare-repository`, we know that it will\noutput either `true` or `false` as the first line. Any subsequent\nnewlines should thus be handled alright.\n\nPatrick\n"},{"id":"502767","messageId":"xmqq4j6jzbss.fsf@gitster.g","threadId":"62100","inReplyTo":"20240912223413.GA649897@coredump.intra.peff.net","subject":"Re: [PATCH 0/2] fix bare repositories with Git.pm","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-09-13T17:46:43Z","receivedAt":"2024-09-13T17:46:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> Yikes, good catch. That's a pretty bad bug. I'm surprised we didn't\n> cover this in the tests, but it's specific to bare repositories.\n>\n>> Bug hunting through the Git.pm code and skimming through the Git SCM\n>> repo, there's a significant change (commit 20da61f25) that makes the\n>> recent Git.pm rely on:\n>> \n>>      git rev-parse --is-bare-repository --git-dir\n>\n> Yep, I confirmed via bisection that that commit is the culprit.\n\nYeah, it is surprising that nobody noticed it since Dec 2022.\n\n> It does fix all cases, but it leaves some redundant code in place.\n> Here are two patches. The first does the minimal fix within the code\n> (what 20da61f25 should have done!) and corrects the problem. The second\n> switches to --absolute-git-dir and drops the now-redundant code.\n>\n> Thank you for a very thorough bug report!\n\nYup, thanks, both.\n\nQueued.\n"}]}