Re: [PATCH v2 00/21] Prepare tests for reftable backend
- From
Ævar Arnfjörð Bjarmason <avarab@gmail.com>
- Date
- May 20, 2021, 16:28 UTC
- Message-ID
- <878s49pdmd.fsf@evledraar.gmail.com>
- In-Reply-To
- <pull.1008.v2.git.git.1619519903.gitgitgadget@gmail.com>
On Tue, Apr 27 2021, Han-Wen Nienhuys via GitGitGadget wrote:
Show 406 quoted lines
> 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 szeder.dev@gmail.com cc: Ævar Arnfjörð Bjarmason
> avarab@gmail.com cc: Han-Wen Nienhuys hanwen@google.com
>
> cc: Han-Wen Nienhuys hanwen@google.com
>
> 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 <hanwen@google.com>
>
> ## 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 <hanwen@google.com>
>
> @@ 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 <hanwen@google.com>
>
> ## 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 <hanwen@google.com>
>
> ## 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 <hanwen@google.com>
>
> + ## 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 <hanwen@google.com>
>
> @@ 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 <hanwen@google.com>
>
> ## 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 <hanwen@google.com>
>
> ## 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 <hanwen@google.com>
>
> ## 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.