[PATCHv2 0/4] submodule helper: cleanup prefix passing

The first two patches fix two subtle bugs that would show up if we'd
apply the third patch without them. The third patch replaces
 
    git submodule--helper --prefix $wt_prefix list

by the more Git idiomatic 

    git -C $wt_prefix submodule--helper list

The series is finished by adding more tests which would have helped
to have when developing this series.

This applies to origin/master.

A prior version can be found at 
$gmane/287620

Thanks,
Stefan

Stefan Beller (4):
  submodule: fix recursive path printing from non root directory
  submodule: fix recursive execution from non root directory
  submodule--helper list: lose the extra prefix option
  submodule: add more tests for recursive submodule behavior

 builtin/submodule--helper.c |  5 +----
 git-submodule.sh            | 19 +++++++++++--------
 t/t7403-submodule-sync.sh   | 13 +++++++++----
 t/t7406-submodule-update.sh | 12 ++++++++++++
 4 files changed, 33 insertions(+), 16 deletions(-)

-- 
2.8.0.rc4.10.g52f3f33

Recursing into submodules currently works by just calling
(cd $submodule && eval <command>) for update, sync and status
command.

Say you have the following setup

repo/ # a superproject repository
repo/untracked/ # an untracked dir in repo/
repo/sub/ # a submodule
repo/sub/subsub # a submodule of a submodule

When being in repo/untracked/ and invoking "git submodule status"
you would expect output like:

    repo/untracked/$ git submodule status --recursive
     <sha1> ../sub (version)
     <sha1> ../sub/subsub (<version>)

We need to take into account that we are in the untracked/ dir,
so we need to prepend ../ to the paths. By using relative_path
to compute the prefix, we'll have that output.

Signed-off-by: Stefan Beller <redacted>
---
 git-submodule.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/git-submodule.sh b/git-submodule.sh
index 43c68de..536ba68 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh

@@ -825,7 +825,7 @@ Maybe you want to use 'update --init'?")"
 		if test -n "$recursive"
 		then
 			(
-				prefix="$prefix$sm_path/"
+				prefix="$(relative_path $prefix$sm_path)/"
 				clear_local_git_env
 				cd "$sm_path" &&
 				eval cmd_update

@@ -1233,13 +1233,13 @@ cmd_sync()
 			then
 			(
 				clear_local_git_env
+				prefix=$(relative_path "$prefix$sm_path/")
 				cd "$sm_path"
 				remote=$(get_default_remote)
 				git config remote."$remote".url "$sub_origin_url"
 
 				if test -n "$recursive"
 				then
-					prefix="$prefix$sm_path/"
 					eval cmd_sync
 				fi
 			)

-- 
2.8.0.rc4.10.g52f3f33

One of the first things that happens in most submodule sub commands is

    git submodule--helper list --prefix "$wt_prefix"

Currently the passed --prefix is used for doing path calculation
as if we were in that path relative to the repository root, which is
why we need to pass "$wt_prefix". The more common way in Git however
would be to use

    git -C "$wt_prefix" submodule--helper list

which I want to change later. That way however does not just
pass the prefix into the submodule command, but also changes
into that directory.

Say you have the following setup

repo/ # a superproject repository
repo/untracked/ # an untracked dir in repo/
repo/sub/ # a submodule
repo/sub/subsub # a submodule of a submodule

When in repo/untracked/ and invoking "git submodule status --recursive",
the recursed instance of the latter version for listing submodules would
try to change into the directory repo/sub/untracked, which is a bug.
This happens as we cd into the submodule in git-submodule.sh without
clearing wt_prefix, which is the assumed relative path inside the working
directory.

Most times that directory doesn't exist and we error out. Fix this bug
by clearing wt_prefix, such that any recursive instances of will assume
to operate from the respective root of the respective submodule.

Signed-off-by: Stefan Beller <redacted>
---
 git-submodule.sh | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/git-submodule.sh b/git-submodule.sh
index 536ba68..6b18a03 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh

@@ -827,6 +827,7 @@ Maybe you want to use 'update --init'?")"
 			(
 				prefix="$(relative_path $prefix$sm_path)/"
 				clear_local_git_env
+				wt_prefix=
 				cd "$sm_path" &&
 				eval cmd_update
 			)

@@ -1159,6 +1160,7 @@ cmd_status()
 			(
 				prefix="$displaypath/"
 				clear_local_git_env
+				wt_prefix=
 				cd "$sm_path" &&
 				eval cmd_status
 			) ||

@@ -1240,6 +1242,7 @@ cmd_sync()
 
 				if test -n "$recursive"
 				then
+					wt_prefix=
 					eval cmd_sync
 				fi
 			)

