Stefan Beller <sbel...@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 <sbel...@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. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
