Re: [PATCH v6 18/20] Reftable support for git-core
From: Han-Wen Nienhuys <hidden>
Date: 2021-04-14 16:45:11
On Tue, Apr 13, 2021 at 9:18 AM Ævar Arnfjörð Bjarmason [off-list ref] wrote:
quoted
It can be activated by passing --ref-storage=reftable to "git init", or setting GIT_TEST_REFTABLE in the environment. [...] +const char *default_ref_storage(void) +{ + const char *test = getenv("GIT_TEST_REFTABLE"); + return test ? "reftable" : "files"; +}Nit: use git_env_bool() here. So GIT_TEST_REFTABLE=true is true, but GIT_TEST_REFTABLE= is not.
thanks, adapted this.
Not a nit: are these tests supposed to work / do they work for you? For me there's a *lot* of failures, e.g. t6120-describe.sh fails for reasons you might expect, manually tweaking things in the FS-refstore, but e.g. t5510-fetch.sh segfaults.
I'll have a look; segfaults should not happen, of course.
quoted
+if test_have_prereq REFTABLE +then + skip_all='skipping tests; incompatible with reftable' + test_done +fi +I understand that getting this series to land has been a pain, but this sort of thing doesn't inspire confidence. Manually running it under reftable anyway I have 17 out of 81 tests failing. A lot of tests won't run under reftable because they're of the form: test_when_finished "remove_object $tag" && echo $tag >.git/refs/tags/wrong && test_when_finished "git update-ref -d refs/tags/wrong" && I.e. we have some hack to get around update-ref guarding things for us. I would think that a better approach here would be to start with some (per-se unrelated) series to teach update-ref some mode like hash-object's --literally, i.e. "YOLO this ref update".
I disagree. I think this would be a job better suited to a test-helper. Then we don't put tools into users' hands that potentially corrupt the repository. I don't understand why hash-object --literally is not a test helper either.
Then adjust our various tests that move, clobber, or intentionally corrupt things in the refstore to use that helper/option. At that point we can actually run things like t1450-fsck.sh, maybe we simply have to skip some of them because we think the sort of corruption we're testing/worried about is impossible with reftable, but that in itself is *very* interesting.
One reason for skipping some files wholesale, is that quite a few tests are not isolated in their test functions, so skipping an individual test functions affects follow-on tests. I can't remember if t1450 is one of them, though.
I.e. do we not have to handle certain edge cases at all in fsck and friends because of inherent properties of the reftable backend (e.g. I'd assume, but don't know, that we're not likely to get one corrupt ref entry with it), but of course we can have a ref pointing to a loose object that then gets removed, and is thus corrupt. Anyway. I'm speculating, but that's also my point, that I'm having to speculate. That comes down to us having a new GIT_TEST_* mode that doesn't pass the test suite._
The speculation is warranted, but there are currently about 80 files that have some sort of test failure, and some of these are for quite common workflows. I would rather focus on those before looking at tests like fsck. Functional failures may have far reaching implications on the code structure (example: worktrees considerably complicated both the reftable library and its git-core glue), while corruption issues if they occur, will be due to localized bugs. -- Han-Wen Nienhuys - Google Munich I work 80%. Don't expect answers from me on Fridays. -- Google Germany GmbH, Erika-Mann-Strasse 33, 80636 Munich Registergericht und -nummer: Hamburg, HRB 86891 Sitz der Gesellschaft: Hamburg Geschäftsführer: Paul Manicle, Halimah DeLaine Prado -- Google Germany GmbH, Erika-Mann-Strasse 33, 80636 Munich Registergericht und -nummer: Hamburg, HRB 86891 Sitz der Gesellschaft: Hamburg Geschäftsführer: Paul Manicle, Halimah DeLaine Prado