{"thread":{"id":"53110","subject":"Re: [gitgitgadget/git] Reftable support git-core (#539)","startedAt":"2020-03-27T11:21:05Z","lastAt":"2020-04-22T15:18:58Z","messageCount":3,"participants":["Han-Wen Nienhuys","Johannes Schindelin"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"394163","messageId":"CAOw_e7Z_uE5sf+n6hc2siKeQbUqwK=YS3an7Aq_MghbgzeiRLg@mail.gmail.com","threadId":"53110","inReplyTo":"gitgitgadget/git/pull/539/c603037045@github.com","subject":"Re: [gitgitgadget/git] Reftable support git-core (#539)","fromName":"Han-Wen Nienhuys","fromEmail":"hanwenn@gmail.com","sentAt":"2020-03-27T11:20:48Z","receivedAt":"2020-03-27T11:21:05Z","isPatch":false,"sender":{"key":"hanwenn@gmail.com","avatar":"https://gravatar.com/avatar/058832acb8d613baeb6ce9d21b009d9772424a309b9a521330423daef909a27a?d=mp&s=160"},"body":"On Tue, Mar 24, 2020 at 7:09 AM gitgitgadget[bot]\n wrote:\n> Contribution format\n> ~~~~~~~~~~~~~~~~~~~\n> This is a bit of an unusual contribution to Git:\n>\n> - the heart of the series is in a single commit\n> - it is under a different license than most of Git\n> - the code doesn't use the same dialect as and \"look like\" the rest of\n> Git\n>\n> All three of these are because the code is meant to be a standalone\n> library that can be used by Git, libgit2, and any other project that\n> wants to understand reftables. But they create obstacles to reviewing\n> (one reviewer said he had spent a two-hour block looking at the patch\n> and not made much headway).\n>\n> On the other hand, people mostly agreed that having the ability to\n> share this code with libgit2 is beneficial. So how can we get a\n> substantial review without losing that benefit?\n>\n> 1. Most importantly, people would like a series of multiple patches\n> that tell a story. If review of an early patch in the series\n> reveals significant changes that should happen in reftable\n> upstream, that's fine --- upstream, that can happen in a patch on\n> top, whereas in the series being contributed, the patch would be\n> amended.\n>\n> In the end, there would be two different series of commits: the\n> contributed series, and the upstream history. The upstream\n> history might be messier, and that's fine. The contributed series\n> would be applied to git.git.\n\nThis not very practical. I currently sync the reftable library into\ngit by means of the reftable/update.sh script,\nand creating a layered commits is a lot of busywork. Also, getting\nthe entire code in one go lets reviewers see how the whole thing fits\ntogether.\n\nI do understand the complaint, though. The good news is that there\nalready is a story in the source code; you just have to read it in the\nright order. That order is (bottom-up):\n\n* record.{c,h} - (de)serialized single records\n* block.{c,h} - read and write blocks\n* writer.c - write a reftable\n* reader.c - read a reftable\n* merged.{c,h}, pq.{c,h} - reading a stack of reftables\n* stack.{c,h} - writing and compacting stacks of reftable on the filesystem.\n\nbefore anyone attempts to review the code, they should read the\nspecification of the format very carefully.\n\n> 2. Both Git and libgit2 have abstractions like strbuf. We'd like\n> reftable to make use of similar abstractions where they make the\n> code cleaner.\n\nHave you looked at slice.{c,h} ? Is there any specific code that\nanyone had an issue with?\n\n> 3. libgit2 has a git_malloc allocator. reftable doesn't necessarily\n> have to use it or make it pluggable --- at worst, we can #define\n> malloc to use it. But it's something to be aware of.\n\nI added pluggable malloc routines.\n\n> Maintenance\n> ~~~~~~~~~~~\n> There's some interest in the maintenance story: if we run into an\n> issue with the reftable library, do we report and fix it on the git\n> mailing list or does the reftable library have its own upstream forums\n> for that?\n\nThere is no specific forum. The most practical solution would be\nreport to the git mailing list with cc to hanwen@google.com.\n\nAlternatively, an issue on the github project would also work.\n\nI don't foresee much need for maintenance, though. (Or do you foresee\na need to store more types of data besides refs and reflogs?)\n\n> Portability\n> ~~~~~~~~~~~\n> There was some confusion about the scope of what the reftable library\n> provides. It provides a reader and a writer and the caller is\n> responsible for connecting those to files.\n\nNo, it also does transactional updates to a stack of reftables on the\nfilesystem.\n\n> There were some questions about mmap usage here (there is none) and\n\nThe format is suitable for mmap, but it seems premature optimization,\ngiven the (lack of) efficiency of loose+packed refs storage that it\nindends to replace.\n\n> whether the library needs some kind of seeking reader interface for\n> abstracting the OS interface to files.\n\nLook at reftable_block_source in reftable.h\n\n> I think the upshot is\n>\n> 4. Some summary documentation would be helpful e.g. in the README.md,\n> to point people to what header file a person should start with to\n> understand the library\n\nreftable.h is a good start, as it declares the public interface, and\nis extensively documented. It also addresses many of the points that\nthis email raised.\n\n> 5. People are also interested in the file-oriented part of reftable,\n> even though this library doesn't implement it (that's patch 6,\n> which is Git-specific).\n\nNo, this is incorrect.\n\n> Testing\n> ~~~~~~~\n> Git's testsuite has various GIT_TEST_* knobs. A GIT_TEST_REF_BACKEND\n> knob would be helpful, to allow running the full testsuite with\n> reftable. That's a good way to suss out edge cases.\n\nI agree, but this outside my personal area of expertise. I think this\nis best tackled by someone else.\n\n> The testsuite should *also* include some specific tests designed for\n> reftable. They can demonstrate edge cases and demonstrate some of the\n> benefits\n\nThere are unittests in the upstream project. Edge cases in the library\nitself should be covered by unittests. Is there interest in adding\nthese to the git project itself?\n\nI can add some Git tests for directory/file conflicts and case sensitivity.\n\n> We also discussed table-driven tests that can be shared between\n> implementations but didn't end up saying anything too concrete about\n> that.\n\nMy plan is to add a cross-language tests, that passes data between\nJGit, Go and C implementations to verify that they interoperate.\n\n\n--\nHan-Wen Nienhuys - hanwenn@gmail.com - http://www.xs4all.nl/~hanwen\n"},{"id":"395896","messageId":"nycvar.QRO.7.76.6.2004221327020.46@tvgsbejvaqbjf.bet","threadId":"53110","inReplyTo":"CAOw_e7azo1Wb9RO=7kH8kXp4RxTzD6SW4a9w_2ifiGxUmt2YKw@mail.gmail.com","subject":"Re: [gitgitgadget/git] Reftable support git-core (#539)","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-04-22T11:27:39Z","receivedAt":"2020-04-22T11:27:43Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Han-Wen,\n\nOn Mon, 20 Apr 2020, Han-Wen Nienhuys wrote:\n\n> On Sat, Apr 18, 2020 at 5:26 AM gitgitgadget[bot] \n> wrote:\n>\n> > On the Git mailing list\n> > , Danh Doan\n> > wrote (reply to this\n> > ):\n> >\n> > On 2020-04-15 16:29:33-0700, Junio C Hamano wrote:\n> > > \"Han-Wen Nienhuys via GitGitGadget\" writes:\n> > >\n> > > > This adds the reftable library, and hooks it up as a ref backend.\n> > >\n> > > Since I queued the topic to 'pu', I needed to apply these fix-up\n> > > patches on top. The change to the Makefile is about \"make clean\"\n> > > which forgot to clean the reftable library. All the rest are ws\n> > > clean-ups.\n> > >\n> > > There are build breakages reported on Windows, with possible fixes\n> > > (which IIRC were reported to segfault X-<), but I do not have URL or\n> > > message-IDs handy.\n> >\n> > FYI, it's also segfault in Linux even if only the test is applied.\n> > IOW, the code to fix breakages on Windows doesn't affect the segfault.\n> >\n> > Here is the message: \n> >\n> >\n> Thanks for the reference. I'll look into it.\n>\n> Johannes, if you have specific comments about the reftable patch, please\n> include me (hanwen@google.com) explicitly on your messages.\n\nSorry, I forgot. Will try to do better next time.\n\nCiao,\nDscho\n"},{"id":"395903","messageId":"CAFQ2z_MCG3pD3i3nAJcvQEKytQdHBO8X9ssmeM=pbypghEvwzQ@mail.gmail.com","threadId":"53110","inReplyTo":"nycvar.QRO.7.76.6.2004221327020.46@tvgsbejvaqbjf.bet","subject":"Re: [gitgitgadget/git] Reftable support git-core (#539)","fromName":"Han-Wen Nienhuys","fromEmail":"hanwen@google.com","sentAt":"2020-04-22T15:18:40Z","receivedAt":"2020-04-22T15:18:58Z","isPatch":false,"sender":{"key":"hanwen@google.com","avatar":"https://avatars.githubusercontent.com/u/31547?v=4"},"body":"x\n\nOn Wed, Apr 22, 2020 at 1:27 PM Johannes Schindelin\n wrote:\n>\n> > > Here is the message: \n> > >\n> > >\n> > Thanks for the reference. I'll look into it.\n> >\n> > Johannes, if you have specific comments about the reftable patch, please\n> > include me (hanwen@google.com) explicitly on your messages.\n>\n> Sorry, I forgot. Will try to do better next time.\n\nI fixed all the outstanding issues in the last push to GGG. For the\nrecord, the following caused segfaults:\n\nhttps://github.com/google/reftable/commit/31078a87067ec9c33f496afb6b478eed6e9c3d12\nhttps://github.com/google/reftable/commit/9107ddd6ed73844cb9092dc18ba92091a1132a9e\n\nThe message about \"bad replace ref name\" was a missing copy of the\nprefix filter.\n\nThe C reftable library was based on the code I wrote in Go first, and\nI keep both versions in sync, hence the null/zero initialization of\ndata throughout.\n\nBy now I'm an expert on reftable, but not so much about Git's test\ninfrastructure, and unfortunately, I lack the time to become an expert\nwithout your guidance, so please give specific feedback.\n\nThe most pressing thing right now is that the windows port on GGG says\n\n ++ git tag file\n fatal: reftable: transaction failure general error\n\nwhich suggests that renames don't work like I think they do on windows.\n\n> Ciao,\n> Dscho\n\n-- \nHan-Wen Nienhuys - Google Munich\nI work 80%. Don't expect answers from me on Fridays.\n--\nGoogle Germany GmbH, Erika-Mann-Strasse 33, 80636 Munich\nRegistergericht und -nummer: Hamburg, HRB 86891\nSitz der Gesellschaft: Hamburg\nGeschäftsführer: Paul Manicle, Halimah DeLaine Prado\n"}]}