Message ID | pull.847.v7.git.git.1618832276.gitgitgadget@gmail.com (mailing list archive) |
---|---|
Headers | show |
Series | reftable library | expand |
On Mon, Apr 19 2021, Han-Wen Nienhuys via GitGitGadget 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. > 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@evledraar.gmail.com/ But running the test suite with GIT_TEST_REFTABLE=true still yields a wall of failed tests. I tested with a merger of this and pr-git-1008/hanwen/reffiles-v1, i.e. what's on "seen" currently. 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. > Due to using uncompress2, this build fails on the Linux32 builder; what is > the magic incantation to run autoconf? Doesn't this just need a NO_UNCOMPRESS2 for Linux32 in ci/lib.sh? See the lines just below that for e.g. NO_REGEX for another CI target.
On Wed, Apr 21, 2021 at 9:45 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> 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@evledraar.gmail.com/ 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. > 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. If you would like this effort to move forward faster, 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). > > Due to using uncompress2, this build fails on the Linux32 builder; what is > > the magic incantation to run autoconf? > > Doesn't this just need a NO_UNCOMPRESS2 for Linux32 in ci/lib.sh? See > the lines just below that for e.g. NO_REGEX for another CI target. Thanks!
On Wed, Apr 21 2021, Han-Wen Nienhuys wrote: > On Wed, Apr 21, 2021 at 9:45 AM Ævar Arnfjörð Bjarmason > <avarab@gmail.com> 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@evledraar.gmail.com/ > > 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@evledraar.gmail.com/ 2. https://lore.kernel.org/git/87pmyo3zvw.fsf@evledraar.gmail.com/ 3. https://lore.kernel.org/git/87wnt2th1v.fsf@evledraar.gmail.com/ 4. https://lore.kernel.org/git/87im4qejpk.fsf@evledraar.gmail.com/
On Wed, Apr 21, 2021 at 1:21 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote: > >> 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 added RFC to the patch introducing reftable-backend.c I consider the code under reftable/*.c to be of production quality, even if it's stylistically different from the git code. > 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. That's disappointing. I know that I can just fix test failures until there are none left, but it's not a terribly efficient method if there is someone who knows how this works. Is there no other way to solicit feedback? > 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. I've had conflicting feedback about this, but it's ultimately not my call to make. Jonathan Nieder suggested that this was asking too much, and it would be OK to have a credible starting point where the effort to address remaining test failures could be shared across multiple people. Jun also expressed he didn't want to put all the work on my shoulders, but so far I've not received much help. > 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. I fixed a number of segfaults, so it's quite likely (I did see the freed transaction BUG). One of the problems of working with a large pending series like this, is that it is a project in itself, and keeping track of which change fixes which bug is something I really need version control for, but the frequent rebasing/squashing erases this kind of information.