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

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

[Stefan Beller]

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

[PATCH 1/4] submodule: fix recursive path printing from non root directory

[Stefan Beller]

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 <sbeller@google.com>
---
 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

[PATCH 2/4] submodule: fix recursive execution from non root directory

[Stefan Beller]

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 <sbeller@google.com>
---
 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

[PATCH 3/4] submodule--helper list: lose the extra prefix option

[Stefan Beller]

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 <sbeller@google.com>
---
 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

[PATCH 4/4] submodule: add more tests for recursive submodule behavior

[Stefan Beller]

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 <sbeller@google.com>
---
 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

Re: [PATCH 1/4] submodule: fix recursive path printing from non root directory

[Jacob Keller]

On Thu, Mar 24, 2016 at 4:34 PM, Stefan Beller <sbeller@google.com> wrote:
> 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 <sbeller@google.com>
> ---
>  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.

>                                 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

Re: [PATCH 2/4] submodule: fix recursive execution from non root directory

[Jacob Keller]

On Thu, Mar 24, 2016 at 4:34 PM, Stefan Beller <sbeller@google.com> wrote:
> 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 <sbeller@google.com>
> ---
>  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=

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

Re: [PATCH 3/4] submodule--helper list: lose the extra prefix option

[Jacob Keller]

On Thu, Mar 24, 2016 at 4:34 PM, Stefan Beller <sbeller@google.com> 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 <sbeller@google.com>
> ---

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>

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

Regards,
Jake

Re: [PATCH 1/4] submodule: fix recursive path printing from non root directory

[Stefan Beller]

On Thu, Mar 24, 2016 at 4:38 PM, Jacob Keller <jacob.keller@gmail.com> wrote:
> On Thu, Mar 24, 2016 at 4:34 PM, Stefan Beller <sbeller@google.com> wrote:
>> 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 <sbeller@google.com>
>> ---
>>  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.

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

Re: [PATCH 4/4] submodule: add more tests for recursive submodule behavior

[Eric Sunshine]

On Thu, Mar 24, 2016 at 7:34 PM, Stefan Beller <sbeller@google.com> 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.

> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
> 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^

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

Re: [PATCH 4/4] submodule: add more tests for recursive submodule behavior

[Stefan Beller]

On Thu, Mar 24, 2016 at 5:25 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Thu, Mar 24, 2016 at 7:34 PM, Stefan Beller <sbeller@google.com> 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.

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!

>
>> Signed-off-by: Stefan Beller <sbeller@google.com>
>> ---
>> 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

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

Re: [PATCH 3/4] submodule--helper list: lose the extra prefix option

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> 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...
>
> Adhere to Gits standard of passing the relative path inside the
> working tree by passing it via -C.