-- 
2.8.0.rc4.10.g52f3f33

The usual early machinery of Git is to change the directory to
the top level of the working tree and pass the actual path inside
the working tree as `prefix` to the command being run.
This is the case both for commands written in C (where the
prefix is passed into the command in a function parameter) as
well as in git-submodule.sh where the setup code runs

  wt_prefix=$(git rev-parse show-prefix)
  cd_to_top_level

So the prefix passed into the `submodule--helper list` is actually
the relative path inside the working tree, but we were not using
the standard way of passing it through.

Adhere to Gits standard of passing the relative path inside the
working tree by passing it via -C.

We do not need to pass it for `submodule foreach` as that command
doesn't take further arguments ('$@') to operate on a subset of
submodules, such that it is irrelevant for listing the submodules.
The computation of the displaypath ('Entering <path>') is done
separately there.

Signed-off-by: Stefan Beller <redacted>
---
 builtin/submodule--helper.c |  5 +----
 git-submodule.sh            | 12 ++++++------
 2 files changed, 7 insertions(+), 10 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index ed764c9..2983783 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c

@@ -68,14 +68,11 @@ static int module_list(int argc, const char **argv, const char *prefix)
 	struct module_list list = MODULE_LIST_INIT;
 
 	struct option module_list_options[] = {
-		OPT_STRING(0, "prefix", &prefix,
-			   N_("path"),
-			   N_("alternative anchor for relative paths")),
 		OPT_END()
 	};
 
 	const char *const git_submodule_helper_usage[] = {
-		N_("git submodule--helper list [--prefix=<path>] [<path>...]"),
+		N_("git submodule--helper list [<path>...]"),
 		NULL
 	};
 

diff --git a/git-submodule.sh b/git-submodule.sh
index 6b18a03..1f7ad6e 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh

@@ -407,7 +407,7 @@ cmd_foreach()
 	# command in the subshell (and a recursive call to this function)
 	exec 3<&0
 
-	git submodule--helper list --prefix "$wt_prefix"|
+	git submodule--helper list |
 	while read mode sha1 stage sm_path
 	do
 		die_if_unmatched "$mode"

@@ -467,7 +467,7 @@ cmd_init()
 		shift
 	done
 
-	git submodule--helper list --prefix "$wt_prefix" "$@" |
+	git -C "$wt_prefix" submodule--helper list "$@" |
 	while read mode sha1 stage sm_path
 	do
 		die_if_unmatched "$mode"

@@ -549,7 +549,7 @@ cmd_deinit()
 		die "$(eval_gettext "Use '.' if you really want to deinitialize all submodules")"
 	fi
 
-	git submodule--helper list --prefix "$wt_prefix" "$@" |
+	git -C "$wt_prefix" submodule--helper list "$@" |
 	while read mode sha1 stage sm_path
 	do
 		die_if_unmatched "$mode"

@@ -683,7 +683,7 @@ cmd_update()
 	fi
 
 	cloned_modules=
