[RFC/PATCH 0/2] submodule: drop the top-level requirement

[RFC/PATCH 0/2] submodule: drop the top-level requirement

[John Keeping]

With the recent discussion, I wondered how hard it would be to add
SUBDIRECTORY_OK=Yes to git-submodule.sh and it doesn't seem that bad.

Note that this series currently lacks both tests and documentation
updates.  Also I have made no attempt to change the output from any
commands, so submodule paths are always printed relative to the
top-level of the repository.

Consider this series a proof-of-concept, to see whether I'm on
completely the wrong course.

John Keeping (2):
  rev-parse: add --filename-prefix option
  submodule: drop the top-level requirement

 builtin/rev-parse.c | 17 ++++++++++++-----
 git-submodule.sh    |  7 +++++++
 2 files changed, 19 insertions(+), 5 deletions(-)

-- 
1.8.2.694.ga76e9c3.dirty

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

[John Keeping]

This adds a prefix string to any filename arguments encountered after it
has been specified.

Signed-off-by: John Keeping <john@keeping.me.uk>
---
 builtin/rev-parse.c | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
index f267a1d..11501a4 100644
--- a/builtin/rev-parse.c
+++ b/builtin/rev-parse.c
@@ -212,11 +212,12 @@ 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, const char *prefix)
 {
 	show_default();
 	if ((filter & (DO_NONFLAGS|DO_NOREV)) == (DO_NONFLAGS|DO_NOREV)) {
-		show(arg);
+		size_t prefixlen = prefix ? strlen(prefix) : 0;
+		show(prefix_filename(prefix, prefixlen, arg));
 		return 1;
 	}
 	return 0;
@@ -470,6 +471,7 @@ N_("git rev-parse --parseopt [options] -- [<args>...]\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;
+	const char *filename_prefix = NULL;
 	unsigned char sha1[20];
 	const char *name = NULL;
 
@@ -503,7 +505,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, filename_prefix) && as_is < 2)
 				verify_filename(prefix, arg, 0);
 			continue;
 		}
@@ -527,7 +529,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, NULL);
 				continue;
 			}
 			if (!strcmp(arg, "--default")) {
@@ -535,6 +537,11 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
 				i++;
 				continue;
 			}
+			if (!strcmp(arg, "--filename-prefix")) {
+				filename_prefix = argv[i+1];
+				i++;
+				continue;
+			}
 			if (!strcmp(arg, "--revs-only")) {
 				filter &= ~DO_NOREV;
 				continue;
@@ -754,7 +761,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, filename_prefix))
 			continue;
 		verify_filename(prefix, arg, 1);
 	}
-- 
1.8.2.694.ga76e9c3.dirty

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

[Jonathan Nieder]

Hi,

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.

How does this interact with the following options?

 * --resolve-git-dir some/relative/path
 * master:./path

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/. :)

Hope that helps,
Jonathan

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

[John Keeping]

On Sun, Apr 07, 2013 at 03:14:58PM -0700, Jonathan Nieder wrote:
> 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.

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

[Junio C Hamano]

John Keeping <john@keeping.me.uk> writes:

> 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.

This is very sensible.

> 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)"

I am not sure in what situation it makes sense to do this.  Where
does "technical/" come from?  For a script that wants to work from a
subdirectory, end-user input would come in the form relative to the
current directory, i.e. "Documentation/" from the top-level, so...

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

[John Keeping]

On Mon, Apr 08, 2013 at 08:07:32AM -0700, Junio C Hamano wrote:
> John Keeping <john@keeping.me.uk> writes:
> 
> > 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.
> 
> This is very sensible.

Which bit specifically?  I assume you agree with the intended usage, but
do you also mean that resetting the prefix returned from
setup_git_directory is the right way to approach this?

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

[Junio C Hamano]

John Keeping <john@keeping.me.uk> writes:

> On Mon, Apr 08, 2013 at 08:07:32AM -0700, Junio C Hamano wrote:
>> John Keeping <john@keeping.me.uk> writes:
>> 
>> > 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.
>> 
>> This is very sensible.
>
> Which bit specifically?  I assume you agree with the intended usage, but
> do you also mean that resetting the prefix returned from
> setup_git_directory is the right way to approach this?

My gut feeling says yes, but you can persuade me easily why it is a
bad idea if you have an example of why it would not work well.

[PATCH 2/2] submodule: drop the top-level requirement

[John Keeping]

Use the new rev-parse --filename-prefix option to process all paths
given to the submodule command, dropping the requirement that it be run
from the top-level of the repository.

Signed-off-by: John Keeping <john@keeping.me.uk>
---
 git-submodule.sh | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/git-submodule.sh b/git-submodule.sh
index 79bfaac..10216aa 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -14,10 +14,13 @@ USAGE="[--quiet] add [-b <branch>] [-f|--force] [--name <name>] [--reference <re
    or: $dashless [--quiet] foreach [--recursive] <command>
    or: $dashless [--quiet] sync [--recursive] [--] [<path>...]"
 OPTIONS_SPEC=
+SUBDIRECTORY_OK=Yes
 . git-sh-setup
 . git-sh-i18n
 . git-parse-remote
 require_work_tree
+wt_prefix=$(git rev-parse --show-prefix)
+cd_to_toplevel
 
 command=
 branch=
