[PATCHv3 0/5] submodule helper: cleanup prefix passing

[PATCHv3 0/5] submodule helper: cleanup prefix passing

[Stefan Beller]

Thanks Junio for the in-depth discussion!
I squashed the first two commits as they are actually fixing the same underlying
issue. Setting wt_prefix=<empty> is the actual preparation for switching 
the submodule--helper to git -C instead of "--prefix$prefix".
But setting wt_prefix=empty would break existing tests, so the different
computation of the prefix (relative_path of sm_path instead of plain sm_path)
is the fix of the fix.

Eric, I picked up the git -C in the tests, which are broken into 2 patches and
have a more concise commit message each.

Jacob, thanks for your 
  Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>
although I changed the code so much, that I did not put it in this series.

New in this series is the last commit, which gets rid of the prefix for
"submodule--helper clone". (It was useless to begin with; probably just
cargo-culted in because we had it in "submodule--helper list")

Thanks,
Stefan

Stefan Beller (5):
  submodule: prepare recursive path from non root directory
  submodule--helper list: lose the extra prefix option
  submodule update: add test for recursive from non root dir
  submodule sync: test syncing one submodule
  submodule--helper clone: lose the extra prefix option

 builtin/submodule--helper.c | 10 ++--------
 git-submodule.sh            | 23 +++++++++++++----------
 t/t7403-submodule-sync.sh   | 13 +++++++++----
 t/t7406-submodule-update.sh | 14 +++++++++++---
 4 files changed, 35 insertions(+), 25 deletions(-)

-- 
2.8.0.rc4.10.g52f3f33.dirty

[PATCHv3 1/5] submodule: prepare recursive path 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". A more common way in Git however
would be to use

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

which I want to change to later as it is easier to understand for
readers of the code base. 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 potential
bug by clearing wt_prefix, such that any recursive instances of will assume
to operate from the root of the respective submodule.

When changing wt_prefix, we need to make sure the reported paths stay
correct, as paths are reported relative to the wt_prefix. We can fix
this by computing the relative path before changing the wt_prefix.

As we do not pass in filenames or pathspecs into submodules recursively,
we do not need to care about filenames being relative to wt_prefix,
so fixing the display path for error reporting is the only thing we
need to do.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 git-submodule.sh | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/git-submodule.sh b/git-submodule.sh
index 43c68de..d83608c 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -825,8 +825,9 @@ 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
+				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
 			) ||
@@ -1239,7 +1241,8 @@ cmd_sync()
 
 				if test -n "$recursive"
 				then
-					prefix="$prefix$sm_path/"
+					prefix=$(relative_path "$prefix$sm_path/")
+					wt_prefix=
 					eval cmd_sync
 				fi
 			)
-- 
2.8.0.rc4.10.g52f3f33.dirty

[PATCHv3 2/5] 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. This is more readable for future
readers of the code.

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 5295b72..6c3ff91 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 d83608c..2dd1b18 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.dirty

[PATCHv3 3/5] submodule update: add test for recursive from non root dir

[Stefan Beller]

Test "submodule update --recursive" being invoked from a
sub directory as this is currently not covered by the test
suite.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 t/t7406-submodule-update.sh | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
index 68ea31d..19d8552 100755
--- a/t/t7406-submodule-update.sh
+++ b/t/t7406-submodule-update.sh
@@ -767,11 +767,19 @@ test_expect_success 'submodule update clone shallow submodule' '
 
 test_expect_success 'submodule update --recursive drops module name before recursing' '
 	(cd super2 &&
-	 (cd deeper/submodule/subsubmodule &&
-	  git checkout HEAD^
-	 ) &&
+	 git -C deeper/submodule/subsubmodule checkout HEAD^
 	 git submodule update --recursive deeper/submodule >actual &&
 	 test_i18ngrep "Submodule path .deeper/submodule/subsubmodule.: checked out" actual
 	)
 '
+
+test_expect_success 'submodule update --recursive works from subdirectory' '
+	(cd super2 &&
+	 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
+	)
+'
 test_done
-- 
2.8.0.rc4.10.g52f3f33.dirty

[PATCHv3 4/5] submodule sync: test syncing one submodule

[Stefan Beller]

The current test suite doesn't cover syncing one module only.
Instead of adding a new test, we can change existing tests to cover this
part as well.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 t/t7403-submodule-sync.sh | 13 +++++++++----
 1 file changed, 9 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
-- 
2.8.0.rc4.10.g52f3f33.dirty

[PATCHv3 5/5] submodule--helper clone: lose the extra prefix option

[Stefan Beller]

