Re: [gitgitgadget/git] Reftable support git-core (#539)

Re: [gitgitgadget/git] Reftable support git-core (#539)

[Han-Wen Nienhuys]

On Tue, Mar 24, 2020 at 7:09 AM gitgitgadget[bot]
<notifications@github.com> wrote:
> Contribution format
> ~~~~~~~~~~~~~~~~~~~
> This is a bit of an unusual contribution to Git:
>
> - the heart of the series is in a single commit
> - it is under a different license than most of Git
> - the code doesn't use the same dialect as and "look like" the rest of
>   Git
>
> All three of these are because the code is meant to be a standalone
> library that can be used by Git, libgit2, and any other project that
> wants to understand reftables.  But they create obstacles to reviewing
> (one reviewer said he had spent a two-hour block looking at the patch
> and not made much headway).
>
> On the other hand, people mostly agreed that having the ability to
> share this code with libgit2 is beneficial.  So how can we get a
> substantial review without losing that benefit?
>
>  1. Most importantly, people would like a series of multiple patches
>     that tell a story.  If review of an early patch in the series
>     reveals significant changes that should happen in reftable
>     upstream, that's fine --- upstream, that can happen in a patch on
>     top, whereas in the series being contributed, the patch would be
>     amended.
>
>     In the end, there would be two different series of commits: the
>     contributed series, and the upstream history.  The upstream
>     history might be messier, and that's fine.  The contributed series
>     would be applied to git.git.

This not very practical. I currently sync the reftable library into
git by means of the reftable/update.sh script,
and creating a layered commits is a lot of busywork.  Also, getting
the entire code in one go lets reviewers see how the whole thing fits
together.

I do understand the complaint, though. The good news is that there
already is a story in the source code; you just have to read it in the
right order. That order is (bottom-up):

* record.{c,h} - (de)serialized single records
* block.{c,h} - read and write blocks
* writer.c - write a reftable
* reader.c - read a reftable
* merged.{c,h}, pq.{c,h} - reading a stack of reftables
* stack.{c,h} - writing and compacting stacks of reftable on the filesystem.

before anyone attempts to review the code, they should read the
specification of the format very carefully.

>  2. Both Git and libgit2 have abstractions like strbuf.  We'd like
>     reftable to make use of similar abstractions where they make the
>     code cleaner.

Have you looked at slice.{c,h} ? Is there any specific code that
anyone had an issue with?

>  3. libgit2 has a git_malloc allocator.  reftable doesn't necessarily
>     have to use it or make it pluggable --- at worst, we can #define
>     malloc to use it.  But it's something to be aware of.

I added pluggable malloc routines.

> Maintenance
> ~~~~~~~~~~~
> There's some interest in the maintenance story: if we run into an
> issue with the reftable library, do we report and fix it on the git
> mailing list or does the reftable library have its own upstream forums
> for that?

There is no specific forum. The most practical solution would be
report to the git mailing list with cc to hanwen@google.com.

Alternatively, an issue on the github project would also work.

I don't foresee much need for maintenance, though. (Or do you foresee
a need to store more types of data besides refs and reflogs?)

> Portability
> ~~~~~~~~~~~
> There was some confusion about the scope of what the reftable library
> provides.  It provides a reader and a writer and the caller is
> responsible for connecting those to files.

No, it also does transactional updates to a stack of reftables on the
filesystem.

> There were some questions about mmap usage here (there is none) and

The format is suitable for mmap, but it seems premature optimization,
given the (lack of) efficiency of loose+packed refs storage that it
indends to replace.

> whether the library needs some kind of seeking reader interface for
> abstracting the OS interface to files.

Look at reftable_block_source in reftable.h

> I think the upshot is
>
>  4. Some summary documentation would be helpful e.g. in the README.md,
>     to point people to what header file a person should start with to
>     understand the library

reftable.h is a good start, as it declares the public interface, and
is extensively documented. It also addresses many of the points that
this email raised.

>  5. People are also interested in the file-oriented part of reftable,
>     even though this library doesn't implement it (that's patch 6,
>     which is Git-specific).

No, this is incorrect.