While -C _also_ works, I do not think it is "standard" in any sense.
What made you think so?  -C is a way to tell Git to chdir there
before doing anything else, without even adjusting the command line
arguments that might be paths, and "chdir there to go to top" may
(or may not--I haven't thought things thru) have the same effect as
passing the prefix into functions, that is merely true as a side
effect, I would think.

So this change may not be wrong per-se, but if the lossage of prefix
is the final goal (as opposed to an approach to gain other benefits,
e.g. "now we do not have to use prefix, we can simplify these other
things"), I do not know if it is worth it.

Re: [PATCH 3/4] submodule--helper list: lose the extra prefix option

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Stefan Beller <sbeller@google.com> writes:
>
>> 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...
>>
>> Adhere to Gits standard of passing the relative path inside the
>> working tree by passing it via -C.
>
> While -C _also_ works, I do not think it is "standard" in any sense.
> What made you think so?  -C is a way to tell Git to chdir there
> before doing anything else, without even adjusting the command line
> arguments that might be paths, and "chdir there to go to top" may
> (or may not--I haven't thought things thru) have the same effect as
> passing the prefix into functions, that is merely true as a side
> effect, I would think.
>
> So this change may not be wrong per-se, but if the lossage of prefix
> is the final goal (as opposed to an approach to gain other benefits,
> e.g. "now we do not have to use prefix, we can simplify these other
> things"), I do not know if it is worth it.

I actually do not care too deeply either way, as I understand that
the long term goal is to have "git submodule" itself rewritten in C,
so that places that currently call submodule--helper would become an
internal call.

The way all the subcommand written in C works is

 - The start-up sequence does the repository discovery, which
   involves crawling up to the top-level of the working tree, and
   compute "prefix", where the end-user was when the command was
   invoked;

 - The subcommand implementation is called with this "prefix";

 - When the subcommand implementation interprets the command line
   arguments and option arguments, it prefixes the "prefix" as
   needed.  If, for example, "git grep -f patterns" is invoked
   inside "sub/" subdirectory, when the command line and option
   arguments are processed, the process is already at the top level
   of the working tree, so it needs to read the patterns from
   "sub/patterns" file.  "git ls-files 'Makefil*'" invoked inside
   "sub/" subdirectory needs to limit the output to those that match
   not "Makefile", but "sub/Makefil*".

The hope of doing an incremental rewrite of the whole thing by
enriching submodule--helper is that the bulk of the code there will
be reusable when the entirety of "git submodule" is rewritten in C,
so they need to take the "prefix" the same way, whether the caller
is calling from "git-submodule.sh" script via submodule--helper, or
the eventual C implementation of "git submodule" is making direct
calls to them.  As long as the correct "prefix" is passed to the
routines that are driven via submodule--helper, it does not matter
and I do not care how it is done.

The current code of "git submodule" whose higher parts are still in
shell would would:

 - The start-up sequence in shell does the cd_to_toplevel and finds
   the prefix;

 - "git submodule--helper list --prefix=$prefix $other_args" is
   called; as this is called from the top-level of the working tree,
   internally its "prefix" is empty, but $other_args must be
   interpreted relative to the $prefix passed with --prefix option.

If we instead call "git -C "$prefix" submodule--helper list $other_args",
then

 - This command first does another chdir(2) back to $prefix;

 - The start-up sequence of "submodule--helper" does the usual
   repository discovery again, which involves crawling up to the
   top-level of the working tree, and compute "prefix".  This
   happens to match what -C "$prefix" gave the command.

Making calls to submodule--helper via "git -C" feels a little bit
roundabout because of this "caller tells to chdir, and then it has
to again chdir back up" only to rediscover the same information.

Again, I actually do not care too deeply either way, though, as long
as the correct "prefix" is passed to the routines that are driven
via submodule--helper, which would assure that the C code you write
today will be reusable when "git submodule" as a whole is redone in
C.

Thanks.

Re: [PATCH 1/4] submodule: fix recursive path printing from non root directory

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

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

tests to demonstrate existing breakage and protect the fix from
future breakages would be needed, no?

>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>  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
>  			)

Re: [PATCH 2/4] submodule: fix recursive execution from non root directory

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

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

As long as the recursive instances do not take any filenames and
pathspecs that needs adjustment with respect to the prefix, this
would be an OK change; I am not sure if that precondition holds,
though.

Thanks.

>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>  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
>  			)

Re: [PATCH 3/4] submodule--helper list: lose the extra prefix option

[Stefan Beller]

On Thu, Mar 24, 2016 at 11:28 PM, Junio C Hamano <gitster@pobox.com> wrote:
>
> So this change may not be wrong per-se, but if the lossage of prefix
> is the final goal (as opposed to an approach to gain other benefits,
> e.g. "now we do not have to use prefix, we can simplify these other
> things"), I do not know if it is worth it.
>

It is the final goal of this series. As motivation, see Jacobs comment above:

    I had wondered why we used --prefix before.

Also when the submodule helper got in, reviewers (Jonathan) were confused and
asked for clarification of the prefix term. So either document the prefix term
(and come up with a reason why we don't use the standard mechanism,
which as you outlined in the other mail, may be performance as we skip one
chdir, but that sounds like weak argument to me) or remove the confusing part
of having a prefix, which is not the standard prefix inside C.

The other reason you gave below is also convincing: By having it in the prefix,
the C code is more likely correct and future proof.

On rewriting the whole submodule command in C (probably reiterating):
It is not my endgoal to rewrite every submodule related part in C. It
would be nice
if it would happen eventually, but for now I only rewrite parts that I
need in C. (i.e.
paralllelisation is hard in shell, if not impossible using posix shell
with no additional
dependencies [xargs, gnu parallel], so I moved that part to C.
Certain parts need a performance boost? Ok I'll do it in C.

That said, we may have the shell/C architecture for a longer time than planned,
which makes it important to comment/document the confusing parts. Instead
I'd rather not have confusing parts, so I see benefit in having the
goal of this series
to remove the --prefix.

Re: [PATCH 1/4] submodule: fix recursive path printing from non root directory

[Stefan Beller]

On Fri, Mar 25, 2016 at 9:43 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> 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.
>
> tests to demonstrate existing breakage and protect the fix from
> future breakages would be needed, no?

I realize this is worded slightly wrong. The bugs described in patch
1&2 would only occur if we'd apply patch 3 without them.
So currently there is no bug.

We currently do have tests for that, e.g. t7403,
 '"git submodule sync" should update submodule URLs - subdirectory'

In patch 4 I added tests for "git submodule update" and enhanced tests for
the sync command.


>
>>
>> Signed-off-by: Stefan Beller <sbeller@google.com>
>> ---
>>  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
>>                       )

Re: [PATCH 3/4] submodule--helper list: lose the extra prefix option

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> The other reason you gave below is also convincing: By having it in the prefix,
> the C code is more likely correct and future proof.
>
> On rewriting the whole submodule command in C (probably
> reiterating): It is not my endgoal to rewrite every submodule
> related part in C. It would be nice if it would happen eventually,
> but for now I only rewrite parts that I need in C.

Well, what you personally would want to do yourself is irrelevant.
Doing submodule--helper in such a way that it is sufficient in a
half-way conversion but cannot be reused in future full rewrite is
something we would want to avoid, whether you would be doing the
full rewrite in the future.  In fact, if you are not inclined to do
so yourself, that is one more reason to make sure that the C pieces
in submodule--helper are reusable (i.e. your "correct and future
proof" above); otherwise you'd be making it _more_ difficult for
other people who would want to pick it up where you left.

Re: [PATCH 3/4] submodule--helper list: lose the extra prefix option

[Stefan Beller]

On Fri, Mar 25, 2016 at 10:01 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> The other reason you gave below is also convincing: By having it in the prefix,
>> the C code is more likely correct and future proof.
>>
>> On rewriting the whole submodule command in C (probably
>> reiterating): It is not my endgoal to rewrite every submodule
>> related part in C. It would be nice if it would happen eventually,
>> but for now I only rewrite parts that I need in C.
>
> Well, what you personally would want to do yourself is irrelevant.
> Doing submodule--helper in such a way that it is sufficient in a
> half-way conversion but cannot be reused in future full rewrite is
> something we would want to avoid, whether you would be doing the
> full rewrite in the future.  In fact, if you are not inclined to do
> so yourself, that is one more reason to make sure that the C pieces
> in submodule--helper are reusable (i.e. your "correct and future
> proof" above); otherwise you'd be making it _more_ difficult for
> other people who would want to pick it up where you left.

Exactly, sorry for not writing my chain of thoughts down completely.

To make them reusable, I'd assume we want them to be easy to
understand, and by using a well known way in git it is easier to
understand.

Re: [PATCH 3/4] submodule--helper list: lose the extra prefix option

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> The way all the subcommand written in C works is
>
>  - The start-up sequence does the repository discovery, which
>    involves crawling up to the top-level of the working tree, and
>    compute "prefix", where the end-user was when the command was
>    invoked;
>
>  - The subcommand implementation is called with this "prefix";
>
>  - When the subcommand implementation interprets the command line
>    arguments and option arguments, it prefixes the "prefix" as
>    needed.  If, for example, "git grep -f patterns" is invoked
>    inside "sub/" subdirectory, when the command line and option
>    arguments are processed, the process is already at the top level
>    of the working tree, so it needs to read the patterns from
>    "sub/patterns" file.  "git ls-files 'Makefil*'" invoked inside
>    "sub/" subdirectory needs to limit the output to those that match
>    not "Makefile", but "sub/Makefil*".
>
> The hope of doing an incremental rewrite of the whole thing by
> enriching submodule--helper is that the bulk of the code there will
> be reusable when the entirety of "git submodule" is rewritten in C,
> so they need to take the "prefix" the same way, whether the caller
> is calling from "git-submodule.sh" script via submodule--helper, or
> the eventual C implementation of "git submodule" is making direct
> calls to them.  As long as the correct "prefix" is passed to the
> routines that are driven via submodule--helper, it does not matter
> and I do not care how it is done.
>
> The current code of "git submodule" whose higher parts are still in
> shell would would:
>
>  - The start-up sequence in shell does the cd_to_toplevel and finds
>    the prefix;
>
>  - "git submodule--helper list --prefix=$prefix $other_args" is
>    called; as this is called from the top-level of the working tree,
>    internally its "prefix" is empty, but $other_args must be
>    interpreted relative to the $prefix passed with --prefix option.
>
> If we instead call "git -C "$prefix" submodule--helper list $other_args",
> then
>
>  - This command first does another chdir(2) back to $prefix;
>
>  - The start-up sequence of "submodule--helper" does the usual
>    repository discovery again, which involves crawling up to the
>    top-level of the working tree, and compute "prefix".  This
>    happens to match what -C "$prefix" gave the command.
>
> Making calls to submodule--helper via "git -C" feels a little bit
> roundabout because of this "caller tells to chdir, and then it has
> to again chdir back up" only to rediscover the same information.

Just to make sure that the discussion is complete.

Another way a script can use the "prefix" information is to use the
"prefix" as such.  Knowing that the cd_to_toplevel() took you to the
root level of the working tree, instead of "git -C $prefix" or
"--prefix $prefix", it could do "git $cmd $prefix$filename".

One consideration when choosing among the approaches is how the
$filename is reported back to the user (e.g. as part of an error
message).  "git $cmd $prefix$filename" invocation would complain
about the full "$prefix$filename" path, but what the user gave it
may only be $filename part.

Re: [PATCH 2/4] submodule: fix recursive execution from non root directory

[Stefan Beller]

On Fri, Mar 25, 2016 at 9:46 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> 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.
>
> As long as the recursive instances do not take any filenames and
> pathspecs that needs adjustment with respect to the prefix, this
> would be an OK change; I am not sure if that precondition holds,
> though.

If we give filenames and pathspecs relative to the prefix in a submodule,
the filename is not recognized by the superproject which would produce an
error at the superproject. Monkey-testing locally here supports my suspicion
and it is not possible to do things like

    git submodule status path/inside/submodule

so if that is already not working, we should not expect to have

    git -C subdir  submodule status ../path/inside/submodule

working.





>
> Thanks.
>
>>
>> Signed-off-by: Stefan Beller <sbeller@google.com>
>> ---
>>  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
>>                       )

Re: [PATCH 3/4] submodule--helper list: lose the extra prefix option

[Stefan Beller]

On Fri, Mar 25, 2016 at 10:25 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
>> The way all the subcommand written in C works is
>>
>>  - The start-up sequence does the repository discovery, which
>>    involves crawling up to the top-level of the working tree, and
>>    compute "prefix", where the end-user was when the command was
>>    invoked;
>>
>>  - The subcommand implementation is called with this "prefix";
>>
>>  - When the subcommand implementation interprets the command line
>>    arguments and option arguments, it prefixes the "prefix" as
>>    needed.  If, for example, "git grep -f patterns" is invoked
>>    inside "sub/" subdirectory, when the command line and option
>>    arguments are processed, the process is already at the top level
>>    of the working tree, so it needs to read the patterns from
>>    "sub/patterns" file.  "git ls-files 'Makefil*'" invoked inside
>>    "sub/" subdirectory needs to limit the output to those that match
>>    not "Makefile", but "sub/Makefil*".
>>
>> The hope of doing an incremental rewrite of the whole thing by
>> enriching submodule--helper is that the bulk of the code there will
>> be reusable when the entirety of "git submodule" is rewritten in C,
>> so they need to take the "prefix" the same way, whether the caller
>> is calling from "git-submodule.sh" script via submodule--helper, or
>> the eventual C implementation of "git submodule" is making direct
>> calls to them.  As long as the correct "prefix" is passed to the
>> routines that are driven via submodule--helper, it does not matter
>> and I do not care how it is done.
>>
>> The current code of "git submodule" whose higher parts are still in
>> shell would would:
>>
>>  - The start-up sequence in shell does the cd_to_toplevel and finds
>>    the prefix;
>>
>>  - "git submodule--helper list --prefix=$prefix $other_args" is
>>    called; as this is called from the top-level of the working tree,
>>    internally its "prefix" is empty, but $other_args must be
>>    interpreted relative to the $prefix passed with --prefix option.
>>
>> If we instead call "git -C "$prefix" submodule--helper list $other_args",
>> then
>>
>>  - This command first does another chdir(2) back to $prefix;
>>
>>  - The start-up sequence of "submodule--helper" does the usual
>>    repository discovery again, which involves crawling up to the
>>    top-level of the working tree, and compute "prefix".  This
>>    happens to match what -C "$prefix" gave the command.
>>
>> Making calls to submodule--helper via "git -C" feels a little bit
>> roundabout because of this "caller tells to chdir, and then it has
>> to again chdir back up" only to rediscover the same information.
>
> Just to make sure that the discussion is complete.
>
> Another way a script can use the "prefix" information is to use the
> "prefix" as such.  Knowing that the cd_to_toplevel() took you to the
> root level of the working tree, instead of "git -C $prefix" or
> "--prefix $prefix", it could do "git $cmd $prefix$filename".
>
> One consideration when choosing among the approaches is how the
> $filename is reported back to the user (e.g. as part of an error
> message).  "git $cmd $prefix$filename" invocation would complain
> about the full "$prefix$filename" path, but what the user gave it
> may only be $filename part.

Right. Using either "git -C $prefix" or "git --prefix $prefix" would report
the $filename only, because both cases assume $prefix was cut
using cd_to_toplevel and the user expects $filename only.

Re: [PATCH 3/4] submodule--helper list: lose the extra prefix option

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> Exactly, sorry for not writing my chain of thoughts down completely.
>
> To make them reusable, I'd assume we want them to be easy to
> understand, and by using a well known way in git it is easier to
> understand.

I already said I do not care too deeply either way, but I have to
point out that "git -C $prefix" would put extra cognitive burden on
those who will be picking it up where you left off.  When they start
by first literally translating shell to C, "git -C" would mislead
them to think if they have to chdir(2) to the directory first, which
is not the case at all.  "git -C $prefix submodule--helper" in the
code after [3/4] is there only because submodule--helper code no
longer is told what the caller already knows (i.e. what is the
prefix) and is forced to find it out by itself.  Contrasting to
that, with an explicit --prefix option, the reader would know there
is no need to chdir(2) anywhere at that point, but the paths that
are held in variables it uses from the surrounding code are not
relative to the current working directory when the code is called
and $prefix is there to help adjusting it.

The reason I do not care too deeply was that I thought people who
will be taking over after you are done (well, after all that might
include you; plans would change) are clueful, but there is no need
for us to make their life more difficult than necessary.