In the rewrite from shell to C (ee8838d157761, 2015-09-08, submodule:
rewrite `module_clone` shell function in C), we never made use of the
prefix. Probably it sneaked in as module_list which was converted in the
same series had the prefix as well.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 builtin/submodule--helper.c | 5 +----
 git-submodule.sh            | 4 ++--
 2 files changed, 3 insertions(+), 6 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 6c3ff91..73b5874 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -153,9 +153,6 @@ static int module_clone(int argc, const char **argv, const char *prefix)
 	struct strbuf sb = STRBUF_INIT;
 
 	struct option module_clone_options[] = {
-		OPT_STRING(0, "prefix", &prefix,
-			   N_("path"),
-			   N_("alternative anchor for relative paths")),
 		OPT_STRING(0, "path", &path,
 			   N_("path"),
 			   N_("where the new submodule will be cloned to")),
@@ -176,7 +173,7 @@ static int module_clone(int argc, const char **argv, const char *prefix)
 	};
 
 	const char *const git_submodule_helper_usage[] = {
-		N_("git submodule--helper clone [--prefix=<path>] [--quiet] "
+		N_("git submodule--helper clone [--quiet] "
 		   "[--reference <repository>] [--name <name>] [--url <url>]"
 		   "[--depth <depth>] [--] [<path>...]"),
 		NULL
diff --git a/git-submodule.sh b/git-submodule.sh
index 2dd1b18..93beada 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -347,7 +347,7 @@ Use -f if you really want to add it." >&2
 				echo "$(eval_gettext "Reactivating local git directory for submodule '\$sm_name'.")"
 			fi
 		fi
-		git submodule--helper clone ${GIT_QUIET:+--quiet} --prefix "$wt_prefix" --path "$sm_path" --name "$sm_name" --url "$realrepo" "$reference" "$depth" || exit
+		git submodule--helper clone ${GIT_QUIET:+--quiet} --path "$sm_path" --name "$sm_name" --url "$realrepo" "$reference" "$depth" || exit
 		(
 			clear_local_git_env
 			cd "$sm_path" &&
@@ -727,7 +727,7 @@ Maybe you want to use 'update --init'?")"
 
 		if ! test -d "$sm_path"/.git && ! test -f "$sm_path"/.git
 		then
-			git submodule--helper clone ${GIT_QUIET:+--quiet} --prefix "$prefix" --path "$sm_path" --name "$name" --url "$url" "$reference" "$depth" || exit
+			git submodule--helper clone ${GIT_QUIET:+--quiet} --path "$sm_path" --name "$name" --url "$url" "$reference" "$depth" || exit
 			cloned_modules="$cloned_modules;$name"
 			subsha1=
 		else
-- 
2.8.0.rc4.10.g52f3f33.dirty

Re: [PATCHv3 1/5] submodule: prepare recursive path from non root directory

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> diff --git a/git-submodule.sh b/git-submodule.sh
> index 43c68de..d83608c 100755
> --- a/git-submodule.sh
> +++ b/git-submodule.sh
> @@ -825,8 +825,9 @@ Maybe you want to use 'update --init'?")"
>  		if test -n "$recursive"
>  		then
>  			(
> -				prefix="$prefix$sm_path/"
> +				prefix="$(relative_path $prefix$sm_path)/"

Don't you need to protect yourself from IFS chars in $prefix and
$sm_path here (the other invocation of relative_path in this patch
does not have this issue, I would think).

As I think about it more, I am more inclined to say "-C $prefix" is
going in a wrong direction, so justifying this change solely on that
move is somewhat sad.

 - Making things relative should not hurt.

 - Clearing wt_prefix feels like the right thing, because we are
   moving to the root of the working tree of a different repository
   and wt_prefix that tells us where the user originally was in the
   superproject is not useful in the context of the submodule, which
   after all is a separate project.  The user however cannot refer
   to things on the filesystem (including use of pathspecs), as they
   are taken relative to the root level of each submodule by
   clearing wt_prefix, though.  i.e.

       $ git submodule $cmd --read-from-this-file=../m/file

   that is started from t/ subdirectory of the superproject that
   recurses into the submodule at m/ (sitting next to t/) should be
   able to read from "file" sitting at the root level of the working
   tree of submodule m/ by stripping ../m/ (and relative-path should
   be able to help with that), but that may become impossible
   because the fact that the user named ../m/file relative to t/
   subdirectory is lost by clearing wt_prefix that used to hold t/
   here.

is the closest justification I can come to, which is weak ("should
not hurt" does not justify, and "users cannot ever do this without
undoing this change" does not, either).

I think the worst part of this change is that the log message does
not make it clear why it is OK not to clear wt_prefix and not to use
relative_path if you use --prefix, while the "-C $prefix" approach
has problems without these changes.  Without that explanation, these
symptoms, i.e. the fact that you need the changes in this patch,
only smells like an indication that "-C $prefix" approach is somehow
flawed.  I cannot quite pinpoint what that "somehow" is, though.

>  				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
>  			) ||
> @@ -1239,7 +1241,8 @@ cmd_sync()
>  
>  				if test -n "$recursive"
>  				then
> -					prefix="$prefix$sm_path/"
> +					prefix=$(relative_path "$prefix$sm_path/")
> +					wt_prefix=
>  					eval cmd_sync
>  				fi
>  			)

Re: [PATCHv3 5/5] submodule--helper clone: lose the extra prefix option

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> In the rewrite from shell to C (ee8838d157761, 2015-09-08, submodule:
> rewrite `module_clone` shell function in C), we never made use of the
> prefix. Probably it sneaked in as module_list which was converted in the
> same series had the prefix as well.
>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---

Hmph.

This helper is called from the root level of the superproject's
working tree (after cd_to_toplevel is done), and has options like
--url.  If the user named --url with a relative pathname to a local
repository directory (or a bundle file), shouldn't it be adjusted,
and wouldn't prefix the only clue what that given path is relative
to?  Same for --reference repository's path.

I am not sure removing "--prefix=$wt_prefix" without doing "git -C
$wt_prefix" on the calling side is the right thing to do.  Even
though the options list used by this function does not seem to use
OPTION_FILENAME, parse-options API takes prefix exactly because
relative pathnames need to be adjusted, and it smells like that the
breakage brought in by this change is merely hidden by existing bugs
in the code that does not use prefix to adjust relative paths.

Re: [PATCHv3 5/5] submodule--helper clone: lose the extra prefix option

[Junio C Hamano]

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

> This helper is called from the root level of the superproject's
> working tree (after cd_to_toplevel is done), and has options like
> --url.  If the user named --url with a relative pathname to a local
> repository directory (or a bundle file), shouldn't it be adjusted,
> and wouldn't prefix the only clue what that given path is relative
> to?  Same for --reference repository's path.
>
> I am not sure removing "--prefix=$wt_prefix" without doing "git -C
> $wt_prefix" on the calling side is the right thing to do.  Even
> though the options list used by this function does not seem to use
> OPTION_FILENAME, parse-options API takes prefix exactly because
> relative pathnames need to be adjusted, and it smells like that the
> breakage brought in by this change is merely hidden by existing bugs
> in the code that does not use prefix to adjust relative paths.

As you may be able to guess from the above, I do not fundamentally
oppose to using "-C $wt_prefix" in place of "--prefix $wt_prefix".

These two should be equivalent, from the callers' point of view.

If you use the "--prefix $wt_prefix" thing, then the called program,
which starts at the root level of the working tree, just overrides
the prefix that it would get from the caller, as "prefix" its own
startup sequence computes when it is invoked by its caller is not
useful for adjusting the things that are relative to the directory
the caller originally was invoked at.

The resulting behaviour should be as if the called program were
originally started inside $wt_prefix directory, the start-up
sequence crawled up the directory hierarchy to find the root level
of the working tree and derived $wt_prefix as the prefix that is
given as the third parameter of any builtin cmd_foo().

And that is what would happen if you used "git -C $wt_prefix" to run
the program.  So "git -C $wt_prefix" should be functionally
equivalent to "--prefix $wt_prefix", even though the former is less
efficient by having to do the discovery and series of chdir(2) only
to discover the prefix the caller already has.

What bugs me the most is that, even though they should be
equivalent, you need 1/5 (or 1&2 in the old series) only when you
use "git -C $wt_prefix" but not "--prefix $wt_prefix".  That means
that they are not equivalent.  And it is not clear why, i.e. where
they differ.

It could be that the way "--prefix $wt_prefix" work is insufficient
and "git -C $wt_prefix" does more complete job, and the codepath
that implements "--prefix $wt_prefix" needs to do more to become
truly equivalent.  If that is a case, it means that there is a bug
in "--prefix $wt_prefix" codepath and the callers of the program may
be compensating for the bug by doing wrong things in order to work
around the bug.  When switching to "git -C $wt_prefix", you may be
seeing the effect of these wrong things the callers do, exposed as
bugs that need to be fixed with 1/5 (or 1&2 in the old series).

And I'd want to see the log message explain how the two ways are not
equivalent and what "--prefix $wt_prefix" and the current callers
that use that mechanism get wrong.

Re: [PATCHv3 5/5] submodule--helper clone: lose the extra prefix option

[Stefan Beller]

On Fri, Mar 25, 2016 at 1:54 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
>> This helper is called from the root level of the superproject's
>> working tree (after cd_to_toplevel is done), and has options like
>> --url.  If the user named --url with a relative pathname to a local
>> repository directory (or a bundle file), shouldn't it be adjusted,
>> and wouldn't prefix the only clue what that given path is relative
>> to?  Same for --reference repository's path.
>>
>> I am not sure removing "--prefix=$wt_prefix" without doing "git -C
>> $wt_prefix" on the calling side is the right thing to do.  Even
>> though the options list used by this function does not seem to use
>> OPTION_FILENAME, parse-options API takes prefix exactly because
>> relative pathnames need to be adjusted, and it smells like that the
>> breakage brought in by this change is merely hidden by existing bugs
>> in the code that does not use prefix to adjust relative paths.
>
> As you may be able to guess from the above, I do not fundamentally
> oppose to using "-C $wt_prefix" in place of "--prefix $wt_prefix".
>
> These two should be equivalent, from the callers' point of view.
>
> If you use the "--prefix $wt_prefix" thing, then the called program,
> which starts at the root level of the working tree, just overrides
> the prefix that it would get from the caller, as "prefix" its own
> startup sequence computes when it is invoked by its caller is not
> useful for adjusting the things that are relative to the directory
> the caller originally was invoked at.
>
> The resulting behaviour should be as if the called program were
> originally started inside $wt_prefix directory, the start-up
> sequence crawled up the directory hierarchy to find the root level
> of the working tree and derived $wt_prefix as the prefix that is
> given as the third parameter of any builtin cmd_foo().
>
> And that is what would happen if you used "git -C $wt_prefix" to run
> the program.  So "git -C $wt_prefix" should be functionally
> equivalent to "--prefix $wt_prefix", even though the former is less
> efficient by having to do the discovery and series of chdir(2) only
> to discover the prefix the caller already has.

And the chdir is the only difference.
So you may easily be tempted to think "git -C $wt_prefix" is the
same as  "--prefix $wt_prefix", like I did in the very first attempt
of this series.

The replacement works fine in all tests except for the recursive
tests as then the chdir is an important detail. In the submodule
there is no $wt_prefix (as it is the parents' wt_prefix we passed in),

So here is the example from before:
        repo/ # a superproject repository
        repo/untracked/ # an untracked dir in repo/
        repo/sub/ # a submodule
        repo/sub/subsub # a submodule of a submodule

When calling "git submodule <cmd> --recursive" from within
repo/untracked/, the recursed instance will end up in
repo/sub/ with the parents prefix ("untracked/")

In case of git -C $wt_prefix we would try to chdir into
repo/sub/untracked/ which doesn't exist and our journey ends here.

In case of "--prefix $wt_prefix" we do not chdir in there, but execute
the command in the root of the submodule i.e. "submodule--helper
list" in repo/sub/. As we do not path any path into the listing
in the recursed submodule, there will be no error reporting from
inside of  "submodule--helper list". All output is done in shell
where we have the parents "wt_prefix" and "prefix" from those two
we can derive the "displaypath" which is $(relative_path "$prefix$sm_path"
which relates to "wt_prefix".

So conclusion at this point is: wt_prefix and prefix must stay in sync.

"wt_prefix" is the relative path inside the repository from where the original
command started, and "prefix" is building up when recursing. So once
we recursed into repo/sub/subsub  we would have "prefix" = sub/subsub
and wt_prefix=untracked/, such that we can compute the displaypath to
be "../sub/subsub".

Observation:
If we had wt_prefix=(null) and prefix= "../sub/subsub", the computed
display path would be the same.

So what needs to happen to make git -C $wt_prefix behave the same as
"--prefix $wt_prefix"?

1) in recursing instances we have to have wt_prefix=(null), so that the
  execution is the same as in "--prefix $wt_prefix". This is the same, as
  we don't try to chdir into non existent directories then.
2) Because we have to keep "wt_prefix" and "prefix" in sync, we need to
  subtract the "wt_prefix" from the prefix, such that we end up with
  passing (wt_prefix=(null), prefix= "../sub/subsub") instead of
  (wt_prefix=untracked/, prefix=sub/subsub")
  I wrote subtract, because wt_prefix is always a "positive" path going
  deeper into a path of the superproject. So we can count the number of
  directories and append so many ../ to prefix, which is roughly what happens
  in relative_path.

>
> What bugs me the most is that, even though they should be
> equivalent, you need 1/5 (or 1&2 in the old series) only when you
> use "git -C $wt_prefix" but not "--prefix $wt_prefix".  That means
> that they are not equivalent.  And it is not clear why, i.e. where
> they differ.

As explained above, the chdir is the difference.
Note that I rewrote the commit message in 1/5, to explain that. Maybe
I need to be even more explicit there. The paragraph which I thought would
convey the message:

   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.

>
> It could be that the way "--prefix $wt_prefix" work is insufficient
> and "git -C $wt_prefix" does more complete job, and the codepath
> that implements "--prefix $wt_prefix" needs to do more to become
> truly equivalent.  If that is a case, it means that there is a bug
> in "--prefix $wt_prefix" codepath and the callers of the program may
> be compensating for the bug by doing wrong things in order to work
> around the bug.  When switching to "git -C $wt_prefix", you may be
> seeing the effect of these wrong things the callers do, exposed as
> bugs that need to be fixed with 1/5 (or 1&2 in the old series).
>
> And I'd want to see the log message explain how the two ways are not
> equivalent and what "--prefix $wt_prefix" and the current callers
> that use that mechanism get wrong.
>

Re: [PATCHv3 5/5] submodule--helper clone: lose the extra prefix option

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> The replacement works fine in all tests except for the recursive
> tests as then the chdir is an important detail. In the submodule
> there is no $wt_prefix (as it is the parents' wt_prefix we passed in),

So the real reason is that we may tweak $wt_prefix to refer to a
non-existing directory, that submodule--helper is buggy and does not
notice it, and that using "-C $wt_prefix" would catch it because it
first tries to chdir to it.

The calling script "git submodule" first sets $wt_prefix to the
original directory, so there is no way chdir'ing back there would
not work, but if we update it (e.g. by appending a path to a
submodule we want to work in), it may very well end up referring to
a directory that does not exist (e.g. it may not have been
"init"ed).  Is that what is going on?

If that is the case, it makes a lot more sense as an explanation.
The justification for the main change 4/5 in the log message,
i.e. "-C $wt_prefix" is more familiar, was an irrelevant subjective
statement that only said "we could change it to use -C" without
explaining why "--prefix $wt_prefix" was wrong, and that was why I
was unhappy about it.

> So here is the example from before:
>         repo/ # a superproject repository
>         repo/untracked/ # an untracked dir in repo/
>         repo/sub/ # a submodule
>         repo/sub/subsub # a submodule of a submodule
>
> When calling "git submodule <cmd> --recursive" from within
> repo/untracked/, the recursed instance will end up in
> repo/sub/ with the parents prefix ("untracked/")
>
> In case of git -C $wt_prefix we would try to chdir into
> repo/sub/untracked/ which doesn't exist and our journey ends here.

That sounds like an unrelated bug, though.  Whether -C or --prefix
is used, we shouldn't be using "repo/sub/untracked" after going to
"repo/sub".  If the <cmd> somehow wanted to refer to a relative path
(e.g. "file") in the original directory, it should be using
../untracked as the base (e.g. it would use "../untracked/file").

Of course by using -C, you might notice that repo/sub/untracked does
not exist, but that is not a proper error checking---what if the
submodule at repo/sub does have a directory with that name?  IOW,
the computation that gave repo/sub/untracked instead of ../untracked
is wrong and using -C would not make it right.

And if you clear the prefix you originally obtained in calling
script "git submodule", which is "untracked/", even if <cmd> somehow
wanted to refer to the "file" in that directory, the only clue to do
so is forever lost.  Again, this is unrelated to -C/--prefix, but
that is what the patch 2 in the original series (which was rolled
into patch 1 in the update) was about.

So I am not sure what the value of using -C is.  At least that
"example from before" does not serve as a good justification.

Re: [PATCHv3 5/5] submodule--helper clone: lose the extra prefix option

[Stefan Beller]

On Fri, Mar 25, 2016 at 3:39 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> The replacement works fine in all tests except for the recursive
>> tests as then the chdir is an important detail. In the submodule
>> there is no $wt_prefix (as it is the parents' wt_prefix we passed in),
>
> So the real reason is that we may tweak $wt_prefix to refer to a
> non-existing directory, that submodule--helper is buggy and does not
> notice it, and that using "-C $wt_prefix" would catch it because it
> first tries to chdir to it.

I agree that we tweak wt_prefix to refer to an existing path.
However I would claim that it's git-submodule.sh which is buggy, not
the submodule--helper, because the shell script gives the non-extistent
path to submodule--helper in the first place.

 "-C $wt_prefix" would catch it indeed, so we need to fix that before we
switch to -C (if at all)

>
> The calling script "git submodule" first sets $wt_prefix to the
> original directory, so there is no way chdir'ing back there would
> not work,

right,

> but if we update it (e.g. by appending a path to a
> submodule we want to work in), it may very well end up referring to
> a directory that does not exist (e.g. it may not have been
> "init"ed).  Is that what is going on?

Or by changing dirs in the shell script, i.e.

wt_prefix=$(git rev-parse --show-toplevel)
# wt_prefix is correct until we do:
cd sm_path

# If we now would "chdir'ing back there", we try to
(in $sm_path) >  cd ./$wt_prefix
# which would be  $sm_path/$wt_prefix and that may not exists.

>
> If that is the case, it makes a lot more sense as an explanation.
> The justification for the main change 4/5 in the log message,
> i.e. "-C $wt_prefix" is more familiar, was an irrelevant subjective
> statement that only said "we could change it to use -C" without
> explaining why "--prefix $wt_prefix" was wrong, and that was why I
> was unhappy about it.
>
>> So here is the example from before:
>>         repo/ # a superproject repository
>>         repo/untracked/ # an untracked dir in repo/
>>         repo/sub/ # a submodule
>>         repo/sub/subsub # a submodule of a submodule
>>
>> When calling "git submodule <cmd> --recursive" from within
>> repo/untracked/, the recursed instance will end up in
>> repo/sub/ with the parents prefix ("untracked/")
>>
>> In case of git -C $wt_prefix we would try to chdir into
>> repo/sub/untracked/ which doesn't exist and our journey ends here.
>
> That sounds like an unrelated bug, though.

It is the thing described above.

>  Whether -C or --prefix
> is used, we shouldn't be using "repo/sub/untracked" after going to
> "repo/sub".  If the <cmd> somehow wanted to refer to a relative path
> (e.g. "file") in the original directory, it should be using
> ../untracked as the base (e.g. it would use "../untracked/file").

In case of --prefix we do not chdir into "repo/sub/untracked"; however
we have cwd="repo/sub/" and wt_prefix=untracked (as it's stale from
the parent). We have no way of knowing where "untracked/" applies to.
As the root of the repository is now "repo/sub/" (It is a different repo,
the submodule), we may assume there is a "repo/sub/untracked/",
but we never compute that value or chdir there. It's just cwd and wt_prefix
being from different repositories. (superproject&submodule)

>
> Of course by using -C, you might notice that repo/sub/untracked does
> not exist, but that is not a proper error checking---what if the
> submodule at repo/sub does have a directory with that name?  IOW,
> the computation that gave repo/sub/untracked instead of ../untracked
> is wrong and using -C would not make it right.

There is no explicit computation of repo/sub/untracked, but it would happen
implicitely in the -C case as we'd be in the repo/sub and try to chdir
to 'untracked'
(interpreted as a relative path)

>
> And if you clear the prefix you originally obtained in calling
> script "git submodule", which is "untracked/", even if <cmd> somehow
> wanted to refer to the "file" in that directory, the only clue to do
> so is forever lost.  Again, this is unrelated to -C/--prefix, but
> that is what the patch 2 in the original series (which was rolled
> into patch 1 in the update) was about.

As of now this file would also be lost I would assume, as it is unclear
which repository you refer to.

If you are in the "subsub" submodule and know that the $wt_prefix=untracked,
you still don't know if the original command was invoked from the root super
project or the intermediate submodule.

In the current situation you /may/ be able to reconstruct that by going back
"prefix" as that seems to be the path we traveled so far.


>
> So I am not sure what the value of using -C is.  At least that
> "example from before" does not serve as a good justification.
>

Re: [PATCHv3 5/5] submodule--helper clone: lose the extra prefix option

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

>> Of course by using -C, you might notice that repo/sub/untracked does
>> not exist, but that is not a proper error checking---what if the
>> submodule at repo/sub does have a directory with that name?  IOW,
>> the computation that gave repo/sub/untracked instead of ../untracked
>> is wrong and using -C would not make it right.
>
> There is no explicit computation of repo/sub/untracked, but it would happen
> implicitely in the -C case as we'd be in the repo/sub and try to chdir
> to 'untracked'
> (interpreted as a relative path)

You are looking at repo/sub/untracked that does not have anything to
do with the reality, and it does not matter if you have an explicit
code to construct "char *" that points at such a pathname, or it
happens implicitly.  Looking for 'untracked' directory after going
inside 'repo/sub/untracked' is simply wrong, just like looking for
'sub/untracked' diretory while staying at 'repo/' is wrong.

If anything, ../tracked might have some relevance to the reality but
nobody is computing it, which may be a bug in "git submodule" if
<cmd> wants to have an access to the original place.

In either case, that is true with either -C/--prefix, no?

>> And if you clear the prefix you originally obtained in calling
>> script "git submodule", which is "untracked/", even if <cmd> somehow
>> wanted to refer to the "file" in that directory, the only clue to do
>> so is forever lost.  Again, this is unrelated to -C/--prefix, but
>> that is what the patch 2 in the original series (which was rolled
>> into patch 1 in the update) was about.
>
> As of now this file would also be lost I would assume, as it is unclear
> which repository you refer to.
>
> If you are in the "subsub" submodule and know that the $wt_prefix=untracked,
> you still don't know if the original command was invoked from the root super
> project or the intermediate submodule.

I am talking about a case where

        cd repo
        cd untracked
	git submodule <cmd> --recurse-submodules --read-from=file

wants to run <cmd>, using information stored in repo/untracked/file,
and work on submodules repo/sub and repo/sub/subsub.  The reference
to the original location somehow needs to be carried through if we
wanted to allow this kind of thing.

>> So I am not sure what the value of using -C is.  At least that
>> "example from before" does not serve as a good justification.

And I do not think your reply does not change anything with respect
to this statement.

Re: [PATCHv3 5/5] submodule--helper clone: lose the extra prefix option

[Stefan Beller]

On Fri, Mar 25, 2016 at 4:15 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>>> Of course by using -C, you might notice that repo/sub/untracked does
>>> not exist, but that is not a proper error checking---what if the
>>> submodule at repo/sub does have a directory with that name?  IOW,
>>> the computation that gave repo/sub/untracked instead of ../untracked
>>> is wrong and using -C would not make it right.
>>
>> There is no explicit computation of repo/sub/untracked, but it would happen
>> implicitely in the -C case as we'd be in the repo/sub and try to chdir
>> to 'untracked'
>> (interpreted as a relative path)
>
> You are looking at repo/sub/untracked that does not have anything to
> do with the reality, and it does not matter if you have an explicit
> code to construct "char *" that points at such a pathname, or it
> happens implicitly.  Looking for 'untracked' directory after going
> inside 'repo/sub/untracked' is simply wrong, just like looking for
> 'sub/untracked' diretory while staying at 'repo/' is wrong.

Right. But we don't do it. We just keep around stale information, such
that it is easily tempted to do such a thing. But we don't currently.

If we were to switch to -C we would do it, if that bug is not fixed.

>
> If anything, ../tracked might have some relevance to the reality but
> nobody is computing it, which may be a bug in "git submodule" if
> <cmd> wants to have an access to the original place.
>
> In either case, that is true with either -C/--prefix, no?

We don't compute  ../tracked in either case.

We don't have to compute that because there is no --read-from=file
switch.

>
>>> And if you clear the prefix you originally obtained in calling
>>> script "git submodule", which is "untracked/", even if <cmd> somehow
>>> wanted to refer to the "file" in that directory, the only clue to do
>>> so is forever lost.  Again, this is unrelated to -C/--prefix, but
>>> that is what the patch 2 in the original series (which was rolled
>>> into patch 1 in the update) was about.
>>
>> As of now this file would also be lost I would assume, as it is unclear
>> which repository you refer to.
>>
>> If you are in the "subsub" submodule and know that the $wt_prefix=untracked,
>> you still don't know if the original command was invoked from the root super
>> project or the intermediate submodule.
>
> I am talking about a case where
>
>         cd repo
>         cd untracked
>         git submodule <cmd> --recurse-submodules --read-from=file
>
> wants to run <cmd>, using information stored in repo/untracked/file,
> and work on submodules repo/sub and repo/sub/subsub.  The reference
> to the original location somehow needs to be carried through if we
> wanted to allow this kind of thing.

I fully agree.

1)
To carry that trough, we need:
* filepath, "the path of the file" (i.e. file)
* wt_prefix, "the path where the user typed the command relative to
repo root" ("untracked")
* prefix, "where we traveled already from repo root" (e.g. sub/)

Then the construction is easy as
    reverse(prefix) + / +  wt_prefix + / + filepath
    ..     /    untracked    /   file

That path is a relative path to the current working directory of the
command, such that we can access file.


2)
Another way to get the same:
* "filepath" , i.e. file
[* wt_prefix "relative path to the repo root", (null) as relative to repo/sub]
* prefix, "path traveled so far from where the user typed the
command", "../untracked"

Then the construction is easy as
    prefix + / + filepath
    ../untracked   /   file

We need to decide to stick with one of both interpretations. Don't mix them!


Notes on 1)
* easy model, because everything is relative to
  the superprojects repo root.
* It may be more work though as we need to resolve
  everything to that superprojects root.
* --prefix $wt_prefix works with that, as we don't try to cd $wt_prefix

Notes on 2)
* we loose the superproject as the reference, which sounds scary!
* it doesn't matter, as we have everything in the prefix
* This model works with both git -C as well as --prefix as has the right
  properties for wt_prefix.

And patch 1/5 switches from model 1) to 2).