> Testing
> ~~~~~~~
> Git's testsuite has various GIT_TEST_* knobs.  A GIT_TEST_REF_BACKEND
> knob would be helpful, to allow running the full testsuite with
> reftable.  That's a good way to suss out edge cases.

I agree, but this outside my personal area of expertise. I think this
is best tackled by someone else.

> The testsuite should *also* include some specific tests designed for
> reftable.  They can demonstrate edge cases and demonstrate some of the
> benefits

There are unittests in the upstream project. Edge cases in the library
itself should be covered by unittests. Is there interest in adding
these to the git project itself?

I can add some Git tests for directory/file conflicts and case sensitivity.

> We also discussed table-driven tests that can be shared between
> implementations but didn't end up saying anything too concrete about
> that.

My plan is to add a cross-language tests, that passes data between
JGit, Go and C implementations to verify that they interoperate.


--
Han-Wen Nienhuys - hanwenn@gmail.com - http://www.xs4all.nl/~hanwen

Re: [gitgitgadget/git] Reftable support git-core (#539)

[Johannes Schindelin]

Hi Han-Wen,

On Mon, 20 Apr 2020, Han-Wen Nienhuys wrote:

> On Sat, Apr 18, 2020 at 5:26 AM gitgitgadget[bot] <notifications@github.com>
> wrote:
>
> > On the Git mailing list
> > <https://lore.kernel.org/git/20200418032252.GA9169@danh.dev>, Danh Doan
> > wrote (reply to this
> > <https://github.com/gitgitgadget/gitgitgadget/wiki/ReplyToThis>):
> >
> > On 2020-04-15 16:29:33-0700, Junio C Hamano <gitster@pobox.com> wrote:
> > > "Han-Wen Nienhuys via GitGitGadget" <gitgitgadget@gmail.com> writes:
> > >
> > > > This adds the reftable library, and hooks it up as a ref backend.
> > >
> > > Since I queued the topic to 'pu', I needed to apply these fix-up
> > > patches on top.  The change to the Makefile is about "make clean"
> > > which forgot to clean the reftable library.  All the rest are ws
> > > clean-ups.
> > >
> > > There are build breakages reported on Windows, with possible fixes
> > > (which IIRC were reported to segfault X-<), but I do not have URL or
> > > message-IDs handy.
> >
> > FYI, it's also segfault in Linux even if only the test is applied.
> > IOW, the code to fix breakages on Windows doesn't affect the segfault.
> >
> > Here is the message: <nycvar.QRO.7.76.6.2004101604210.46@tvgsbejvaqbjf.bet>
> >
> >
> Thanks for the reference. I'll look into it.
>
> Johannes, if you have specific comments about the reftable patch, please
> include me (hanwen@google.com) explicitly on your messages.

Sorry, I forgot. Will try to do better next time.

Ciao,
Dscho

Re: [gitgitgadget/git] Reftable support git-core (#539)

[Han-Wen Nienhuys]

x

On Wed, Apr 22, 2020 at 1:27 PM Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
>
> > > Here is the message: <nycvar.QRO.7.76.6.2004101604210.46@tvgsbejvaqbjf.bet>
> > >
> > >
> > Thanks for the reference. I'll look into it.
> >
> > Johannes, if you have specific comments about the reftable patch, please
> > include me (hanwen@google.com) explicitly on your messages.
>
> Sorry, I forgot. Will try to do better next time.

I fixed all the outstanding issues in the last push to GGG. For the
record, the following caused segfaults:

https://github.com/google/reftable/commit/31078a87067ec9c33f496afb6b478eed6e9c3d12
https://github.com/google/reftable/commit/9107ddd6ed73844cb9092dc18ba92091a1132a9e

The message about "bad replace ref name" was a missing copy of the
prefix filter.

The C reftable library was based on the code I wrote in Go first, and
I keep both versions in sync, hence the null/zero initialization of
data throughout.

By now I'm an expert on reftable, but not so much about Git's test
infrastructure, and unfortunately, I lack the time to become an expert
without your guidance, so please give specific feedback.

The most pressing thing right now is that the windows port on GGG says

 ++ git tag file
 fatal: reftable: transaction failure general error

which suggests that renames don't work like I think they do on windows.

> Ciao,
> Dscho

-- 
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