Re: [PATCH v2 1/2] rev-parse: add --filename-prefix option
- From
John Keeping <john@keeping.me.uk>
- Date
- Apr 18, 2013, 14:42 UTC
- Message-ID
- <20130418144257.GZ2278@serenity.lan>
- In-Reply-To
- <CALkWK0nir7pJF-7YLRQF0jCR4dbb-JNBheD-4zKeQR0K9F9nZg@mail.gmail.com>
On Thu, Apr 18, 2013 at 07:58:25PM +0530, Ramkumar Ramachandra wrote:
Show 9 quoted lines
> John Keeping wrote: > > This adds a prefix string to any filename arguments encountered after it > > has been specified. > > Very nice. I thought we'd have to resort to path mangling in shell to > fix git-submodule.sh. Glad to see that we can go with something > cleaner. > > Perhaps pull some bits from your nice Documentation into the commit message?
Good idea. I intended to re-write the commit message for v2 since the patch was completely re-written but forgot by the time I'd sorted out patch 2 as well. I will do for v3.
Show 31 quoted lines
> > diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
> > index f267a1d..de894c7 100644
> > --- a/builtin/rev-parse.c
> > +++ b/builtin/rev-parse.c
> > @@ -212,11 +212,17 @@ static void show_datestring(const char *flag, const char *datestr)
> > show(buffer);
> > }
> >
> > -static int show_file(const char *arg)
> > +static int show_file(const char *arg, int output_prefix)
>
> Okay, so you've essentially patched show_file() to accept an
> additional argument, and modified callers to call with this additional
> argument. I suppose
> show_(rev|reference|default|flag|rev|with_type|datestring|abbrev)
> don't need to be patched, as they are path-independent.
>
> > {
> > show_default();
> > if ((filter & (DO_NONFLAGS|DO_NOREV)) == (DO_NONFLAGS|DO_NOREV)) {
> > - show(arg);
> > + if (output_prefix) {
> > + const char *prefix = startup_info->prefix;
> > + show(prefix_filename(prefix,
> > + prefix ? strlen(prefix) : 0,
> > + arg));
> > + } else
> > + show(arg);
>
> Uh, why do you need output_prefix? If startup_info->prefix is set,
> use it. Is startup_info->prefix set by anyone by cmd_rev_parse()?output_prefix is a flag to say "do we want to show the prefix". We need it because show_file is used for the "--" argument separator as well as file paths. Without a separate flag we end up prefixing "--" with the prefix path.
Show 19 quoted lines
> > @@ -470,6 +476,7 @@ N_("git rev-parse --parseopt [options] -- [<args>...]\n"
> > int cmd_rev_parse(int argc, const char **argv, const char *prefix)
> > @@ -535,6 +542,13 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
> > i++;
> > continue;
> > }
> > + if (!strcmp(arg, "--prefix")) {
> > + prefix = argv[i+1];
> > + startup_info->prefix = prefix;
> > + output_prefix = 1;
> > + i++;
> > + continue;
> > + }
>
> Wait, why isn't prefix filled in when run_builtin() calls this? Oh,
> right: because we didn't mark this builtin with RUN_SETUP or
> RUN_SETUP_GENTLY. Okay, now why didn't we change that? Because it
> would be a major problem (all our scripts would break) if rev-parse
> did cd-to-toplevel.prefix is already set, by setup_git_git_directory. The point is that we just change the values set in setup_git_directory so that the command behaves as if it were run from a subdirectory.
Show 14 quoted lines
> Why are you setting prefix to argv[i+1], and then setting > startup_info->prefix to that? Is anyone else in cmd_rev_parse() going > to use it? > > > +prefix=$(git rev-parse --show-prefix) > > +cd "$(git rev-parse --show-toplevel)" > > +eval "set -- $(git rev-parse --sq --prefix "$prefix" "$@")" > > I'm wondering if you need such a convoluted usage though. Will you > ever need to specify a prefix by hand that is different from what git > rev-parse --show-toplevel returns? If not, why don't you just > rev-parse --emulate-toplevel, and get rid of specifying prefix by hand > altogether? Then again, this is a plumbing command, so the simplicity > is probably more valuable.
How does that work? When we run rev-parse with the --prefix argument we're no longer in the subdirectory.
While this may look convoluted here, I don't think it is in normal usage inside a script. If you look at the way it's used in patch 2 we're careful not to just remap all the arguments but to extract the flags before remapping file paths when we know that everything we have is a file path.
Show 17 quoted lines
> > diff --git a/t/t1513-rev-parse-prefix.sh b/t/t1513-rev-parse-prefix.sh > > new file mode 100755 > > index 0000000..5ef48d2 > > --- /dev/null > > +++ b/t/t1513-rev-parse-prefix.sh > > +test_expect_success 'empty prefix -- file' ' > > + git rev-parse --prefix "" -- top sub1/file1 >actual && > > + cat <<-EOF >expected && > > Nit: when you're not putting in variables, you can cat <<-\EOF. > > > +test_expect_success 'empty prefix HEAD:./path' ' > > + git rev-parse --prefix "" HEAD:./top >actual && > > + git rev-parse HEAD:top >expected && > > Nit: why did you change "./top" to "top"? Your --prefix option > doesn't require you to change your arguments accordingly, does it?
The point is to show that the case where a prefix is applied ("HEAD:./top") is the same as the canonical form ("HEAD:top").
I should probably add a test for "HEAD:top" with a prefix to ensure that we don't modify arguments like that.