-	git submodule--helper list --prefix "$wt_prefix" "$@" | {
+	git -C "$wt_prefix" submodule--helper list "$@" | {
 	err=
 	while read mode sha1 stage sm_path
 	do

@@ -1121,7 +1121,7 @@ cmd_status()
 		shift
 	done
 
-	git submodule--helper list --prefix "$wt_prefix" "$@" |
+	git -C "$wt_prefix" submodule--helper list "$@" |
 	while read mode sha1 stage sm_path
 	do
 		die_if_unmatched "$mode"

@@ -1199,7 +1199,7 @@ cmd_sync()
 		esac
 	done
 	cd_to_toplevel
-	git submodule--helper list --prefix "$wt_prefix" "$@" |
+	git -C "$wt_prefix" submodule--helper list "$@" |
 	while read mode sha1 stage sm_path
 	do
 		die_if_unmatched "$mode"

-- 
2.8.0.rc4.10.g52f3f33

This adds a test for "submodule update", wich calls "submodule update"
from an untracked repository in the superproject. When doing creating
the parent patch a similar test failed for "submodule sync", but
all tests passed for "submodule update". It took me a long time
to figure out this was a difference in test coverage instead of
commands behaving differently. Let's improve the test coverage such
to make it a better place.

When trying to fix the issue in the parent patch I could get
the test suite passing when removing the $@ argument from module_list
in the sync command. This also indicates a low test coverage, so
fix that.

Signed-off-by: Stefan Beller <redacted>
---
 t/t7403-submodule-sync.sh   | 13 +++++++++----
 t/t7406-submodule-update.sh | 12 ++++++++++++
 2 files changed, 21 insertions(+), 4 deletions(-)

diff --git a/t/t7403-submodule-sync.sh b/t/t7403-submodule-sync.sh
index 79bc135..5dde123 100755
--- a/t/t7403-submodule-sync.sh
+++ b/t/t7403-submodule-sync.sh

@@ -28,6 +28,9 @@ test_expect_success setup '
 		git submodule add ../submodule submodule &&
 		test_tick &&
 		git commit -m "submodule"
+		git submodule add ../submodule submodule2 &&
+		test_tick &&
+		git commit -m "second submodule"
 	) &&
 	git clone super super-clone &&
 	(

@@ -149,15 +152,16 @@ test_expect_success 'reset submodule URLs' '
 	reset_submodule_urls super-clone
 '
 
-test_expect_success '"git submodule sync" should update submodule URLs - subdirectory' '
+test_expect_success '"git submodule sync" should update specified submodule URLs - subdirectory' '
 	(
 		cd super-clone &&
 		git pull --no-recurse-submodules &&
 		mkdir -p sub &&
 		cd sub &&
-		git submodule sync >../../output
+		git submodule sync ../submodule >../../output
 	) &&
 	grep "\\.\\./submodule" output &&
+	! grep submodule2 output &&
 	test -d "$(
 		cd super-clone/submodule &&
 		git config remote.origin.url

@@ -177,7 +181,7 @@ test_expect_success '"git submodule sync" should update submodule URLs - subdire
 	)
 '
 
-test_expect_success '"git submodule sync --recursive" should update all submodule URLs - subdirectory' '
+test_expect_success '"git submodule sync --recursive" should update all specified submodule URLs - subdirectory' '
 	(
 		cd super-clone &&
 		(

@@ -186,9 +190,10 @@ test_expect_success '"git submodule sync --recursive" should update all submodul
 		) &&
 		mkdir -p sub &&
 		cd sub &&
-		git submodule sync --recursive >../../output
+		git submodule sync --recursive ../submodule >../../output
 	) &&
 	grep "\\.\\./submodule/sub-submodule" output &&
+	! grep submodule2 output &&
 	test -d "$(
 		cd super-clone/submodule &&
 		git config remote.origin.url

diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
index 68ea31d..628da7f 100755
--- a/t/t7406-submodule-update.sh
+++ b/t/t7406-submodule-update.sh

@@ -774,4 +774,16 @@ test_expect_success 'submodule update --recursive drops module name before recur
 	 test_i18ngrep "Submodule path .deeper/submodule/subsubmodule.: checked out" actual
 	)
 '
+
+test_expect_success 'submodule update --recursive works from subdirectory' '
+	(cd super2 &&
+	 (cd deeper/submodule/subsubmodule &&
+	  git checkout HEAD^
+	 ) &&
+	 mkdir untracked &&
+	 cd untracked &&
+	 git submodule update --recursive >actual &&
+	 test_i18ngrep "Submodule path .../deeper/submodule/subsubmodule.: checked out" actual
+	)
+'
 test_done

