Re: Infinite loop regression in git-fsck in v2.12.0

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Oct 30, 2018, 21:56 UTC
Message-ID: <877ehzksjd.fsf@evledraar.gmail.com>
In-Reply-To: <878t2fkxrn.fsf@evledraar.gmail.com>

On Tue, Oct 30 2018, Ævar Arnfjörð Bjarmason wrote:

Show 15 quoted lines

> The test is easy, just add a 'git fsck' at the end of t5000-tar-tree.sh,
> but more generally it seems having something like GIT_TEST_FSCK=true is
> a good idea. We do a bunch of stress testing of the object store in the
> test suite that we're unlikely to encounter in the wild.
>
> Of course my idea of how to do that in my
> <20181030184331.27264-3-avarab@gmail.com> would be counterproductive,
> i.e. it seems we want to catch all the cases where there's a bad fsck,
> just that it returns in a certain way.
>
> So maybe a good approach would be that we'd annotate all those test
> whose fsck fails with "this is how it should fail", and run those tests
> under GIT_TEST_FSCK=true, and GIT_TEST_FSCK=true would also be asserting
> that no tests other than those marked as failing the fsck check at the
> end fail it.

WIP patch for doing that:

    diff --git a/Makefile b/Makefile
    index b08d5ea258..ca624c381f 100644
    --- a/Makefile
    +++ b/Makefile
    @@ -723,6 +723,7 @@ TEST_BUILTINS_OBJS += test-dump-fsmonitor.o
     TEST_BUILTINS_OBJS += test-dump-split-index.o
     TEST_BUILTINS_OBJS += test-dump-untracked-cache.o
     TEST_BUILTINS_OBJS += test-example-decorate.o
    +TEST_BUILTINS_OBJS += test-env-bool.o
     TEST_BUILTINS_OBJS += test-genrandom.o
     TEST_BUILTINS_OBJS += test-hashmap.o
     TEST_BUILTINS_OBJS += test-index-version.o
    diff --git a/t/helper/test-tool.c b/t/helper/test-tool.c
    index 5df8b682aa..c4481085c4 100644
    --- a/t/helper/test-tool.c
    +++ b/t/helper/test-tool.c
    @@ -17,6 +17,7 @@ static struct test_cmd cmds[] = {
     	{ "dump-fsmonitor", cmd__dump_fsmonitor },
     	{ "dump-split-index", cmd__dump_split_index },
     	{ "dump-untracked-cache", cmd__dump_untracked_cache },
    +	{ "env-bool", cmd__env_bool },
     	{ "example-decorate", cmd__example_decorate },
     	{ "genrandom", cmd__genrandom },
     	{ "hashmap", cmd__hashmap },
    diff --git a/t/helper/test-tool.h b/t/helper/test-tool.h
    index 71f470b871..f7845fbc56 100644
    --- a/t/helper/test-tool.h
    +++ b/t/helper/test-tool.h
    @@ -13,6 +13,7 @@ int cmd__dump_cache_tree(int argc, const char **argv);
     int cmd__dump_fsmonitor(int argc, const char **argv);
     int cmd__dump_split_index(int argc, const char **argv);
     int cmd__dump_untracked_cache(int argc, const char **argv);
    +int cmd__env_bool(int argc, const char **argv);
     int cmd__example_decorate(int argc, const char **argv);
     int cmd__genrandom(int argc, const char **argv);
     int cmd__hashmap(int argc, const char **argv);
    diff --git a/t/t1305-config-include.sh b/t/t1305-config-include.sh
    index 635918505d..92fbce2920 100755
    --- a/t/t1305-config-include.sh
    +++ b/t/t1305-config-include.sh
    @@ -313,4 +313,8 @@ test_expect_success 'include cycles are detected' '
     	test_i18ngrep "exceeded maximum include depth" stderr
     '

    +GIT_FSCK_FAILS=true
    +GIT_FSCK_FAILS_TEST='
    +	test_i18ngrep "exceeded maximum include depth" fsck.err
    +'
     test_done
    diff --git a/t/t3103-ls-tree-misc.sh b/t/t3103-ls-tree-misc.sh
    index 14520913af..06abf84ef4 100755
    --- a/t/t3103-ls-tree-misc.sh
    +++ b/t/t3103-ls-tree-misc.sh
    @@ -22,4 +22,10 @@ test_expect_success 'ls-tree fails with non-zero exit code on broken tree' '
     	test_must_fail git ls-tree -r HEAD
     '

    +GIT_FSCK_FAILS=true
    +GIT_FSCK_FAILS_TEST='
    +	test_i18ngrep "invalid sha1 pointer in cache-tree" fsck.err &&
    +	test_i18ngrep "broken link from" fsck.out &&
    +	test_i18ngrep "missing tree" fsck.out
    +'
     test_done
    diff --git a/t/test-lib.sh b/t/test-lib.sh
    index 897e6fcc94..d4ebb94998 100644
    --- a/t/test-lib.sh
    +++ b/t/test-lib.sh
    @@ -454,6 +454,8 @@ GIT_EXIT_OK=
     trap 'die' EXIT
     trap 'exit $?' INT

    +GIT_FSCK_FAILS=
    +
     # The user-facing functions are loaded from a separate file so that
     # test_perf subshells can have them too
     . "$TEST_DIRECTORY/test-lib-functions.sh"
    @@ -790,6 +792,25 @@ test_at_end_hook_ () {
     }

     test_done () {
    +	if test_have_prereq TEST_FSCK
    +	then
    +		desc='git fsck at end (due to GIT_TEST_FSCK)'
    +		if test -n "$GIT_FSCK_FAILS"
    +		then
    +			test_expect_success "$desc (expected to fail)" '
    +				test_must_fail git fsck 2>fsck.err >fsck.out
    +			'
    +			test_expect_success "$descriptor (expected to fail) -- assert failure mode" "
    +				test_path_exists fsck.err &&
    +				test_path_exists fsck.out &&
    +				$GIT_FSCK_FAILS_TEST
    +			"
    +		else
    +			test_expect_success "$desc" '
    +				git fsck
    +			'
    +		fi
    +	fi
     	GIT_EXIT_OK=t

     	if test -z "$HARNESS_ACTIVE"
    @@ -1268,3 +1289,5 @@ test_lazy_prereq CURL '
     test_lazy_prereq SHA1 '
     	test $(git hash-object /dev/null) = e69de29bb2d1d6434b8b29ae775ad8c2e48c5391
     '
    +
    +test_lazy_prereq TEST_FSCK 'test-tool env-bool GIT_TEST_FSCK'

Could be made prettier by turning that work in test_done() into a utility function, but is (I think) worth the effort to do.

Jeff: Gotta turn in for the night, but maybe Something you're maybe
interested in carrying forward for this fix? It's not that much work to
mark up the failing tests, there's 10-20 of them from some quick
eyeballing.