Re: [PATCH 2/2] Git.pm: use "rev-parse --absolute-git-dir" rather than perl code

[Patrick Steinhardt <ps@pks.im>]

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