I think model 2 is better in this context
* because it works with either git -C or --prefix
  (because of wt_prefix = (null) in all submdirectories
* only need 'prefix' to reference to 'file' (less work!)
* is easier to get right [1]


[1] Looking closer at patch1/5, cmd_status already has
  prefix="$displaypath/" with displaypath="$(relative_path $prefix$sm_path)/"
  as is done for cmd_sync and cmd_update. So before patch 1/5
  we have a bug in cmd_status as it mixes the two models
  (wt_prefix from 1) and prefix from 2)
  After patch 1/5 every command uses model 2)



>
>>> So I am not sure what the value of using -C is.  At least that
>>> "example from before" does not serve as a good justification.
>
> And I do not think your reply does not change anything with respect
> to this statement.

Re: [PATCHv3 5/5] submodule--helper clone: lose the extra prefix option

[Junio C Hamano]

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

> I am talking about a case where
>
>       cd repo
>       cd untracked
> 	git submodule <cmd> --recurse-submodules --read-from=file
>
> wants to run <cmd>, using information stored in repo/untracked/file,
> and work on submodules repo/sub and repo/sub/subsub.  The reference
> to the original location somehow needs to be carried through if we
> wanted to allow this kind of thing.

Let us step back a bit and try a simplified scenario to help us
think "prefix" through from the first principle.

