Re: [PATCH v2 1/2] rev-parse: add --filename-prefix option
On Thu, Apr 18, 2013 at 07:58:25PM +0530, Ramkumar Ramachandra wrote:
> 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.
> > 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.
> > @@ -470,6 +476,7 @@ N_("git rev-parse --parseopt [options] -- [...]\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.
> 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.
> > diff --git a/t/t1513-rev-parse-prefix.sh b/t/t1513-rev-parse-prefix.sh
> > new file mode 100755
> > index 000..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 &&
> > +
Re: [PATCH v2 1/2] rev-parse: add --filename-prefix option
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?
> 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()?
> @@ -470,6 +476,7 @@ N_("git rev-parse --parseopt [options] -- [...]\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.
> diff --git a/t/t1513-rev-parse-prefix.sh b/t/t1513-rev-parse-prefix.sh
> new file mode 100755
> index 000..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?
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/2] rev-parse: add --filename-prefix option
John Keeping writes: > It's not guessing on all of "$@" in git-submodule - we know that > everything left is a path. OK, then. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/2] rev-parse: add --filename-prefix option
On Tue, Apr 09, 2013 at 01:57:21PM -0700, Junio C Hamano wrote: > John Keeping writes: > > > This adds a prefix string to any filename arguments encountered after it > > has been specified. > > > > Signed-off-by: John Keeping > > --- > > Stale subject? Yep. Sorry. > > +--prefix :: > > + Behave as if 'git rev-parse' was invoked from the `` > > + subdirectory of the working tree. Any relative filenames are > > + resolved as if they are prefixed by `` and will be printed > > + in that form. > > ++ > > +This can be used to convert arguments to a command run in a subdirectory > > +so that they can still be used after moving to the top-level of the > > +repository. For example: > > ++ > > + > > +prefix=$(git rev-parse --show-prefix) > > +cd "$(git rev-parse --show-toplevel)" > > +eval "set -- $(git rev-parse --sq --prefix "$prefix" "$@")" > > I think you should tighten rev-parse parameter to reject options and > revisions, especially as an example to teach how to use it. When > the user said > > git mylog -U20 master..next -- README > > inside Documentation/ directory, "git-mylog" script would want to > see README prefixed with Documentation/ but want to see -U20 or > master..next untouched. And it will if it runs: git rev-parse --prefix Documentation/ -U20 master..next -- README Which gives: -U20 f131fb6eb2a1e09f7ce53d148e21ce6960f42422 ^fa7285dc3dce8bd01fd8c665b032603ed55348e5 -- Documentation/README > Historically, rev-parse was a way to sift > options and args meant for rev-list from those mant for diff-tree > so that a variant of > > git rev-list $(git rev-parse --revs) "$@" | > git diff-tree --stdin $(git rev-parse --no-revs) > > can be used to implement such "git mylog" script. > > I think "--no-revs --no-flags" is how you ask it to give you only > the paths, but I am writing from memory so please double check. > > Having said all that. > > Existing scripts (e.g. "git am") do this kind of things without such > an option added to rev-parse. They first do: > > prefix=$(git rev-parse --show-prefix) > > and refer to "$prefix/$1", "$prefix/$2", etc., I think. > > Is this option really necessary to update "git submodule"? Don't we > have a much better idea which parameter holds user-supplied path in the > script than having rev-parse make a guess on the entire "$@"? It's not guessing on all of "$@" in git-submodule - we know that everything left is a path. I've looked at git-am and hadn't thought of doing that, just thought this was a reasonably elegant way of processing the arguments. I'm happy to try another approach if that's going to be more acceptable. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/2] rev-parse: add --filename-prefix option
John Keeping writes: > This adds a prefix string to any filename arguments encountered after it > has been specified. > > Signed-off-by: John Keeping > --- Stale subject? > +--prefix :: > + Behave as if 'git rev-parse' was invoked from the `` > + subdirectory of the working tree. Any relative filenames are > + resolved as if they are prefixed by `` and will be printed > + in that form. > ++ > +This can be used to convert arguments to a command run in a subdirectory > +so that they can still be used after moving to the top-level of the > +repository. For example: > ++ > + > +prefix=$(git rev-parse --show-prefix) > +cd "$(git rev-parse --show-toplevel)" > +eval "set -- $(git rev-parse --sq --prefix "$prefix" "$@")" I think you should tighten rev-parse parameter to reject options and revisions, especially as an example to teach how to use it. When the user said git mylog -U20 master..next -- README inside Documentation/ directory, "git-mylog" script would want to see README prefixed with Documentation/ but want to see -U20 or master..next untouched. Historically, rev-parse was a way to sift options and args meant for rev-list from those mant for diff-tree so that a variant of git rev-list $(git rev-parse --revs) "$@" | git diff-tree --stdin $(git rev-parse --no-revs) can be used to implement such "git mylog" script. I think "--no-revs --no-flags" is how you ask it to give you only the paths, but I am writing from memory so please double check. Having said all that. Existing scripts (e.g. "git am") do this kind of things without such an option added to rev-parse. They first do: prefix=$(git rev-parse --show-prefix) and refer to "$prefix/$1", "$prefix/$2", etc., I think. Is this option really necessary to update "git submodule"? Don't we have a much better idea which parameter holds user-supplied path in the script than having rev-parse make a guess on the entire "$@"? -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 1/2] rev-parse: add --filename-prefix option
This adds a prefix string to any filename arguments encountered after it
has been specified.
Signed-off-by: John Keeping
---
Documentation/git-rev-parse.txt | 16
builtin/rev-parse.c | 24 ---
t/t1513-rev-parse-prefix.sh | 90 +
3 files changed, 125 insertions(+), 5 deletions(-)
create mode 100755 t/t1513-rev-parse-prefix.sh
diff --git a/Documentation/git-rev-parse.txt b/Documentation/git-rev-parse.txt
index 1f9ed6c..0ab2b77 100644
--- a/Documentation/git-rev-parse.txt
+++ b/Documentation/git-rev-parse.txt
@@ -59,6 +59,22 @@ OPTIONS
If there is no parameter given by the user, use ``
instead.
+--prefix ::
+ Behave as if 'git rev-parse' was invoked from the ``
+ subdirectory of the working tree. Any relative filenames are
+ resolved as if they are prefixed by `` and will be printed
+ in that form.
++
+This can be used to convert arguments to a command run in a subdirectory
+so that they can still be used after moving to the top-level of the
+repository. For example:
++
+
+prefix=$(git rev-parse --show-prefix)
+cd "$(git rev-parse --show-toplevel)"
+eval "set -- $(git rev-parse --sq --prefix "$prefix" "$@")"
+
+
--verify::
Verify that exactly one parameter is provided, and that it
can be turned into a raw 20-byte SHA-1 that can be used to
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)
{
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);
return 1;
}
return 0;
@@ -470,6 +476,7 @@ N_("git rev-parse --parseopt [options] -- [...]\n"
int cmd_rev_parse(int argc, const char **argv, const char *prefix)
{
int i, as_is = 0, verify = 0, quiet = 0, revs_count = 0, type = 0;
+ int output_prefix = 0;
unsigned char sha1[20];
const char *name = NULL;
@@ -503,7 +510,7 @@ int cmd_rev_parse(int argc, const char **argv, const char
*prefix)
const char *arg = argv[i];
if (as_is) {
- if (show_file(arg) && as_is < 2)
+ if (show_file(arg, output_prefix) && as_is < 2)
verify_filename(prefix, arg, 0);
continue;
}
@@ -527,7 +534,7 @@ int cmd_rev_parse(int argc, const char **argv, const char
*prefix)
as_is = 2;
/* Pass on the "--" if we show anything but
files.. */
if (filter & (DO_FLAGS | DO_REVS))
- show_file(arg);
+ show_file(arg, 0);
continue;
}
if (!strcmp(arg, "--default")) {
@@ -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;
+ }
if (!strcmp(arg, "--revs-only")) {
filter &= ~DO_NOREV;
continue;
@@ -754,7 +768,7 @@ int cmd_rev_parse(int argc, const char **argv, const char
*prefix)
if (verify)
die_no_single_rev(quiet);
as_is = 1;
- if (!show_file(arg))
+ if (!show_file(arg, output_prefix))
continue;
verify_filename(prefix, arg, 1);
}
diff --git a/t/t1513-rev-parse-prefix.sh b/t/t1513-rev-parse-prefix.sh
new file mode 100755
index 000..5ef48d2
--- /dev/null
+++ b/t/t1513-rev-parse-prefix.sh
@@ -0,0 +1,90 @@
+#!/bin/sh
+
+test_description='Tests for rev-parse --prefix'
+
+. ./test-lib.sh
+
+test_expect_success 'setup' '
+ mkdir -p sub1/sub2 &&
+ echo top >top &&
+ echo file1 >sub1/file1 &&
+ echo file2 >sub1/sub2/file2 &&
