diff mbox series

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

Message ID a3abc4f70e7d6bd833024f2c9b42ff731896e14d.1618829583.git.gitgitgadget@gmail.com (mailing list archive)
State Superseded
Headers show
Series Prepare tests for reftable backend | expand

Commit Message

Han-Wen Nienhuys April 19, 2021, 10:52 a.m. UTC
From: Han-Wen Nienhuys <hanwen@google.com>

Signed-off-by: Han-Wen Nienhuys <hanwen@google.com>
---
 t/t1404-update-ref-errors.sh | 30 +++++++++++++++---------------
 1 file changed, 15 insertions(+), 15 deletions(-)

Comments

Ævar Arnfjörð Bjarmason April 21, 2021, 6:57 a.m. UTC | #1
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.
Han-Wen Nienhuys April 26, 2021, 6:36 p.m. UTC | #2
On Wed, Apr 21, 2021 at 8:57 AM Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
> > -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.

I think it's not possible. In the files backend a broken ref is a
shortread (less than 40 digits hex) or garbage following the hex (or
maybe non-hex digits). This is all impossible with reftable.

I've removed EINVAL from reftable-backend.c

It's possible to have a corrupt reftable file, but that would surface
as  REFTABLE_FORMAT_ERROR, and is morally equivalent to an I/O error.
I added a test for this; it says:

++ git update-ref refs/heads/main 69af1687b671131ed0cfa61b7fcdc907a4c21f2c
fatal: update_ref failed for ref 'refs/heads/main': reftable:
transaction prepare: corrupt reftable file

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

I added some more explanation. I did not dig into the history or the
blame, as the tests seem relevant given what I know about how the
files backend works, but for that same reason irrelevant to reftable.
Maybe it should be documented more explicitly how the files backend
works?
diff mbox series

Patch

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
 '
 
-test_expect_success 'empty directory should not fool create' '
+test_expect_success REFFILES 'empty directory should not fool create' '
 	prefix=refs/e-create &&
 	mkdir -p .git/$prefix/foo/bar/baz &&
 	printf "create %s $C\n" $prefix/foo |
 	git update-ref --stdin
 '
 
-test_expect_success 'empty directory should not fool verify' '
+test_expect_success REFFILES 'empty directory should not fool verify' '
 	prefix=refs/e-verify &&
 	git update-ref $prefix/foo $C &&
 	git pack-refs --all &&
@@ -225,7 +225,7 @@  test_expect_success 'empty directory should not fool verify' '
 	git update-ref --stdin
 '
 
-test_expect_success 'empty directory should not fool 1-arg update' '
+test_expect_success REFFILES 'empty directory should not fool 1-arg update' '
 	prefix=refs/e-update-1 &&
 	git update-ref $prefix/foo $C &&
 	git pack-refs --all &&
@@ -234,7 +234,7 @@  test_expect_success 'empty directory should not fool 1-arg update' '
 	git update-ref --stdin
 '
 
-test_expect_success 'empty directory should not fool 2-arg update' '
+test_expect_success REFFILES 'empty directory should not fool 2-arg update' '
 	prefix=refs/e-update-2 &&
 	git update-ref $prefix/foo $C &&
 	git pack-refs --all &&
@@ -243,7 +243,7 @@  test_expect_success 'empty directory should not fool 2-arg update' '
 	git update-ref --stdin
 '
 
-test_expect_success 'empty directory should not fool 0-arg delete' '
+test_expect_success REFFILES 'empty directory should not fool 0-arg delete' '
 	prefix=refs/e-delete-0 &&
 	git update-ref $prefix/foo $C &&
 	git pack-refs --all &&
@@ -252,7 +252,7 @@  test_expect_success 'empty directory should not fool 0-arg delete' '
 	git update-ref --stdin
 '
 
-test_expect_success 'empty directory should not fool 1-arg delete' '
+test_expect_success REFFILES 'empty directory should not fool 1-arg delete' '
 	prefix=refs/e-delete-1 &&
 	git update-ref $prefix/foo $C &&
 	git pack-refs --all &&
@@ -466,7 +466,7 @@  test_expect_success 'incorrect old value blocks indirect no-deref delete' '
 	test_cmp expected output.err
 '
 
-test_expect_success 'non-empty directory blocks create' '
+test_expect_success REFFILES 'non-empty directory blocks create' '
 	prefix=refs/ne-create &&
 	mkdir -p .git/$prefix/foo/bar &&
 	: >.git/$prefix/foo/bar/baz.lock &&
@@ -485,7 +485,7 @@  test_expect_success 'non-empty directory blocks create' '
 	test_cmp expected output.err
 '
 
-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
 '
 
-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
 '
 
-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 &&