| Message ID | 20240912223725.GB650605@coredump.intra.peff.net (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | d3edb0bddec29c97fb0314d9b1ee77d2e7d22382 |
| Headers | show |
| Series | fix bare repositories with Git.pm | expand |
On Thu, Sep 12, 2024 at 06:37:25PM -0400, Jeff King wrote: > When we open a repository with the "Directory" option, we use "rev-parse > --git-dir" to get the path relative to that directory, and then use > Cwd::abs_path() to make it absolute (since our process working directory > may not be the same). > > These days we can just ask for "--absolute-git-dir" instead, which saves > us a little code. That option was added in Git v2.13.0 via a2f5a87626 > (rev-parse: add '--absolute-git-dir' option, 2017-02-03). I don't think > we make any promises about running mismatched versions of git and > Git.pm, but even if somebody tries it, that's sufficiently old that it > should be OK. Agreed. We should eventually be able to rely on things that we have implemented many years ago. > Signed-off-by: Jeff King <peff@peff.net> > --- > I retained the "require Cwd" here since we use it in the conditional > (but moved it closer to the point of use). It's not strictly necessary, > as earlier code will have required it as a side effect, but it's > probably best not to rely on that. > > perl/Git.pm | 10 +++++----- > 1 file changed, 5 insertions(+), 5 deletions(-) > > diff --git a/perl/Git.pm b/perl/Git.pm > index cf1ef0b32a..667152c6c6 100644 > --- a/perl/Git.pm > +++ b/perl/Git.pm > @@ -187,7 +187,7 @@ sub repository { > try { > # Note that "--is-bare-repository" must come first, as > # --git-dir output could contain newlines. > - $out = $search->command([qw(rev-parse --is-bare-repository --git-dir)], > + $out = $search->command([qw(rev-parse --is-bare-repository --absolute-git-dir)], > STDERR => 0); > } catch Git::Error::Command with { > throw Error::Simple("fatal: not a git repository: $opts{Directory}"); > @@ -196,12 +196,12 @@ sub repository { > chomp $out; > my ($bare, $dir) = split /\n/, $out, 2; This line here made me think for a second what happens if the absolute path contains newlines. But it should be fine, because we only split at the first newline character we find. And as the first parameter that we pass to git-rev-parse(1) is `--is-bare-repository`, we know that it will output either `true` or `false` as the first line. Any subsequent newlines should thus be handled alright. Patrick
diff --git a/perl/Git.pm b/perl/Git.pm index cf1ef0b32a..667152c6c6 100644 --- a/perl/Git.pm +++ b/perl/Git.pm @@ -187,7 +187,7 @@ sub repository { try { # Note that "--is-bare-repository" must come first, as # --git-dir output could contain newlines. - $out = $search->command([qw(rev-parse --is-bare-repository --git-dir)], + $out = $search->command([qw(rev-parse --is-bare-repository --absolute-git-dir)], STDERR => 0); } catch Git::Error::Command with { throw Error::Simple("fatal: not a git repository: $opts{Directory}"); @@ -196,12 +196,12 @@ sub repository { chomp $out; my ($bare, $dir) = split /\n/, $out, 2; - require Cwd; - require File::Spec; - File::Spec->file_name_is_absolute($dir) or $dir = $opts{Directory} . '/' . $dir; - $opts{Repository} = Cwd::abs_path($dir); + # We know this is an absolute path, because we used + # --absolute-git-dir above. + $opts{Repository} = $dir; if ($bare ne 'true') { + require Cwd; # 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}) . '/';
When we open a repository with the "Directory" option, we use "rev-parse --git-dir" to get the path relative to that directory, and then use Cwd::abs_path() to make it absolute (since our process working directory may not be the same). These days we can just ask for "--absolute-git-dir" instead, which saves us a little code. That option was added in Git v2.13.0 via a2f5a87626 (rev-parse: add '--absolute-git-dir' option, 2017-02-03). I don't think we make any promises about running mismatched versions of git and Git.pm, but even if somebody tries it, that's sufficiently old that it should be OK. Signed-off-by: Jeff King <peff@peff.net> --- I retained the "require Cwd" here since we use it in the conditional (but moved it closer to the point of use). It's not strictly necessary, as earlier code will have required it as a side effect, but it's probably best not to rely on that. perl/Git.pm | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)