Prepare tests for reftable backend

From: Han-Wen Nienhuys <[email protected]>

Reftable will prohibit invalid hashes at the storage level, but git-symbolic-ref can still create branches ending in ".lock".

Signed-off-by Han-Wen Nienhuys <[email protected]>

---
 t/t4202-log.sh | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

t/t4202-log.sh +0 -0

index 350cfa35936a..c575deaad4fb 100755

t/t4202-log.sh +1 -0

+	git --git-dir empty/.git symbolic-ref HEAD refs/heads/invalid.lock &&
 	test_must_fail git -C empty log 2>stderr &&
 	test_i18ngrep broken stderr &&
 	test_must_fail git -C empty log --default totally-bogus 2>stderr &&

Signature · 1 line

gitgitgadget

t/t4202-log.sh test_expect_success 'log --graph --no-walk is forbidden' ' · line 1846

Quoted 2 lines

t/t4202-log.sh +2 -0

+test_expect_success 'log diagnoses bogus HEAD symref' '
+     git init empty &&

... and this test create a new repo in 'empty' as well. Or rather re-initializes the already existing repository there.

Is this intentional? It surely cases some confusion.

From: Han-Wen Nienhuys <[email protected]>

Signed-off-by Han-Wen Nienhuys <[email protected]>

---
 t/t9300-fast-import.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

t/t9300-fast-import.sh +0 -0

index 5c47ac4465cb..087ddf097036 100755

It seems to me that this breaks the test, to the extent that it's testing something we care about at all.

I.e. reading ea08a6fd19 (Actually allow TAG_FIXUP branches in fast-import, 2007-08-02) the whole point is to test that a "TEMP_TAG" ref is accepted by fast-import, as opposed to "refs/heads/TEMP_TAG".

From: Han-Wen Nienhuys <[email protected]>

Signed-off-by Han-Wen Nienhuys <[email protected]>

---
 t/t1401-symbolic-ref.sh | 23 +++++++++--------------
 1 file changed, 9 insertions(+), 14 deletions(-)

t/t1401-symbolic-ref.sh +1 -4

index a4ebb0b65fec..fd5980d3fb40 100755
@@ -7,18 +7,13 @@ test_description='basic symbolic-ref tests'
 # the git repo, meaning that further tests will operate on
 # the surrounding git repo instead of the trash directory.
 reset_to_sane() {
-	echo ref: refs/heads/foo >.git/HEAD
+	git --git-dir .git symbolic-ref HEAD refs/heads/foo
 }
 
-test_expect_success 'symbolic-ref writes HEAD' '
-	git symbolic-ref HEAD refs/heads/foo &&
-	echo ref: refs/heads/foo >expect &&

t/t1401-symbolic-ref.sh +2 -2

 '
 reset_to_sane
 
 test_expect_success 'symbolic-ref can delete dangling symref' '
 	git symbolic-ref NOTHEAD refs/heads/missing &&
 	git symbolic-ref -d NOTHEAD &&
-	test_path_is_missing .git/refs/heads/missing &&
-	test_path_is_missing .git/NOTHEAD
+	! git rev-parse refs/heads/missing &&
+	! git symbolic-ref NOTHEAD
 '
 reset_to_sane
 

Signature · 1 line

gitgitgadget

Quoted 1 line

+ git symbolic-ref HEAD > actual &&

Style nit: no space between redirection and filename, i.e. these should be '>expect' and '>actual'.

Quoted 1 line

+ ! git symbolic-ref NOTHEAD

Please use 'test_must_fail git ...', because otherwise a segfault could go unnoticed.

From: Han-Wen Nienhuys <[email protected]>

Signed-off-by Han-Wen Nienhuys <[email protected]>

---
 t/t1401-symbolic-ref.sh | 23 +++++++++--------------
 1 file changed, 9 insertions(+), 14 deletions(-)

t/t1401-symbolic-ref.sh +0 -0

index a4ebb0b65fec..fd5980d3fb40 100755

t/t1401-symbolic-ref.sh +0 -2

 }
 
-test_expect_success 'symbolic-ref writes HEAD' '
-	git symbolic-ref HEAD refs/heads/foo &&

Quoted 7 lines · Han-Wen Nienhuys via GitGitGadget

From: Han-Wen Nienhuys <[email protected]>

Signed-off-by Han-Wen Nienhuys <[email protected]>

---
 t/t1401-symbolic-ref.sh | 23 +++++++++--------------
 1 file changed, 9 insertions(+), 14 deletions(-)

t/t1401-symbolic-ref.sh test_description='basic symbolic-ref tests' · line 10

Quoted 10 lines

t/t1401-symbolic-ref.sh +1 -1

