Reftable support git-core (#539)
Quoted 26 lines · gitgitgadget[bot]
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 GitAll 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.
Quoted 4 lines
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.
Quoted 3 lines
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?
Quoted 3 lines
3. libgit2 has a
git_mallocallocator. 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.
Quoted 6 lines
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 [email protected].
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?)
Quoted 5 lines
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.
Quoted 1 line
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.
Quoted 2 lines
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
Quoted 5 lines
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.
Quoted 3 lines
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.
Quoted 5 lines
Testing
~~~~~~~
Git's testsuite has various GIT_TEST_* knobs. AGIT_TEST_REF_BACKENDknob 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.
Quoted 3 lines
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.
Quoted 3 lines
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 - [email protected] - http://www.xs4all.nl/~hanwen
Hi Han-Wen,
Quoted 25 lines · Han-Wen Nienhuys
Quoted 22 lines · gitgitgadget[bot]
On the Git mailing list
<https://lore.kernel.org/git/[email protected]>, Danh Doan wrote (reply to this
<https://github.com/gitgitgadget/gitgitgadget/wiki/ReplyToThis>):Quoted 4 lines · Junio C Hamano
Quoted 1 line · Han-Wen Nienhuys via GitGitGadget
This adds the reftable library, and hooks it up as a ref backend.
Quoted 8 lines
Since I queued the topic to 'pu', I needed to apply these fix-up patches on top. The change to the
Makefileis 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.
Quoted 7 lines
Quoted 3 lines
Here is the message: <[email protected]>
Thanks for the reference. I'll look into it.
Johannes, if you have specific comments about the reftable patch, please include me ([email protected]) explicitly on your messages.
Sorry, I forgot. Will try to do better next time.
Ciao,
Dscho
x
Quoted 10 lines · Johannes Schindelin
Quoted 8 lines
Quoted 4 lines
Here is the message: <[email protected]>
Thanks for the reference. I'll look into it.
Johannes, if you have specific comments about the reftable patch, please include me ([email protected]) 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.
Quoted 2 lines
Ciao,
Dscho
Signature · 7 lines
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