| Message ID | 20240912223604.GA650605@coredump.intra.peff.net (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | e4b353d0a138f946b2321efa7ba9094cf747dd34 |
| Headers | show |
| Series | fix bare repositories with Git.pm | expand |
On Thu, Sep 12, 2024 at 06:36:04PM -0400, Jeff King wrote: > diff --git a/perl/Git.pm b/perl/Git.pm > index aebfe0c6e0..cf1ef0b32a 100644 > --- a/perl/Git.pm > +++ b/perl/Git.pm > @@ -197,11 +197,11 @@ sub repository { > my ($bare, $dir) = split /\n/, $out, 2; > > require Cwd; > - if ($bare ne 'true') { > - require File::Spec; > - File::Spec->file_name_is_absolute($dir) or $dir = $opts{Directory} . '/' . $dir; > - $opts{Repository} = Cwd::abs_path($dir); > + require File::Spec; > + File::Spec->file_name_is_absolute($dir) or $dir = $opts{Directory} . '/' . $dir; > + $opts{Repository} = Cwd::abs_path($dir); > > + if ($bare ne 'true') { > # If --git-dir went ok, this shouldn't die either. > my $prefix = $search->command_oneline('rev-parse', '--show-prefix'); > $dir = Cwd::abs_path($opts{Directory}) . '/'; > @@ -214,8 +214,6 @@ sub repository { > $opts{WorkingCopy} = $dir; > $opts{WorkingSubdir} = $prefix; > > - } else { > - $opts{Repository} = Cwd::abs_path($dir); > } > > delete $opts{Directory}; Makes sense. We already knew that the $dir was relative, but only remembered to handle this in case the repository was non-bare. Now both cases use the same code to translate the relative path to an absolute one. > diff --git a/t/t9700-perl-git.sh b/t/t9700-perl-git.sh > index ccc8212d73..4431697122 100755 > --- a/t/t9700-perl-git.sh > +++ b/t/t9700-perl-git.sh > @@ -45,7 +45,8 @@ test_expect_success 'set up test repository' ' > ' > > test_expect_success 'set up bare repository' ' > - git init --bare bare.git > + git init --bare bare.git && > + git -C bare.git --work-tree=. commit --allow-empty -m "bare commit" > ' > > test_expect_success 'use t9700/test.pl to test Git.pm' ' I didn't even know that this hack was possible. I guess the alternative would be to use git-commit-tree(1) with the empty tree ID, but that'd also require us to update branches manually via git-update-ref(1). So... a bit gross, but hey, if it works... Patrick
diff --git a/perl/Git.pm b/perl/Git.pm index aebfe0c6e0..cf1ef0b32a 100644 --- a/perl/Git.pm +++ b/perl/Git.pm @@ -197,11 +197,11 @@ sub repository { my ($bare, $dir) = split /\n/, $out, 2; require Cwd; - if ($bare ne 'true') { - require File::Spec; - File::Spec->file_name_is_absolute($dir) or $dir = $opts{Directory} . '/' . $dir; - $opts{Repository} = Cwd::abs_path($dir); + require File::Spec; + File::Spec->file_name_is_absolute($dir) or $dir = $opts{Directory} . '/' . $dir; + $opts{Repository} = Cwd::abs_path($dir); + if ($bare ne 'true') { # If --git-dir went ok, this shouldn't die either. my $prefix = $search->command_oneline('rev-parse', '--show-prefix'); $dir = Cwd::abs_path($opts{Directory}) . '/'; @@ -214,8 +214,6 @@ sub repository { $opts{WorkingCopy} = $dir; $opts{WorkingSubdir} = $prefix; - } else { - $opts{Repository} = Cwd::abs_path($dir); } delete $opts{Directory}; diff --git a/t/t9700-perl-git.sh b/t/t9700-perl-git.sh index ccc8212d73..4431697122 100755 --- a/t/t9700-perl-git.sh +++ b/t/t9700-perl-git.sh @@ -45,7 +45,8 @@ test_expect_success 'set up test repository' ' ' test_expect_success 'set up bare repository' ' - git init --bare bare.git + git init --bare bare.git && + git -C bare.git --work-tree=. commit --allow-empty -m "bare commit" ' test_expect_success 'use t9700/test.pl to test Git.pm' ' diff --git a/t/t9700/test.pl b/t/t9700/test.pl index d8e85482ab..2e1d50d4d1 100755 --- a/t/t9700/test.pl +++ b/t/t9700/test.pl @@ -147,6 +147,11 @@ sub adjust_dirsep { unlink $tmpfile3; chdir($abs_repo_dir); +# open alternate bare repo +my $r4 = Git->repository(Directory => "$abs_repo_dir/bare.git"); +is($r4->command_oneline(qw(log --format=%s)), "bare commit", + "log of bare repo works"); + # unquoting paths is(Git::unquote_path('abc'), 'abc', 'unquote unquoted path'); is(Git::unquote_path('"abc def"'), 'abc def', 'unquote simple quoted path');
When opening a bare repository like: Git->repository(Directory => '/path/to/bare.git'); we will incorrectly point the repository object at the _current_ directory, not the one specified by the option. The bug was introduced by 20da61f25f (Git.pm: trust rev-parse to find bare repositories, 2022-10-22). Before then, we'd ask "rev-parse --git-dir" if it was a Git repo, and if it returned anything, we'd correctly convert that result to an absolute path using File::Spec and Cwd::abs_path(). If it didn't, we'd guess it might be a bare repository and find it ourselves, which was wrong (rev-parse should find even a bare repo, and our search circumvented some of its rules). That commit dropped most of the custom bare-repo search code in favor of using "rev-parse --is-bare-repository" and trusting the "--git-dir" it returned. But it mistakenly left some of the bare-repo code path in place, which was now broken. That code calls Cwd::abs_path($dir); prior to 20da61f25f $dir contained the "Directory" option the user passed in. But afterwards, it contains the output of "rev-parse --git-dir". And since our tentative rev-parse command is invoked after changing directory, it will always be the relative path "."! So we'll end up with the absolute path of the process's current directory, not the Directory option the caller asked for. So the non-bare case is correct, but the bare one is broken. Our tests only check the non-bare one, so we didn't notice. We can fix this by running the same absolute-path fixup code for both sides. Helped-by: Rodrigo <rodrigolive@gmail.com> Signed-off-by: Jeff King <peff@peff.net> --- perl/Git.pm | 10 ++++------ t/t9700-perl-git.sh | 3 ++- t/t9700/test.pl | 5 +++++ 3 files changed, 11 insertions(+), 7 deletions(-)