Re: [PATCH 2/7] submodule foreach: correct path computation in recursive submodules
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Mar 29, 2016, 05:44 UTC
- Message-ID
- <xmqq60w525yd.fsf@gitster.mtv.corp.google.com>
- In-Reply-To
- <1459207703-1635-3-git-send-email-sbeller@google.com>
Stefan Beller <sbeller@google.com> writes:
Show 13 quoted lines
> 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.
Show 14 quoted lines
> 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.