Re: [PATCH v2 1/1] submodule: fix 'submodule status' when called from a subdirectory

>
> "Manish Goregaokar via GitGitGadget" <gitgitgadget@gmail.com>
> writes:
>
> > From: Manish Goregaokar <manishsmail@gmail.com>
> >
> > When calling `git submodule status` while in a subdirectory, we are
> > incorrectly not detecting modified submodules and
> > thus reporting that all of the submodules are unchanged.
> >
> > This is because the submodule helper is calling `diff-index` with the
> > submodule path assuming the path is relative to the current prefix
> > directory, however the submodule path used is actually relative to the root.
> >
> > Always pass NULL as the `prefix` when running diff-files on the
> > submodule, to make sure the submodule's path is interpreted as relative
> > to the superproject's repository root.
> >
> > Signed-off-by: Manish Goregaokar <manishsmail@gmail.com>
> > ---
> >  builtin/submodule--helper.c |  3 ++-
> >  t/t7400-submodule-basic.sh  | 19 +++++++++++++++++++
> >  2 files changed, 21 insertions(+), 1 deletion(-)
>
> Thanks.
>
> Will queue as-is for now, but others may have comments on the patch
> (and certainly the test part would see a few issues pointed out),
> which we may want to address before this hits the 'next' and lower
> branches.
>
> > diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
> > index a208cb26e1..4545b47ca4 100755
> > --- a/t/t7400-submodule-basic.sh
> > +++ b/t/t7400-submodule-basic.sh
> > @@ -356,6 +356,25 @@ test_expect_success 'status should only print one line' '
> >       test_line_count = 1 lines
> >  '
> >
> > +test_expect_success 'status from subdirectory should have the same SHA1' '
> > +     test_when_finished "rmdir addtest/subdir" &&
> > +     (
> > +             cd addtest &&
>
> > +             git status > /tmp/foo &&
>
> I think that you added this line for debugging the test; because
> what it does has no effect on anything in the test, let's remove it.
>
> > +             git submodule status | awk "{print \$1}" >expected &&
>
> This construct to have "git submodule status" on the left hand side
> of a pipe hides its exit status.  We wouldn't notice even if it
> crashes with a segfault, which is bad especially if it does so after
> showing the output we expect.  This instance is doubly bad because
> the output is not even compared against a known-good copy.  In fact,
> this is to create a known-good copy, so if "git submodule status"
> gets broken so badly that it crashes even before emitting anything,
> we would get an empty "expected" file (by the way, we tend to
> compare 'expect' and 'actual', not 'expected' and 'actual',
> especially in our newer tests) here, which would be compared with
> outputs from other invocations of "git submodule status" later in
> the test.  If "git ubmodule status" is so broken that it crashes
> immediately, these later invocations would die without showing any
> output, so all the actual* files would also be empty and out
> test_cmp would be very happy to report that all tests are good.
>
> Not so good.
>
>         git submodule status >output &&
>         sed -e "s/ .*// >expect &&
>
> perhaps?
>
> > +             mkdir subdir &&
>
> If the test fails before reaching this line for whatever reason,
> addtest/subdir directory won't exist, and test-when-finished would
> not be so happy.
>
> > +             cd subdir &&
> > +             git submodule status | awk "{print \$1}" >../actual &&
> > +             test_cmp ../expected ../actual &&
> > +             git -C ../submod checkout @^ &&
>
> Gahh.  Please stick to human language HEAD^ and avoid line noise @^.
>
> > +             git submodule status | awk "{print \$1}" >../actual2 &&
> > +             cd .. &&
> > +             git submodule status | awk "{print \$1}" >expected2 &&
> > +             test_cmp actual2 expected2 &&
> > +             test_must_fail test_cmp actual actual2
>
> Please do not apply test_must_fail to anything but "git subcmd".
> For now,
>
>         ! test_cmp actual actual2
>
> is a safer alternative.  Right now we are cooking a topic to allow
> us to write it as
>
>         test_cmp ! actual actual2
>
> but it hasn't been merged to 'master' yet.
>
> > +     )
> > +'
> > +
> >  test_expect_success 'setup - fetch commit name from submodule' '
> >       rev1=$(cd .subrepo && git rev-parse HEAD) &&
> >       printf "rev1: %s\n" "$rev1" &&