@@ -112,6 +115,7 @@ resolve_relative_url ()
 #
 module_list()
 {
+	eval "set $(git rev-parse --sq --filename-prefix "$wt_prefix" -- "$@")"
 	(
 		git ls-files --error-unmatch --stage -- "$@" ||
 		echo "unmatched pathspec exists"
@@ -335,6 +339,8 @@ cmd_add()
 		usage
 	fi
 
+	sm_path="$wt_prefix$sm_path"
+
 	# assure repo is absolute or relative to parent
 	case "$repo" in
 	./*|../*)
@@ -942,6 +948,7 @@ cmd_summary() {
 	fi
 
 	cd_to_toplevel
+	eval "set $(git rev-parse --sq --filename-prefix "$wt_prefix" -- "$@")"
 	# Get modified modules cared by user
 	modules=$(git $diff_cmd $cached --ignore-submodules=dirty --raw $head -- "$@" |
 		sane_egrep '^:([0-7]* )?160000' |
-- 
1.8.2.694.ga76e9c3.dirty

Re: [RFC/PATCH 0/2] submodule: drop the top-level requirement

[Jens Lehmann]

Am 07.04.2013 21:55, schrieb John Keeping:
> With the recent discussion, I wondered how hard it would be to add
> SUBDIRECTORY_OK=Yes to git-submodule.sh and it doesn't seem that bad.
> 
> Note that this series currently lacks both tests and documentation
> updates.  Also I have made no attempt to change the output from any
> commands, so submodule paths are always printed relative to the
> top-level of the repository.
> 
> Consider this series a proof-of-concept, to see whether I'm on
> completely the wrong course.

Cool! Thanks for working on that, I'll take a deeper look in my next
Git timeslot.

> John Keeping (2):
>   rev-parse: add --filename-prefix option
>   submodule: drop the top-level requirement
> 
>  builtin/rev-parse.c | 17 ++++++++++++-----
>  git-submodule.sh    |  7 +++++++
>  2 files changed, 19 insertions(+), 5 deletions(-)
> 

[PATCH v2 0/2] submodule: drop the top-level requirement

[John Keeping]

Since version 1, patch 1 has been completely re-written using the
approach proposed by Jonathan and Junio.

Also, there's now a documentation update and some tests.

John Keeping (2):
  rev-parse: add --filename-prefix option
  submodule: drop the top-level requirement

 Documentation/git-rev-parse.txt | 16 ++++++++
 builtin/rev-parse.c             | 24 ++++++++---
 git-submodule.sh                |  7 ++++
 t/t1513-rev-parse-prefix.sh     | 90 +++++++++++++++++++++++++++++++++++++++++
 t/t7400-submodule-basic.sh      | 26 ++++++++++++
 t/t7401-submodule-summary.sh    |  9 +++++
 6 files changed, 167 insertions(+), 5 deletions(-)
 create mode 100755 t/t1513-rev-parse-prefix.sh

-- 
1.8.2.694.ga76e9c3.dirty

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

[John Keeping]

This adds a prefix string to any filename arguments encountered after it
has been specified.

Signed-off-by: John Keeping <john@keeping.me.uk>
---
 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 `<arg>`
 	instead.
 
+--prefix <arg>::
+	Behave as if 'git rev-parse' was invoked from the `<arg>`
+	subdirectory of the working tree.  Any relative filenames are
+	resolved as if they are prefixed by `<arg>` 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] -- [<args>...]\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 0000000..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 &&
+	git add top sub1/file1 sub1/sub2/file2 &&
+	git commit -m commit
+'
+
+test_expect_success 'empty prefix -- file' '
+	git rev-parse --prefix "" -- top sub1/file1 >actual &&
+	cat <<-EOF >expected &&
+	--
+	top
+	sub1/file1
+	EOF
+	test_cmp expected actual
+'
+
+test_expect_success 'valid prefix -- file' '
+	git rev-parse --prefix sub1/ -- file1 sub2/file2 >actual &&
+	cat <<-EOF >expected &&
+	--
+	sub1/file1
+	sub1/sub2/file2
+	EOF
+	test_cmp expected actual
+'
+
+test_expect_success 'valid prefix -- ../file' '
+	git rev-parse --prefix sub1/ -- ../top sub2/file2 >actual &&
+	cat <<-EOF >expected &&
+	--
+	sub1/../top
+	sub1/sub2/file2
+	EOF
+	test_cmp expected actual
+'
+
+test_expect_success 'empty prefix HEAD:./path' '
+	git rev-parse --prefix "" HEAD:./top >actual &&
+	git rev-parse HEAD:top >expected &&
+	test_cmp expected actual
+'
+
+test_expect_success 'valid prefix HEAD:./path' '
+	git rev-parse --prefix sub1/ HEAD:./file1 >actual &&
+	git rev-parse HEAD:sub1/file1 >expected &&
+	test_cmp expected actual
+'
+
+test_expect_success 'valid prefix HEAD:../path' '
+	git rev-parse --prefix sub1/ HEAD:../top >actual &&
+	git rev-parse HEAD:top >expected &&
+	test_cmp expected actual
+'
+
+test_expect_success 'disambiguate path with valid prefix' '
+	git rev-parse --prefix sub1/ file1 >actual &&
+	cat <<-EOF >expected &&
+	sub1/file1
+	EOF
+	test_cmp expected actual
+'
+
+test_expect_success 'file and refs with prefix' '
+	git rev-parse --prefix sub1/ master file1 >actual &&
+	cat <<-EOF >expected &&
+	$(git rev-parse master)
+	sub1/file1
+	EOF
+	test_cmp expected actual
+'
+
+test_expect_success 'two-levels deep' '
+	git rev-parse --prefix sub1/sub2/ -- file2 >actual &&
+	cat <<-EOF >expected &&
+	--
+	sub1/sub2/file2
+	EOF
+	test_cmp expected actual
+'
+
+test_done
-- 
1.8.2.694.ga76e9c3.dirty

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

[Junio C Hamano]

John Keeping <john@keeping.me.uk> writes:

> This adds a prefix string to any filename arguments encountered after it
> has been specified.
>
> Signed-off-by: John Keeping <john@keeping.me.uk>
> ---

Stale subject?

> +--prefix <arg>::
> +	Behave as if 'git rev-parse' was invoked from the `<arg>`
> +	subdirectory of the working tree.  Any relative filenames are
> +	resolved as if they are prefixed by `<arg>` 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 "$@"?

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

[John Keeping]

On Tue, Apr 09, 2013 at 01:57:21PM -0700, Junio C Hamano wrote:
> John Keeping <john@keeping.me.uk> writes:
> 
> > This adds a prefix string to any filename arguments encountered after it
> > has been specified.
> >
> > Signed-off-by: John Keeping <john@keeping.me.uk>
> > ---
> 
> Stale subject?

Yep.  Sorry.

> > +--prefix <arg>::
> > +	Behave as if 'git rev-parse' was invoked from the `<arg>`
> > +	subdirectory of the working tree.  Any relative filenames are
> > +	resolved as if they are prefixed by `<arg>` 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.

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

[Junio C Hamano]

John Keeping <john@keeping.me.uk> writes:

> It's not guessing on all of "$@" in git-submodule - we know that
> everything left is a path.

OK, then.

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

[Ramkumar Ramachandra]

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] -- [<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.

> 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?

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

[John Keeping]

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] -- [<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.

> 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 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.

[PATCH v2 2/2] submodule: drop the top-level requirement

[John Keeping]

Use the new rev-parse --prefix option to process all paths given to the
submodule command, dropping the requirement that it be run from the
top-level of the repository.

Signed-off-by: John Keeping <john@keeping.me.uk>
---
 git-submodule.sh             |  7 +++++++
 t/t7400-submodule-basic.sh   | 26 ++++++++++++++++++++++++++
 t/t7401-submodule-summary.sh |  9 +++++++++
 3 files changed, 42 insertions(+)

diff --git a/git-submodule.sh b/git-submodule.sh
index 79bfaac..bbf7983 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -14,10 +14,13 @@ USAGE="[--quiet] add [-b <branch>] [-f|--force] [--name <name>] [--reference <re
    or: $dashless [--quiet] foreach [--recursive] <command>
    or: $dashless [--quiet] sync [--recursive] [--] [<path>...]"
 OPTIONS_SPEC=
+SUBDIRECTORY_OK=Yes
 . git-sh-setup
 . git-sh-i18n
 . git-parse-remote
 require_work_tree
+wt_prefix=$(git rev-parse --show-prefix)
+cd_to_toplevel
 
 command=
 branch=
@@ -112,6 +115,7 @@ resolve_relative_url ()
 #
 module_list()
 {
+	eval "set $(git rev-parse --sq --prefix "$wt_prefix" -- "$@")"
 	(
 		git ls-files --error-unmatch --stage -- "$@" ||
 		echo "unmatched pathspec exists"
@@ -335,6 +339,8 @@ cmd_add()
 		usage
 	fi
 
+	sm_path="$wt_prefix$sm_path"
+
 	# assure repo is absolute or relative to parent
 	case "$repo" in
 	./*|../*)
@@ -942,6 +948,7 @@ cmd_summary() {
 	fi
 
 	cd_to_toplevel
+	eval "set $(git rev-parse --sq --prefix "$wt_prefix" -- "$@")"
 	# Get modified modules cared by user
 	modules=$(git $diff_cmd $cached --ignore-submodules=dirty --raw $head -- "$@" |
 		sane_egrep '^:([0-7]* )?160000' |
diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
index ff26535..7795f21 100755
--- a/t/t7400-submodule-basic.sh
+++ b/t/t7400-submodule-basic.sh
@@ -212,6 +212,23 @@ test_expect_success 'submodule add with ./, /.. and // in path' '
 	test_cmp empty untracked
 '
 
+test_expect_success 'submodule add in subdir' '
+	echo "refs/heads/master" >expect &&
+	>empty &&
+	(
+		mkdir addtest/sub &&
+		cd addtest/sub &&
+		git submodule add "$submodurl" ../realsubmod3 &&
+		git submodule init
+	) &&
+
+	rm -f heads head untracked &&
+	inspect addtest/realsubmod3 ../.. &&
+	test_cmp expect heads &&
+	test_cmp expect head &&
+	test_cmp empty untracked
+'
+
 test_expect_success 'setup - add an example entry to .gitmodules' '
 	GIT_CONFIG=.gitmodules \
 	git config submodule.example.url git://example.com/init.git
@@ -319,6 +336,15 @@ test_expect_success 'status should be "up-to-date" after update' '
 	grep "^ $rev1" list
 '
 
+test_expect_success 'status works correctly from a subdirectory' '
+	mkdir sub &&
+	(
+		cd sub &&
+		git submodule status >../list
+	) &&
+	grep "^ $rev1" list
+'
+
 test_expect_success 'status should be "modified" after submodule commit' '
 	(
 		cd init &&
diff --git a/t/t7401-submodule-summary.sh b/t/t7401-submodule-summary.sh
index 30b429e..8f5c1e0 100755
--- a/t/t7401-submodule-summary.sh
+++ b/t/t7401-submodule-summary.sh
@@ -45,6 +45,15 @@ EOF
 	test_cmp expected actual
 "
 
+test_expect_success 'run summary from subdir' '
+	mkdir sub &&
+	(
+		cd sub &&
+		git submodule summary >../actual
+	) &&
+	test_cmp expected actual
+'
+
 commit_file sm1 &&
 head2=$(add_file sm1 foo3)
 
-- 
1.8.2.694.ga76e9c3.dirty

Re: [PATCH v2 2/2] submodule: drop the top-level requirement

[Junio C Hamano]

John Keeping <john@keeping.me.uk> writes:

> +	eval "set $(git rev-parse --sq --prefix "$wt_prefix" -- "$@")"

This may be handier than having to do the "for arg" loop git-am uses
yourself.

>  	(
>  		git ls-files --error-unmatch --stage -- "$@" ||
>  		echo "unmatched pathspec exists"
> @@ -335,6 +339,8 @@ cmd_add()
>  		usage
>  	fi
>  
> +	sm_path="$wt_prefix$sm_path"

But this is doing fine without "rev-parse --prefix" at all.

Re: [PATCH v2 2/2] submodule: drop the top-level requirement

[John Keeping]

On Tue, Apr 09, 2013 at 02:00:52PM -0700, Junio C Hamano wrote:
> John Keeping <john@keeping.me.uk> writes:
> 
> > +	eval "set $(git rev-parse --sq --prefix "$wt_prefix" -- "$@")"
> 
> This may be handier than having to do the "for arg" loop git-am uses
> yourself.
> 
> >  	(
> >  		git ls-files --error-unmatch --stage -- "$@" ||
> >  		echo "unmatched pathspec exists"
> > @@ -335,6 +339,8 @@ cmd_add()
> >  		usage
> >  	fi
> >  
> > +	sm_path="$wt_prefix$sm_path"
> 
> But this is doing fine without "rev-parse --prefix" at all.

In this case we only have a single argument (and it must have a value).
In the cases using "rev-parse --prefix" we can have any number of
arguments (including zero).

Re: [PATCH v2 2/2] submodule: drop the top-level requirement

[Ramkumar Ramachandra]

John Keeping wrote:
> Use the new rev-parse --prefix option to process all paths given to the
> submodule command, dropping the requirement that it be run from the
> top-level of the repository.

Yay!

> diff --git a/git-submodule.sh b/git-submodule.sh
> index 79bfaac..bbf7983 100755
> --- a/git-submodule.sh
> +++ b/git-submodule.sh
> @@ -112,6 +115,7 @@ resolve_relative_url ()
>  #
>  module_list()
>  {
> +       eval "set $(git rev-parse --sq --prefix "$wt_prefix" -- "$@")"

Nit: Why not use "--" to disambiguate between options and arguments,
like you showed in your intended usage?

So, this essentially converts all the paths specified in "$@" to
toplevel-relative paths.  Beautiful, as this is practically the only
change you require in each function.

> +       sm_path="$wt_prefix$sm_path"

Wait, isn't sm_path the value of submodule.<name>.path in .gitmodules?
 Why does it need to be prefixed?

> @@ -942,6 +948,7 @@ cmd_summary() {
>         fi
>
>         cd_to_toplevel
> +       eval "set $(git rev-parse --sq --prefix "$wt_prefix" -- "$@")"

Um, what about other functions?  Yeah, it looks like all of them
except this one call module_list().

> diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
> index ff26535..7795f21 100755
> --- a/t/t7400-submodule-basic.sh
> +++ b/t/t7400-submodule-basic.sh
> @@ -212,6 +212,23 @@ test_expect_success 'submodule add with ./, /.. and // in path' '
>         test_cmp empty untracked
>  '
>
> +test_expect_success 'submodule add in subdir' '
> +       echo "refs/heads/master" >expect &&
> +       >empty &&

Nit: Use : >empty.  It's clearer.

> +       (
> +               mkdir addtest/sub &&

Why is the mkdir inside the subshell?  The next statement (cd) onwards
should be in the subshell, to save you cd'ing back.

> +               cd addtest/sub &&
> +               git submodule add "$submodurl" ../realsubmod3 &&
> +               git submodule init
> +       ) &&
> +
> +       rm -f heads head untracked &&

Huh?  What is this in the middle?  The next statement (call to
inspect) write-truncates them anyway, so this is unnecessary.

Re: [PATCH v2 2/2] submodule: drop the top-level requirement

[John Keeping]

On Thu, Apr 18, 2013 at 08:16:42PM +0530, Ramkumar Ramachandra wrote:
> John Keeping wrote:
> > Use the new rev-parse --prefix option to process all paths given to the
> > submodule command, dropping the requirement that it be run from the
> > top-level of the repository.
> 
> Yay!
> 
> > diff --git a/git-submodule.sh b/git-submodule.sh
> > index 79bfaac..bbf7983 100755
> > --- a/git-submodule.sh
> > +++ b/git-submodule.sh
> > @@ -112,6 +115,7 @@ resolve_relative_url ()
> >  #
> >  module_list()
> >  {
> > +       eval "set $(git rev-parse --sq --prefix "$wt_prefix" -- "$@")"
> 
> Nit: Why not use "--" to disambiguate between options and arguments,
> like you showed in your intended usage?

Erm... it does.  What's before "$@"?

Ah, you mean after "set".  In this case, rev-parse will keep the "--" we
supply to it, so we get that from there and do not want to give set an
extra one.

> So, this essentially converts all the paths specified in "$@" to
> toplevel-relative paths.  Beautiful, as this is practically the only
> change you require in each function.
> 
> > +       sm_path="$wt_prefix$sm_path"
> 
> Wait, isn't sm_path the value of submodule.<name>.path in .gitmodules?
>  Why does it need to be prefixed?

I think you've lost too much context here.  This is in cmd_add and at
this point sm_path holds the argument supplied by the user, which we
must adjust as it will be relative to the current directory.

We should probably be testing for an absolute path here though.

> > @@ -942,6 +948,7 @@ cmd_summary() {
> >         fi
> >
> >         cd_to_toplevel
> > +       eval "set $(git rev-parse --sq --prefix "$wt_prefix" -- "$@")"
> 
> Um, what about other functions?  Yeah, it looks like all of them
> except this one call module_list().

Exactly.  Apart from this and cmd_add everything uses module_list.

> > diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
> > index ff26535..7795f21 100755
> > --- a/t/t7400-submodule-basic.sh
> > +++ b/t/t7400-submodule-basic.sh
> > @@ -212,6 +212,23 @@ test_expect_success 'submodule add with ./, /.. and // in path' '
> >         test_cmp empty untracked
> >  '
> >
> > +test_expect_success 'submodule add in subdir' '
> > +       echo "refs/heads/master" >expect &&
> > +       >empty &&
> 
> Nit: Use : >empty.  It's clearer.
> 
> > +       (
> > +               mkdir addtest/sub &&
> 
> Why is the mkdir inside the subshell?  The next statement (cd) onwards
> should be in the subshell, to save you cd'ing back.
>
> > +               cd addtest/sub &&
> > +               git submodule add "$submodurl" ../realsubmod3 &&
> > +               git submodule init
> > +       ) &&
> > +
> > +       rm -f heads head untracked &&
> 
> Huh?  What is this in the middle?  The next statement (call to
> inspect) write-truncates them anyway, so this is unnecessary.

I just followed the surrounding tests here.  I think it's better for
follow the pattern of the surrounding code here than get this one test
perfect.  A cleanup can follow on top if someone wants to do it.

[PATCH v3 0/2] submodule: drop the top-level requirement

[John Keeping]

Thanks to Ram and Jens for the feedback on v2.  I've addressed most of
their comments in this version, but I've ignored a couple of Ram's nits
about test cases where leaving it how it is makes the added tests
consistent with those already in the same file.

Since v2, the main improvement is that the output from git-submodule now
contains paths relative to the directory in which the command is run,
not the top-level of the working tree.

John Keeping (2):
  rev-parse: add --prefix option
  submodule: drop the top-level requirement

 Documentation/git-rev-parse.txt |  16 ++++++
 builtin/rev-parse.c             |  24 +++++++--
 git-submodule.sh                | 113 ++++++++++++++++++++++++++++++----------
 t/t1513-rev-parse-prefix.sh     |  96 ++++++++++++++++++++++++++++++++++
 t/t7400-submodule-basic.sh      |  27 ++++++++++
 t/t7401-submodule-summary.sh    |  14 +++++
 6 files changed, 258 insertions(+), 32 deletions(-)
 create mode 100755 t/t1513-rev-parse-prefix.sh

-- 
1.8.2.1.424.g1899e5e.dirty

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

[John Keeping]

This makes 'git rev-parse' behave as if it were invoked from the
specified subdirectory of a repository, with the difference that any
file paths which it prints are prefixed with the full path from the top
of the working tree.

This is useful for shell scripts where we may want to cd to the top of
the working tree but need to handle relative paths given by the user on
the command line.

Signed-off-by: John Keeping <john@keeping.me.uk>
---
Changes since v2:
    - Rewrite commit message
    - Add a new test for 'prefix ignored with HEAD:top'
    - Use '<<-\EOF' where appropriate in t1513

 Documentation/git-rev-parse.txt | 16 +++++++
 builtin/rev-parse.c             | 24 ++++++++---
 t/t1513-rev-parse-prefix.sh     | 96 +++++++++++++++++++++++++++++++++++++++++
 3 files changed, 131 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 `<arg>`
 	instead.
 
+--prefix <arg>::
+	Behave as if 'git rev-parse' was invoked from the `<arg>`
+	subdirectory of the working tree.  Any relative filenames are
+	resolved as if they are prefixed by `<arg>` 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] -- [<args>...]\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 0000000..87ec3ae
--- /dev/null
+++ b/t/t1513-rev-parse-prefix.sh
@@ -0,0 +1,96 @@
+#!/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 &&
+	git add top sub1/file1 sub1/sub2/file2 &&
+	git commit -m commit
+'
+
+test_expect_success 'empty prefix -- file' '
+	git rev-parse --prefix "" -- top sub1/file1 >actual &&
+	cat <<-\EOF >expected &&
+	--
+	top
+	sub1/file1
+	EOF
+	test_cmp expected actual
+'
+
+test_expect_success 'valid prefix -- file' '
+	git rev-parse --prefix sub1/ -- file1 sub2/file2 >actual &&
+	cat <<-\EOF >expected &&
+	--
+	sub1/file1
+	sub1/sub2/file2
+	EOF
+	test_cmp expected actual
+'
+
+test_expect_success 'valid prefix -- ../file' '
+	git rev-parse --prefix sub1/ -- ../top sub2/file2 >actual &&
+	cat <<-\EOF >expected &&
+	--
+	sub1/../top
+	sub1/sub2/file2
+	EOF
+	test_cmp expected actual
+'
+
+test_expect_success 'empty prefix HEAD:./path' '
+	git rev-parse --prefix "" HEAD:./top >actual &&
+	git rev-parse HEAD:top >expected &&
+	test_cmp expected actual
+'
+
+test_expect_success 'valid prefix HEAD:./path' '
+	git rev-parse --prefix sub1/ HEAD:./file1 >actual &&
+	git rev-parse HEAD:sub1/file1 >expected &&
+	test_cmp expected actual
+'
+
+test_expect_success 'valid prefix HEAD:../path' '
+	git rev-parse --prefix sub1/ HEAD:../top >actual &&
+	git rev-parse HEAD:top >expected &&
+	test_cmp expected actual
+'
+
+test_expect_success 'prefix ignored with HEAD:top' '
+	git rev-parse --prefix sub1/ HEAD:top >actual &&
+	git rev-parse HEAD:top >expected &&
+	test_cmp expected actual
+'
+
+test_expect_success 'disambiguate path with valid prefix' '
+	git rev-parse --prefix sub1/ file1 >actual &&
+	cat <<-\EOF >expected &&
+	sub1/file1
+	EOF
+	test_cmp expected actual
+'
+
+test_expect_success 'file and refs with prefix' '
+	git rev-parse --prefix sub1/ master file1 >actual &&
+	cat <<-EOF >expected &&
+	$(git rev-parse master)
+	sub1/file1
+	EOF
+	test_cmp expected actual
+'
+
+test_expect_success 'two-levels deep' '
+	git rev-parse --prefix sub1/sub2/ -- file2 >actual &&
+	cat <<-\EOF >expected &&
+	--
+	sub1/sub2/file2
+	EOF
+	test_cmp expected actual
+'
+
+test_done
-- 
1.8.2.1.424.g1899e5e.dirty

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

[Ramkumar Ramachandra]

John Keeping wrote:
> Changes since v2:
>     - Rewrite commit message
>     - Add a new test for 'prefix ignored with HEAD:top'
>     - Use '<<-\EOF' where appropriate in t1513

Thanks for the re-roll.

In the previous iteration, I wasn't sure this was the right approach
because I thought it would be better to pass RUN_SETUP and let
run_builtin_command() take care of the prefix-setting.  Unfortunately,
as 5410a02 (git-rev-parse --parseopt, 2007-11-06) indicates, we have
to run setup_git_directory() in cmd_rev_parse() after parsing
--parseopt, as 'git rev-parse --parseopt' can be run outside a git
repository.  You might want to include this note in your commit
message for the benefit of other readers.

Other than that, I just have one small suggestion: it's possible to
avoid passing output_prefix around, and simplify show_file() a bit if
we acknowledge that printing "--" is not the same as printing a file
(although the condition is the same).  Would you like to squash this
in?

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
-- 8< --
diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
index de894c7..7e69b3f 100644
--- a/builtin/rev-parse.c
+++ b/builtin/rev-parse.c
@@ -29,6 +29,7 @@ static int abbrev;
 static int abbrev_ref;
 static int abbrev_ref_strict;
 static int output_sq;
+static int output_prefix;
 
 /*
  * Some arguments are relevant "revision" arguments,
@@ -212,15 +213,13 @@ static void show_datestring(const char *flag, const char *datestr)
 	show(buffer);
 }
 
-static int show_file(const char *arg, int output_prefix)
+static int show_file(const char *arg)
 {
 	show_default();
 	if ((filter & (DO_NONFLAGS|DO_NOREV)) == (DO_NONFLAGS|DO_NOREV)) {
 		if (output_prefix) {
 			const char *prefix = startup_info->prefix;
-			show(prefix_filename(prefix,
-					     prefix ? strlen(prefix) : 0,
-					     arg));
+			show(prefix_filename(prefix, strlen(prefix), arg));
 		} else
 			show(arg);
 		return 1;
@@ -228,6 +227,16 @@ static int show_file(const char *arg, int output_prefix)
 	return 0;
 }
 
+static int show_dashdash()
+{
+	show_default();
+	if ((filter & (DO_NONFLAGS | DO_NOREV)) == (DO_NONFLAGS | DO_NOREV)) {
+		show("--");
+		return 1;
+	}
+	return 0;
+}
+
 static int try_difference(const char *arg)
 {
 	char *dotdot;
@@ -476,7 +485,6 @@ N_("git rev-parse --parseopt [options] -- [<args>...]\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;
 
@@ -510,7 +518,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, output_prefix) && as_is < 2)
+			if (show_file(arg) && as_is < 2)
 				verify_filename(prefix, arg, 0);
 			continue;
 		}
@@ -534,7 +542,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, 0);
+					show_dashdash();
 				continue;
 			}
 			if (!strcmp(arg, "--default")) {
@@ -543,7 +551,7 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
 				continue;
 			}
 			if (!strcmp(arg, "--prefix")) {
-				prefix = argv[i+1];
+				prefix = argv[i + 1];
 				startup_info->prefix = prefix;
 				output_prefix = 1;
 				i++;
@@ -768,7 +776,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, output_prefix))
+		if (!show_file(arg))
 			continue;
 		verify_filename(prefix, arg, 1);
 	}

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

[John Keeping]

On Fri, Apr 19, 2013 at 03:23:37PM +0530, Ramkumar Ramachandra wrote:
> John Keeping wrote:
> > Changes since v2:
> >     - Rewrite commit message
> >     - Add a new test for 'prefix ignored with HEAD:top'
> >     - Use '<<-\EOF' where appropriate in t1513
> 
> Thanks for the re-roll.
> 
> In the previous iteration, I wasn't sure this was the right approach
> because I thought it would be better to pass RUN_SETUP and let
> run_builtin_command() take care of the prefix-setting.  Unfortunately,
> as 5410a02 (git-rev-parse --parseopt, 2007-11-06) indicates, we have
> to run setup_git_directory() in cmd_rev_parse() after parsing
> --parseopt, as 'git rev-parse --parseopt' can be run outside a git
> repository.  You might want to include this note in your commit
> message for the benefit of other readers.
> 
> Other than that, I just have one small suggestion: it's possible to
> avoid passing output_prefix around, and simplify show_file() a bit if
> we acknowledge that printing "--" is not the same as printing a file
> (although the condition is the same).  Would you like to squash this
> in?

I'm not actually sure this is better.  I'm more afraid of the condition
for outputting files changing in the future than of passing
output_prefix around, so I'd much rather keep that condition in one
place.

If you really feel strongly about it, I'd prefer to extract the if
condition to a function and use that directly when deciding whether to
print "--".

[Also, you introduce a potential segfault via passing a NULL prefix to
strlen.]

> Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
> -- 8< --
> diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
> index de894c7..7e69b3f 100644
> --- a/builtin/rev-parse.c
> +++ b/builtin/rev-parse.c
> @@ -29,6 +29,7 @@ static int abbrev;
>  static int abbrev_ref;
>  static int abbrev_ref_strict;
>  static int output_sq;
> +static int output_prefix;
>  
>  /*
>   * Some arguments are relevant "revision" arguments,
> @@ -212,15 +213,13 @@ static void show_datestring(const char *flag, const char *datestr)
>  	show(buffer);
>  }
>  
> -static int show_file(const char *arg, int output_prefix)
> +static int show_file(const char *arg)
>  {
>  	show_default();
>  	if ((filter & (DO_NONFLAGS|DO_NOREV)) == (DO_NONFLAGS|DO_NOREV)) {
>  		if (output_prefix) {
>  			const char *prefix = startup_info->prefix;
> -			show(prefix_filename(prefix,
> -					     prefix ? strlen(prefix) : 0,
> -					     arg));
> +			show(prefix_filename(prefix, strlen(prefix), arg));
>  		} else
>  			show(arg);
>  		return 1;
> @@ -228,6 +227,16 @@ static int show_file(const char *arg, int output_prefix)
>  	return 0;
>  }
>  
> +static int show_dashdash()
> +{
> +	show_default();
> +	if ((filter & (DO_NONFLAGS | DO_NOREV)) == (DO_NONFLAGS | DO_NOREV)) {
> +		show("--");
> +		return 1;
> +	}
> +	return 0;
> +}
> +
>  static int try_difference(const char *arg)
>  {
>  	char *dotdot;
> @@ -476,7 +485,6 @@ N_("git rev-parse --parseopt [options] -- [<args>...]\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;
>  
> @@ -510,7 +518,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, output_prefix) && as_is < 2)
> +			if (show_file(arg) && as_is < 2)
>  				verify_filename(prefix, arg, 0);
>  			continue;
>  		}
> @@ -534,7 +542,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, 0);
> +					show_dashdash();
>  				continue;
>  			}
>  			if (!strcmp(arg, "--default")) {
> @@ -543,7 +551,7 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
>  				continue;
>  			}
>  			if (!strcmp(arg, "--prefix")) {
> -				prefix = argv[i+1];
> +				prefix = argv[i + 1];
>  				startup_info->prefix = prefix;
>  				output_prefix = 1;
>  				i++;
> @@ -768,7 +776,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, output_prefix))
> +		if (!show_file(arg))
>  			continue;
>  		verify_filename(prefix, arg, 1);
>  	}

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

[Ramkumar Ramachandra]

John Keeping wrote:
> I'm not actually sure this is better.  I'm more afraid of the condition
> for outputting files changing in the future than of passing
> output_prefix around, so I'd much rather keep that condition in one
> place.
>
> If you really feel strongly about it, I'd prefer to extract the if
> condition to a function and use that directly when deciding whether to
> print "--".

Yeah, it would probably make more sense to extract the conditional.  I
just thought it was unnecessary to pass the argument around, but feel
free to go either way on this one.

> [Also, you introduce a potential segfault via passing a NULL prefix to
> strlen.]

Isn't prefix guaranteed to be set by setup_git_directory()?  If I
wanted to check it nevertheless, I'd probably put in a die("internal
error") before this line.  Feel free to go either way though.

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

[John Keeping]

On Fri, Apr 19, 2013 at 04:45:38PM +0530, Ramkumar Ramachandra wrote:
> John Keeping wrote:
> > I'm not actually sure this is better.  I'm more afraid of the condition
> > for outputting files changing in the future than of passing
> > output_prefix around, so I'd much rather keep that condition in one
> > place.
> >
> > If you really feel strongly about it, I'd prefer to extract the if
> > condition to a function and use that directly when deciding whether to
> > print "--".
> 
> Yeah, it would probably make more sense to extract the conditional.  I
> just thought it was unnecessary to pass the argument around, but feel
> free to go either way on this one.
> 
> > [Also, you introduce a potential segfault via passing a NULL prefix to
> > strlen.]
> 
> Isn't prefix guaranteed to be set by setup_git_directory()?  If I
> wanted to check it nevertheless, I'd probably put in a die("internal
> error") before this line.  Feel free to go either way though.

It looks to me like setup_git_directory() returns NULL if we're at the
top of the working tree so we do need that check.  I suspect that's so
that "!prefix" does the right thing.

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

[Ramkumar Ramachandra]

John Keeping wrote:
> It looks to me like setup_git_directory() returns NULL if we're at the
> top of the working tree so we do need that check.  I suspect that's so
> that "!prefix" does the right thing.

Ah, yes.  My bad.  I expected it to return "/" on the toplevel
directory actually.

[PATCH v3 2/2] submodule: drop the top-level requirement

[John Keeping]

Use the new rev-parse --prefix option to process all paths given to the
submodule command, dropping the requirement that it be run from the
top-level of the repository.

Signed-off-by: John Keeping <john@keeping.me.uk>
---
Changes since v2:
    - Handle relative paths given to "submodule add --reference=./..."
    - Check for absolute path before prepending $wt_prefix in cmd_add
    - Export relative sm_path in cmd_foreach
    - Change displayed sm_path to be relative
    - Move a mkdir out of a subshell in t7400

I'm not sure if the relative_path function here is sufficient on
Windows.  We should be okay with the "target" argument since that comes
from git-ls-files but I think we're in trouble if "git rev-parse
--show-prefix" output the prefix with '\' instead of '/'.

 git-submodule.sh             | 113 ++++++++++++++++++++++++++++++++-----------
 t/t7400-submodule-basic.sh   |  27 +++++++++++
 t/t7401-submodule-summary.sh |  14 ++++++
 3 files changed, 127 insertions(+), 27 deletions(-)

diff --git a/git-submodule.sh b/git-submodule.sh
index 79bfaac..0eee703 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -14,10 +14,13 @@ USAGE="[--quiet] add [-b <branch>] [-f|--force] [--name <name>] [--reference <re
    or: $dashless [--quiet] foreach [--recursive] <command>
    or: $dashless [--quiet] sync [--recursive] [--] [<path>...]"
 OPTIONS_SPEC=
+SUBDIRECTORY_OK=Yes
 . git-sh-setup
 . git-sh-i18n
 . git-parse-remote
 require_work_tree
+wt_prefix=$(git rev-parse --show-prefix)
+cd_to_toplevel
 
 command=
 branch=
@@ -106,12 +109,48 @@ resolve_relative_url ()
 	echo "${is_relative:+${up_path}}${remoteurl#./}"
 }
 
+# Resolve a path to be relative to another path.  This is intended for
+# converting submodule paths when git-submodule is run in a subdirectory
+# and only handles paths where the directory separator is '/'.
+#
+# The output is the first argument as a path relative to the second argument,
+# which defaults to $wt_prefix if it is omitted.
+relative_path ()
+{
+	local target curdir result
+	target=$1
+	curdir=${2-$wt_prefix}
+	curdir=${curdir%/}
+	result=
+
+	while test -n "$curdir"
+	do
+		case "$target" in
+		"$curdir/"*)
+			target=${target#$curdir/}
+			break
+			;;
+		esac
+
+		result="${result}../"
+		if test "$curdir" = "${curdir%/*}"
+		then
+			curdir=
+		else
+			curdir="${curdir%/*}"
+		fi
+	done
+
+	echo "$result$target"
+}
+
 #
 # Get submodule info for registered submodules
 # $@ = path to limit submodule list
 #
 module_list()
 {
+	eval "set $(git rev-parse --sq --prefix "$wt_prefix" -- "$@")"
 	(
 		git ls-files --error-unmatch --stage -- "$@" ||
 		echo "unmatched pathspec exists"
@@ -282,6 +321,7 @@ isnumber()
 cmd_add()
 {
 	# parse $args after "submodule ... add".
+	reference_path=
 	while test $# -ne 0
 	do
 		case "$1" in
@@ -298,11 +338,11 @@ cmd_add()
 			;;
 		--reference)
 			case "$2" in '') usage ;; esac
-			reference="--reference=$2"
+			reference_path=$2
 			shift
 			;;
 		--reference=*)
-			reference="$1"
+			reference_path="${1#--reference=}"
 			;;
 		--name)
 			case "$2" in '') usage ;; esac
@@ -323,6 +363,14 @@ cmd_add()
 		shift
 	done
 
+	if test -n "$reference_path"
+	then
+		is_absolute_path "$reference_path" ||
+		reference_path="$wt_prefix$reference_path"
+
+		reference="--reference=$reference_path"
+	fi
+
 	repo=$1
 	sm_path=$2
 
@@ -335,6 +383,8 @@ cmd_add()
 		usage
 	fi
 
+	is_absolute_path "$sm_path" || sm_path="$wt_prefix$sm_path"
+
 	# assure repo is absolute or relative to parent
 	case "$repo" in
 	./*|../*)
@@ -479,6 +529,7 @@ cmd_foreach()
 				# we make $path available to scripts ...
 				path=$sm_path
 				cd "$sm_path" &&
+				sm_path=$(relative_path "$sm_path") &&
 				eval "$@" &&
 				if test -n "$recursive"
 				then
@@ -594,27 +645,29 @@ cmd_deinit()
 		die_if_unmatched "$mode"
 		name=$(module_name "$sm_path") || exit
 
+		displaypath=$(relative_path "$sm_path")
+
 		# Remove the submodule work tree (unless the user already did it)
 		if test -d "$sm_path"
 		then
 			# Protect submodules containing a .git directory
 			if test -d "$sm_path/.git"
 			then
-				echo >&2 "$(eval_gettext "Submodule work tree '\$sm_path' contains a .git directory")"
+				echo >&2 "$(eval_gettext "Submodule work tree '\$displaypath' contains a .git directory")"
 				die "$(eval_gettext "(use 'rm -rf' if you really want to remove it including all of its history)")"
 			fi
 
 			if test -z "$force"
 			then
 				git rm -qn "$sm_path" ||
-				die "$(eval_gettext "Submodule work tree '\$sm_path' contains local modifications; use '-f' to discard them")"
+				die "$(eval_gettext "Submodule work tree '\$displaypath' contains local modifications; use '-f' to discard them")"
 			fi
 			rm -rf "$sm_path" &&
-			say "$(eval_gettext "Cleared directory '\$sm_path'")" ||
-			say "$(eval_gettext "Could not remove submodule work tree '\$sm_path'")"
+			say "$(eval_gettext "Cleared directory '\$displaypath'")" ||
+			say "$(eval_gettext "Could not remove submodule work tree '\$displaypath'")"
 		fi
 
-		mkdir "$sm_path" || say "$(eval_gettext "Could not create empty submodule directory '\$sm_path'")"
+		mkdir "$sm_path" || say "$(eval_gettext "Could not create empty submodule directory '\$displaypath'")"
 
 		# Remove the .git/config entries (unless the user already did it)
 		if test -n "$(git config --get-regexp submodule."$name\.")"
@@ -623,7 +676,7 @@ cmd_deinit()
 			# the user later decides to init this submodule again
 			url=$(git config submodule."$name".url)
 			git config --remove-section submodule."$name" 2>/dev/null &&
-			say "$(eval_gettext "Submodule '\$name' (\$url) unregistered for path '\$sm_path'")"
+			say "$(eval_gettext "Submodule '\$name' (\$url) unregistered for path '\$displaypath'")"
 		fi
 	done
 }
@@ -717,9 +770,11 @@ cmd_update()
 			update_module=$(git config submodule."$name".update)
 		fi
 
+		displaypath=$(relative_path "$prefix$sm_path")
+
 		if test "$update_module" = "none"
 		then
-			echo "Skipping submodule '$prefix$sm_path'"
+			echo "Skipping submodule '$displaypath'"
 			continue
 		fi
 
@@ -728,7 +783,7 @@ cmd_update()
 			# Only mention uninitialized submodules when its
 			# path have been specified
 			test "$#" != "0" &&
-			say "$(eval_gettext "Submodule path '\$prefix\$sm_path' not initialized
+			say "$(eval_gettext "Submodule path '\$displaypath' not initialized
 Maybe you want to use 'update --init'?")"
 			continue
 		fi
@@ -741,7 +796,7 @@ Maybe you want to use 'update --init'?")"
 		else
 			subsha1=$(clear_local_git_env; cd "$sm_path" &&
 				git rev-parse --verify HEAD) ||
-			die "$(eval_gettext "Unable to find current revision in submodule path '\$prefix\$sm_path'")"
+			die "$(eval_gettext "Unable to find current revision in submodule path '\$displaypath'")"
 		fi
 
 		if test -n "$remote"
@@ -774,7 +829,7 @@ Maybe you want to use 'update --init'?")"
 				(clear_local_git_env; cd "$sm_path" &&
 					( (rev=$(git rev-list -n 1 $sha1 --not --all 2>/dev/null) &&
 					 test -z "$rev") || git-fetch)) ||
-				die "$(eval_gettext "Unable to fetch in submodule path '\$prefix\$sm_path'")"
+				die "$(eval_gettext "Unable to fetch in submodule path '\$displaypath'")"
 			fi
 
 			# Is this something we just cloned?
@@ -788,20 +843,20 @@ Maybe you want to use 'update --init'?")"
 			case "$update_module" in
 			rebase)
 				command="git rebase"
-				die_msg="$(eval_gettext "Unable to rebase '\$sha1' in submodule path '\$prefix\$sm_path'")"
-				say_msg="$(eval_gettext "Submodule path '\$prefix\$sm_path': rebased into '\$sha1'")"
+				die_msg="$(eval_gettext "Unable to rebase '\$sha1' in submodule path '\$displaypath'")"
+				say_msg="$(eval_gettext "Submodule path '\$displaypath': rebased into '\$sha1'")"
 				must_die_on_failure=yes
 				;;
 			merge)
 				command="git merge"
-				die_msg="$(eval_gettext "Unable to merge '\$sha1' in submodule path '\$prefix\$sm_path'")"
-				say_msg="$(eval_gettext "Submodule path '\$prefix\$sm_path': merged in '\$sha1'")"
+				die_msg="$(eval_gettext "Unable to merge '\$sha1' in submodule path '\$displaypath'")"
+				say_msg="$(eval_gettext "Submodule path '\$displaypath': merged in '\$sha1'")"
 				must_die_on_failure=yes
 				;;
 			*)
 				command="git checkout $subforce -q"
-				die_msg="$(eval_gettext "Unable to checkout '\$sha1' in submodule path '\$prefix\$sm_path'")"
-				say_msg="$(eval_gettext "Submodule path '\$prefix\$sm_path': checked out '\$sha1'")"
+				die_msg="$(eval_gettext "Unable to checkout '\$sha1' in submodule path '\$displaypath'")"
+				say_msg="$(eval_gettext "Submodule path '\$displaypath': checked out '\$sha1'")"
 				;;
 			esac
 
@@ -828,7 +883,7 @@ Maybe you want to use 'update --init'?")"
 			res=$?
 			if test $res -gt 0
 			then
-				die_msg="$(eval_gettext "Failed to recurse into submodule path '\$prefix\$sm_path'")"
+				die_msg="$(eval_gettext "Failed to recurse into submodule path '\$displaypath'")"
 				if test $res -eq 1
 				then
 					err="${err};$die_msg"
@@ -942,6 +997,7 @@ cmd_summary() {
 	fi
 
 	cd_to_toplevel
+	eval "set $(git rev-parse --sq --prefix "$wt_prefix" -- "$@")"
 	# Get modified modules cared by user
 	modules=$(git $diff_cmd $cached --ignore-submodules=dirty --raw $head -- "$@" |
 		sane_egrep '^:([0-7]* )?160000' |
@@ -991,16 +1047,18 @@ cmd_summary() {
 		! GIT_DIR="$name/.git" git-rev-parse -q --verify $sha1_dst^0 >/dev/null &&
 		missing_dst=t
 
+		display_name=$(relative_path "$name")
+
 		total_commits=
 		case "$missing_src,$missing_dst" in
 		t,)
-			errmsg="$(eval_gettext "  Warn: \$name doesn't contain commit \$sha1_src")"
+			errmsg="$(eval_gettext "  Warn: \$display_name doesn't contain commit \$sha1_src")"
 			;;
 		,t)
-			errmsg="$(eval_gettext "  Warn: \$name doesn't contain commit \$sha1_dst")"
+			errmsg="$(eval_gettext "  Warn: \$display_name doesn't contain commit \$sha1_dst")"
 			;;
 		t,t)
-			errmsg="$(eval_gettext "  Warn: \$name doesn't contain commits \$sha1_src and \$sha1_dst")"
+			errmsg="$(eval_gettext "  Warn: \$display_name doesn't contain commits \$sha1_src and \$sha1_dst")"
 			;;
 		*)
 			errmsg=
@@ -1029,12 +1087,12 @@ cmd_summary() {
 			submodule="$(gettext "submodule")"
 			if test $mod_dst = 160000
 			then
-				echo "* $name $sha1_abbr_src($blob)->$sha1_abbr_dst($submodule)$total_commits:"
+				echo "* $display_name $sha1_abbr_src($blob)->$sha1_abbr_dst($submodule)$total_commits:"
 			else
-				echo "* $name $sha1_abbr_src($submodule)->$sha1_abbr_dst($blob)$total_commits:"
+				echo "* $display_name $sha1_abbr_src($submodule)->$sha1_abbr_dst($blob)$total_commits:"
 			fi
 		else
-			echo "* $name $sha1_abbr_src...$sha1_abbr_dst$total_commits:"
+			echo "* $display_name $sha1_abbr_src...$sha1_abbr_dst$total_commits:"
 		fi
 		if test -n "$errmsg"
 		then
@@ -1118,7 +1176,7 @@ cmd_status()
 		die_if_unmatched "$mode"
 		name=$(module_name "$sm_path") || exit
 		url=$(git config submodule."$name".url)
-		displaypath="$prefix$sm_path"
+		displaypath=$(relative_path "$prefix$sm_path")
 		if test "$stage" = U
 		then
 			say "U$sha1 $displaypath"
@@ -1213,7 +1271,8 @@ cmd_sync()
 
 		if git config "submodule.$name.url" >/dev/null 2>/dev/null
 		then
-			say "$(eval_gettext "Synchronizing submodule url for '\$prefix\$sm_path'")"
+			displaypath=$(relative_path "$prefix$sm_path")
+			say "$(eval_gettext "Synchronizing submodule url for '\$displaypath'")"
 			git config submodule."$name".url "$super_config_url"
 
 			if test -e "$sm_path"/.git
diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
index ff26535..ca0a6ab 100755
--- a/t/t7400-submodule-basic.sh
+++ b/t/t7400-submodule-basic.sh
@@ -212,6 +212,24 @@ test_expect_success 'submodule add with ./, /.. and // in path' '
 	test_cmp empty untracked
 '
 
+test_expect_success 'submodule add in subdir' '
+	echo "refs/heads/master" >expect &&
+	>empty &&
+
+	mkdir addtest/sub &&
+	(
+		cd addtest/sub &&
+		git submodule add "$submodurl" ../realsubmod3 &&
+		git submodule init
+	) &&
+
+	rm -f heads head untracked &&
+	inspect addtest/realsubmod3 ../.. &&
+	test_cmp expect heads &&
+	test_cmp expect head &&
+	test_cmp empty untracked
+'
+
 test_expect_success 'setup - add an example entry to .gitmodules' '
 	GIT_CONFIG=.gitmodules \
 	git config submodule.example.url git://example.com/init.git
@@ -319,6 +337,15 @@ test_expect_success 'status should be "up-to-date" after update' '
 	grep "^ $rev1" list
 '
 
+test_expect_success 'status works correctly from a subdirectory' '
+	mkdir sub &&
+	(
+		cd sub &&
+		git submodule status >../list
+	) &&
+	grep "^ $rev1" list
+'
+
 test_expect_success 'status should be "modified" after submodule commit' '
 	(
 		cd init &&
diff --git a/t/t7401-submodule-summary.sh b/t/t7401-submodule-summary.sh
index 30b429e..992b66b 100755
--- a/t/t7401-submodule-summary.sh
+++ b/t/t7401-submodule-summary.sh
@@ -45,6 +45,20 @@ EOF
 	test_cmp expected actual
 "
 
+test_expect_success 'run summary from subdir' '
+	mkdir sub &&
+	(
+		cd sub &&
+		git submodule summary >../actual
+	) &&
+	cat >expected <<-EOF &&
+* ../sm1 0000000...$head1 (2):
+  > Add foo2
+
+EOF
+	test_cmp expected actual
+'
+
 commit_file sm1 &&
 head2=$(add_file sm1 foo3)
 
-- 
1.8.2.1.424.g1899e5e.dirty

Re: [PATCH v3 2/2] submodule: drop the top-level requirement

[Junio C Hamano]

John Keeping <john@keeping.me.uk> writes:

> +relative_path ()
> +{
> +	local target curdir result
> +	target=$1
> +	curdir=${2-$wt_prefix}
> +	curdir=${curdir%/}
> +	result=
> +
> +	while test -n "$curdir"
> +	do
> +		case "$target" in
> +		"$curdir/"*)
> +			target=${target#$curdir/}
> +			break
> +			;;
> +		esac

Could $curdir have glob wildcard to throw this part of the logic
off?  It is OK to have limitations like "you cannot have a glob
characters in your path to submodule working tree" (at least until
we start rewriting these in C or Perl or Python), but we need to be
aware of them.

>  module_list()
>  {
> +	eval "set $(git rev-parse --sq --prefix "$wt_prefix" -- "$@")"

An efficient reuse of "--" ;-)

> +test_expect_success 'run summary from subdir' '
> +	mkdir sub &&
> +	(
> +		cd sub &&
> +		git submodule summary >../actual
> +	) &&
> +	cat >expected <<-EOF &&
> +* ../sm1 0000000...$head1 (2):
> +  > Add foo2
> +
> +EOF

It somewhat looks strange to start with "<<-EOF" and then not to
indent the body nor EOF.

Re: [PATCH v3 2/2] submodule: drop the top-level requirement

[John Keeping]

On Thu, Apr 18, 2013 at 03:40:41PM -0700, Junio C Hamano wrote:
> John Keeping <john@keeping.me.uk> writes:
> 
> > +relative_path ()
> > +{
> > +	local target curdir result
> > +	target=$1
> > +	curdir=${2-$wt_prefix}
> > +	curdir=${curdir%/}
> > +	result=
> > +
> > +	while test -n "$curdir"
> > +	do
> > +		case "$target" in
> > +		"$curdir/"*)
> > +			target=${target#$curdir/}
> > +			break
> > +			;;
> > +		esac
> 
> Could $curdir have glob wildcard to throw this part of the logic
> off?  It is OK to have limitations like "you cannot have a glob
> characters in your path to submodule working tree" (at least until
> we start rewriting these in C or Perl or Python), but we need to be
> aware of them.

I think the use of "#" instead of "##" saves us here because even with a
wildcard in $curdir the case statement matches literally, so we know
that "$target" starts with "$curdir/", so "${target#$curdir/}" can't
match anything longer than the literal "$curdir" prefix.

> >  module_list()
> >  {
> > +	eval "set $(git rev-parse --sq --prefix "$wt_prefix" -- "$@")"
> 
> An efficient reuse of "--" ;-)
> 
> > +test_expect_success 'run summary from subdir' '
> > +	mkdir sub &&
> > +	(
> > +		cd sub &&
> > +		git submodule summary >../actual
> > +	) &&
> > +	cat >expected <<-EOF &&
> > +* ../sm1 0000000...$head1 (2):
> > +  > Add foo2
> > +
> > +EOF
> 
> It somewhat looks strange to start with "<<-EOF" and then not to
> indent the body nor EOF.

Yes, but I copied this from the preceding test.  I'd rather leave this
as it is for consistency and let later style fixes change all of the
tests in this file.

Re: [PATCH v3 2/2] submodule: drop the top-level requirement

[Junio C Hamano]

John Keeping <john@keeping.me.uk> writes:

> On Thu, Apr 18, 2013 at 03:40:41PM -0700, Junio C Hamano wrote:
>> John Keeping <john@keeping.me.uk> writes:
>> 
>> > +relative_path ()
>> > +{
>> > +	local target curdir result
>> > +	target=$1
>> > +	curdir=${2-$wt_prefix}
>> > +	curdir=${curdir%/}
>> > +	result=
>> > +
>> > +	while test -n "$curdir"
>> > +	do
>> > +		case "$target" in
>> > +		"$curdir/"*)
>> > +			target=${target#$curdir/}
>> > +			break
>> > +			;;
>> > +		esac
>> 
>> Could $curdir have glob wildcard to throw this part of the logic
>> off?  It is OK to have limitations like "you cannot have a glob
>> characters in your path to submodule working tree" (at least until
>> we start rewriting these in C or Perl or Python), but we need to be
>> aware of them.
>
> I think the use of "#" instead of "##" saves us here because even with a
> wildcard in $curdir the case statement matches literally, 

If you have curdir=a*b and target=adropb/c/d/e, the chopping itself

	target=${target#$curdir/}

would happily chop "adropb/" from the target, but because the dq
around "$curdir/"* in the case arm label enforces that target must
literally match curdir followed by a slash, we do not even come to
the chomping part.

I still have not convinced myself that it is impossible for somebody
more clever than I to craft a pair of target and curdir that breaks
it, though.  (target="a*b/c/d", curdir="a*b") is correctly chopped,
so that is not it.

Re: [PATCH v3 2/2] submodule: drop the top-level requirement

[Johannes Sixt]

Am 19.04.2013 18:45, schrieb Junio C Hamano:
> John Keeping <john@keeping.me.uk> writes:
> 
>> On Thu, Apr 18, 2013 at 03:40:41PM -0700, Junio C Hamano wrote:
>>> John Keeping <john@keeping.me.uk> writes:
>>>
>>>> +relative_path ()
>>>> +{
>>>> +	local target curdir result
>>>> +	target=$1
>>>> +	curdir=${2-$wt_prefix}
>>>> +	curdir=${curdir%/}
>>>> +	result=
>>>> +
>>>> +	while test -n "$curdir"
>>>> +	do
>>>> +		case "$target" in
>>>> +		"$curdir/"*)
>>>> +			target=${target#$curdir/}
>>>> +			break
>>>> +			;;
>>>> +		esac
>>>
>>> Could $curdir have glob wildcard to throw this part of the logic
>>> off?  It is OK to have limitations like "you cannot have a glob
>>> characters in your path to submodule working tree" (at least until
>>> we start rewriting these in C or Perl or Python), but we need to be
>>> aware of them.
>>
>> I think the use of "#" instead of "##" saves us here because even with a
>> wildcard in $curdir the case statement matches literally, 
> 
> If you have curdir=a*b and target=adropb/c/d/e, the chopping itself
> 
> 	target=${target#$curdir/}
> 
> would happily chop "adropb/" from the target, but because the dq
> around "$curdir/"* in the case arm label enforces that target must
> literally match curdir followed by a slash, we do not even come to
> the chomping part.
> 
> I still have not convinced myself that it is impossible for somebody
> more clever than I to craft a pair of target and curdir that breaks
> it, though.  (target="a*b/c/d", curdir="a*b") is correctly chopped,
> so that is not it.

Why not just replace the six-liner by this one-liner:

		target=${target#"$curdir"/}

-- Hannes

Re: [PATCH v3 2/2] submodule: drop the top-level requirement

[Junio C Hamano]

Johannes Sixt <j6t@kdbg.org> writes:

> Why not just replace the six-liner by this one-liner:
>
> 		target=${target#"$curdir"/}

Simple enough ;-)

[PATCH] submodule: fix quoting in relative_path()

[John Keeping]

Commit caca2c1 (submodule: drop the top-level requirement) introduced a
relative_path helper but does not quote $curdir when it is stripped from
the front of a target path.  In this particular case this should be safe
even with special characters because we only do this after checking that
$target begins with "$curdir/" which is quoted correctly, but we should
quote the variable to be certain that there is not some obscure case
where we this could strip more or less than we want.

Signed-off-by: John Keeping <john@keeping.me.uk>
---
On Fri, Apr 19, 2013 at 02:03:14PM -0700, Junio C Hamano wrote:
> Johannes Sixt <j6t@kdbg.org> writes:
> 
> > Why not just replace the six-liner by this one-liner:
> >
> > 		target=${target#"$curdir"/}
> 
> Simple enough ;-)

This seems to have arrived on next without this fix, so here's a patch
on top.

 git-submodule.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/git-submodule.sh b/git-submodule.sh
index 0eee703..db9f260 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -127,7 +127,7 @@ relative_path ()
 	do
 		case "$target" in
 		"$curdir/"*)
-			target=${target#$curdir/}
+			target=${target#"$curdir"/}
 			break
 			;;
 		esac
-- 
1.8.2.1.715.gb260f47

Re: [PATCH] submodule: fix quoting in relative_path()

[Junio C Hamano]

John Keeping <john@keeping.me.uk> writes:

>> > Why not just replace the six-liner by this one-liner:
>> >
>> > 		target=${target#"$curdir"/}
>> 
>> Simple enough ;-)
>
> This seems to have arrived on next without this fix, so here's a patch
> on top.
>
>  git-submodule.sh | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/git-submodule.sh b/git-submodule.sh
> index 0eee703..db9f260 100755
> --- a/git-submodule.sh
> +++ b/git-submodule.sh
> @@ -127,7 +127,7 @@ relative_path ()
>  	do
>  		case "$target" in
>  		"$curdir/"*)
> -			target=${target#$curdir/}
> +			target=${target#"$curdir"/}
>  			break
>  			;;
>  		esac

J6t meant a patch to remove the entire case...esac and replace it
with a single liner (target=${target#"$curdir"/}).

Re: [PATCH] submodule: fix quoting in relative_path()

[John Keeping]

On Wed, Apr 24, 2013 at 09:21:38AM -0700, Junio C Hamano wrote:
> John Keeping <john@keeping.me.uk> writes:
> 
> >> > Why not just replace the six-liner by this one-liner:
> >> >
> >> > 		target=${target#"$curdir"/}
> >> 
> >> Simple enough ;-)
> >
> > This seems to have arrived on next without this fix, so here's a patch
> > on top.
> >
> >  git-submodule.sh | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/git-submodule.sh b/git-submodule.sh
> > index 0eee703..db9f260 100755
> > --- a/git-submodule.sh
> > +++ b/git-submodule.sh
> > @@ -127,7 +127,7 @@ relative_path ()
> >  	do
> >  		case "$target" in
> >  		"$curdir/"*)
> > -			target=${target#$curdir/}
> > +			target=${target#"$curdir"/}
> >  			break
> >  			;;
> >  		esac
> 
> J6t meant a patch to remove the entire case...esac and replace it
> with a single liner (target=${target#"$curdir"/}).

Ah, I missed the "six-liner" part.  But that doesn't work because we
break out of the do...while loop when "$target" starts with "$curdir/"
and replacing the case statement will remove that.

Re: [PATCH] submodule: fix quoting in relative_path()

[Johannes Sixt]

Am 24.04.2013 18:28, schrieb John Keeping:
> On Wed, Apr 24, 2013 at 09:21:38AM -0700, Junio C Hamano wrote:
>> J6t meant a patch to remove the entire case...esac and replace it
>> with a single liner (target=${target#"$curdir"/}).
> 
> Ah, I missed the "six-liner" part.  But that doesn't work because we
> break out of the do...while loop when "$target" starts with "$curdir/"
> and replacing the case statement will remove that.

Yeah, right, _that_ is what I missed ;-)

-- Hannes

Re: [PATCH v3 2/2] submodule: drop the top-level requirement

[Eric Sunshine]

On Thu, Apr 18, 2013 at 3:50 PM, John Keeping <john@keeping.me.uk> wrote:
> Use the new rev-parse --prefix option to process all paths given to the
> submodule command, dropping the requirement that it be run from the
> top-level of the repository.
>
> Signed-off-by: John Keeping <john@keeping.me.uk>
> ---
> diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
> index ff26535..ca0a6ab 100755
> --- a/t/t7400-submodule-basic.sh
> +++ b/t/t7400-submodule-basic.sh
> @@ -212,6 +212,24 @@ test_expect_success 'submodule add with ./, /.. and // in path' '
>         test_cmp empty untracked
>  '
>
> +test_expect_success 'submodule add in subdir' '

A particularly minor nit. Existing subdirectory-related tests in t7400
spell out "subdirectory" fully, so perhaps for consistency:
s/subdir/subdirectory/

> +       echo "refs/heads/master" >expect &&
> +       >empty &&
> +
> +       mkdir addtest/sub &&
> +       (
> +               cd addtest/sub &&
> +               git submodule add "$submodurl" ../realsubmod3 &&
> +               git submodule init
> +       ) &&
> +
> +       rm -f heads head untracked &&
> +       inspect addtest/realsubmod3 ../.. &&
> +       test_cmp expect heads &&
> +       test_cmp expect head &&
> +       test_cmp empty untracked
> +'
> +
>  test_expect_success 'setup - add an example entry to .gitmodules' '
>         GIT_CONFIG=.gitmodules \
>         git config submodule.example.url git://example.com/init.git
> @@ -319,6 +337,15 @@ test_expect_success 'status should be "up-to-date" after update' '
>         grep "^ $rev1" list
>  '
>
> +test_expect_success 'status works correctly from a subdirectory' '

Good: "subdirectory"

> +       mkdir sub &&
> +       (
> +               cd sub &&
> +               git submodule status >../list
> +       ) &&
> +       grep "^ $rev1" list
> +'
> +
>  test_expect_success 'status should be "modified" after submodule commit' '
>         (
>                 cd init &&
> diff --git a/t/t7401-submodule-summary.sh b/t/t7401-submodule-summary.sh
> index 30b429e..992b66b 100755
> --- a/t/t7401-submodule-summary.sh
> +++ b/t/t7401-submodule-summary.sh
> @@ -45,6 +45,20 @@ EOF
>         test_cmp expected actual
>  "
>
> +test_expect_success 'run summary from subdir' '

t7401 does not have any existing subdirectory-related tests, but for
consistency with t7400, perhaps: s/subdir/subdirectory/

> +       mkdir sub &&
> +       (
> +               cd sub &&
> +               git submodule summary >../actual
> +       ) &&
> +       cat >expected <<-EOF &&
> +* ../sm1 0000000...$head1 (2):
> +  > Add foo2
> +
> +EOF
> +       test_cmp expected actual
> +'
> +
>  commit_file sm1 &&
>  head2=$(add_file sm1 foo3)