-- 
2.8.0.rc4.10.g52f3f33

On Thu, Mar 24, 2016 at 4:34 PM, Stefan Beller [off-list ref] wrote:

Not really sure why this got moved, but I don't think it hurts
anything, though we will have prefix defined now regardless of if
we're recursive or not. But I think that's correct.

                                cd "$sm_path"
                                remote=$(get_default_remote)
                                git config remote."$remote".url "$sub_origin_url"

                                if test -n "$recursive"
                                then
-                                       prefix="$prefix$sm_path/"
                                        eval cmd_sync
                                fi
                        )
--
2.8.0.rc4.10.g52f3f33

Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>

Regards,
Jake

On Thu, Mar 24, 2016 at 4:34 PM, Stefan Beller [off-list ref] wrote:

And here I think I see why we moved the original prefix code up some.
This looks good.

                                        eval cmd_sync
                                fi
                        )
--
2.8.0.rc4.10.g52f3f33

Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>

Regards,
Jake

On Thu, Mar 24, 2016 at 4:38 PM, Jacob Keller [off-list ref] wrote:

On Thu, Mar 24, 2016 at 4:34 PM, Stefan Beller [off-list ref] wrote:

quoted

Recursing into submodules currently works by just calling
(cd $submodule && eval <command>) for update, sync and status
command.

Say you have the following setup

repo/ # a superproject repository
repo/untracked/ # an untracked dir in repo/
repo/sub/ # a submodule
repo/sub/subsub # a submodule of a submodule

When being in repo/untracked/ and invoking "git submodule status"
you would expect output like:

    repo/untracked/$ git submodule status --recursive
     <sha1> ../sub (version)
     <sha1> ../sub/subsub (<version>)

We need to take into account that we are in the untracked/ dir,
so we need to prepend ../ to the paths. By using relative_path
to compute the prefix, we'll have that output.

Signed-off-by: Stefan Beller <redacted>
---
 git-submodule.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/git-submodule.sh b/git-submodule.sh
index 43c68de..536ba68 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh

@@ -825,7 +825,7 @@ Maybe you want to use 'update --init'?")"
                if test -n "$recursive"
                then
                        (
-                               prefix="$prefix$sm_path/"
+                               prefix="$(relative_path $prefix$sm_path)/"
                                clear_local_git_env
                                cd "$sm_path" &&
                                eval cmd_update

@@ -1233,13 +1233,13 @@ cmd_sync()
                        then
                        (
                                clear_local_git_env
+                               prefix=$(relative_path "$prefix$sm_path/")

Not really sure why this got moved, but I don't think it hurts
anything, though we will have prefix defined now regardless of if
we're recursive or not. But I think that's correct.

"Because we need to move it before the cd". At least I thought so at
the time of writing. That is actually not the case.

At the time of writing this was intertangled with the next patch,
and we need to put the call to relative_path before the reassignment
of wt_prefix as relative_path depends on wt_prefix, which I put at the
same place as the cd in an initial version when coming up with that patch.

In case of a resend, consider this fixed.

quoted

                                cd "$sm_path"
                                remote=$(get_default_remote)
                                git config remote."$remote".url "$sub_origin_url"

                                if test -n "$recursive"
                                then
-                                       prefix="$prefix$sm_path/"
                                        eval cmd_sync
                                fi
                        )
--
2.8.0.rc4.10.g52f3f33

Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>

Regards,
Jake

On Thu, Mar 24, 2016 at 4:34 PM, Stefan Beller [off-list ref] wrote:

The usual early machinery of Git is to change the directory to
the top level of the working tree and pass the actual path inside
the working tree as `prefix` to the command being run.
This is the case both for commands written in C (where the
prefix is passed into the command in a function parameter) as
well as in git-submodule.sh where the setup code runs

  wt_prefix=$(git rev-parse show-prefix)
  cd_to_top_level

So the prefix passed into the `submodule--helper list` is actually
the relative path inside the working tree, but we were not using
the standard way of passing it through.

Adhere to Gits standard of passing the relative path inside the
working tree by passing it via -C.

We do not need to pass it for `submodule foreach` as that command
doesn't take further arguments ('$@') to operate on a subset of
submodules, such that it is irrelevant for listing the submodules.
The computation of the displaypath ('Entering <path>') is done
separately there.

Signed-off-by: Stefan Beller <redacted>
---

It is nice to see the format for doing this standardized, and reduce
extra code in the submodule--helper. I had wondered why we used
--prefix before.

Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>

Regards,
Jake

On Thu, Mar 24, 2016 at 7:34 PM, Stefan Beller [off-list ref] wrote:

This adds a test for "submodule update", wich calls "submodule update"

s/wich/which/

from an untracked repository in the superproject. When doing creating

Grammo: "doing creating"

the parent patch a similar test failed for "submodule sync", but
all tests passed for "submodule update". It took me a long time
to figure out this was a difference in test coverage instead of
commands behaving differently. Let's improve the test coverage such
to make it a better place.

When trying to fix the issue in the parent patch I could get
the test suite passing when removing the $@ argument from module_list
in the sync command. This also indicates a low test coverage, so
fix that.

These two paragraphs are almost entirely commentary, thus probably
belong below the "---" line. I'm having a difficult time trying to
decipher from this commit message what this patch is actually about.
Perhaps the commit message could do a better job explaining exactly
what shortcoming(s) it's addressing.

Maybe use -C and drop the sub-subshell:

    git -C deeper/submodule/subsubmodule checkout HEAD^

+        mkdir untracked &&
+        cd untracked &&
+        git submodule update --recursive >actual &&
+        test_i18ngrep "Submodule path .../deeper/submodule/subsubmodule.: checked out" actual
+       )
+'

