I'll make the changes and submit, thanks.
> sed -e "s/ .*// >expect &&
This won't work since the output may start with a space, but I'll do
this as a two-step thing with awk
-Manish Goregaokar
On Sun, Nov 24, 2019 at 5:56 PM Junio C Hamano <gitster@xxxxxxxxx> wrote:
>
> "Manish Goregaokar via GitGitGadget" <gitgitgadget@xxxxxxxxx>
> writes:
>
> > From: Manish Goregaokar <manishsmail@xxxxxxxxx>
> >
> > 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@xxxxxxxxx>
> > ---
> > 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" &&
