Re: [PATCH 2/7] submodule foreach: correct path computation in recursive submodules

[Junio C Hamano <gitster@pobox.com>]

Stefan Beller <sbeller@google.com> writes:

> The test which is fixed by this patch would report
>     Entering 'nested1/nested2/../nested3'
> instead of the expected
>     Entering '../nested1/nested2/nested3'
>
> because the prefix is put unconditionally in front and after that a
> computed display path with is affected by `wt_prefix`. This is wrong as
> any relative path computation would need to be at the front. By emptying
> the `wt_prefix` in recursive submodules and adding the information of any
> relative path into the `prefix` this is fixed.
>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---

Nicely explained and executed.

FWIW, it is fine to have a fix and new tests to demonstrate the fix
to pre-existing breakages in a single step.  It is easier to review
when we can see the body of the test (as opposed to just the change
s/expect_failure/expect_success/) in the same patch as the change to
the code for a focused and small fix like these patches 1 & 2; it is
easy to partially revert the patch for such a focused and small fix
when a reviewer wants to verify that the new tests fail without the
code change.

>  git-submodule.sh             | 3 ++-
>  t/t7407-submodule-foreach.sh | 2 +-
>  2 files changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/git-submodule.sh b/git-submodule.sh
> index 43c68de..2838069 100755
> --- a/git-submodule.sh
> +++ b/git-submodule.sh
> @@ -417,10 +417,11 @@ cmd_foreach()
>  			say "$(eval_gettext "Entering '\$prefix\$displaypath'")"
>  			name=$(git submodule--helper name "$sm_path")
>  			(
> -				prefix="$prefix$sm_path/"
> +				prefix="$(relative_path $prefix$sm_path)/"

Make sure that "$prefix$sm_path" is given as a single string to
relative_path.  I'd probably write this like so:

-				prefix="$prefix$sm_path/"
+				prefix=$(relative_path "$prefix$sm_path")/

Thanks.