index a4ebb0b65fec..fd5980d3fb40 100755
@@ -7,18 +7,13 @@ test_description='basic symbolic-ref tests'
 # the git repo, meaning that further tests will operate on
 # the surrounding git repo instead of the trash directory.
 reset_to_sane() {
-     echo ref: refs/heads/foo >.git/HEAD
+     git --git-dir .git symbolic-ref HEAD refs/heads/foo

Isn't that "--git-dir .git" entirely redundant?

Quoted 4 lines

Quoted 4 lines

t/t1401-symbolic-ref.sh +0 -0

index a4ebb0b65fec..fd5980d3fb40 100755

Quoted 8 lines

t/t1401-symbolic-ref.sh test_description='basic symbolic-ref tests' · line 10

Quoted 6 lines

t/t1401-symbolic-ref.sh +1 -1

@@ -7,18 +7,13 @@ test_description='basic symbolic-ref tests'
 # the git repo, meaning that further tests will operate on
 # the surrounding git repo instead of the trash directory.
 reset_to_sane() {
-     echo ref: refs/heads/foo >.git/HEAD
+     git --git-dir .git symbolic-ref HEAD refs/heads/foo

Isn't that "--git-dir .git" entirely redundant?

See the comment above the changed line: we don't want auto-detection to clobber the surrounding git repo.

Quoted 2 lines

See the comment above the changed line: we don't want auto-detection to clobber the surrounding git repo.

Indeed, but then this is not a faithful conversion of the original. That 'echo' will write sane content to HEAD no matter what state the repository is in. That 'symbolic-ref' command, however, won't, because 'git --git-dir .git' turns off only repository discovery, but not repository verification, and in case of a corrupt '.git/HEAD' it will bail out.

  $ cd test
  $ git init
  Initialized empty Git repository in /home/szeder/src/git/test/.git/
  $ git commit --allow-empty -m initial
  [master (root-commit) ec0df0b] initial
  $ echo "foo bar baz" >.git/HEAD
  $ git --git-dir .git symbolic-ref HEAD refs/heads/master
  fatal: not a git repository: '.git'

Quoted 10 lines · SZEDER Gábor

Quoted 2 lines

See the comment above the changed line: we don't want auto-detection to clobber the surrounding git repo.

Indeed, but then this is not a faithful conversion of the original. That 'echo' will write sane content to HEAD no matter what state the repository is in. That 'symbolic-ref' command, however, won't, because 'git --git-dir .git' turns off only repository discovery, but not repository verification, and in case of a corrupt '.git/HEAD' it will bail out.

Quoted 8 lines

  $ cd test
  $ git init
  Initialized empty Git repository in /home/szeder/src/git/test/.git/
  $ git commit --allow-empty -m initial
  [master (root-commit) ec0df0b] initial
  $ echo "foo bar baz" >.git/HEAD
  $ git --git-dir .git symbolic-ref HEAD refs/heads/master
  fatal: not a git repository: '.git'

But then it's working as intended, no? It will not corrupt the surrounding repository.

I see it as the test writer's job to clean up to the extent that git-symbolic-ref can reset to a sane state.

We could reset back to a known state in a more drastic manner (extracting .git from a tar archive), but that could interfere with the test functions if they're not isolated from each other.

Quoted 2 lines

I see it as the test writer's job to clean up to the extent that git-symbolic-ref can reset to a sane state.

No, it's the job of whoever updates the cleanup routine to make sure that the updated cleanup routine still works just as well as it did in the past.

Quoted 3 lines · SZEDER Gábor

Quoted 2 lines

I see it as the test writer's job to clean up to the extent that git-symbolic-ref can reset to a sane state.

Quoted 3 lines

No, it's the job of whoever updates the cleanup routine to make sure that the updated cleanup routine still works just as well as it did in the past.

Sorry, I don't mean to shirk my responsibility. What I mean is: is there a way to signal "This test is fubar. Stop further execution of test shell script".

Quoted 9 lines · Han-Wen Nienhuys

Quoted 6 lines · SZEDER Gábor

Quoted 2 lines

I see it as the test writer's job to clean up to the extent that git-symbolic-ref can reset to a sane state.

No, it's the job of whoever updates the cleanup routine to make sure that the updated cleanup routine still works just as well as it did in the past.

Quoted 3 lines

Sorry, I don't mean to shirk my responsibility. What I mean is: is there a way to signal "This test is fubar. Stop further execution of test shell script".

Or, more generally: if a cleanup routine fails, what is the point of continuing to run the test script? If that happens, the test directory is an indeterminate state, so the outcome of follow-on test-functions is indeterminate as well.

It's not a cleanup routine that's failing, but potentially the "setup the next test" routine.

In this case it's probably best to invert the logic, i.e. to make the tests do:

        test_when_finished "rm -rf copy" &&
        git clone . copy &&
        <test-logic>

Rather than:

        reset_state &&
        <test-logic>

That also makes it more obvious what's setup and what/where tests rely on things carried forward from previous tests.

From: Han-Wen Nienhuys <[email protected]>

This makes the test independent of the particulars of the storage formats.

Signed-off-by Han-Wen Nienhuys <[email protected]>

---
 t/t1413-reflog-detach.sh | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

t/t1413-reflog-detach.sh +0 -0

index bde05208ae6a..b699c2bb7c31 100755

Quoted 1 line

+ rm -rf .git && tar -xf .git-saved.tar

Unlike GNUism longer option names like "tar --extract", you do not need a single dash when you ask "tar xf" (see our Makefile).

Looking at t/t5000-tar-tree.sh, we seem to be supposed to use "$TAR" (with double quotes) to name the tar utility, so that people can say TAR=gtar on certain platforms. I suspect that there already are a few existing violations but let's not make it worse.

From: Han-Wen Nienhuys <[email protected]>

Signed-off-by Han-Wen Nienhuys <[email protected]>

---
 t/t5000-tar-tree.sh | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

t/t5000-tar-tree.sh +0 -0

index 7204799a0b52..472fec4ee64a 100755

If we're doing a "re-style while we're at it" s/<spaces>/<tabs>/ let's just go all the way and convert it to a more modern style of:

test_expect_success 'git get-tar-commit-id' '

But maybe such re-styling would be best left alone, and just a narrow change of the needed code. There's various surrounding tests in the file that use the same style, unless we're going to re-style all of them....

From: Han-Wen Nienhuys <[email protected]>

This is more explicit, and reduces the depency between test functions. It also is more amenable to use with reftable, which has no concept of (non)existence of a reflog

Signed-off-by Han-Wen Nienhuys <[email protected]>

---
 t/t5304-prune.sh | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

t/t5304-prune.sh +0 -0

index b447ce56a9b2..4ffc0076801e 100755

Quoted 2 lines

test_expect_success 'prune: prune unreachable heads' '
-

Similar to a previous comments, a few tests in that file use this whitespacing. I'd say let's just leave it alone unlress we're doing some whitespace-only commit earlier.

Quoted 3 lines

      git config core.logAllRefUpdates false &&
-     mv .git/logs .git/logs.old &&
      : > file2 &&

Also, if we're re-styling things: ">foo" instead of ": >foo".

From: Han-Wen Nienhuys <[email protected]>

REFFILES can be used to mark tests that are specific to the packed/loose ref storage format and its limitations. Marking such tests is a preparation for introducing the reftable storage backend.

Signed-off-by Han-Wen Nienhuys <[email protected]>

---
 t/test-lib.sh | 2 ++
 1 file changed, 2 insertions(+)

t/test-lib.sh +2 -0

index d3f6af6a6545..ea7397c633db 100644
@@ -1481,6 +1481,8 @@ parisc* | hppa*)
 	;;
 esac
 
+test_set_prereq REFFILES
+
 ( COLUMNS=1 && test $COLUMNS = 1 ) && test_set_prereq COLUMNS_CAN_BE_1
 test -z "$NO_PERL" && test_set_prereq PERL
 test -z "$NO_PTHREADS" && test_set_prereq PTHREADS

Quoted 5 lines · Han-Wen Nienhuys via GitGitGadget

From: Han-Wen Nienhuys <[email protected]>

REFFILES can be used to mark tests that are specific to the packed/loose ref storage format and its limitations. Marking such tests is a preparation for introducing the reftable storage backend.

We'd want a bit of documentation either here or in t/README to explain things like how these two interact with each other, if both can be active at the same time, etc.

Quoted 20 lines

Signed-off-by Han-Wen Nienhuys <[email protected]>

---
 t/test-lib.sh | 2 ++
 1 file changed, 2 insertions(+)

t/test-lib.sh +2 -0

index d3f6af6a6545..ea7397c633db 100644
@@ -1481,6 +1481,8 @@ parisc* | hppa*)
 	;;
 esac
 
+test_set_prereq REFFILES
+
 ( COLUMNS=1 && test $COLUMNS = 1 ) && test_set_prereq COLUMNS_CAN_BE_1
 test -z "$NO_PERL" && test_set_prereq PERL
 test -z "$NO_PTHREADS" && test_set_prereq PTHREADS

Quoted 8 lines · Junio C Hamano

Quoted 5 lines · Han-Wen Nienhuys via GitGitGadget

From: Han-Wen Nienhuys <[email protected]>

REFFILES can be used to mark tests that are specific to the packed/loose ref storage format and its limitations. Marking such tests is a preparation for introducing the reftable storage backend.

Quoted 3 lines

We'd want a bit of documentation either here or in t/README to explain things like how these two interact with each other, if both can be active at the same time, etc.

By "here", I meant the place in the code where the test_set_prereq was added, not in the proposed log message that will become hard to see when test writers need to work with the sources.

Quoted 3 lines

REFFILES can be used to mark tests that are specific to the packed/loose ref storage format and its limitations. Marking such tests is a preparation for introducing the reftable storage backend.

We'd want a bit of documentation either here or in t/README to explain things like how these two interact with each other, if both can be active at the same time, etc.

From: Han-Wen Nienhuys <[email protected]>

It tries to setup a reflog by catting to .git/logs/

Signed-off-by Han-Wen Nienhuys <[email protected]>

---
 t/t1407-worktree-ref-store.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

t/t1407-worktree-ref-store.sh +0 -0

index d3fe77751122..27b57f248a94 100755

t/t1407-worktree-ref-store.sh test_expect_success 'create_symref(FOO, refs/heads/main)' ' · line 58

Quoted 5 lines

t/t1407-worktree-ref-store.sh +1 -1

-test_expect_success 'for_each_reflog()' '
+test_expect_success REFFILES 'for_each_reflog()' '
      echo $ZERO_OID > .git/logs/PSEUDO-MAIN &&
      mkdir -p     .git/logs/refs/bisect &&
      echo $ZERO_OID > .git/logs/refs/bisect/random &&

Hrm, so already the first use of REFFILES has me questioning the need for it.

I mean obviously this depends on ref-files in the strict sense.

..

* We have a PSEUDO-MAIN ref
* It's set to $ZERO_OID
* We have a $ZERO_OID in a refs/bisect/random

From: Han-Wen Nienhuys <[email protected]>

