Re: [PATCH 1/2] rev-parse: add --filename-prefix option

From: John Keeping <john@keeping.me.uk>
Date: Apr 8, 2013, 08:31 UTC
Message-ID: <20130408083123.GN2222@serenity.lan>
In-Reply-To: <20130407221458.GE19857@elie.Belkin>

On Sun, Apr 07, 2013 at 03:14:58PM -0700, Jonathan Nieder wrote:

Show 11 quoted lines

> John Keeping wrote:
> 
> > This adds a prefix string to any filename arguments encountered after it
> > has been specified.
> 
> I assume this is a way of passing the prefix in?  In that case, I
> think a good UI would be
> 
> 	git rev-parse --prefix=Documentation/ <usual rev-parse args>
> 
> That sounds like a useful thing and would make the meaning very clear.

Yes (ish), the intended usage is something like this:

    prefix=$(git rev-parse --show-prefix)
    cd_to_toplevel
    ... parse options here ...
    # Convert remaining arguments (filenames) into top-level paths:
    eval "set $(git rev-parse --prefix "$prefix" --sq -- "$@")"

The "ish" is that my current implementation introduced a new variable instead of simply resetting the existing "prefix" variable, which I assume is what you mean. That is probably simpler than my implementation, but loses the ability to be at an intermediate level, for example:

    cd Documentation/
    eval "set $(git rev-parse --prefix technical/ --sq -- api-strbuf.txt)"

> How does this interact with the following options?
> 
>  * --resolve-git-dir some/relative/path

It doesn't change this since --resolve-git-dir is handled separately from the other argument parsing at the moment and you cannot specify any other options with it.

>  * master:./path

I hadn't considered this case, but I think it should be inserting the prefix into the path. I suspect the easiest thing to do is simply make the path part of that absolute, by combining both the prefix based on $PWD and the prefix specified on the command line, but I haven't looked at doing this yet.

The other think that's missing at the moment is that the prefix passed to verify_filename should be modified by the one specified on the command line.

> As for the patch itself, I haven't looked at it closely.  My only
> immediate reaction is that I wish it touched Documentation/ and t/. :)

I'll make sure the next version does. This version was doing the minimum required to make patch 2/2 possible, it certainly needs some polish before it's more than a proof-of-concept.