On Thu, Mar 24, 2016 at 5:25 PM, Eric Sunshine [off-list ref] wrote:

On Thu, Mar 24, 2016 at 7:34 PM, Stefan Beller [off-list ref] wrote:

quoted

This adds a test for "submodule update", wich calls "submodule update"

s/wich/which/

quoted

from an untracked repository in the superproject. When doing creating

Grammo: "doing creating"

quoted

the parent patch a similar test failed for "submodule sync", but
all tests passed for "submodule update". It took me a long time
to figure out this was a difference in test coverage instead of
commands behaving differently. Let's improve the test coverage such
to make it a better place.

When trying to fix the issue in the parent patch I could get
the test suite passing when removing the $@ argument from module_list
in the sync command. This also indicates a low test coverage, so
fix that.

These two paragraphs are almost entirely commentary, thus probably
belong below the "---" line. I'm having a difficult time trying to
decipher from this commit message what this patch is actually about.
Perhaps the commit message could do a better job explaining exactly
what shortcoming(s) it's addressing.

The tests on submodule commands executed not from the top level are very sparse.
I had a hard time to developing patches 1 and 2.
And I felt

    "This patch adds more test coverage."

is not a sufficient commit message.

The current tests have found issues, which
lead to fixing them in patch 1,2, but I think only by accident, as one command
(sync) was testing from the non root. By having more test coverage it
is easier to
have a guess what is wrong with the code.

Any hint on how to write that into a commit message without being commentatory
would be great!

quoted

Signed-off-by: Stefan Beller <redacted>
---

diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh

@@ -774,4 +774,16 @@ test_expect_success 'submodule update --recursive drops module name before recur
+test_expect_success 'submodule update --recursive works from subdirectory' '
+       (cd super2 &&
+        (cd deeper/submodule/subsubmodule &&
+         git checkout HEAD^
+        ) &&

Maybe use -C and drop the sub-subshell:

    git -C deeper/submodule/subsubmodule checkout HEAD^

ok

quoted

+        mkdir untracked &&
+        cd untracked &&
+        git submodule update --recursive >actual &&
+        test_i18ngrep "Submodule path .../deeper/submodule/subsubmodule.: checked out" actual
+       )
+'