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

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

"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" &&