Re: [PATCH v2 1/2] rev-parse: add --filename-prefix option
- From
Ramkumar Ramachandra <artagnon@gmail.com>
- Date
- Apr 18, 2013, 14:28 UTC
- Message-ID
- <CALkWK0nir7pJF-7YLRQF0jCR4dbb-JNBheD-4zKeQR0K9F9nZg@mail.gmail.com>
- In-Reply-To
- <0d570e110dbf714310f9cbc4fa47e711630707f2.1365539059.git.john@keeping.me.uk>
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?
Show 10 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 11 quoted lines
> {
> 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()?
Show 13 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.
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.
Show 8 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?