In reftable, there is no notion of a per-ref 'existence' of a reflog. Each reflog entry has its own key, so it is not possible to distinguish between {reflog doesn't exist,reflog exists but is empty}. This makes the logic in log_ref_setup() (file refs/files-backend.c), which depends on the existence of the reflog file infeasible.

Signed-off-by Han-Wen Nienhuys <[email protected]>

---
 t/t2017-checkout-orphan.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

t/t2017-checkout-orphan.sh +0 -0

index c7adbdd39ab9..88d6992a5e1f 100755

Quoted 5 lines

In reftable, there is no notion of a per-ref 'existence' of a reflog. Each reflog entry has its own key, so it is not possible to distinguish between {reflog doesn't exist,reflog exists but is empty}. This makes the logic in log_ref_setup() (file refs/files-backend.c), which depends on the existence of the reflog file infeasible.

Okey, so I'd follow this if the test was doing something like "test -e .git/logs" to test whether we didn't have reflogs for a specific branch or something....

In your v7[1] of the reftable series there's no patch to Documentation/revisions.txt altering that blurb.

So it seems to me that between this & that series there's some closing of the gap needed with how this "must have an existing log" even works under reftable.

Per [2] I had assumed that this worked under reftable by abstracting away the syntax to some query for the ref name, and faking up "file does not exist" as "there were no records" to anything like rev-parse, but it doesn't work like that?

From: Han-Wen Nienhuys <[email protected]>

Signed-off-by Han-Wen Nienhuys <[email protected]>

---
 t/t1404-update-ref-errors.sh | 30 +++++++++++++++---------------
 1 file changed, 15 insertions(+), 15 deletions(-)

t/t1404-update-ref-errors.sh +2 -2

index 8b51c4efc135..b729c1f48030 100755
@@ -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 &&

t/t1404-update-ref-errors.sh +2 -2

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

t/t1404-update-ref-errors.sh +2 -2

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

Quoted 5 lines

t/t1404-update-ref-errors.sh +1 -1

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

Review test_expect_success 'broken reference blocks create' ' · line 505

Quoted 3 lines

(quoted diff) +0 -0

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

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.

From: Han-Wen Nienhuys <[email protected]>

Signed-off-by Han-Wen Nienhuys <[email protected]>

---
 t/t7900-maintenance.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

t/t7900-maintenance.sh +0 -0

index 2412d8c5c006..6f2f55a6c51d 100755

Re [1] maybe this is ok/fine for now, but I think we should really split out the "is specific to file" part more narrowly (not just here, but in general).

E.g. I assume that "pack-refs" is simply redundant under reftable, no?

Should the current behavior documented in Documentation/git-maintenance.txt change with your series under reftable etc?

From: Han-Wen Nienhuys <[email protected]>

Signed-off-by Han-Wen Nienhuys <[email protected]>

---
 t/t7003-filter-branch.sh | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

t/t7003-filter-branch.sh +0 -0

index cf30055c88dd..380bdf934317 100755

t/t7003-filter-branch.sh +0 -0

 '
 
 test_expect_success '--remap-to-ancestor with filename filters' '

t/t7003-filter-branch.sh test_expect_success '--prune-empty is able to prune entire branch' ' · line 401

Quoted 7 lines

      git branch prune-entire B &&
      git filter-branch -f --prune-empty --index-filter "git update-index --remove A.t B.t" prune-entire &&
      test_must_fail git rev-parse refs/heads/prune-entire &&

t/t7003-filter-branch.sh +3 -1

-     test_must_fail git reflog exists refs/heads/prune-entire
+     if test_have_prereq REFFILES ; then
+             test_must_fail git reflog exists refs/heads/prune-entire
+     fi

Style. Replace " ; " with a LF and necessary number of tabs.

Quoted 6 lines

t/t7003-filter-branch.sh +0 -0

index cf30055c88dd..380bdf934317 100755
@@ -396,7 +396,9 @@ test_expect_success '--prune-empty is able to prune entire branch' '
      git branch prune-entire B &&

t/t7003-filter-branch.sh test_expect_success '--prune-empty is able to prune entire branch' ' · line 401

Quoted 6 lines

t/t7003-filter-branch.sh +3 -1

      git filter-branch -f --prune-empty --index-filter "git update-index --remove A.t B.t" prune-entire &&
      test_must_fail git rev-parse refs/heads/prune-entire &&
-     test_must_fail git reflog exists refs/heads/prune-entire
+     if test_have_prereq REFFILES ; then
+             test_must_fail git reflog exists refs/heads/prune-entire
+     fi

Style. Replace " ; " with a LF and necessary number of tabs.

From: Han-Wen Nienhuys <[email protected]>

Signed-off-by Han-Wen Nienhuys <[email protected]>

---
 t/t7003-filter-branch.sh | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

t/t7003-filter-branch.sh +0 -0

index cf30055c88dd..380bdf934317 100755

From: Han-Wen Nienhuys <[email protected]>

In reftable, hashes are correctly formed by design

Signed-off-by Han-Wen Nienhuys <[email protected]>

---
 t/t4202-log.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

t/t4202-log.sh +1 -1

index c575deaad4fb..ed6d4ecd3a28 100755
@@ -1834,7 +1834,7 @@ test_expect_success 'log --graph --no-walk is forbidden' '
 	test_must_fail git log --graph --no-walk
 '
 
-test_expect_success 'log diagnoses bogus HEAD hash' '
+test_expect_success REFFILES 'log diagnoses bogus HEAD hash' '
 	git init empty &&
 	test_must_fail git -C empty log 2>stderr &&
 	test_i18ngrep does.not.have.any.commits stderr &&

Quoted 1 line

In reftable, hashes are correctly formed by design

With the file backend this hits this in revision.c:

        if (!refname || !(flags & REF_ISSYMREF) || (flags & REF_ISBROKEN))
                die(_("your current branch appears to be broken"));

There's a REF_ISBROKEN is the reftable code, so is hitting this codepath impossible under reftable and therefore we won't need this test at all?

From: Han-Wen Nienhuys <[email protected]>

Signed-off-by Han-Wen Nienhuys <[email protected]>

---
 t/t1415-worktree-refs.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

t/t1415-worktree-refs.sh +0 -0

index 7ab91241ab7c..a8083a0af3af 100755

Quoted 12 lines · Han-Wen Nienhuys via GitGitGadget

From: Han-Wen Nienhuys <[email protected]>

Signed-off-by Han-Wen Nienhuys <[email protected]>

---
 t/t1415-worktree-refs.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

t/t1415-worktree-refs.sh +0 -0

index 7ab91241ab7c..a8083a0af3af 100755

t/t1415-worktree-refs.sh test_expect_success 'setup' ' · line 22

Quoted 8 lines

t/t1415-worktree-refs.sh +1 -1

@@ -16,7 +16,7 @@ test_expect_success 'setup' '
      git -C wt2 update-ref refs/worktree/foo HEAD
 '
 
-test_expect_success 'refs/worktree must not be packed' '
+test_expect_success REFFILES 'refs/worktree must not be packed' '
      git pack-refs --all &&
      test_path_is_missing .git/refs/tags/wt1 &&
      test_path_is_file .git/refs/worktree/foo &&

So a naïve:

    diff --git a/refs/files-backend.c b/refs/files-backend.c
    index 3f29f8c143..01e2dc8bc3 100644
    --- a/refs/files-backend.c
    +++ b/refs/files-backend.c
    @@ -212,7 +212,7 @@ static void files_ref_path(struct files_ref_store *refs,
      */
     static void add_per_worktree_entries_to_dir(struct ref_dir *dir, const char *dirname)
     {
    -       const char *prefixes[] = { "refs/bisect/", "refs/worktree/", "refs/rewritten/" };
    +       const char *prefixes[] = { "refs/bisect/", "refs/rewritten/" };
            int ip;

            if (strcmp(dirname, "refs/"))

Will fail the test under non-reftable, i.e. we somehow conflate "is packed" with correctly discovering these refs?

A discussion of how
8aff1a9ca5 (Add a place for (not) sharing stuff between worktrees, 2018-09-29) relates to reftable would be valuable here.

But on the tip of "seen" currently this test fails entirely with GIT_TEST_REFTABLE=true, so I'm not sure if it got rid of the need for this abstraction in the files backend or what...

From: Han-Wen Nienhuys <[email protected]>

Reftable will prohibit invalid hashes at the storage level, but git-symbolic-ref can still create branches ending in ".lock".

Signed-off-by Han-Wen Nienhuys <[email protected]>

---
 t/t4202-log.sh | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

t/t4202-log.sh +1 -2

index 350cfa35936a..a8c5a00d012d 100755
@@ -1834,14 +1834,18 @@ test_expect_success 'log --graph --no-walk is forbidden' '
 	test_must_fail git log --graph --no-walk
 '
 
-test_expect_success 'log diagnoses bogus HEAD' '
+test_expect_success 'log diagnoses bogus HEAD hash' '
 	git init empty &&
 	test_must_fail git -C empty log 2>stderr &&
 	test_i18ngrep does.not.have.any.commits stderr &&
 	echo 1234abcd >empty/.git/refs/heads/main &&
 	test_must_fail git -C empty log 2>stderr &&
-	test_i18ngrep broken stderr &&

When splitting up tests like:

git init xyz &&
[do with xyz]

Let's do:

    # test 1
    test_when_finished "rm -rf xyz" &&

From: Han-Wen Nienhuys <[email protected]>

This will print $ZERO_OID when asking for a non-existent ref from the test-helper.

Since resolve-ref provides direct access to refs_resolve_ref_unsafe(), it provides a reliable mechanism for accessing REFNAME, while avoiding the implicit resolution to refs/heads/REFNAME.

Signed-off-by Han-Wen Nienhuys <[email protected]>

---
 t/helper/test-ref-store.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

t/helper/test-ref-store.c +0 -0

index bba5f841c6ab..01d8f3285dc8 100644

Quoted 19 lines · Han-Wen Nienhuys via GitGitGadget

From: Han-Wen Nienhuys <[email protected]>

This will print $ZERO_OID when asking for a non-existent ref from the test-helper.

Since resolve-ref provides direct access to refs_resolve_ref_unsafe(), it provides a reliable mechanism for accessing REFNAME, while avoiding the implicit resolution to refs/heads/REFNAME.

Signed-off-by Han-Wen Nienhuys <[email protected]>

---
 t/helper/test-ref-store.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

t/helper/test-ref-store.c +0 -0

index bba5f841c6ab..01d8f3285dc8 100644

t/helper/test-ref-store.c static int cmd_for_each_ref(struct ref_store *refs, const char **argv) · line 124

Quoted 8 lines

t/helper/test-ref-store.c +1 -1

@@ -118,7 +118,7 @@ static int cmd_for_each_ref(struct ref_store *refs, const char **argv)
 
 static int cmd_resolve_ref(struct ref_store *refs, const char **argv)
 {
-     struct object_id oid;
+     struct object_id oid = { 0 };
      const char *refname = notnull(*argv++, "refname");
      int resolve_flags = arg_flags(*argv++, "resolve-flags");
      int flags;

This feels a bit magical, later we have this:

        printf("%s %s 0x%x\n", oid_to_hex(&oid), ref ? ref : "(null)", flags);

Isn't ref always going to be NULL in that case too? Wouldn't it make more sense to not zero this out and instead do:

    if (ref)
        /* current code, mostly */
    else
        use zero_oid()

That seems more straightforward to me than this implicit proxy for zero_oid(). Also, isn't the point of zero_oid() to not make this

From: Han-Wen Nienhuys <[email protected]>

Signed-off-by Han-Wen Nienhuys <[email protected]>

---
 t/t9300-fast-import.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

t/t9300-fast-import.sh +0 -0

index 5c47ac4465cb..1aea943bef72 100755

From: Han-Wen Nienhuys <[email protected]>

Signed-off-by Han-Wen Nienhuys <[email protected]>

---
 t/t1401-symbolic-ref.sh | 34 +++++++++++++++++-----------------
 1 file changed, 17 insertions(+), 17 deletions(-)

t/t1401-symbolic-ref.sh +15 -15

index a4ebb0b65fec..315a62f78019 100755
@@ -3,21 +3,20 @@
 test_description='basic symbolic-ref tests'
 . ./test-lib.sh
 
-# If the tests munging HEAD fail, they can break detection of
-# the git repo, meaning that further tests will operate on
-# the surrounding git repo instead of the trash directory.
-reset_to_sane() {
-	echo ref: refs/heads/foo >.git/HEAD
-}
-
-test_expect_success 'symbolic-ref writes HEAD' '
+test_expect_success 'setup' '
 	git symbolic-ref HEAD refs/heads/foo &&
-	echo ref: refs/heads/foo >expect &&
-	test_cmp expect .git/HEAD
+	test_commit file &&
+	"$TAR" cf .git.tar .git/
 '
 
-test_expect_success 'symbolic-ref reads HEAD' '
-	echo refs/heads/foo >expect &&
+reset_to_sane() {
+	rm -rf .git &&
+	"$TAR" xf .git.tar
+}
+
+test_expect_success 'symbolic-ref read/write roundtrip' '
+	git symbolic-ref HEAD refs/heads/read-write-roundtrip &&
+	echo refs/heads/read-write-roundtrip >expect &&
 	git symbolic-ref HEAD >actual &&
 	test_cmp expect actual
 '

@@ -25,12 +24,13 @@ test_expect_success 'symbolic-ref reads HEAD' '
 test_expect_success 'symbolic-ref refuses non-ref for HEAD' '
 	test_must_fail git symbolic-ref HEAD foo
 '
+
 reset_to_sane
 
 test_expect_success 'symbolic-ref refuses bare sha1' '
-	echo content >file && git add file && git commit -m one &&
 	test_must_fail git symbolic-ref HEAD $(git rev-parse HEAD)
 '
+
 reset_to_sane
 
 test_expect_success 'HEAD cannot be removed' '

@@ -42,16 +42,16 @@ reset_to_sane
 test_expect_success 'symbolic-ref can be deleted' '
 	git symbolic-ref NOTHEAD refs/heads/foo &&
 	git symbolic-ref -d NOTHEAD &&
-	test_path_is_file .git/refs/heads/foo &&
-	test_path_is_missing .git/NOTHEAD
+	git rev-parse refs/heads/foo &&
+	test_must_fail git symbolic-ref NOTHEAD
 '
 reset_to_sane

But since we're encountering this "reset_to_sane" pattern, can't we just as easily fix it up with something more obvious than replacing an echo to a specific ref with a tarring up and untarring of the whole .git each time?

I.e. something like:

   # setup the .git initially

Then:

   test_when_finished "rm -rf copy" &&
   git clone . copy &&
   # munge the repo and use "git -C copy" for the tests"

From: Han-Wen Nienhuys <[email protected]>

This makes the test independent of the particulars of the storage formats.

Signed-off-by Han-Wen Nienhuys <[email protected]>

---
 t/t1413-reflog-detach.sh | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

t/t1413-reflog-detach.sh +1 -2

index bde05208ae6a..934688a1ee82 100755
@@ -7,8 +7,7 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 . ./test-lib.sh
 
 reset_state () {
-	git checkout main &&
-	cp saved_reflog .git/logs/HEAD
+	rm -rf .git && "$TAR" xf .git-saved.tar
 }
 
 test_expect_success setup '

Quoted 1 line

test_expect_success baseline '

So what I said in 05, but also didn't the commit messages of 05 and 06 get mixed up / bad fixup in a WIP version? I.e. the first commit says "use symbolic ref", but it's mostly about introducing the use of this tar/untar pattern.

Whereas this continues the tarring pattern, and doesn't start using it, and (presumably) is the mis-squashed commit that should have added this whole tar thing after the first commit does the isolated s/echo/symbolic-ref/ fix.

From: Han-Wen Nienhuys <[email protected]>

Signed-off-by Han-Wen Nienhuys <[email protected]>

---
 t/t5000-tar-tree.sh | 104 ++++++++++++++++++++++----------------------
 1 file changed, 53 insertions(+), 51 deletions(-)

t/t5000-tar-tree.sh +9 -19

index 7204799a0b52..b6734cba7e65 100755
@@ -111,25 +111,25 @@ test_expect_success 'setup' '
 	EOF
 '
 
-test_expect_success \
-    'populate workdir' \
-    'mkdir a &&
-     echo simple textfile >a/a &&
-     ten=0123456789 && hundred=$ten$ten$ten$ten$ten$ten$ten$ten$ten$ten &&
-     echo long filename >a/four$hundred &&
-     mkdir a/bin &&
-     test-tool genrandom "frotz" 500000 >a/bin/sh &&
-     printf "A\$Format:%s\$O" "$SUBSTFORMAT" >a/substfile1 &&
-     printf "A not substituted O" >a/substfile2 &&
-     if test_have_prereq SYMLINKS; then
-	ln -s a a/l1
-     else
-	printf %s a > a/l1
-     fi &&
-     (p=long_path_to_a_file && cd a &&
-      for depth in 1 2 3 4 5; do mkdir $p && cd $p; done &&
-      echo text >file_with_long_path) &&
-     (cd a && find .) | sort >a.lst'
+test_expect_success 'populate workdir' '
+	mkdir a &&
+	echo simple textfile >a/a &&
+	ten=0123456789 && hundred=$ten$ten$ten$ten$ten$ten$ten$ten$ten$ten &&
+	echo long filename >a/four$hundred &&
+	mkdir a/bin &&
+	test-tool genrandom "frotz" 500000 >a/bin/sh &&
+	printf "A\$Format:%s\$O" "$SUBSTFORMAT" >a/substfile1 &&
+	printf "A not substituted O" >a/substfile2 &&

t/t5000-tar-tree.sh +11 -13

 test_expect_success \
     'add ignored file' \

@@ -147,18 +147,18 @@ test_expect_success 'setup export-subst' '
 		>a/substfile1
 '
 
-test_expect_success \
-    'create bare clone' \
-    'git clone --bare . bare.git &&
-     cp .git/info/attributes bare.git/info/attributes'
+test_expect_success 'create bare clone' '
+	git clone --bare . bare.git &&
+	cp .git/info/attributes bare.git/info/attributes
+'
 
-test_expect_success \
-    'remove ignored file' \
-    'rm a/ignored'
+test_expect_success 'remove ignored file' '
+	rm a/ignored
+'
 
-test_expect_success \
-    'git archive' \
-    'git archive HEAD >b.tar'
+test_expect_success 'git archive' '
+	git archive HEAD >b.tar
+'
 
 check_tar b
 

@@ -194,26 +194,28 @@ check_added with_untracked2 untracked one/untracked
 check_added with_untracked2 untracked two/untracked
 
 test_expect_success 'git archive on large files' '
-    test_config core.bigfilethreshold 1 &&
-    git archive HEAD >b3.tar &&
-    test_cmp_bin b.tar b3.tar
+	test_config core.bigfilethreshold 1 &&

t/t5000-tar-tree.sh +18 -16

-test_expect_success \
-    'git archive vs. the same in a bare repo' \
-    'test_cmp_bin b.tar b3.tar'
+test_expect_success 'git archive vs. the same in a bare repo' '
+	test_cmp_bin b.tar b3.tar
+'
 
-test_expect_success 'git archive with --output' \
-    'git archive --output=b4.tar HEAD &&
-    test_cmp_bin b.tar b4.tar'
+test_expect_success 'git archive with --output' '
+	git archive --output=b4.tar HEAD &&
+	test_cmp_bin b.tar b4.tar
+'
 
-test_expect_success 'git archive --remote' \
-    'git archive --remote=. HEAD >b5.tar &&
-    test_cmp_bin b.tar b5.tar'
+test_expect_success 'git archive --remote' '
+	git archive --remote=. HEAD >b5.tar &&
+	test_cmp_bin b.tar b5.tar
+'
 
 test_expect_success 'git archive --remote with configured remote' '
 	git config remote.foo.url . &&

@@ -224,13 +226,13 @@ test_expect_success 'git archive --remote with configured remote' '
 	test_cmp_bin b.tar b5-nick.tar
 '
 
-test_expect_success \
-    'validate file modification time' \
-    'mkdir extract &&
-     "$TAR" xf b.tar -C extract a/a &&
-     test-tool chmtime --get extract/a/a >b.mtime &&
-     echo "1117231200" >expected.mtime &&
-     test_cmp expected.mtime b.mtime'
+test_expect_success 'validate file modification time' '
+	mkdir extract &&
+	"$TAR" xf b.tar -C extract a/a &&
+	test-tool chmtime --get extract/a/a >b.mtime &&
+	echo "1117231200" >expected.mtime &&
+	test_cmp expected.mtime b.mtime
+'
 
 test_expect_success \
     'git get-tar-commit-id' \

Ditto subshell etc. on one line, can't this be git -C bare.git archive HEAD >b3.tar?

From: Han-Wen Nienhuys <[email protected]>

This is more explicit, and reduces the depency between test functions. It also is more amenable to use with reftable, which has no concept of (non)existence of a reflog

Quoted 5 lines · Han-Wen Nienhuys via GitGitGadget

From: Han-Wen Nienhuys <[email protected]>

This is more explicit, and reduces the depency between test functions. It also is more amenable to use with reftable, which has no concept of (non)existence of a reflog

Sounds good in principle, but:

t/t5304-prune.sh test_expect_success 'prune: prune unreachable heads' ' · line 100

Quoted 11 lines

t/t5304-prune.sh +1 -3

@@ -94,9 +94,7 @@ test_expect_success 'prune: prune unreachable heads' '
 test_expect_success 'prune: do not prune detached HEAD with no reflog' '
      git checkout --detach --quiet &&
      git commit --allow-empty -m "detached commit" &&
-     # verify that there is no reflogs
-     # (should be removed and disabled by previous test)
-     test_path_is_missing .git/logs &&
+     git reflog expire --all &&
      git prune -n >prune_actual &&
      test_must_be_empty prune_actual
 '

Isn't the point of the existing test to check that there isn't an existing reflog, not to just expire it if we find it, or does expire --all return non-zero if none was found ?

t/t5304-prune.sh test_expect_success 'prune: do not prune detached HEAD with no reflog' ' · line 108

Quoted 8 lines

t/t5304-prune.sh +1 -0

@@ -104,6 +102,7 @@ test_expect_success 'prune: do not prune detached HEAD with no reflog' '
 test_expect_success 'prune: prune former HEAD after checking out branch' '
      head_oid=$(git rev-parse HEAD) &&
      git checkout --quiet main &&
+     git reflog expire --all &&
      git prune -v >prune_actual &&
      grep "$head_oid" prune_actual
 '

Just skimming this I'm perplexed why this needs expiring now, as opposed to being s/file/command/ changes like the rest...

From: Han-Wen Nienhuys <[email protected]>

The reftable format guarantees that reflog entries are well-formed

Signed-off-by Han-Wen Nienhuys <[email protected]>

---
 t/t1414-reflog-walk.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

t/t1414-reflog-walk.sh +0 -0

index 80d94704d012..72a5ac61a520 100755

From: Han-Wen Nienhuys <[email protected]>

In reftable, there is no notion of a per-ref 'existence' of a reflog. Each reflog entry has its own key, so it is not possible to distinguish between {reflog doesn't exist,reflog exists but is empty}. This makes the logic in log_ref_setup() (file refs/files-backend.c), which depends on the existence of the reflog file infeasible.

From: Han-Wen Nienhuys <[email protected]>

Reftable automatically compacts tables on writes. The functionality of incremental compaction is unittested in reftable/stack_test.c (test_reftable_stack_auto_compaction)

In addition, pack-refs triggers a full compaction of the entire stack. This is exercised in t0031-reftable.sh.

Given that git-maintenance simply calls out git-pack-refs, it seems superfluous to test this further for reftable.

Signed-off-by Han-Wen Nienhuys <[email protected]>

---
 t/t7900-maintenance.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

t/t7900-maintenance.sh +0 -0

index 2412d8c5c006..6f2f55a6c51d 100755

t/t7900-maintenance.sh test_expect_success 'maintenance.incremental-repack.auto' ' · line 349

Quoted 5 lines

t/t7900-maintenance.sh +1 -1

-test_expect_success 'pack-refs task' '
+test_expect_success REFFILES 'pack-refs task' '
      for n in $(test_seq 1 5)
      do
              git branch -f to-pack/$n HEAD || return 1

I don't think it's superfluous to test what "git maintenance" does anyway, i.e. the test ends with:

    test_subcommand git pack-refs --all --prune

Shouldn't we test that we .. don't run that, abort, warn, whatever?

From: Han-Wen Nienhuys <[email protected]>

Signed-off-by Han-Wen Nienhuys <[email protected]>

---
 t/t7003-filter-branch.sh | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

t/t7003-filter-branch.sh +0 -0

index cf30055c88dd..e18a21895238 100755
@@ -396,7 +396,10 @@ test_expect_success '--prune-empty is able to prune entire branch' '
 	git branch prune-entire B &&

Quoted 7 lines

t/t7003-filter-branch.sh +4 -1

-     test_must_fail git reflog exists refs/heads/prune-entire
+     if test_have_prereq REFFILES
+     then
+             test_must_fail git reflog exists refs/heads/prune-entire
+     fi
 '

Quoted 1 line

test_expect_success '--remap-to-ancestor with filename filters' '

Ditto chicken and egg, but isn't this conflating "we always write logs" v.s. reftable just behaving differently, i.e. shouldn't this be positively asserting that we have the log *for that branch* still after its deletion?

From: Han-Wen Nienhuys <[email protected]>

In reftable, hashes are correctly formed by design

Signed-off-by Han-Wen Nienhuys <[email protected]>

---
 t/t4202-log.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

t/t4202-log.sh +0 -0

index a8c5a00d012d..3f969b01508c 100755

t/t4202-log.sh test_expect_success 'log --graph --no-walk is forbidden' ' · line 1840

Quoted 5 lines

t/t4202-log.sh +1 -1

-test_expect_success 'log diagnoses bogus HEAD hash' '
+test_expect_success REFFILES 'log diagnoses bogus HEAD hash' '
      git init empty &&
      test_must_fail git -C empty log 2>stderr &&
      test_i18ngrep does.not.have.any.commits stderr &&

Okey, they're correctly formed by design, but the first test here is just checking what happens when we do a "git log" on a repo with no commits. What does that have to do with reflog's guarantees that we have a valid-looking SHA in some entry in its database?

Surely we also want to test for the same thing, the ref backend doesn't change that we have no commits or refs yet, no?

From: Han-Wen Nienhuys <[email protected]>

Packing refs (and therefore checking that certain refs are not packed) is a property of the packed/loose ref storage. Add a comment to explain what the test checks.

Signed-off-by Han-Wen Nienhuys <[email protected]>

---
 t/t1415-worktree-refs.sh | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

t/t1415-worktree-refs.sh +0 -0

index 7ab91241ab7c..d6e6e523bbba 100755
@@ -16,7 +16,10 @@ test_expect_success 'setup' '
 	git -C wt2 update-ref refs/worktree/foo HEAD

Quoted 5 lines

t/t1415-worktree-refs.sh +4 -1

-test_expect_success 'refs/worktree must not be packed' '
+# The 'packed-refs' files is stored directly in .git/. This means it is global
+# to the repository, and can only contain refs that are shared across all
+# worktrees.
+test_expect_success REFFILES 'refs/worktree must not be packed' '

Quoted 3 lines

      git pack-refs --all &&
      test_path_is_missing .git/refs/tags/wt1 &&
      test_path_is_file .git/refs/worktree/foo &&

Nit but also chicken & egg: Let's keep the "pack-refs --all" though under reftable in its own test, and do the test_path_* assertions just under REFFILES, i.e. does/should pack-refs warn/error under reftable as redundant, succeed silently?

Quoted 5 lines · Ævar Arnfjörð Bjarmason

Quoted 5 lines

t/t1415-worktree-refs.sh +4 -1

-test_expect_success 'refs/worktree must not be packed' '
+# The 'packed-refs' files is stored directly in .git/. This means it is global
+# to the repository, and can only contain refs that are shared across all
+# worktrees.
+test_expect_success REFFILES 'refs/worktree must not be packed' '

Quoted 8 lines

Quoted 3 lines

      git pack-refs --all &&
      test_path_is_missing .git/refs/tags/wt1 &&
      test_path_is_file .git/refs/worktree/foo &&

Nit but also chicken & egg: Let's keep the "pack-refs --all" though under reftable in its own test, and do the test_path_* assertions just under REFFILES, i.e. does/should pack-refs warn/error under reftable as redundant, succeed silently?

I don't understand your comment. This test is checking constraints on worktrees that are only relevant for loose/packed storage. In fact, under reftable, there is no such thing as a "packed ref" storage class. This test is just not relevant for reftable, and I tried explaining in the comment why this is so. Happy to hear how to further clarify this comment.

Rewrites some tests to avoid direct filesystem access.

Introduces the test prereq REFFILES to mark other tests that depend on specifics of the files ref backend.

changes in v2 (relative to v1 from Apr 19)

* t9300: use ref-store test-helper to read toplevel tag.
* t1401: use $TAR for restoring.
* t1407: clarify what test is doing.
* t1417: "$TAR" rather than tar
* t1415: clarify test objective.
* t7003: formatting.
* README entry for REFFILES.

cc: SZEDER Gábor [email protected] cc: Ævar Arnfjörð Bjarmason [email protected] cc: Han-Wen Nienhuys [email protected]

cc Han-Wen Nienhuys [email protected]

Han-Wen Nienhuys (21):
t4202: split testcase for invalid HEAD symref and HEAD hash t/helper/ref-store: initialize oid in resolve-ref
t9300: check ref existence using test-helper rather than a file system
check
t5601: read HEAD using rev-parse
t1401-symbolic-ref: avoid direct filesystem access
t1413: use tar to save and restore entire .git directory
t1301: fix typo in error message
t5000: reformat indentation to the latest fashion
t5000: inspect HEAD using git-rev-parse
t7003: use rev-parse rather than FS inspection
t5304: restyle: trim empty lines, drop ':' before >
t5304: use "reflog expire --all" to clear the reflog
test-lib: provide test prereq REFFILES
t1407: require REFFILES for for_each_reflog test
t1414: mark corruption test with REFFILES
t2017: mark --orphan/logAllRefUpdates=false test as REFFILES
t1404: mark tests that muck with .git directly as REFFILES.
t7900: mark pack-refs tests as REFFILES
t7003: check reflog existence only for REFFILES
t4202: mark bogus head hash test with REFFILES
t1415: set REFFILES for test specific to storage format

 t/README                      |   6 ++
 t/helper/test-ref-store.c     |   2 +-
 t/t1301-shared-repo.sh        |   2 +-
 t/t1401-symbolic-ref.sh       |  34 +++++-----
 t/t1404-update-ref-errors.sh  |  30 ++++-----
 t/t1407-worktree-ref-store.sh |   9 ++-
 t/t1413-reflog-detach.sh      |   5 +-
 t/t1414-reflog-walk.sh        |   2 +-
 t/t1415-worktree-refs.sh      |   5 +-
 t/t2017-checkout-orphan.sh    |   2 +-
 t/t4202-log.sh                |  10 ++-
 t/t5000-tar-tree.sh           | 113 +++++++++++++++++-----------------
 t/t5304-prune.sh              |  83 ++++++++-----------------
 t/t5601-clone.sh              |   3 +-
 t/t7003-filter-branch.sh      |   7 ++-
 t/t7900-maintenance.sh        |   2 +-
 t/t9300-fast-import.sh        |   2 +-
 t/test-lib.sh                 |   2 +
 18 files changed, 156 insertions(+), 163 deletions(-)

base-commit: 311531c9de557d25ac087c1637818bd2aad6eb3a
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1008%2Fhanwen%2Freffiles-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1008/hanwen/reffiles-v2
Pull-Request: https://github.com/git/git/pull/1008

Range-diff vs v1:

1: 91ef012cbcc9 ! 1: 8ad4a35cda70 t4202: split testcase for invalid HEAD symref and HEAD hash

     @@ t/t4202-log.sh: test_expect_success 'log --graph --no-walk is forbidden' '
      +	test_i18ngrep broken stderr'
      +
      +test_expect_success 'log diagnoses bogus HEAD symref' '
     ++	rm -rf empty &&
      +	git init empty &&
      +	git --git-dir empty/.git symbolic-ref HEAD refs/heads/invalid.lock &&
       	test_must_fail git -C empty log 2>stderr &&
  -:  ------------ >  2:  e6222944a3eb t/helper/ref-store: initialize oid in resolve-ref
  2:  ccc26a8950be !  3:  c5855b0caa73 t9300: check ref existence using git-rev-parse rather than FS check
     @@ Metadata
      Author: Han-Wen Nienhuys <[email protected]>
      
       ## Commit message ##
     -    t9300: check ref existence using git-rev-parse rather than FS check
     +    t9300: check ref existence using test-helper rather than a file system check
      
          Signed-off-by: Han-Wen Nienhuys <[email protected]>
      
     @@ t/t9300-fast-import.sh: test_expect_success 'B: accept branch name "TEMP_TAG"' '
       		git prune" &&
       	git fast-import <input &&
      -	test -f .git/TEMP_TAG &&
     -+	git rev-parse TEMP_TAG &&
     ++	test $(test-tool ref-store main resolve-ref TEMP_TAG 0 | cut -f1 -d " " ) != "$ZERO_OID" &&
       	test $(git rev-parse main) = $(git rev-parse TEMP_TAG^)
       '

  3:  47b5ec56a383 =  4:  369c968ab837 t5601: read HEAD using rev-parse
  4:  53cf1069552b !  5:  248d9ffe7927 t1401-symbolic-ref: avoid direct filesystem access
     @@ Commit message
          Signed-off-by: Han-Wen Nienhuys <[email protected]>
      
       ## t/t1401-symbolic-ref.sh ##
     -@@ t/t1401-symbolic-ref.sh: test_description='basic symbolic-ref tests'
     - # the git repo, meaning that further tests will operate on
     - # the surrounding git repo instead of the trash directory.
     - reset_to_sane() {
     --	echo ref: refs/heads/foo >.git/HEAD
     -+	git --git-dir .git symbolic-ref HEAD refs/heads/foo
     - }
     +@@
     + test_description='basic symbolic-ref tests'
     + . ./test-lib.sh
       
     +-# If the tests munging HEAD fail, they can break detection of
     +-# the git repo, meaning that further tests will operate on
     +-# the surrounding git repo instead of the trash directory.
     +-reset_to_sane() {
     +-	echo ref: refs/heads/foo >.git/HEAD
     +-}
     +-
      -test_expect_success 'symbolic-ref writes HEAD' '
     --	git symbolic-ref HEAD refs/heads/foo &&
     ++test_expect_success 'setup' '
     + 	git symbolic-ref HEAD refs/heads/foo &&
      -	echo ref: refs/heads/foo >expect &&
      -	test_cmp expect .git/HEAD
     --'
     --
     ++	test_commit file &&
     ++	"$TAR" cf .git.tar .git/
     + '
     + 
      -test_expect_success 'symbolic-ref reads HEAD' '
      -	echo refs/heads/foo >expect &&
     --	git symbolic-ref HEAD >actual &&
     ++reset_to_sane() {
     ++	rm -rf .git &&
     ++	"$TAR" xf .git.tar
     ++}
     ++
      +test_expect_success 'symbolic-ref read/write roundtrip' '
      +	git symbolic-ref HEAD refs/heads/read-write-roundtrip &&
     -+	echo refs/heads/read-write-roundtrip > expect &&
     -+	git symbolic-ref HEAD > actual &&
     ++	echo refs/heads/read-write-roundtrip >expect &&
     + 	git symbolic-ref HEAD >actual &&
       	test_cmp expect actual
       '
     +@@ t/t1401-symbolic-ref.sh: test_expect_success 'symbolic-ref reads HEAD' '
     + test_expect_success 'symbolic-ref refuses non-ref for HEAD' '
     + 	test_must_fail git symbolic-ref HEAD foo
     + '
     ++
     + reset_to_sane
     + 
     + test_expect_success 'symbolic-ref refuses bare sha1' '
     +-	echo content >file && git add file && git commit -m one &&
     + 	test_must_fail git symbolic-ref HEAD $(git rev-parse HEAD)
     + '
     ++
     + reset_to_sane
       
     + test_expect_success 'HEAD cannot be removed' '
      @@ t/t1401-symbolic-ref.sh: reset_to_sane
       test_expect_success 'symbolic-ref can be deleted' '
       	git symbolic-ref NOTHEAD refs/heads/foo &&
     @@ t/t1401-symbolic-ref.sh: reset_to_sane
      -	test_path_is_file .git/refs/heads/foo &&
      -	test_path_is_missing .git/NOTHEAD
      +	git rev-parse refs/heads/foo &&
     -+	! git symbolic-ref NOTHEAD
     ++	test_must_fail git symbolic-ref NOTHEAD
       '
       reset_to_sane
       
     @@ t/t1401-symbolic-ref.sh: reset_to_sane
       	git symbolic-ref -d NOTHEAD &&
      -	test_path_is_missing .git/refs/heads/missing &&
      -	test_path_is_missing .git/NOTHEAD
     -+	! git rev-parse refs/heads/missing &&
     -+	! git symbolic-ref NOTHEAD
     ++	test_must_fail git rev-parse refs/heads/missing &&
     ++	test_must_fail git symbolic-ref NOTHEAD
       '
       reset_to_sane

  5:  223583594c00 !  6:  e4e8fc1d4b4f t1413: use tar to save and restore entire .git directory
     @@ t/t1413-reflog-detach.sh: export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
       reset_state () {
      -	git checkout main &&
      -	cp saved_reflog .git/logs/HEAD
     -+	rm -rf .git && tar -xf .git-saved.tar
     ++	rm -rf .git && "$TAR" xf .git-saved.tar
       }

       test_expect_success setup '
     @@ t/t1413-reflog-detach.sh: test_expect_success setup '
       	test_tick &&
       	git commit --allow-empty -m second &&
      -	cat .git/logs/HEAD >saved_reflog
     -+	tar -cf .git-saved.tar .git
     ++	"$TAR" cf .git-saved.tar .git
       '
       
       test_expect_success baseline '
  6:  70da8f5631d0 =  7:  89cc215c6014 t1301: fix typo in error message
  -:  ------------ >  8:  e67b90847c4e t5000: reformat indentation to the latest fashion
  7:  79843c0d5727 !  9:  d6072a70ae7d t5000: inspect HEAD using git-rev-parse

@@ Commit message

Signed-off-by Han-Wen Nienhuys <[email protected]>

## t/t5000-tar-tree.sh ##
-@@ t/t5000-tar-tree.sh: test_expect_success \
- test_cmp expected.mtime b.mtime'
+@@ t/t5000-tar-tree.sh: test_expect_success 'validate file modification time' '
+ test_cmp expected.mtime b.mtime
+ '

- test_expect_success \
+-test_expect_success \
- 'git get-tar-commit-id' \
- 'git get-tar-commit-id <b.tar >b.commitid &&
- test_cmp .git/$(git symbolic-ref HEAD) b.commitid'
-+ 'git get-tar-commit-id' \
-+ 'git get-tar-commit-id <b.tar >actual &&
-+ git rev-parse HEAD > expect &&
-+ test_cmp expect actual'
++test_expect_success 'git get-tar-commit-id' '
++ git get-tar-commit-id <b.tar >actual &&
++ git rev-parse HEAD >expect &&
++ test_cmp expect actual
++'

test_expect_success 'git archive with --output, override inferred format' '
git archive --format=tar --output=d4.zip HEAD &&

  8:  dbb81b5b89d8 = 10:  4bf1bf16bca3 t7003: use rev-parse rather than FS inspection
  -:  ------------ > 11:  6f15c15573af t5304: restyle: trim empty lines, drop ':' before >
  9:  ba575839e422 ! 12:  d8e80d83b6f8 t5304: use "reflog expire --all" to clear the reflog
     @@ Commit message
      
       ## t/t5304-prune.sh ##
      @@ t/t5304-prune.sh: test_expect_success 'prune: prune nonsense parameters' '
     - '
       
       test_expect_success 'prune: prune unreachable heads' '
     --
       	git config core.logAllRefUpdates false &&
      -	mv .git/logs .git/logs.old &&
     - 	: > file2 &&
     +-	: > file2 &&
     ++	>file2 &&
       	git add file2 &&
       	git commit -m temporary &&
       	tmp_head=$(git rev-list -1 HEAD) &&
     @@ t/t5304-prune.sh: test_expect_success 'prune: prune nonsense parameters' '
      +	git reflog expire --all &&
       	git prune &&
       	test_must_fail git reset $tmp_head --
     --
       '
     - 
     +@@ t/t5304-prune.sh: test_expect_success 'prune: prune unreachable heads' '
       test_expect_success 'prune: do not prune detached HEAD with no reflog' '
     - 
       	git checkout --detach --quiet &&
       	git commit --allow-empty -m "detached commit" &&
      -	# verify that there is no reflogs
     @@ t/t5304-prune.sh: test_expect_success 'prune: prune nonsense parameters' '
      +	git reflog expire --all &&
       	git prune -n >prune_actual &&
       	test_must_be_empty prune_actual
     - 
     -@@ t/t5304-prune.sh: test_expect_success 'prune: prune former HEAD after checking out branch' '
     - 
     + '
     +@@ t/t5304-prune.sh: test_expect_success 'prune: do not prune detached HEAD with no reflog' '
     + test_expect_success 'prune: prune former HEAD after checking out branch' '
       	head_oid=$(git rev-parse HEAD) &&
       	git checkout --quiet main &&
      +	git reflog expire --all &&
       	git prune -v >prune_actual &&
       	grep "$head_oid" prune_actual
     - 
     + '

10: 3d3b733c3127 ! 13: 180847f4db14 test-lib: provide test prereq REFFILES
@@ Commit message

Signed-off-by Han-Wen Nienhuys <[email protected]>

+ ## t/README ##
+@@ t/README: use these, and "test_set_prereq" for how to define your own. +
+ Git wasn't compiled with NO_PTHREADS=YesPlease.
+
++ - REFFILES
++
++ Test is specific to packed/loose ref storage, and should be
++ disabled for other ref storage backends
++
++
+ Tips for Writing Tests
+ ----------------------
+
+
## t/test-lib.sh ##
@@ t/test-lib.sh: parisc* | hppa*)
;;
11: dd1f6969c28d ! 14: f3307b62bfd7 t1407: require REFFILES for for_each_reflog test
@@ Metadata
## Commit message ##
t1407: require REFFILES for for_each_reflog test

- It tries to setup a reflog by catting to .git/logs/
+ Add extensive comment why this test needs a REFFILES annotation.
+
+ I tried forcing universal reflog creation with core.logAllRefUpdates=true, but
+ that apparently also doesn't cause reflogs to be created for pseudorefs

Signed-off-by Han-Wen Nienhuys <[email protected]>

@@ t/t1407-worktree-ref-store.sh: test_expect_success 'create_symref(FOO, refs/head
'

-test_expect_success 'for_each_reflog()' '
++# Some refs (refs/bisect/*, pseudorefs) are kept per worktree, so they should ++# only appear in the for-each-reflog output if it is called from the correct ++# worktree, which is exercised in this test. This test is poorly written (and ++# therefore marked REFFILES) for mulitple reasons: 1) it creates invalidly ++# formatted log entres. 2) it uses direct FS access for creating the reflogs. 3) ++# PSEUDO-WT and refs/bisect/random do not create reflogs by default, so it is ++# not testing a realistic scenario.
+test_expect_success REFFILES 'for_each_reflog()' '
echo $ZERO_OID > .git/logs/PSEUDO-MAIN &&
mkdir -p .git/logs/refs/bisect &&

 12:  86951eb39cb6 = 15:  0d3b18cd3542 t1414: mark corruption test with REFFILES
 13:  1ce545043846 = 16:  b64e3e7ade15 t2017: mark --orphan/logAllRefUpdates=false test as REFFILES
 14:  a3abc4f70e7d ! 17:  fcc2b714dd50 t1404: mark tests that muck with .git directly as REFFILES.

@@ Metadata
## Commit message ##
t1404: mark tests that muck with .git directly as REFFILES.

+ The packed/loose ref storage is an overlay combination of packed-refs (refs and
+ tags in a single file) and one-file-per-ref. This creates all kinds of edge
+ cases related to directory/file conflicts, (non-)empty directories, and the
+ locking scheme, none of which applies to reftable.
+

Signed-off-by Han-Wen Nienhuys <[email protected]>

## t/t1404-update-ref-errors.sh ##
15: 2b3021c4ba62 ! 18: ff3b67c84c41 t7900: mark pack-refs tests as REFFILES
@@ Metadata
## Commit message ##
t7900: mark pack-refs tests as REFFILES

+ Reftable automatically compacts tables on writes. The functionality of
+ incremental compaction is unittested in reftable/stack_test.c
+ (test_reftable_stack_auto_compaction)
+
+ In addition, pack-refs triggers a full compaction of the entire stack. This is
+ exercised in t0031-reftable.sh.
+
+ Given that git-maintenance simply calls out git-pack-refs, it seems superfluous
+ to test this further for reftable.
+

Signed-off-by Han-Wen Nienhuys <[email protected]>

## t/t7900-maintenance.sh ##
16: a5b9439192db ! 19: 24dcf05d8fa6 t7003: check reflog existence only for REFFILES
@@ t/t7003-filter-branch.sh: test_expect_success '--prune-empty is able to prune en
git filter-branch -f --prune-empty --index-filter "git update-index --remove A.t B.t" prune-entire && test_must_fail git rev-parse refs/heads/prune-entire &&
- test_must_fail git reflog exists refs/heads/prune-entire
-+ if test_have_prereq REFFILES ; then
++ if test_have_prereq REFFILES
++ then
+ test_must_fail git reflog exists refs/heads/prune-entire
+ fi
'

 17:  a2cce772d44f = 20:  a33cdfda74ff t4202: mark bogus head hash test with REFFILES
 18:  0665edb1308b ! 21:  d7e5de8dba46 t1415: set REFFILES for test specific to storage format

@@ Metadata
## Commit message ##
t1415: set REFFILES for test specific to storage format

+ Packing refs (and therefore checking that certain refs are not packed) is a
+ property of the packed/loose ref storage. Add a comment to explain what the test
+ checks.
+

Signed-off-by Han-Wen Nienhuys <[email protected]>

## t/t1415-worktree-refs.sh ##
@@ t/t1415-worktree-refs.sh: test_expect_success 'setup' '
'

-test_expect_success 'refs/worktree must not be packed' '
++# The 'packed-refs' files is stored directly in .git/. This means it is global ++# to the repository, and can only contain refs that are shared across all ++# worktrees.
+test_expect_success REFFILES 'refs/worktree must not be packed' '
git pack-refs --all &&
test_path_is_missing .git/refs/tags/wt1 &&

I looked this all over and left some nits, suspect/probably/definite bug comments on specific patches, the ones with no comments from me LGTM.

I intentionally didn't review my own earlier feedback on v1 to look at this with fresh eyes, I'd forgetten what points I raised, aside from the general "let's not skip tests but test the new behavior" mantra I think I either mentioned there or in related discussions.

Quoted 3 lines · Ævar Arnfjörð Bjarmason

I looked this all over and left some nits, suspect/probably/definite bug comments on specific patches, the ones with no comments from me LGTM.

Awesome. Thanks for reviewing. I'll try to go over your comments this week.

Quoted 4 lines

I intentionally didn't review my own earlier feedback on v1 to look at this with fresh eyes, I'd forgetten what points I raised, aside from the general "let's not skip tests but test the new behavior" mantra I think I either mentioned there or in related discussions.

"test new behavior" is troublesome, because it requires merging the reftable support first, which in itself is a tough job, and predicated on getting reviews for that code. This is why I split this series off, because it can be merged early without impacting coverage of the existing loose/packed backend.

How about I document more clearly, for each test marked REFFILES, what is going on, and what should be done for coverage with reftable? We can go over it as part of the reftable series. "REFFILES" will be an easy term to grep for.

Quoted 4 lines

Quoted 2 lines

I looked this all over and left some nits, suspect/probably/definite bug comments on specific patches, the ones with no comments from me LGTM.

Awesome. Thanks for reviewing. I'll try to go over your comments this week.

Quoted 11 lines

Quoted 4 lines

I intentionally didn't review my own earlier feedback on v1 to look at this with fresh eyes, I'd forgetten what points I raised, aside from the general "let's not skip tests but test the new behavior" mantra I think I either mentioned there or in related discussions.

"test new behavior" is troublesome, because it requires merging the reftable support first, which in itself is a tough job, and predicated on getting reviews for that code. This is why I split this series off, because it can be merged early without impacting coverage of the existing loose/packed backend.

Quoted 4 lines

How about I document more clearly, for each test marked REFFILES, what is going on, and what should be done for coverage with reftable? We can go over it as part of the reftable series. "REFFILES" will be an easy term to grep for.

Indeed, as I pointed out in some (particularly later patches) v2 commentary there's that chicken & egg problem.

Having re-skimmed my v2 reviews I think these would help:

A. Just leaning on the side of splitting up tests, e.g in my comment on
20/21[1], there's a test with file backend specific logic there, but also a general assertion. Should just be two tests.

B. My suggestion in 15/21[2] to split up maybe make these two
prerequisites, i.e. REFFILES and REFTABLE.

One one hand it's a bit silly to have two prerequisites required for the one reftable mode we don't have yet, and as long as we just have two backends it'll be an informed guess at best.

But I'm leaning on the side of it being a good idea, to clearly document those cases that are really files-backend.c specific, such as low-level test to check that a .git/HEAD exists along with a "git rev-parse HEAD" for that backend.

And then REFTABLE (and !REFTABLE) to test/skip things that are specific to the behavior of reftable, but not in a way that has to do with the "ref files" v.s. "a ref database" distinction. E.g. a maintenance command calling pack-refs is skipped under REFTABLE, but one could imagine us e.g. having a PostgreSQL backend where that