Imagine there is a project P like this:

	/leading/path/Q/P/.git/
	/leading/path/Q/P/one/file
	/leading/path/Q/P/one/msg

and we do

	cd /leading/path/Q/P/one
	edit file
        git commit -F msg file

This example illustrates that there are two quite different uses of
"prefix".  It is used to serve as a name of the path that is used in
the history of the project (i.e. "file" is prefixed with "one/" to
form "one/file" that is the full path recorded in the history).  It
also is used to name an entity on the filesystem that does not have
anything to do with any path recorded in the history (i.e. the
message is read from "one/msg", even though the argument given to
"-F" is "msg").

This distinction may be hard to realize until we start doing some
unusual thing.  Suppose there is a directory Q/R just next to P and
we did this:

	cd /leading/path/Q/R
	cp ../P/one/file ../P/one/msg .
        edit file msg
	GIT_WORK_TREE=/leading/path/Q/P
        GIT_DIR=$GIT_WORK_TREE/.git
	export GIT_WORK_TREE GIT_DIR
        git commit -F msg file

What should happen (we are trying to think things through from the
first principle, so this is not asking about the implemented
behaviour)?

As this "file" refers to /leading/path/Q/R/file that is outside of
the working tree of the project, the reference to it by "git commit"
does not make any sense.  The reference to "msg", however, does make
sense--there is no reason why the contents of the message read by
"-F" option have to be inside the working tree.  So it probably is
sensible to fail this command.  It is the same story if you replace
"file" with ".", i.e. "git commit -F msg .".  The reference to
"current directory" the latter makes to name set of paths to be
recorded in the history, made outside the working tree, makes the
command a nonsense.

	Side note: I suspect that the current implementation of
	"commit" actually does wrong things to these two uses of the
	prefix in that (1) it would not find "msg" and (2) it would
	happily use an empty string as the "prefix" without failing.
	The former is something we may want to fix, but I suspect
	this is not a low-hanging fruit.  The latter is taking the
	only sensible fallback position and I do not feel a strong
	need to change it to error out.

