Re: [PATCH 13/21] rev-parse: add '--absolute-git-dir' option

From: Jeff King <peff@peff.net>
Date: Apr 26, 2016, 05:35 UTC
Message-ID: <20160426053522.GA23949@sigill.intra.peff.net>
In-Reply-To: <CANoM8SXO_Rz_CVOz9ptsaVCzcQ2D1FQrSuFFW4vZ4SdRYMzD=w@mail.gmail.com>

On Mon, Apr 25, 2016 at 10:33:13PM -0400, Mike Rappazzo wrote:

Show 15 quoted lines

> I propose that it might make more sense to use something like
> `--abs-path` to indicate
> that the result should include an absolute path (or we could also just spell out
> `--absolute-path`).  That way we don't have to add additional options
> for any other type
> that might want an absolute path.
> 
>     git rev-parse --git-dir --abs-path
>     git rev-parse --git-common-dir --absolute-path
> 
> I do understand that this might be more work than is necessary for the
> completion series
> here.  Would it be unreasonable to suggest a partial implementation
> that, for now, only
> works with `--git-dir`?

I do like the concept of keeping "--absolute-path" orthogonal. The only trick is that we need to either support it for all appropriate options, or document which options it _does_ work with. Otherwise, we're going to get bug reports when somebody tries "--absolute-path --git-common-dir".

It would be cleaner to provide a separate option to let people compose the options, like:

  git rev-parse --git-dir | git rev-parse --realpath

but that's a lot less efficient.

Show 7 quoted lines

> > +                                       if (gitdir) {
> > +                                               char absolute_path[PATH_MAX];
> > +                                               if (!realpath(gitdir, absolute_path))
> > +                                                       die_errno(_("unable to get absolute path"));
> > +                                               puts(absolute_path);
> > +                                               continue;
> > +                                       }

I don't recall if this came up in earlier review, but I happened to notice the use of realpath() here. We should be using our custom real_path() instead. There are some platforms without realpath(), I think, and our real_path() is not limited to the static PATH_MAX (which is too small on some platforms).

-Peff