Re: [PATCH v1 6/7] t1404: don't create unused file

From: Junio C Hamano <gitster@pobox.com>
To: Andrei Rybak <rybak.a.v@gmail.com>
Cc: git@vger.kernel.org, "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>,
	"Michael J Gruber" <git@grubix.eu>, "Jeff King" <peff@peff.net>,
	"Patrick Steinhardt" <ps@pks.im>,
	"Michael Haggerty" <mhagger@alum.mit.edu>
Subject: Re: [PATCH v1 6/7] t1404: don't create unused file
Date: Mon, 13 Mar 2023 14:56:36 -0700	[thread overview]
Message-ID: <xmqqo7ows4bv.fsf@gitster.g> (raw)
In-Reply-To: <20230312201520.370234-8-rybak.a.v@gmail.com> (Andrei Rybak's message of "Sun, 12 Mar 2023 21:15:19 +0100")

Andrei Rybak <rybak.a.v@gmail.com> writes:

> Some tests in file t1404-update-ref-errors.sh create file "unchanged" as
> the expected side for a test_cmp assertion at the end of the test for
> output of "git for-each-ref".  The filename conveys the expectation that
> the output won't change between two invocations of "git for-each-ref".
>
> Test 'no bogus intermediate values during delete' also creates a file
> named "unchanged".  However, in this test the reference is being
> deleted, i.e. it _does change_.  The file itself isn't used for any
> assertions in the test.

I think the name "unchanged" is a reference to: the state recorded
in this file is before all the interesting changes done in this
test.

So another for-each-ref, after the "lock, start a process that waits
for and then removes the ref" begins but while the other process is
still waiting, whose output is compared with "unchanged" may have
been another way to perform this test, but we have "it could be $D
that is what we want, and two plausible 'wrong' answers are $C and
undefined" that is sufficient.  So I agree with removing the line
that creates "unchanged".

The other test to the file added by the same commit 6a2a7736 (t1404:
demonstrate two problems with reference transactions, 2017-09-08)
creates the "unchanged" file in the same way, but it does get used
after running "update-ref" that is tested.  I would not be surprised
if the one removed by this patch was created by a cut-and-paste by
mistake.

Thanks.

> Don't create the unused and slightly misleading file "unchanged".
>
> Signed-off-by: Andrei Rybak <rybak.a.v@gmail.com>
> ---
>  t/t1404-update-ref-errors.sh | 1 -
>  1 file changed, 1 deletion(-)
>
> diff --git a/t/t1404-update-ref-errors.sh b/t/t1404-update-ref-errors.sh
> index b5606d93b5..937ae0d733 100755
> --- a/t/t1404-update-ref-errors.sh
> +++ b/t/t1404-update-ref-errors.sh
> @@ -551,7 +551,6 @@ test_expect_success REFFILES 'no bogus intermediate values during delete' '
>  	git update-ref $prefix/foo $C &&
>  	git pack-refs --all &&
>  	git update-ref $prefix/foo $D &&
> -	git for-each-ref $prefix >unchanged &&
>  	# Now try to update the reference, but hold the `packed-refs` lock
>  	# for a while to see what happens while the process is blocked:
>  	: >.git/packed-refs.lock &&