Without "file" parameter, however, it may make sense to allow making
the commit.  After all, the command is not talking about any paths
at the point, and telling us to work on the entire working tree.

Up to this point, the scenario is simplified by not having any
submodule.  Now, imagine the case where Q is the top-level superproject
of P, i.e.

	/leading/path/Q/.git/
	/leading/path/Q/P/.git/
	/leading/path/Q/P/one/file
	/leading/path/Q/P/one/msg
	/leading/path/Q/R/file
	/leading/path/Q/R/msg

When "submodule <cmd> --recurse-submodules" does the work of <cmd>
in a submodule, as far as the submodule repository that gets
recursed into (i.e. P) is concerned, the situation is exactly like
the above example: a command that is meant to work on project P is
launched from ../R that is outside its working tree.

So a parameter like "--read-from=file" in the original example may
make sense, while taking any pathspec to be applied inside the
visited submodules does not, and instead we should declare that the
work of <cmd> for each visited submodule is to be done on the entire
working tree of the submodule.

In other words, the correct value of "prefix" for its two different
uses are different.  The "prefix" for the paths recorded in the
history (i.e. those that correspond to "file" in the above example)
has to be an empty string to denote that the work of <cmd> applies
to the entire tree, while the "prefix" for the filesystem entities
that does not have anything to do with the paths in the recorded
history (i.e. those that correspond to "msg" in the above example)
would be pointing outside the working tree (e.g. "../R").

