git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: Stefan Beller <sbeller@google.com>
Cc: git@vger.kernel.org, Jens.Lehmann@web.de, jacob.keller@gmail.com
Subject: Re: [PATCH 5/7] t7407: make expectation as clear as possible
Date: Tue, 29 Mar 2016 12:30:58 -0700	[thread overview]
Message-ID: <xmqqfuv9xerx.fsf@gitster.mtv.corp.google.com> (raw)
In-Reply-To: <1459207703-1635-6-git-send-email-sbeller@google.com> (Stefan Beller's message of "Mon, 28 Mar 2016 16:28:21 -0700")

Stefan Beller <sbeller@google.com> writes:

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

>  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' '
>  	(

  reply	other threads:[~2016-03-29 19:31 UTC|newest]

Thread overview: 17+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-03-28 23:28 [PATCH 0/7] Fix path bugs in submodule commands executed from sub dir [WAS: submodule--helper clone: lose the extra prefix option] Stefan Beller
2016-03-28 23:28 ` [PATCH 1/7] submodule foreach: test path handling in recursive submodules Stefan Beller
2016-03-28 23:28 ` [PATCH 2/7] submodule foreach: correct path computation " Stefan Beller
2016-03-29  5:44   ` Junio C Hamano
2016-03-29 19:00     ` Junio C Hamano
2016-03-29 19:21       ` Stefan Beller
2016-03-29 19:26         ` Stefan Beller
2016-03-28 23:28 ` [PATCH 3/7] submodule update --init: test path handling " Stefan Beller
2016-03-29  5:48   ` Junio C Hamano
2016-03-28 23:28 ` [PATCH 4/7] submodule update --init: correct " Stefan Beller
2016-03-29  5:50   ` Junio C Hamano
2016-03-29 19:46   ` Junio C Hamano
2016-03-29 19:49     ` Stefan Beller
2016-03-28 23:28 ` [PATCH 5/7] t7407: make expectation as clear as possible Stefan Beller
2016-03-29 19:30   ` Junio C Hamano [this message]
2016-03-28 23:28 ` [PATCH 6/7] submodule status: test path handling in recursive submodules Stefan Beller
2016-03-28 23:28 ` [PATCH 7/7] submodule status: fix " Stefan Beller

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=xmqqfuv9xerx.fsf@gitster.mtv.corp.google.com \
    --to=gitster@pobox.com \
    --cc=Jens.Lehmann@web.de \
    --cc=git@vger.kernel.org \
    --cc=jacob.keller@gmail.com \
    --cc=sbeller@google.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).