Re: [PATCH 14/18] t1404: mark tests that muck with .git directly as REFFILES.

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Mon, Apr 19 2021, Han-Wen Nienhuys via GitGitGadget wrote:

> From: Han-Wen Nienhuys <hanwen@google.com>

Some commit message summarizing why we won't need this at all would be
better e.g. maybe:

    [...]Most of the tests being skipped here were added in 19dd7d06e5
    (t1404: demonstrate a bug resolving references, 2016-05-05), as the
    subsequent 5387c0d883 (commit_ref(): if there is an empty dir in the
    way, delete it, 2016-05-05) and e167a5673e (read_raw_ref(): don't
    get confused by an empty directory, 2016-05-05) show the code is all
    specific to the files backend, and the scenario of an "empty
    directory" "refs/heads/foo/" existing, and us wanting to create a
    "file" under "refs/heads/foo/bar" is impossible under reftable.

But:

> Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
> ---
>  t/t1404-update-ref-errors.sh | 30 +++++++++++++++---------------
>  1 file changed, 15 insertions(+), 15 deletions(-)
>
> diff --git a/t/t1404-update-ref-errors.sh b/t/t1404-update-ref-errors.sh
> index 8b51c4efc135..b729c1f48030 100755
> --- a/t/t1404-update-ref-errors.sh
> +++ b/t/t1404-update-ref-errors.sh
> @@ -189,7 +189,7 @@ test_expect_success 'one new ref is a simple prefix of another' '
>  
>  '
>  
> -test_expect_success 'empty directory should not fool rev-parse' '
> +test_expect_success REFFILES 'empty directory should not fool rev-parse' '
>  	prefix=refs/e-rev-parse &&
>  	git update-ref $prefix/foo $C &&
>  	git pack-refs --all &&
> @@ -199,7 +199,7 @@ test_expect_success 'empty directory should not fool rev-parse' '
>  	test_cmp expected actual
>  '
>  
> -test_expect_success 'empty directory should not fool for-each-ref' '
> +test_expect_success REFFILES 'empty directory should not fool for-each-ref' '
>  	prefix=refs/e-for-each-ref &&
>  	git update-ref $prefix/foo $C &&
>  	git for-each-ref $prefix >expected &&
> @@ -209,14 +209,14 @@ test_expect_success 'empty directory should not fool for-each-ref' '
>  	test_cmp expected actual
>  '


[Snip a few tests that are all of the same "empty directory form]

So far this looks good / all covering that bug

> -test_expect_success 'broken reference blocks create' '
> +test_expect_success REFFILES 'broken reference blocks create' '
>  	prefix=refs/broken-create &&
>  	mkdir -p .git/$prefix &&
>  	echo "gobbledigook" >.git/$prefix/foo &&
> @@ -504,7 +504,7 @@ test_expect_success 'broken reference blocks create' '
>  	test_cmp expected output.err
>  '

This doesn't seem specific to the files backend at all. I.e. if you grep
for "reference broken" in the file backends you'll find EINVAL and
REF_ISBROKEN handling, and your refs/reftable-backend.c has the same
exception handling, so presumably we can end up with broken refs.

Maybe not "gobbledigook", but isn't this losing coverage for other invalid ref handling under reftable?

> -test_expect_success 'non-empty directory blocks indirect create' '
> +test_expect_success REFFILES 'non-empty directory blocks indirect create' '
>  	prefix=refs/ne-indirect-create &&
>  	git symbolic-ref $prefix/symref $prefix/foo &&
>  	mkdir -p .git/$prefix/foo/bar &&
> @@ -524,7 +524,7 @@ test_expect_success 'non-empty directory blocks indirect create' '
>  	test_cmp expected output.err
>  '
>  
> -test_expect_success 'broken reference blocks indirect create' '
> +test_expect_success REFFILES 'broken reference blocks indirect create' '
>  	prefix=refs/broken-indirect-create &&
>  	git symbolic-ref $prefix/symref $prefix/foo &&
>  	echo "gobbledigook" >.git/$prefix/foo &&
> @@ -543,7 +543,7 @@ test_expect_success 'broken reference blocks indirect create' '
>  	test_cmp expected output.err
>  '

Here we seem to be back to truly file-specific thing (or maybe we were
all along):

> -test_expect_success 'no bogus intermediate values during delete' '
> +test_expect_success REFFILES 'no bogus intermediate values during delete' '
>  	prefix=refs/slow-transaction &&
>  	# Set up a reference with differing loose and packed versions:
>  	git update-ref $prefix/foo $C &&
> @@ -600,7 +600,7 @@ test_expect_success 'no bogus intermediate values during delete' '
>  	test_must_fail git rev-parse --verify --quiet $prefix/foo
>  '
>  
> -test_expect_success 'delete fails cleanly if packed-refs file is locked' '
> +test_expect_success REFFILES 'delete fails cleanly if packed-refs file is locked' '
>  	prefix=refs/locked-packed-refs &&
>  	# Set up a reference with differing loose and packed versions:
>  	git update-ref $prefix/foo $C &&
> @@ -616,7 +616,7 @@ test_expect_success 'delete fails cleanly if packed-refs file is locked' '
>  	test_cmp unchanged actual
>  '
>  
> -test_expect_success 'delete fails cleanly if packed-refs.new write fails' '
> +test_expect_success REFFILES 'delete fails cleanly if packed-refs.new write fails' '
>  	# Setup and expectations are similar to the test above.
>  	prefix=refs/failed-packed-refs &&
>  	git update-ref $prefix/foo $C &&

Anyway, much of reviewing this commit was trying to rediscover thing
that should really be in the commit message. Presumably you had to run
blame, log etc. to find the commits from Michael Haggerty and dig into
if they were relevant to reftable. Having that information in the commit
message would be *very* helpful.