{"thread":{"id":"53389","subject":"[PATCH 0/6] Minimal patch series to fix the CI runs of hn/reftable","startedAt":"2020-05-04T13:31:30Z","lastAt":"2020-05-05T15:03:36Z","messageCount":12,"participants":["Johannes Schindelin via GitGitGadget","Junio C Hamano","Han-Wen Nienhuys","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":6},"messages":[{"id":"396896","messageId":"pull.623.git.1588599086.gitgitgadget@gmail.com","threadId":"53389","inReplyTo":null,"subject":"[PATCH 0/6] Minimal patch series to fix the CI runs of hn/reftable","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-05-04T13:31:20Z","receivedAt":"2020-05-04T13:31:30Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"These patches are not intended to be complete, not by any stretch of\nimagination. They are just enough to get the CI run to pass, even in the\nWindows-specific parts.\n\nAs I mentioned elsewhere, I would much prefer for hn/reftable to not \nre-invent get_be*(), struct strbuf, struct string_list, struct lock_file \netc.\n\nHowever, in the context of the test failures, I do not think that this would\nhave made a big difference. Apart from the unportable constructs, and from\nthe \"delete/rename while there is still a handle on the file\" issues, it\nwould appear that one big reason why it was so hard to debug and fix the\ntest is the recursive complexity of the data structures.\n\nTo elaborate on that: struct reftable_stack has an attribute called merged \nthat has an array of struct reftable_reader * (confusingly called \"stack\").\nEach of these readers has an attribute of type struct reftable_block_source \nthat uses a vtable and an opaque pointer to abstract three types of block\nsources: file, slice (which is apparently unused, i.e. it is apparently just\ndead weight for now) and malloc.\n\nI am not sure that this abstraction fest serves us well here.\n\nQuite honestly, I would have expected the packed_git data structure to be\nimitated more closely, as it strikes me as similar in purpose, and it has\nseen a ton of testing over the past decade and a half. I could not recognize\nthat design in the reftable, though.\n\nIt is quite obvious, of course, that the code tries to imitate the\nobject-oriented nature of the Go code, but it seems quite obvious from my\ndifficulties to address the problem where stack_compact_range() would try to\ndelete stale reftable files (without even so much as a warning when the\nfiles could not be deleted!) without releasing all file handles to all\nreftable files, even the ones that do not need to be deleted. To be smarter\nabout this, the code has to know more about the nature of the block source\nthan the abstraction layer suggests. It has to know that a block source\nrefers to a file, and that that file is marked for deletion. My heavy-handed\nwork-around, even if it works, is not really a good solution, but it is a\ntestament that there is a lot of opportunity to simplify the code\ndrastically while at the same time simplifying the design, too.\n\nI know you have been putting a lot of effort into this library, so I feel a\nbit bad about saying the following: The hn/reftable patches will need\nsubstantial work before we can merge it with confidence. Part of the reason\nwhy it is so hard to review the reftable patches is that they intentionally \nrefuse to integrate well within Git's source code, such as (re-)implementing\nits own fundamental data structures, intentionally using a totally different\ncoding style, and it concerns me that the stated requirement for bug fixes\nis to treat Git's source code as a downstream of the actual project. I am\nnot too bad a C developer and would consider myself competent in debugging\nissues, even hard ones, in Git, and yet... it was really hard to wade\nthrough the layers of abstraction to figure out where the file handles\nshould be closed that were opened and prevented deleting/renaming files.\n\nAt this point, I don't feel that it makes sense to keep insisting on having\nthis in a separate library. The only other user of that library will be\nlibgit2, and I really think that given libgit2's heritage, it won't be a\nproblem to adapt the code after it stabilized in git.git (and since libgit2\ntreats git.git as upstream for the libxdiff code, it won't be a problem to\ndo the same for the reftable code, too). I believe that the best course of\naction is to reuse the data structures libgit.a provides, and to delete the\nre-implementations in reftable/. Only then can we start working effectively\non refactoring the code to simplify the data structures in order to clarify\nresource usage (which was the root cause for the bugs I fixed, although I am\nsure that there are way more lurking in there, hidden by the fact that the\ncode is not covered thoroughly by our tests).\n\nJohannes Schindelin (6):\n fixup! Add reftable library\n fixup! Add reftable library\n fixup! Add reftable library\n fixup! Add reftable library\n fixup! Add reftable library\n vcxproj: adjust for the reftable changes\n\n config.mak.uname | 2 +-\n contrib/buildsystems/Generators/Vcxproj.pm | 11 ++++++++++-\n reftable/record.c | 2 +-\n reftable/stack.c | 17 +++++++++++++++--\n 4 files changed, 27 insertions(+), 5 deletions(-)\n\n\nbase-commit: 1b749f9d62f2c8b5a8e91f382d2be14902459cba\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-623%2Fdscho%2Freftable-on-windows-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-623/dscho/reftable-on-windows-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/623\n-- \ngitgitgadget\n"},{"id":"396897","messageId":"a11d1cd304153718363f18c33d1ae87f38b3fb21.1588599086.git.gitgitgadget@gmail.com","threadId":"53389","inReplyTo":"pull.623.git.1588599086.gitgitgadget@gmail.com","subject":"[PATCH 1/6] fixup! Add reftable library","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-05-04T13:31:21Z","receivedAt":"2020-05-04T13:31:30Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nClose files before trying to unlink them. This is actually required on\nWindows.\n\nNote: this is probably not the only part of the code that requires this\ntype of fix, but it seems to be enough to let the _current_ version of\nt0031 pass.\n\nSigned-off-by: Johannes Schindelin \n---\n reftable/stack.c | 4 ++++\n 1 file changed, 4 insertions(+)\n\ndiff --git a/reftable/stack.c b/reftable/stack.c\nindex e7b625d924a..2e32f7671c2 100644\n--- a/reftable/stack.c\n+++ b/reftable/stack.c\n@@ -852,6 +852,10 @@ static int stack_compact_range(struct reftable_stack *st, int first, int last,\n \t\t}\n \t}\n \n+\tif (lock_file_fd > 0) {\n+\t\tclose(lock_file_fd);\n+\t\tlock_file_fd = 0;\n+\t}\n \terr = unlink(slice_as_string(&lock_file_name));\n \tif (err < 0) {\n \t\tgoto exit;\n-- \ngitgitgadget\n\n"},{"id":"396898","messageId":"ff60fde10919b6b8c54ecb8f38b710fac37624e3.1588599086.git.gitgitgadget@gmail.com","threadId":"53389","inReplyTo":"pull.623.git.1588599086.gitgitgadget@gmail.com","subject":"[PATCH 2/6] fixup! Add reftable library","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-05-04T13:31:22Z","receivedAt":"2020-05-04T13:31:33Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nDo close the file descriptors to obsolete files before trying to delete\nthem. This is actually required on Windows.\n\nNote: this patch is probably a bit heavy-handed, releasing more than\nnecessary (which will then have to be re-read). But it seems to be\nenough to let t0031 pass on Windows, so it is at least a clue as to what\nthe fix will look like eventually.\n\nSigned-off-by: Johannes Schindelin \n---\n reftable/stack.c | 4 ++++\n 1 file changed, 4 insertions(+)\n\ndiff --git a/reftable/stack.c b/reftable/stack.c\nindex 2e32f7671c2..3c5f4a43130 100644\n--- a/reftable/stack.c\n+++ b/reftable/stack.c\n@@ -941,6 +941,10 @@ static int stack_compact_range(struct reftable_stack *st, int first, int last,\n \t}\n \thave_lock = false;\n \n+\treftable_merged_table_close(st->merged);\n+\tmerged_table_clear(st->merged);\n+\treftable_merged_table_free(st->merged);\n+\tst->merged = NULL;\n \t{\n \t\tchar **p = delete_on_success;\n \t\twhile (*p) {\n-- \ngitgitgadget\n\n"},{"id":"396899","messageId":"c41b25e963973dac1eb50cd3ae1f79741bc89ed1.1588599086.git.gitgitgadget@gmail.com","threadId":"53389","inReplyTo":"pull.623.git.1588599086.gitgitgadget@gmail.com","subject":"[PATCH 4/6] fixup! Add reftable library","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-05-04T13:31:24Z","receivedAt":"2020-05-04T13:31:35Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\n`usleep()` is unportable. We need to find a way _not_ to use it.\n\nFor Visual C, we can use `sleep_millisec()`, but the current design of\nlibreftable seems to be _really_ keen _not_ to depend on anything in\nlibgit.a.\n\nSigned-off-by: Johannes Schindelin \n---\n reftable/stack.c | 5 +++++\n 1 file changed, 5 insertions(+)\n\ndiff --git a/reftable/stack.c b/reftable/stack.c\nindex 3c5f4a43130..2d0b831dda1 100644\n--- a/reftable/stack.c\n+++ b/reftable/stack.c\n@@ -289,7 +289,12 @@ static int reftable_stack_reload_maybe_reuse(struct reftable_stack *st,\n \t\tfree_names(names_after);\n \n \t\tdelay = delay + (delay * rand()) / RAND_MAX + 100;\n+#ifdef _MSC_VER\n+\t\tsleep_millisec(delay/1000);\n+#else\n+\t\t/* TODO! fix this, `usleep()` is not portable enough for us to use */\n \t\tusleep(delay);\n+#endif\n \t}\n \n \treturn 0;\n-- \ngitgitgadget\n\n"},{"id":"396900","messageId":"6f1868a2fe21e59d80a81b837810b09aa86a6838.1588599086.git.gitgitgadget@gmail.com","threadId":"53389","inReplyTo":"pull.623.git.1588599086.gitgitgadget@gmail.com","subject":"[PATCH 3/6] fixup! Add reftable library","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-05-04T13:31:23Z","receivedAt":"2020-05-04T13:31:35Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nA `void` function cannot return a value.\n\nSigned-off-by: Johannes Schindelin \n---\n reftable/record.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/reftable/record.c b/reftable/record.c\nindex b0f18f26c55..5f48be1639e 100644\n--- a/reftable/record.c\n+++ b/reftable/record.c\n@@ -1005,7 +1005,7 @@ int record_decode(struct record rec, struct slice key, byte extra,\n \n void record_clear(struct record rec)\n {\n-\treturn rec.ops->clear(rec.data);\n+\trec.ops->clear(rec.data);\n }\n \n bool record_is_deletion(struct record rec)\n-- \ngitgitgadget\n\n"},{"id":"396901","messageId":"d05b9b9a2e0a49b1bacf0dafb99d36cd213181dc.1588599086.git.gitgitgadget@gmail.com","threadId":"53389","inReplyTo":"pull.623.git.1588599086.gitgitgadget@gmail.com","subject":"[PATCH 6/6] vcxproj: adjust for the reftable changes","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-05-04T13:31:26Z","receivedAt":"2020-05-04T13:31:36Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nThis allows Git to be compiled via Visual Studio again after integrating\nthe `hn/reftable` branch.\n\nSigned-off-by: Johannes Schindelin \n---\n config.mak.uname | 2 +-\n contrib/buildsystems/Generators/Vcxproj.pm | 11 ++++++++++-\n 2 files changed, 11 insertions(+), 2 deletions(-)\n\ndiff --git a/config.mak.uname b/config.mak.uname\nindex 0ab8e009383..8a01a0da3f1 100644\n--- a/config.mak.uname\n+++ b/config.mak.uname\n@@ -700,7 +700,7 @@ vcxproj:\n \t# Make .vcxproj files and add them\n \tunset QUIET_GEN QUIET_BUILT_IN; \\\n \tperl contrib/buildsystems/generate -g Vcxproj\n-\tgit add -f git.sln {*,*/lib,t/helper/*}/*.vcxproj\n+\tgit add -f git.sln {*,*/lib,*/libreftable,t/helper/*}/*.vcxproj\n \n \t# Generate the LinkOrCopyBuiltins.targets and LinkOrCopyRemoteHttp.targets file\n \t(echo '' && \\\ndiff --git a/contrib/buildsystems/Generators/Vcxproj.pm b/contrib/buildsystems/Generators/Vcxproj.pm\nindex 5c666f9ac03..33a08d31652 100644\n--- a/contrib/buildsystems/Generators/Vcxproj.pm\n+++ b/contrib/buildsystems/Generators/Vcxproj.pm\n@@ -77,7 +77,7 @@ sub createProject {\n my $libs_release = \"\\n \";\n my $libs_debug = \"\\n \";\n if (!$static_library) {\n- $libs_release = join(\";\", sort(grep /^(?!libgit\\.lib|xdiff\\/lib\\.lib|vcs-svn\\/lib\\.lib)/, @{$$build_structure{\"$prefix${name}_LIBS\"}}));\n+ $libs_release = join(\";\", sort(grep /^(?!libgit\\.lib|xdiff\\/lib\\.lib|vcs-svn\\/lib\\.lib|reftable\\/libreftable\\.lib)/, @{$$build_structure{\"$prefix${name}_LIBS\"}}));\n $libs_debug = $libs_release;\n $libs_debug =~ s/zlib\\.lib/zlibd\\.lib/g;\n $libs_debug =~ s/libcurl\\.lib/libcurl-d\\.lib/g;\n@@ -231,6 +231,7 @@ sub createProject {\n EOM\n if (!$static_library || $target =~ 'vcs-svn' || $target =~ 'xdiff') {\n my $uuid_libgit = $$build_structure{\"LIBS_libgit_GUID\"};\n+ my $uuid_libreftable = $$build_structure{\"LIBS_reftable/libreftable_GUID\"};\n my $uuid_xdiff_lib = $$build_structure{\"LIBS_xdiff/lib_GUID\"};\n \n print F << \"EOM\";\n@@ -240,6 +241,14 @@ sub createProject {\n false\n \n EOM\n+ if (!($name =~ /xdiff|libreftable/)) {\n+ print F << \"EOM\";\n+ \n+ $uuid_libreftable\n+ false\n+ \n+EOM\n+ }\n if (!($name =~ 'xdiff')) {\n print F << \"EOM\";\n \n-- \ngitgitgadget\n"},{"id":"396902","messageId":"36616471d4afea57ca90825389e896e6c71013e2.1588599086.git.gitgitgadget@gmail.com","threadId":"53389","inReplyTo":"pull.623.git.1588599086.gitgitgadget@gmail.com","subject":"[PATCH 5/6] fixup! Add reftable library","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-05-04T13:31:25Z","receivedAt":"2020-05-04T13:31:39Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nYet another instance of `= {}` initialization.\n\nSigned-off-by: Johannes Schindelin \n---\n reftable/stack.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/reftable/stack.c b/reftable/stack.c\nindex 2d0b831dda1..2f3dfd51861 100644\n--- a/reftable/stack.c\n+++ b/reftable/stack.c\n@@ -19,7 +19,7 @@ int reftable_new_stack(struct reftable_stack **dest, const char *dir,\n {\n \tstruct reftable_stack *p =\n \t\treftable_calloc(sizeof(struct reftable_stack));\n-\tstruct slice list_file_name = {};\n+\tstruct slice list_file_name = { 0 };\n \tint err = 0;\n \n \tif (config.hash_id == 0) {\n@@ -417,7 +417,7 @@ static int reftable_stack_init_addition(struct reftable_addition *add,\n void reftable_addition_close(struct reftable_addition *add)\n {\n \tint i = 0;\n-\tstruct slice nm = {};\n+\tstruct slice nm = { 0 };\n \tfor (i = 0; i < add->new_tables_len; i++) {\n \t\tslice_set_string(&nm, add->stack->list_file);\n \t\tslice_append_string(&nm, \"/\");\n-- \ngitgitgadget\n\n"},{"id":"396916","messageId":"xmqq8si7ithq.fsf@gitster.c.googlers.com","threadId":"53389","inReplyTo":"pull.623.git.1588599086.gitgitgadget@gmail.com","subject":"Re: [PATCH 0/6] Minimal patch series to fix the CI runs of hn/reftable","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-05-04T15:56:33Z","receivedAt":"2020-05-04T15:56:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Johannes Schindelin via GitGitGadget\" \nwrites:\n\n> These patches are not intended to be complete, not by any stretch of\n> imagination. They are just enough to get the CI run to pass, even in the\n> Windows-specific parts.\n\nThanks. Now I know at least somebody has been paying attention to\ncall for help in the past few issues of the \"What's cooking\" report ;-) \n\n"},{"id":"396932","messageId":"CAFQ2z_MR2JTAmxcLCzHiVJnufdHRzzbo02kM_wxM7b6agZKc1g@mail.gmail.com","threadId":"53389","inReplyTo":"pull.623.git.1588599086.gitgitgadget@gmail.com","subject":"Re: [PATCH 0/6] Minimal patch series to fix the CI runs of hn/reftable","fromName":"Han-Wen Nienhuys","fromEmail":"hanwen@google.com","sentAt":"2020-05-04T17:04:03Z","receivedAt":"2020-05-04T17:04:22Z","isPatch":true,"sender":{"key":"hanwen@google.com","avatar":"https://avatars.githubusercontent.com/u/31547?v=4"},"body":"On Mon, May 4, 2020 at 3:31 PM Johannes Schindelin via GitGitGadget\n wrote:\n>\n> These patches are not intended to be complete, not by any stretch of\n> imagination. They are just enough to get the CI run to pass, even in the\n> Windows-specific parts.\n\nThanks for the investigation. I had begun to setup a Windows\ninstallation, but you beat me to it.\n\nHere are the two fixes as I applied them.\n\n https://github.com/google/reftable/commit/201f21aa39f993c970ef4d19122fedd5b91e716d\n https://github.com/google/reftable/commit/18b041472306b31813dbd6408f6a61daaba60013\n\n> As I mentioned elsewhere, I would much prefer for hn/reftable to not\n> re-invent get_be*(), struct strbuf, struct string_list, struct lock_file\n> etc.\n>\n> However, in the context of the test failures, I do not think that this would\n> have made a big difference. Apart from the unportable constructs, and from\n> the \"delete/rename while there is still a handle on the file\" issues, it\n> would appear that one big reason why it was so hard to debug and fix the\n> test is the recursive complexity of the data structures.\n>\n> To elaborate on that: struct reftable_stack has an attribute called merged\n> that has an array of struct reftable_reader * (confusingly called \"stack\").\n> Each of these readers has an attribute of type struct reftable_block_source\n> that uses a vtable and an opaque pointer to abstract three types of block\n> sources: file, slice (which is apparently unused, i.e. it is apparently just\n> dead weight for now) and malloc.\n\nAll of the unittests use slices to read and write single reftables in-memory.\n\nThe malloc block source is used to swap out a block coming from the\nfile directly, with zlib uncompressed data (log blocks are zlib\ncompressed). I think there should be a mmap block source too, at some\npoint.\n\n> I am not sure that this abstraction fest serves us well here.\n>\n> Quite honestly, I would have expected the packed_git data structure to be\n> imitated more closely, as it strikes me as similar in purpose, and it has\n> seen a ton of testing over the past decade and a half. I could not recognize\n> that design in the reftable, though.\n>\n> It is quite obvious, of course, that the code tries to imitate the\n> object-oriented nature of the Go code, but it seems quite obvious from my\n> difficulties to address the problem where stack_compact_range() would try to\n> delete stale reftable files (without even so much as a warning when the\n> files could not be deleted!) without releasing all file handles to all\n> reftable files, even the ones that do not need to be deleted. To be smarter\n> about this, the code has to know more about the nature of the block source\n> than the abstraction layer suggests. It has to know that a block source\n> refers to a file, and that that file is marked for deletion. My heavy-handed\n> work-around, even if it works, is not really a good solution, but it is a\n> testament that there is a lot of opportunity to simplify the code\n> drastically while at the same time simplifying the design, too.\n\nIf the code tries to delete a file while it is open, that is a\ntestament to the fact that I haven't written code for Windows for many\nyears now. It is not a testament to any fundamental problems with the\nlibrary design.\n\nThe library which you are complaining of weighs in at about 7500 lines\nof code (excluding tests), which compares pretty well to the original\nJGit code which is now ~6500 lines of code. I don't think there is\nroom to simplify it further, and I say this as a person who has\nsignificant experience with this format.\n\nIf you wish to prove me wrong, you can send me patch. Until then, I\ndon't buy your arguments.\n\n> I know you have been putting a lot of effort into this library, so I feel a\n> bit bad about saying the following: The hn/reftable patches will need\n> substantial work before we can merge it with confidence. Part of the reason\n> why it is so hard to review the reftable patches is that they intentionally\n> refuse to integrate well within Git's source code, such as (re-)implementing\n> its own fundamental data structures, intentionally using a totally different\n> coding style, and it concerns me that the stated requirement for bug fixes\n> is to treat Git's source code as a downstream of the actual project. I am\n> not too bad a C developer and would consider myself competent in debugging\n> issues, even hard ones, in Git, and yet... it was really hard to wade\n> through the layers of abstraction to figure out where the file handles\n> should be closed that were opened and prevented deleting/renaming files.\n>\n> At this point, I don't feel that it makes sense to keep insisting on having\n> this in a separate library. The only other user of that library will be\n> libgit2, and I really think that given libgit2's heritage, it won't be a\n> problem to adapt the code after it stabilized in git.git (and since libgit2\n> treats git.git as upstream for the libxdiff code, it won't be a problem to\n> do the same for the reftable code, too). I believe that the best course of\n> action is to reuse the data structures libgit.a provides, and to delete the\n> re-implementations in reftable/. Only then can we start working effectively\n> on refactoring the code to simplify the data structures in order to clarify\n> resource usage (which was the root cause for the bugs I fixed, although I am\n> sure that there are way more lurking in there, hidden by the fact that the\n> code is not covered thoroughly by our tests).\n\nI'm happy to change the library to use more common primitives that are\nshared between libgit2 and git. Could you point them out to me? Note\nthat the basics that you are complaining of (put_be*, strbuf vs slice\nroutines, etc.) constitute around 700 lines of code. It's not going to\nmake an appreciable difference in complexity.\n\nHere is my counterpoint to your proposal:\n\nReftable was developed outside of git-core. If you feel this series\nwith its giant patch is too much to review, you can have a look at\nthe incremental story of how it came to be, both in the history and\ncode reviews of the JGit project, and in the commit history of\nhttps://github.com/google/reftable.\n\nThe idea to put the code into git-core, and especially your proposed\n(but unsubstantiated) plans to \"simplify\" its design, make me very\nworried about interoperability with the JGit reference implementation.\n\nCould you explain to me how you would qualify a reftable-in-git\nlibrary against the JGit implementation?\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"},{"id":"396935","messageId":"xmqqbln3hbe7.fsf@gitster.c.googlers.com","threadId":"53389","inReplyTo":"d05b9b9a2e0a49b1bacf0dafb99d36cd213181dc.1588599086.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 6/6] vcxproj: adjust for the reftable changes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-05-04T17:12:48Z","receivedAt":"2020-05-04T17:12:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Johannes Schindelin via GitGitGadget\" \nwrites:\n\n> From: Johannes Schindelin \n>\n> This allows Git to be compiled via Visual Studio again after integrating\n> the `hn/reftable` branch.\n>\n> Signed-off-by: Johannes Schindelin \n> ---\n> config.mak.uname | 2 +-\n> contrib/buildsystems/Generators/Vcxproj.pm | 11 ++++++++++-\n> 2 files changed, 11 insertions(+), 2 deletions(-)\n>\n> diff --git a/config.mak.uname b/config.mak.uname\n> index 0ab8e009383..8a01a0da3f1 100644\n> --- a/config.mak.uname\n> +++ b/config.mak.uname\n> @@ -700,7 +700,7 @@ vcxproj:\n> \t# Make .vcxproj files and add them\n> \tunset QUIET_GEN QUIET_BUILT_IN; \\\n> \tperl contrib/buildsystems/generate -g Vcxproj\n> -\tgit add -f git.sln {*,*/lib,t/helper/*}/*.vcxproj\n> +\tgit add -f git.sln {*,*/lib,*/libreftable,t/helper/*}/*.vcxproj\n> \n> \t# Generate the LinkOrCopyBuiltins.targets and LinkOrCopyRemoteHttp.targets file\n> \t(echo '' && \\\n> diff --git a/contrib/buildsystems/Generators/Vcxproj.pm b/contrib/buildsystems/Generators/Vcxproj.pm\n> index 5c666f9ac03..33a08d31652 100644\n> --- a/contrib/buildsystems/Generators/Vcxproj.pm\n> +++ b/contrib/buildsystems/Generators/Vcxproj.pm\n> @@ -77,7 +77,7 @@ sub createProject {\n> my $libs_release = \"\\n \";\n> my $libs_debug = \"\\n \";\n> if (!$static_library) {\n> - $libs_release = join(\";\", sort(grep /^(?!libgit\\.lib|xdiff\\/lib\\.lib|vcs-svn\\/lib\\.lib)/, @{$$build_structure{\"$prefix${name}_LIBS\"}}));\n> + $libs_release = join(\";\", sort(grep /^(?!libgit\\.lib|xdiff\\/lib\\.lib|vcs-svn\\/lib\\.lib|reftable\\/libreftable\\.lib)/, @{$$build_structure{\"$prefix${name}_LIBS\"}}));\n> $libs_debug = $libs_release;\n> $libs_debug =~ s/zlib\\.lib/zlibd\\.lib/g;\n> $libs_debug =~ s/libcurl\\.lib/libcurl-d\\.lib/g;\n> @@ -231,6 +231,7 @@ sub createProject {\n> EOM\n> if (!$static_library || $target =~ 'vcs-svn' || $target =~ 'xdiff') {\n> my $uuid_libgit = $$build_structure{\"LIBS_libgit_GUID\"};\n> + my $uuid_libreftable = $$build_structure{\"LIBS_reftable/libreftable_GUID\"};\n> my $uuid_xdiff_lib = $$build_structure{\"LIBS_xdiff/lib_GUID\"};\n> \n> print F << \"EOM\";\n> @@ -240,6 +241,14 @@ sub createProject {\n> false\n> \n> EOM\n> + if (!($name =~ /xdiff|libreftable/)) {\n\nWow. That is messy. I find it somewhat funny that this is not just\nlibreftable, i.e. \"don't include/refer to libreftable in itself\",\nbut as long as it works, I won't complain ;-)\n\nThanks for helping the tip of 'pu' moving forward.\n\n> + print F << \"EOM\";\n> + \n> + $uuid_libreftable\n> + false\n> + \n> +EOM\n> + }\n> if (!($name =~ 'xdiff')) {\n> print F << \"EOM\";\n> \n"},{"id":"397099","messageId":"nycvar.QRO.7.76.6.2005050839410.56@tvgsbejvaqbjf.bet","threadId":"53389","inReplyTo":"CAFQ2z_MR2JTAmxcLCzHiVJnufdHRzzbo02kM_wxM7b6agZKc1g@mail.gmail.com","subject":"Re: [PATCH 0/6] Minimal patch series to fix the CI runs of hn/reftable","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-05-05T08:00:21Z","receivedAt":"2020-05-05T15:01:25Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Han-Wen,\n\nOn Mon, 4 May 2020, Han-Wen Nienhuys wrote:\n\n> On Mon, May 4, 2020 at 3:31 PM Johannes Schindelin via GitGitGadget\n> wrote:\n> >\n> > These patches are not intended to be complete, not by any stretch of\n> > imagination. They are just enough to get the CI run to pass, even in the\n> > Windows-specific parts.\n>\n> Thanks for the investigation. I had begun to setup a Windows\n> installation, but you beat me to it.\n>\n> Here are the two fixes as I applied them.\n>\n> https://github.com/google/reftable/commit/201f21aa39f993c970ef4d19122fedd5b91e716d\n> https://github.com/google/reftable/commit/18b041472306b31813dbd6408f6a61daaba60013\n\nIf you ever needed _any_ kind of proof that the current setup with that\nmonster patch and a totally separate \"upstream\" does simply _not_ work, it\nis these two commits.\n\n_Every_ bug fix originating from git.git would have to manually port the\nfixes because of the vendor copy, just like you did, losing all provenance\napart from a meager \"Thanks to\" (which mentions just the bug report\nforgetting about the actually-tested patch).\n\nBut I don't get the impression that my objections are heard, so I might\njust as well stop talking about it. Please do not misinterpret my silence\nabout your patches as any indication that I am happy with `hn/reftable`: I\nam not. Should Junio accept them without drastic changes (which I hope he\nwill not), I expect this to turn into a complete maintenance nightmare.\n\nBesides, it is a bit disappointing to see only those two patches. I made\nvery clear in my patch series that I got the _distinct_ impression that\nthose two bug fixes were just the tip of the iceberg: there are most\nlikely a lot more bugs waiting to be discovered where a file is not closed\nwhen it is no longer needed (or related: memory not being released as soon\nas it can be released). I expect _many_ more similar issues to lurk in the\ncode base.\n\n> > As I mentioned elsewhere, I would much prefer for hn/reftable to not\n> > re-invent get_be*(), struct strbuf, struct string_list, struct lock_file\n> > etc.\n> >\n> > However, in the context of the test failures, I do not think that this would\n> > have made a big difference. Apart from the unportable constructs, and from\n> > the \"delete/rename while there is still a handle on the file\" issues, it\n> > would appear that one big reason why it was so hard to debug and fix the\n> > test is the recursive complexity of the data structures.\n> >\n> > To elaborate on that: struct reftable_stack has an attribute called merged\n> > that has an array of struct reftable_reader * (confusingly called \"stack\").\n> > Each of these readers has an attribute of type struct reftable_block_source\n> > that uses a vtable and an opaque pointer to abstract three types of block\n> > sources: file, slice (which is apparently unused, i.e. it is apparently just\n> > dead weight for now) and malloc.\n>\n> All of the unittests use slices to read and write single reftables\n> in-memory.\n>\n>\n> The malloc block source is used to swap out a block coming from the\n> file directly, with zlib uncompressed data (log blocks are zlib\n> compressed). I think there should be a mmap block source too, at some\n> point.\n\nIf that was intended to convince me that I am wrong in my assessment of\nthe unnecessary complexity of libreftable, I am afraid it did not.\n\nOnly code should be included in the patch adding libreftable to git.git\nwhich is actually needed in git.git. Everything above that is just\nunnecessary complexity.\n\n> > I am not sure that this abstraction fest serves us well here.\n> >\n> > Quite honestly, I would have expected the packed_git data structure to be\n> > imitated more closely, as it strikes me as similar in purpose, and it has\n> > seen a ton of testing over the past decade and a half. I could not recognize\n> > that design in the reftable, though.\n> >\n> > It is quite obvious, of course, that the code tries to imitate the\n> > object-oriented nature of the Go code, but it seems quite obvious from my\n> > difficulties to address the problem where stack_compact_range() would try to\n> > delete stale reftable files (without even so much as a warning when the\n> > files could not be deleted!) without releasing all file handles to all\n> > reftable files, even the ones that do not need to be deleted. To be smarter\n> > about this, the code has to know more about the nature of the block source\n> > than the abstraction layer suggests. It has to know that a block source\n> > refers to a file, and that that file is marked for deletion. My heavy-handed\n> > work-around, even if it works, is not really a good solution, but it is a\n> > testament that there is a lot of opportunity to simplify the code\n> > drastically while at the same time simplifying the design, too.\n>\n> If the code tries to delete a file while it is open, that is a\n> testament to the fact that I haven't written code for Windows for many\n> years now. It is not a testament to any fundamental problems with the\n> library design.\n\nTo the contrary. The library design neglects taking good care of\nresources (such as closing file descriptors when they are clearly no\nlonger needed). C code _has_ to be careful with resources.\n\nBy the way, I am not really trying to convince you of this. That would be\nthe wrong direction: you have to convince _me_, the reviewer, that your\npatches do the right thing, in the right way. So far, I am unconvinced.\n\nEven worse: my debugging session two days ago convinced me that there _is_\nunnecessary complexity, and unclear resource management. Based on how much\nexperience I have with debugging and developing in unfamiliar code bases,\nthis means quite a bit. I am not a junior intern who gets easily\noverwhelmed by code they haven't seen before.\n\nWhen I point out the unnecessary complexity of having a vtable for a mere\ntwo \"subclasses\", of course, as an open source contributor you are free to\ndismiss this without even so much as trying to refute it. Just like the\nGit project is free to reject contributions when the reviewer comments are\nnot taken seriously.\n\n> The library which you are complaining of weighs in at about 7500 lines\n> of code (excluding tests), which compares pretty well to the original\n> JGit code which is now ~6500 lines of code. I don't think there is\n> room to simplify it further, and I say this as a person who has\n> significant experience with this format.\n>\n> If you wish to prove me wrong, you can send me patch. Until then, I\n> don't buy your arguments.\n\nAgain, it is not so much a question whether _you_ buy _my_ arguments (and\nat the same time, it is somewhat shocking that you dismiss that it took me\ntwo hours to figure out a single bug, just because the code and the data\nstructures are unnecessarily complex/abstract).\n\nIt is more a question whether you can convince the reviewers that your\ncode is correct.\n\nIn short: I am not selling anything here that you need to buy. You are\ntrying to sell me something, and I do not like the shape, and I like even\nless how easily you seem to brush aside my explanations how this would need\nto be improved to make it viable in Git's source code.\n\n> > I know you have been putting a lot of effort into this library, so I feel a\n> > bit bad about saying the following: The hn/reftable patches will need\n> > substantial work before we can merge it with confidence. Part of the reason\n> > why it is so hard to review the reftable patches is that they intentionally\n> > refuse to integrate well within Git's source code, such as (re-)implementing\n> > its own fundamental data structures, intentionally using a totally different\n> > coding style, and it concerns me that the stated requirement for bug fixes\n> > is to treat Git's source code as a downstream of the actual project. I am\n> > not too bad a C developer and would consider myself competent in debugging\n> > issues, even hard ones, in Git, and yet... it was really hard to wade\n> > through the layers of abstraction to figure out where the file handles\n> > should be closed that were opened and prevented deleting/renaming files.\n> >\n> > At this point, I don't feel that it makes sense to keep insisting on having\n> > this in a separate library. The only other user of that library will be\n> > libgit2, and I really think that given libgit2's heritage, it won't be a\n> > problem to adapt the code after it stabilized in git.git (and since libgit2\n> > treats git.git as upstream for the libxdiff code, it won't be a problem to\n> > do the same for the reftable code, too). I believe that the best course of\n> > action is to reuse the data structures libgit.a provides, and to delete the\n> > re-implementations in reftable/. Only then can we start working effectively\n> > on refactoring the code to simplify the data structures in order to clarify\n> > resource usage (which was the root cause for the bugs I fixed, although I am\n> > sure that there are way more lurking in there, hidden by the fact that the\n> > code is not covered thoroughly by our tests).\n>\n> I'm happy to change the library to use more common primitives that are\n> shared between libgit2 and git.\n\nI never talked about primitives that are shared between libgit2. I talked\nabout data structures and helpers that already exist in Git's source code.\n\nAnd I said that I am fairly convinced that it won't be hard to adapt the\ncode to libgit2 after it enters git.git.\n\nI am sorry if I did not make myself clear.\n\nIt is a bit unreasonable to reject a review on the basis that git.git\nshould accept cruft that _might_ be required to support libgit2 better. It\nwould be very unreasonable to accept a patch series that demands to\ninclude code in git.git that it does not need just because a separate\nproject might need that code.\n\nAnd if you truly care about libgit2 support, you will include the libgit2\ndevelopers in the development of the reftable support, not throw a\nnominally finalized library at them that might, or might not integrate\nwell into libgit2.\n\n> Could you point them out to me? Note that the basics that you are\n> complaining of (put_be*, strbuf vs slice routines, etc.) constitute\n> around 700 lines of code. It's not going to make an appreciable\n> difference in complexity.\n\nWas I unclear? This additional code _makes it harder to review the monster\npatch_.\n\nThe complexity _will_ need to be reduced. In addition.\n\nBut that cannot be the first step: in order to integrate reasonably well\nwith Git's source code, it will _have_ to stop duplicating code. You\ncannot possibly deny that you reimplemented your own string manipulation\nlibrary in libreftable. Such code adds to the maintenance burden, and in\nthis instance (and in other, similar instances) for no good reason at all\nbecause libgit.a already has code for that.\n\nWithout integrating the code better in git.git, there is no sense to even\nstart reducing the complexity. Both will be necessary.\n\n> Here is my counterpoint to your proposal:\n>\n> Reftable was developed outside of git-core.\n\nThat was your decision, not mine. I merely pointed out that it would have\nbeen wiser to develop it within.\n\nAnd I offered that it is undesirable to take `hn/reftable` as long as it\nlooks very much like it _wants_ to stay out of core Git. I mean, what's\nthe point of accepting code into our code base that does not really want\nto live there?\n\n> If you feel this series with its giant patch is too much to review, you\n> can have a look at the incremental story of how it came to be, both in\n> the history and code reviews of the JGit project, and in the commit\n> history of\n> https://github.com/google/reftable.\n\nSo now you want to place _even more_ of a burden on reviewers? Seriously?\n\n> The idea to put the code into git-core, and especially your proposed\n> (but unsubstantiated) plans to \"simplify\" its design, make me very\n> worried about interoperability with the JGit reference implementation.\n\nThe JGit reference implementation is written in Java. In an\nobject-oriented language with its own idiosyncracies demanded by the\ndesign of said language (the lengths to which Shawn had to go to make it\nperformant are recorded in the commit history).\n\nIt will be better to have a clean-room implementation in C that does not\nsuffer from transpiling artifacts.\n\nAbout ways to simplify its design: I mentioned the layers of abstraction.\nI mentioned the design of `struct packed_git`, which could (and should)\nserve as an example to follow.\n\nIf those points weren't ignored, I would mention other suggestions, such\nas how the code builds file contents via `slice_append_string()` only to\nwrite them out in one go, when the contents could have been written much\nmore simply via `fprintf()` instead. And that is just _one_ more example\nhow complexity could be reduced, very, very easily. There's much more.\nWould there be a point of me writing these up? So far, I don't get that\nimpression. All I get is \"I don't buy this\". That is unnecessarily\nfrustrating: code review is already thankless (as much as it is\nnecessary!) as it is.\n\n> Could you explain to me how you would qualify a reftable-in-git\n> library against the JGit implementation?\n\nOf course: by a comprehensive set of unit/regression tests, including\npre-generated data that the implementation must read and parse correctly.\n\nThe fact that keeping the code close to its non-C origins resulted in\nlong-undetected segmentation faults makes me rather certain the idea of\nkeeping the code close to said reference implementation is overrated: it\ndoes not build the confidence in the code that you think it does. Not\nafter discovering those segmentation faults, it doesn't.\n\nAt the end of the day, a comprehensive set of unit/regression tests will\nbe required for implementing reftable support in other projects, anyway,\nin particular for projects that cannot, or do not want to, rely on your\nlibreftable implementation, such as isomorphic-git or Dulwich.\n\nCiao,\nJohannes\n"},{"id":"397100","messageId":"nycvar.QRO.7.76.6.2005051000360.56@tvgsbejvaqbjf.bet","threadId":"53389","inReplyTo":"xmqq8si7ithq.fsf@gitster.c.googlers.com","subject":"Re: [PATCH 0/6] Minimal patch series to fix the CI runs of hn/reftable","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-05-05T08:02:33Z","receivedAt":"2020-05-05T15:03:36Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Mon, 4 May 2020, Junio C Hamano wrote:\n\n> \"Johannes Schindelin via GitGitGadget\" \n> writes:\n>\n> > These patches are not intended to be complete, not by any stretch of\n> > imagination. They are just enough to get the CI run to pass, even in\n> > the Windows-specific parts.\n>\n> Thanks.\n\nYou're welcome!\n\n> Now I know at least somebody has been paying attention to call for help\n> in the past few issues of the \"What's cooking\" report ;-)\n\nYes, I saw those requests for assistance, and I would have gladly jumped\nin earlier, if my time had allowed.\n\nLet's see whether my efforts bear fruit...\n\nCiao,\nDscho\n"}]}