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

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 25, 2019, 01:56 UTC
Message-ID: <xmqqfticzpke.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <e4c932bd0907daa53d1d721f9c9400bdad17fb62.1574582473.git.gitgitgadget@gmail.com>

"Manish Goregaokar via GitGitGadget" <gitgitgadget@gmail.com> writes:

Show 19 quoted lines

> 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.

Show 12 quoted lines

> 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 @^.

Show 5 quoted lines

> +		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.

Show 6 quoted lines

> +	)
> +'
> +
>  test_expect_success 'setup - fetch commit name from submodule' '
>  	rev1=$(cd .subrepo && git rev-parse HEAD) &&
>  	printf "rev1: %s\n" "$rev1" &&