Now, "git submodule" uses $wt_prefix to grab the return value of
"git rev-parse --show-prefix" upfront, and it _is_ possible to use
the variable to refer to the "file" in the original location in "git
submodule <cmd> --recurse-submodules --read-from=file" if we wanted
to, if its mechanism is taught to correctly maintain the variable
relative to the root of the working tree of submodules as it visits
them.

But even if we did so, we shouldn't be using that same variable for
the other use of "prefix", i.e. "which subdirectory in the working
tree of the submodule being visited is the one that is being worked
on and paths should be understood relative to".  That "prefix" has
to be empty string, and for this reason, your patch 1&2 is the right
thing to do within the context of the current code.  As we'd need
two correct values for "prefix" for two different needs, if we ever
want to allow an option like "--read-from=file", we'd need to add a
new variable different from $wt_prefix that is not cleared to an
empty string but instead is maintained to serve as "the original
location".

Two pieces of concluding remarks:

 - I am OK if we decide to postpone introducing a new variable to
   hold the value of "prefix" suitable for the other use, so that we
   do not close the door to submodule enhancements that need an
   option like "--read-from=file" in the example.

 - I suspect that the fix in your 1&2 may be demonstratable without
   forcing an early failure by switching to "git -C".  If the
   command gets a wrong prefix, its pathspec limitation would become
   incorrect, so the work of "<cmd> --recurse-submodules" that is
   done on P when run from /leading/path/Q/R/ directory with "R/" as
   a wrong prefix, would try to limit its operation to subdirectory
   "R/" in project P (which does not exist) and would miss things in
   directory "one/", even though we would want the entire tree of P/
   to be affected.