Re: [PATCH v3] t2200,t9832: avoid using 'git' upstream in a pipe

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 18, 2020, 21:01 UTC
Message-ID: <xmqqv9f7uu2l.fsf@gitster.c.googlers.com>
In-Reply-To: <pull.885.v3.git.git.1603054088108.gitgitgadget@gmail.com>

"Amanda  Shafack  via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Amanda Shafack <shafack.likhene@gmail.com>
>
> Avoid placing `git` upstream in a pipe since doing so throws away
> its exit code, thus an unexpected failure may go unnoticed.

Well explained.

> Signed-off-by: Amanda Shafack <shafack.likhene@gmail.com>
> ---

Show 12 quoted lines

> diff --git a/t/t2200-add-update.sh b/t/t2200-add-update.sh
> index f764b7e3f5..7cb7a70382 100755
> --- a/t/t2200-add-update.sh
> +++ b/t/t2200-add-update.sh
> @@ -179,7 +179,8 @@ test_expect_success 'add -u resolves unmerged paths' '
>  
>  test_expect_success '"add -u non-existent" should fail' '
>  	test_must_fail git add -u non-existent &&
> -	! (git ls-files | grep "non-existent")
> +	git ls-files >actual &&
> +	! grep "non-existent" actual
>  '

In the scope of this patch, the above change is a good rewrite. Let's stop the iteration here---you've demonstrated through the microproject that you can now comfortably be involved in the review discussion.

In a larger scope of "can we write this particular line better?", however, the above may not be the _best_ answer. For example,

	test_must_fail git ls-files --error-unmatch non-existent

would be another and a more direct way to ensure that the named path does not appear in the index.

Thanks.