Re: [PATCH v7 00/28] reftable library

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

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/