[0/2] fix bare repositories with Git.pm

Message

Jeff King Sept. 12, 2024, 10:34 p.m. UTC

On Thu, Sep 12, 2024 at 06:32:00PM +0200, Rodrigo wrote:

> We're having an issue migrating from 2.31.1 to 2.46.0. The following
> Perl code does not work in 2.46.0 as it did in 2.31.1 (tested linux
> and darwin, did not check Windows):
> 
>     # test.pl
>     use Git;
>     my $repo = Git->repository( Directory => '/repo/bare.git' );
>     my ($fh, $ctx) = $repo->command_output_pipe('rev-list', "2acf3456");
>     print do { local $/; <$fh> };
> 
> Run:
> 
>    $ cd /home/rodrigo
>    $ perl test.pl
> 
> Fails with the error:
> 
>     fatal: not a git repository: '/home/rodrigo'

Yikes, good catch. That's a pretty bad bug. I'm surprised we didn't
cover this in the tests, but it's specific to bare repositories.

> Bug hunting through the Git.pm code and skimming through the Git SCM
> repo, there's a significant change (commit 20da61f25) that makes the
> recent Git.pm rely on:
> 
>      git rev-parse --is-bare-repository --git-dir

Yep, I confirmed via bisection that that commit is the culprit. Your
analysis is mostly good, though...

> for locating the correct (maybe a parent) repo directory. The change
> was understandably done for security (and other many) reasons. It
> started using --is-bare-repository to detect if it's a bare repository
> we're dealing with, instead of relying on the old Git.pm redundant
> code for bare repo detection, which was a sound decision. But some
> crucial code was taken out.

...the problem is actually that not enough code was taken out. ;) I left
the code in the bare-only code to turn the relative path to absolute,
but it used the wrong path (the one returned by rev-parse, rather than
the original Directory option that was passed in). That bare-only path
should just go away entirely, and both should use the same (correct)
code to get the absolute path.

> SOLUTION: I propose using "--absolute-git-dir" instead of "--git-dir":
> 
>     git rev-parse --is-bare-repository --absolute-git-dir
> 
> Which will replace the `.`  rev-parse response with a full path
> resolved by git itself (and not Perl). This means the change to the
> Perl code is minimal. I don't know if this will resolve all possible
> cases, but it does fix our issue.

It does fix all cases, but it leaves some redundant code in place.
Here are two patches. The first does the minimal fix within the code
(what 20da61f25 should have done!) and corrects the problem. The second
switches to --absolute-git-dir and drops the now-redundant code.

Thank you for a very thorough bug report!

  [1/2]: Git.pm: fix bare repository search with Directory option
  [2/2]: Git.pm: use "rev-parse --absolute-git-dir" rather than perl code

 perl/Git.pm         | 14 ++++++--------
 t/t9700-perl-git.sh |  3 ++-
 t/t9700/test.pl     |  5 +++++
 3 files changed, 13 insertions(+), 9 deletions(-)

-Peff

Comments

Junio C Hamano Sept. 13, 2024, 5:46 p.m. UTC | #1

Jeff King <[e-mail]> writes:

> Yikes, good catch. That's a pretty bad bug. I'm surprised we didn't
> cover this in the tests, but it's specific to bare repositories.
>
>> Bug hunting through the Git.pm code and skimming through the Git SCM
>> repo, there's a significant change (commit 20da61f25) that makes the
>> recent Git.pm rely on:
>> 
>>      git rev-parse --is-bare-repository --git-dir
>
> Yep, I confirmed via bisection that that commit is the culprit.

Yeah, it is surprising that nobody noticed it since Dec 2022.

> It does fix all cases, but it leaves some redundant code in place.
> Here are two patches. The first does the minimal fix within the code
> (what 20da61f25 should have done!) and corrects the problem. The second
> switches to --absolute-git-dir and drops the now-redundant code.
>
> Thank you for a very thorough bug report!

Yup, thanks, both.

Queued.