Re: [PATCH] rev-parse: add option for absolute or relative path formatting

From: SZEDER Gábor <szeder.dev@gmail.com>
Date: Sep 10, 2020, 15:19 UTC
Message-ID: <20200910151935.GA5265@szeder.dev>
In-Reply-To: <20200909222333.GH241078@camp.crustytoothpaste.net>

On Wed, Sep 09, 2020 at 10:23:33PM +0000, brian m. carlson wrote:

Show 33 quoted lines

> On 2020-09-09 at 14:51:14, SZEDER Gábor wrote:
> > On Tue, Sep 08, 2020 at 06:50:17PM +0000, brian m. carlson wrote:
> > > diff --git a/Documentation/git-rev-parse.txt b/Documentation/git-rev-parse.txt
> > > index 19b12b6d43..6b95292559 100644
> > > --- a/Documentation/git-rev-parse.txt
> > > +++ b/Documentation/git-rev-parse.txt
> > > @@ -208,6 +208,15 @@ Options for Files
> > >  	Only the names of the variables are listed, not their value,
> > >  	even if they are set.
> > >  
> > > +--path-format=(absolute|relative)::
> > > +	Controls the behavior of certain other options following it on the command
> > > +	line.
> > 
> > Does it affect only one subsequent such option on the command line, or
> > all such options?  IOW, while standing in the top directory of the
> > worktree, would the following command
> > 
> >   git rev-parse --path-format=absolute --git-dir --git-path foo --show-toplevel
> > 
> > print three absolute paths, or one absolute paths and two relative
> > paths?
> > 
> > The wording here is not clear on this, the commit message doesn't
> > mention it, and the tests added in this patch only check one such
> > option, but looking at the code and doing some testing of my own I
> > found that it affects all subsequent such options.
> 
> It affects all subsequent options.  Moreover, I believe it's possible to
> switch in the middle of the command line if you want some things
> relative and some absolute.  That seemed to be both the easiest solution
> and the most flexible, so I went with it.  I'll add more tests for this
> case and improve the commit message.

Well, on first sight it seems nice that scripts have to specify '--path-format=absolute' only once when querying multiple paths at once, though on second thought no prudent script would query multiple paths at once, because they might contain newlines.

> >   $ ./git -C Documentation/ rev-parse --path-format=relative --git-dir --show-toplevel
> >   /home/szeder/src/git/.git
> >   /home/szeder/src/git

> >   $ ./git rev-parse --path-format=absolute --git-path foo/bar
> >   fatal: Invalid path '/home/szeder/src/git/.git/foo': No such file or directory
> 
> That's going to be a little tricky to fix, but I'll look into it.

Yeah, I think I found trouble that way, too. I wonder whether '--path-format=relative' is really worth having, though. Clearly there's a need for absolute paths, because getting relative paths causes difficulties for some scripts; I described one such use case in a2f5a87626 (rev-parse: add '--absolute-git-dir' option, 2017-02-03), and you just ran into another with Git LFS. However, is there really a use case that requires relative paths, because an absolute path would cause similar difficulties? I couldn't come with any.

So perhaps it would be sufficient to introduce only '--path-format=absolute' (or equivalent) for now, and add a relative path variant only when there will be an actual compelling reason to do so. And that would save you from the pain of addressing these bugs shown above.