threads / patch / 41820

patch, 4 partssubmodule: fix recursive path printing from non root directory

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

## tl;dr

23 messages between Mar 24, 2016 and Mar 25, 2016. Diffs are folded; open one to read it.

replies: 22people: 4as markdown or json

Stefan Beller· Mar 24, 2016, 23:34 UTC · lore

[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
Stefan Beller· Mar 24, 2016, 23:34 UTC · re: Stefan Beller · lore

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(-)
Show changes to git-submodule.sh +2 −2
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
Jacob Keller· Mar 24, 2016, 23:38 UTC · re: Stefan Beller · lore

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

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

Show 13 quoted lines
>                                 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

Stefan Beller· Mar 24, 2016, 23:44 UTC · re: Jacob Keller · lore

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

On Thu, Mar 24, 2016 at 4:38 PM, Jacob Keller <jacob.keller@gmail.com> wrote:
Show 50 quoted lines
> 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.
Show 19 quoted lines
>
>>                                 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
Junio C Hamano· Mar 25, 2016, 16:43 UTC · re: Stefan Beller · lore

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

Stefan Beller <sbeller@google.com> writes:
Show 21 quoted lines
> 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?

Show 34 quoted lines
>
> 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
>  			)
Stefan Beller· Mar 25, 2016, 16:54 UTC · re: Junio C Hamano · lore

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

On Fri, Mar 25, 2016 at 9:43 AM, Junio C Hamano <gitster@pobox.com> wrote:
Show 26 quoted lines
> 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.

Show 35 quoted lines
>
>>
>> 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
>>                       )
Stefan Beller· Mar 24, 2016, 23:34 UTC · re: Stefan Beller · lore

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

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(+)
Show changes to git-submodule.sh +3 −0
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
Jacob Keller· Mar 24, 2016, 23:41 UTC · re: Stefan Beller · lore

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

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

Show 6 quoted lines
>                                         eval cmd_sync
>                                 fi
>                         )
> --
> 2.8.0.rc4.10.g52f3f33
>
Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>

Regards, Jake

Junio C Hamano· Mar 25, 2016, 16:46 UTC · re: Stefan Beller · lore

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

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.
Show 34 quoted lines
>
> 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
>  			)
Stefan Beller· Mar 25, 2016, 17:27 UTC · re: Junio C Hamano · lore

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

On Fri, Mar 25, 2016 at 9:46 AM, Junio C Hamano <gitster@pobox.com> wrote:
Show 10 quoted lines
> 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.
Show 37 quoted lines
>
> 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
>>                       )
Stefan Beller· Mar 24, 2016, 23:34 UTC · re: Stefan Beller · lore

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

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(-)
Show changes to 2 files +7 −10

builtin/submodule--helper.c, git-submodule.sh

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
Jacob Keller· Mar 24, 2016, 23:44 UTC · re: Stefan Beller · lore

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

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

Junio C Hamano· Mar 25, 2016, 06:28 UTC · re: Stefan Beller · lore

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

Stefan Beller <sbeller@google.com> writes:
Show 10 quoted lines
> 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.

Junio C Hamano· Mar 25, 2016, 16:02 UTC · re: Junio C Hamano · lore

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

Junio C Hamano <gitster@pobox.com> writes:
Show 25 quoted lines
> 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.
Junio C Hamano· Mar 25, 2016, 17:25 UTC · re: Junio C Hamano · lore

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

Junio C Hamano <gitster@pobox.com> writes:
Show 53 quoted lines
> 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.

Stefan Beller· Mar 25, 2016, 17:31 UTC · re: Junio C Hamano · lore

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

On Fri, Mar 25, 2016 at 10:25 AM, Junio C Hamano <gitster@pobox.com> wrote:
Show 68 quoted lines
> 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.

Stefan Beller· Mar 25, 2016, 16:49 UTC · re: Junio C Hamano · lore

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

On Thu, Mar 24, 2016 at 11:28 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 6 quoted lines
>
> 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.

Junio C Hamano· Mar 25, 2016, 17:01 UTC · re: Stefan Beller · lore

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

Stefan Beller <sbeller@google.com> writes:
Show 7 quoted lines
> 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.

Stefan Beller· Mar 25, 2016, 17:05 UTC · re: Junio C Hamano · lore

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

On Fri, Mar 25, 2016 at 10:01 AM, Junio C Hamano <gitster@pobox.com> wrote:
Show 19 quoted lines
> 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.

Junio C Hamano· Mar 25, 2016, 18:32 UTC · re: Stefan Beller · lore

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

Stefan Beller <sbeller@google.com> writes:
Show 5 quoted lines
> 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.

Stefan Beller· Mar 24, 2016, 23:34 UTC · re: Stefan Beller · lore

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

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(-)
Show changes to 2 files +21 −4

t/t7403-submodule-sync.sh, t/t7406-submodule-update.sh

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
Eric Sunshine· Mar 25, 2016, 00:25 UTC · re: Stefan Beller · lore

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

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"
Show 10 quoted lines
> 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.

Show 9 quoted lines
> 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^
Show 6 quoted lines
> +        mkdir untracked &&
> +        cd untracked &&
> +        git submodule update --recursive >actual &&
> +        test_i18ngrep "Submodule path .../deeper/submodule/subsubmodule.: checked out" actual
> +       )
> +'
Stefan Beller· Mar 25, 2016, 00:33 UTC · re: Eric Sunshine · lore

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

On Thu, Mar 24, 2016 at 5:25 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
Show 25 quoted lines
> 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!

Show 14 quoted lines
>
>> 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
Show 7 quoted lines
>
>> +        mkdir untracked &&
>> +        cd untracked &&
>> +        git submodule update --recursive >actual &&
>> +        test_i18ngrep "Submodule path .../deeper/submodule/subsubmodule.: checked out" actual
>> +       )
>> +'

← back to recent threads