On Wed, Apr 21 2021, Han-Wen Nienhuys wrote: > On Wed, Apr 21, 2021 at 9:45 AM Ævar Arnfjörð Bjarmason > <avarab@xxxxxxxxx> wrote: >> > The commits up to "hash.h: provide constants for the hash IDs" should be >> > good to merge to 'next'. >> >> With how Junio queues things up perhaps submitting this as another >> "prep" series would be good, to reduce future reviewer fatigue, >> i.e. anything we can make land on master makes re-rolls that much >> smaller. > > will do. > >> > There are several test fixups, but I've put them in another series because >> > GGG enforces max 30 commits. >> >> I left a bunch of comments on the test prep series now. Probably good to >> have it split up regardless of GGG limits. >> >> Re the comments I left on the test series. I'm very happy to see the >> start of addressing the "it must be tested" concerns I had in >> https://lore.kernel.org/git/87wnt2th1v.fsf@xxxxxxxxxxxxxxxxxxx/ > > It may look like the start, but I've been improving the number of > tests that pass continuously since I posted the first version of this > code over a year ago. FWIW I meant or meant to say something closer to "a start at the numerous failures I noted in the v6 discussion", not "no work has been done at all on this front". Sorry. >> I don't see the point of having re-rolls of this topic while the test >> changes topic it's based on hasn't finished >> marking/splitting/refactoring the various tests that do and don't depend >> on the file backend. >> >> At least when I review it I'm just left with going in circles digging >> into one of those failing test, figuring out if it's really >> refs/files-backend.c specific or not etc., and as long as we can't turn >> on GIT_TEST_REFTABLE=true in CI as part of this series I don't see a >> path to making it advance to next->master. > > The point of posting updates is to garner feedback, especially from > people familiar with the Git code itself. So you agree that we should make the tests pass first, then shouldn't these me marked as RFC/PATCH? I for one would find that a lot less confusing, "PATCH" means "the author considers this ready to land on master sans undiscovered bugs etc. > If you would like this effort to move forward faster, Yes, I'm keen to help move it forward. I am saying that I think for me or anyone else to do that in any sensible way the path forward is to make the tests pass with GIT_TEST_REFTABLE=true. IOW a large part of the feedback you're looking for is already part of the codebase. Nobody can keep all of it in their head, but we've encoded all the tricky edge cases we could think of in the tests. So in particular per my feedback on the test series: It's only when we start digging into those tests that we discover the interesting bits, i.e. how things like .git/SOME_FILE behaves per[1], and my comments about reflog behavior in [2]. Anyway, I think I'm just repeating myself at this point, but part of the reason is that I don't think I've gotten a straight answer about the point-by-point questions I had in [3]. I.e. reaching some consensus on things like whether GIT_TEST_REFTABLE=true passing under CI being a hard-prereq for this series or not is surely one of the first things to sketch out before figuring out how to move this forward. Also, with how unhappy Junio is with my patch-dumping I'm probably the last person to give advice about managing mailing list attention, but still: here's an attempt: In v6 I noted that t5510-fetch.sh had a segfault[4], you said you'd check it out, and reading your cover letter nothing stood out about that, so I assumed it was sorted out somehow. But running it now yields a BUG() instead: BUG: refs.c:1038: free called on a prepared reference transaction Aborted [...] not ok 18 - fetch --atomic aborts all reference updates if hook aborts And trying the whole test suite with --verbose-log yields: $ grep -e 'Segmentation fault' -e BUG: test-results/* test-results/t0210-trace2-normal.out:BUG: t/helper/test-trace2.c:206: the bug message test-results/t1400-update-ref.out:BUG: refs.c:1038: free called on a prepared reference transaction test-results/t1400-update-ref.out:BUG: refs.c:1038: free called on a prepared reference transaction test-results/t4058-diff-duplicates.out:Segmentation fault test-results/t4058-diff-duplicates.out:Segmentation fault test-results/t4058-diff-duplicates.out:Segmentation fault test-results/t4058-diff-duplicates.out:Segmentation fault test-results/t5510-fetch.out:BUG: refs.c:1038: free called on a prepared reference transaction test-results/t5510-fetch.out:BUG: refs.c:1038: free called on a prepared reference transaction test-results/t5600-clone-fail-cleanup.out:Segmentation fault test-results/t5600-clone-fail-cleanup.out:Segmentation fault test-results/t5600-clone-fail-cleanup.out:Segmentation fault test-results/t5601-clone.out:Segmentation fault test-results/t6423-merge-rename-directories.out: test_i18ngrep ! BUG: err && Not all of that is yours, FWIW "seen" is currently doing, and the "diff" failures are ac14de13b2 (t4058: explore duplicate tree entry handling in a bit more detail, 2020-12-11). $ grep -e 'Segmentation fault' -e BUG: test-results/* test-results/t0210-trace2-normal.out:BUG: t/helper/test-trace2.c:206: the bug message test-results/t1406-submodule-ref-store.out:BUG: refs/files-backend.c:139: operation pack_refs requires abilities 0x6, but only have 0x5 test-results/t1406-submodule-ref-store.out:BUG: refs/files-backend.c:139: operation create_symref requires abilities 0x2, but only have 0x5 test-results/t1406-submodule-ref-store.out:BUG: refs/files-backend.c:139: operation delete_refs requires abilities 0x2, but only have 0x5 test-results/t1406-submodule-ref-store.out:BUG: refs/files-backend.c:139: operation rename_ref requires abilities 0x2, but only have 0x5 test-results/t1406-submodule-ref-store.out:BUG: refs/files-backend.c:139: operation delete_reflog requires abilities 0x2, but only have 0x5 test-results/t1406-submodule-ref-store.out:BUG: refs/files-backend.c:139: operation create_reflog requires abilities 0x2, but only have 0x5 test-results/t4058-diff-duplicates.out:Segmentation fault test-results/t4058-diff-duplicates.out:Segmentation fault test-results/t4058-diff-duplicates.out:Segmentation fault test-results/t4058-diff-duplicates.out:Segmentation fault test-results/t6423-merge-rename-directories.out: test_i18ngrep ! BUG: err && Anyway, the "clone"/"fetch"/"update-ref" failures look new with GIT_TEST_REFTABLE=true. So to close up the attempt at getting feedback: I think we'd probably be better off still discussing "I tried this to fix the segfault, but now I get this BUG" rather than a 30 patch re-roll. > I am most in need of review for the part that glues the library > together with the git code itself (ie. the commit introducing > refs/reftable-backend.c). In other and briefer words I don't think this series is in need of review at this point, it's in need of *addressing* review, where review is both the ghosts from the past of failing tests, and outstanding segfaults/BUG()s being hit. 1. https://lore.kernel.org/git/87k0ow2n29.fsf@xxxxxxxxxxxxxxxxxxx/ 2. https://lore.kernel.org/git/87pmyo3zvw.fsf@xxxxxxxxxxxxxxxxxxx/ 3. https://lore.kernel.org/git/87wnt2th1v.fsf@xxxxxxxxxxxxxxxxxxx/ 4. https://lore.kernel.org/git/87im4qejpk.fsf@xxxxxxxxxxxxxxxxxxx/
