Re: [PATCH 5/7] t7407: make expectation as clear as possible

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 29, 2016, 19:30 UTC
Message-ID: <xmqqfuv9xerx.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <1459207703-1635-6-git-send-email-sbeller@google.com>

Stefan Beller <sbeller@google.com> writes:

Show 5 quoted lines

> Not everyone (including me) grasps the sed expression in a split second as
> they would grasp the 4 lines printed as is.
>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---

I agree that the overlong sed expression is not very readable. Spelling the expectation out like this patch does would make sense. A slight possible downside is that future changes in the test setup may require updating two places now instead of one, but I would say that would not make a very strong objection--after all, such future changes may affect the line that is munged by the sed script, in which case you'd need to change two places anyway (i.e. the previous "expect" and also the script), and updating two explicit expectation would be far easier than updating one explicit expectation and the overlong sed expression.

Perhaps this wants to come much earlier in the series? It felt a bit distracting while reading the series in sequence, derailing my train of thought.

Thanks.

Show 22 quoted lines

>  t/t7407-submodule-foreach.sh | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/t/t7407-submodule-foreach.sh b/t/t7407-submodule-foreach.sh
> index 776b349..808c6c6 100755
> --- a/t/t7407-submodule-foreach.sh
> +++ b/t/t7407-submodule-foreach.sh
> @@ -262,8 +262,12 @@ test_expect_success 'test "status --recursive"' '
>  	test_cmp expect actual
>  '
>  
> -sed -e "/nested2 /s/.*/+$nested2sha1 nested1\/nested2 (file2~1)/;/sub[1-3]/d" < expect > expect2
> -mv -f expect2 expect
> +cat > expect <<EOF
> + $nested1sha1 nested1 (heads/master)
> ++$nested2sha1 nested1/nested2 (file2~1)
> + $nested3sha1 nested1/nested2/nested3 (heads/master)
> + $submodulesha1 nested1/nested2/nested3/submodule (heads/master)
> +EOF
>  
>  test_expect_success 'ensure "status --cached --recursive" preserves the --cached flag' '
>  	(