{"thread":{"id":"54727","subject":"[PATCH 1/6] fixup! reftable: rest of library","startedAt":"2020-11-28T22:16:56Z","lastAt":"2020-12-03T13:57:53Z","messageCount":22,"participants":["Johannes Schindelin via GitGitGadget","Han-Wen Nienhuys","Jeff King","Johannes Schindelin","Ævar Arnfjörð Bjarmason"],"isPatch":true,"patchVersion":1,"patchTotal":6},"messages":[{"id":"410941","messageId":"878bffcdfe5ca7657f839de8f7993d9098726636.1606545878.git.gitgitgadget@gmail.com","threadId":"54727","inReplyTo":"pull.801.git.1606545878.gitgitgadget@gmail.com","subject":"[PATCH 1/6] fixup! reftable: rest of library","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-11-28T06:44:33Z","receivedAt":"2020-11-28T22:16:56Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin <johannes.schindelin@gmx.de>\n\nClose the file descriptors to obsolete files before trying to delete or\nrename them. This is actually required on Windows.\n\nNote: this patch is just a band-aid to get the tests pass on Windows.\nThe fact that it is needed raises concerns about the overall resource\nhandling: are file descriptors closed properly whenever appropriate, or\nare they closed much later (which can lead to rename() problems on\nWindows, and risks running into ulimits)?\n\nAlso, a `reftable_stack_destroy()` call had to be moved in\n`test_reftable_stack_uptodate()` to avoid the prompt complaining that a\n`.ref` file could not be deleted on Windows. This raises the question\nwhether the code does the right thing when two concurrent processes want\nto access the reftable, and one wants to compact it. At the moment, it\ndoes not appear to fail gracefully.\n\nSigned-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n---\n reftable/stack.c      | 37 ++++++++++++++++++++++++++++---------\n reftable/stack_test.c |  2 +-\n 2 files changed, 29 insertions(+), 10 deletions(-)\n\ndiff --git a/reftable/stack.c b/reftable/stack.c\nindex 1d632937d7..02c6a370ba 100644\n--- a/reftable/stack.c\n+++ b/reftable/stack.c\n@@ -212,7 +212,6 @@ static int reftable_stack_reload_once(struct reftable_stack *st, char **names,\n \t\tgoto done;\n \n \tnew_tables = NULL;\n-\tst->readers_len = new_readers_len;\n \tif (st->merged != NULL) {\n \t\tmerged_table_release(st->merged);\n \t\treftable_merged_table_free(st->merged);\n@@ -220,6 +219,7 @@ static int reftable_stack_reload_once(struct reftable_stack *st, char **names,\n \tif (st->readers != NULL) {\n \t\treftable_free(st->readers);\n \t}\n+\tst->readers_len = new_readers_len;\n \tst->readers = new_readers;\n \tnew_readers = NULL;\n \tnew_readers_len = 0;\n@@ -939,14 +939,6 @@ static int stack_compact_range(struct reftable_stack *st, int first, int last,\n \tstrbuf_addstr(&new_table_path, \"/\");\n \tstrbuf_addbuf(&new_table_path, &new_table_name);\n \n-\tif (!is_empty_table) {\n-\t\terr = rename(temp_tab_file_name.buf, new_table_path.buf);\n-\t\tif (err < 0) {\n-\t\t\terr = REFTABLE_IO_ERROR;\n-\t\t\tgoto done;\n-\t\t}\n-\t}\n-\n \tfor (i = 0; i < first; i++) {\n \t\tstrbuf_addstr(&ref_list_contents, st->readers[i]->name);\n \t\tstrbuf_addstr(&ref_list_contents, \"\\n\");\n@@ -960,6 +952,32 @@ static int stack_compact_range(struct reftable_stack *st, int first, int last,\n \t\tstrbuf_addstr(&ref_list_contents, \"\\n\");\n \t}\n \n+\t/*\n+\t * Now release the merged tables and readers\n+\t */\n+\tif (st->merged != NULL) {\n+\t\treftable_merged_table_free(st->merged);\n+\t\tst->merged = NULL;\n+\t}\n+\n+\tif (st->readers != NULL) {\n+\t\tint i = 0;\n+\t\tfor (i = 0; i < st->readers_len; i++) {\n+\t\t\treader_close(st->readers[i]);\n+\t\t\treftable_reader_free(st->readers[i]);\n+\t\t}\n+\t\tst->readers_len = 0;\n+\t\tFREE_AND_NULL(st->readers);\n+\t}\n+\n+\tif (!is_empty_table) {\n+\t\terr = rename(temp_tab_file_name.buf, new_table_path.buf);\n+\t\tif (err < 0) {\n+\t\t\terr = REFTABLE_IO_ERROR;\n+\t\t\tgoto done;\n+\t\t}\n+\t}\n+\n \terr = write(lock_file_fd, ref_list_contents.buf, ref_list_contents.len);\n \tif (err < 0) {\n \t\terr = REFTABLE_IO_ERROR;\n@@ -1242,6 +1260,7 @@ static int stack_check_addition(struct reftable_stack *st,\n \n \tfree(refs);\n \treftable_iterator_destroy(&it);\n+\treader_close(rd);\n \treftable_reader_free(rd);\n \treturn err;\n }\ndiff --git a/reftable/stack_test.c b/reftable/stack_test.c\nindex 11d3d30799..c35abd7301 100644\n--- a/reftable/stack_test.c\n+++ b/reftable/stack_test.c\n@@ -159,12 +159,12 @@ static void test_reftable_stack_uptodate(void)\n \terr = reftable_stack_add(st2, &write_test_ref, &ref2);\n \tEXPECT(err == REFTABLE_LOCK_ERROR);\n \n+\treftable_stack_destroy(st1);\n \terr = reftable_stack_reload(st2);\n \tEXPECT_ERR(err);\n \n \terr = reftable_stack_add(st2, &write_test_ref, &ref2);\n \tEXPECT_ERR(err);\n-\treftable_stack_destroy(st1);\n \treftable_stack_destroy(st2);\n \tclear_dir(dir);\n }\n-- \ngitgitgadget\n\n"},{"id":"410942","messageId":"ded8d502d97d3d48dc0e4397b6153f4b06fa319b.1606545878.git.gitgitgadget@gmail.com","threadId":"54727","inReplyTo":"pull.801.git.1606545878.gitgitgadget@gmail.com","subject":"[PATCH 6/6] fixup! reftable: rest of library","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-11-28T06:44:38Z","receivedAt":"2020-11-28T22:16:56Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin <johannes.schindelin@gmx.de>\n\nThe stack_test hard-codes `/tmp/`. That is a particular problem on\nWindows where the temp directory is never at that location.\n\nLet's not do that, but instead use `TMPDIR` as we do in similar\nscenarios in the rest of Git's source code.\n\nSigned-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n---\n reftable/stack_test.c | 38 ++++++++++++++++++++++++--------------\n 1 file changed, 24 insertions(+), 14 deletions(-)\n\ndiff --git a/reftable/stack_test.c b/reftable/stack_test.c\nindex cf2e32a25c..c9beaf4dbf 100644\n--- a/reftable/stack_test.c\n+++ b/reftable/stack_test.c\n@@ -28,9 +28,19 @@ static void clear_dir(const char *dirname)\n \tstrbuf_release(&path);\n }\n \n+static char *get_tmp_template(const char *prefix)\n+{\n+\tstatic struct strbuf path = STRBUF_INIT;\n+\tconst char *tmp = getenv(\"TMPDIR\");\n+\n+\tstrbuf_reset(&path);\n+\tstrbuf_addf(&path, \"%s/%s.XXXXXX\", tmp ? tmp : \"/tmp\", prefix);\n+\treturn path.buf;\n+}\n+\n static void test_read_file(void)\n {\n-\tchar fn[256] = \"/tmp/stack.test_read_file.XXXXXX\";\n+\tchar *fn = get_tmp_template(\"stack.test_read_file\");\n \tint fd = mkstemp(fn);\n \tchar out[1024] = \"line1\\n\\nline2\\nline3\";\n \tint n, err;\n@@ -99,7 +109,7 @@ static int write_test_log(struct reftable_writer *wr, void *arg)\n \n static void test_reftable_stack_add_one(void)\n {\n-\tchar dir[256] = \"/tmp/stack_test.XXXXXX\";\n+\tchar *dir = get_tmp_template(\"stack_test\");\n \tstruct reftable_write_options cfg = { 0 };\n \tstruct reftable_stack *st = NULL;\n \tint err;\n@@ -132,7 +142,7 @@ static void test_reftable_stack_uptodate(void)\n \tstruct reftable_write_options cfg = { 0 };\n \tstruct reftable_stack *st1 = NULL;\n \tstruct reftable_stack *st2 = NULL;\n-\tchar dir[256] = \"/tmp/stack_test.XXXXXX\";\n+\tchar *dir = get_tmp_template(\"stack_test\");\n \tint err;\n \tstruct reftable_ref_record ref1 = {\n \t\t.refname = \"HEAD\",\n@@ -171,7 +181,7 @@ static void test_reftable_stack_uptodate(void)\n \n static void test_reftable_stack_transaction_api(void)\n {\n-\tchar dir[256] = \"/tmp/stack_test.XXXXXX\";\n+\tchar *dir = get_tmp_template(\"stack_test\");\n \tstruct reftable_write_options cfg = { 0 };\n \tstruct reftable_stack *st = NULL;\n \tint err;\n@@ -216,7 +226,7 @@ static void test_reftable_stack_validate_refname(void)\n \tstruct reftable_write_options cfg = { 0 };\n \tstruct reftable_stack *st = NULL;\n \tint err;\n-\tchar dir[256] = \"/tmp/stack_test.XXXXXX\";\n+\tchar *dir = get_tmp_template(\"stack_test\");\n \tint i;\n \tstruct reftable_ref_record ref = {\n \t\t.refname = \"a/b\",\n@@ -254,7 +264,7 @@ static int write_error(struct reftable_writer *wr, void *arg)\n \n static void test_reftable_stack_update_index_check(void)\n {\n-\tchar dir[256] = \"/tmp/stack_test.XXXXXX\";\n+\tchar *dir = get_tmp_template(\"stack_test\");\n \tstruct reftable_write_options cfg = { 0 };\n \tstruct reftable_stack *st = NULL;\n \tint err;\n@@ -284,7 +294,7 @@ static void test_reftable_stack_update_index_check(void)\n \n static void test_reftable_stack_lock_failure(void)\n {\n-\tchar dir[256] = \"/tmp/stack_test.XXXXXX\";\n+\tchar *dir = get_tmp_template(\"stack_test\");\n \tstruct reftable_write_options cfg = { 0 };\n \tstruct reftable_stack *st = NULL;\n \tint err, i;\n@@ -309,7 +319,7 @@ static void test_reftable_stack_add(void)\n \t\t.exact_log_message = 1,\n \t};\n \tstruct reftable_stack *st = NULL;\n-\tchar dir[256] = \"/tmp/stack_test.XXXXXX\";\n+\tchar *dir = get_tmp_template(\"stack_test\");\n \tstruct reftable_ref_record refs[2] = { { NULL } };\n \tstruct reftable_log_record logs[2] = { { NULL } };\n \tint N = ARRAY_SIZE(refs);\n@@ -385,7 +395,7 @@ static void test_reftable_stack_log_normalize(void)\n \t\t0,\n \t};\n \tstruct reftable_stack *st = NULL;\n-\tchar dir[256] = \"/tmp/stack_test.XXXXXX\";\n+\tchar *dir = get_tmp_template(\"stack_test\");\n \n \tuint8_t h1[SHA1_SIZE] = { 0x01 }, h2[SHA1_SIZE] = { 0x02 };\n \n@@ -436,7 +446,7 @@ static void test_reftable_stack_log_normalize(void)\n static void test_reftable_stack_tombstone(void)\n {\n \tint i = 0;\n-\tchar dir[256] = \"/tmp/stack_test.XXXXXX\";\n+\tchar *dir = get_tmp_template(\"stack_test\");\n \tstruct reftable_write_options cfg = { 0 };\n \tstruct reftable_stack *st = NULL;\n \tint err;\n@@ -511,7 +521,7 @@ static void test_reftable_stack_tombstone(void)\n \n static void test_reftable_stack_hash_id(void)\n {\n-\tchar dir[256] = \"/tmp/stack_test.XXXXXX\";\n+\tchar *dir = get_tmp_template(\"stack_test\");\n \tstruct reftable_write_options cfg = { 0 };\n \tstruct reftable_stack *st = NULL;\n \tint err;\n@@ -621,7 +631,7 @@ static void test_suggest_compaction_segment_nothing(void)\n \n static void test_reflog_expire(void)\n {\n-\tchar dir[256] = \"/tmp/stack.test_reflog_expire.XXXXXX\";\n+\tchar *dir = get_tmp_template(\"stack.test_reflog_expire\");\n \tstruct reftable_write_options cfg = { 0 };\n \tstruct reftable_stack *st = NULL;\n \tstruct reftable_log_record logs[20] = { { NULL } };\n@@ -701,7 +711,7 @@ static void test_empty_add(void)\n \tstruct reftable_write_options cfg = { 0 };\n \tstruct reftable_stack *st = NULL;\n \tint err;\n-\tchar dir[256] = \"/tmp/stack_test.XXXXXX\";\n+\tchar *dir = get_tmp_template(\"stack_test\");\n \tstruct reftable_stack *st2 = NULL;\n \n \tEXPECT(mkdtemp(dir));\n@@ -723,7 +733,7 @@ static void test_reftable_stack_auto_compaction(void)\n {\n \tstruct reftable_write_options cfg = { 0 };\n \tstruct reftable_stack *st = NULL;\n-\tchar dir[256] = \"/tmp/stack_test.XXXXXX\";\n+\tchar *dir = get_tmp_template(\"stack_test\");\n \tint err, i;\n \tint N = 100;\n \tEXPECT(mkdtemp(dir));\n-- \ngitgitgadget\n"},{"id":"410943","messageId":"pull.801.git.1606545878.gitgitgadget@gmail.com","threadId":"54727","inReplyTo":null,"subject":"[PATCH 0/6] Minimal patches to let reftable pass the CI builds","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-11-28T06:44:32Z","receivedAt":"2020-11-28T22:16:56Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"These patches fix the PR build failures at \nhttps://github.com/git/git/pull/847/checks?check_run_id=1460683728, and are\ndesigned to be squashed into the 16 patches of the libreftable v3 patch\nseries as submitted at \nhttps://lore.kernel.org/git/pull.847.v3.git.git.1606419752.gitgitgadget@gmail.com/\n.\n\nA smaller form of the first patch was offered on the mailing list on May\n4th: \nhttps://lore.kernel.org/git/ff60fde10919b6b8c54ecb8f38b710fac37624e3.1588599086.git.gitgitgadget@gmail.com/\n. The next three patches were presented to the mailing list on October 2nd: \nhttps://lore.kernel.org/git/nycvar.QRO.7.76.6.2010021555290.50@tvgsbejvaqbjf.bet/\n, and \nhttps://lore.kernel.org/git/nycvar.QRO.7.76.6.2010021557570.50@tvgsbejvaqbjf.bet/\n. Some of the patches required slight adjustments to accommodate for context\nchanges.\n\nGoing forward, I would like to avoid the impression that it is the\nresponsibility of the Git for Windows maintainer to keep the CI build\npassing on Windows. I am happy to assist in case it is unclear how to fix\ncertain issues. I am not happy having to implement and test those fixes\nmyself. How can we ensure this doesn't happen in the future?\n\nJohannes Schindelin (6):\n  fixup! reftable: rest of library\n  fixup! reftable: utility functions\n  fixup! reftable: rest of library\n  fixup! reftable: rest of library\n  fixup! reftable: rest of library\n  fixup! reftable: rest of library\n\n contrib/buildsystems/CMakeLists.txt | 14 +++++++--\n reftable/dump.c                     | 36 ++++++++++-------------\n reftable/stack.c                    | 37 ++++++++++++++++++------\n reftable/stack_test.c               | 44 ++++++++++++++++++-----------\n 4 files changed, 82 insertions(+), 49 deletions(-)\n\n\nbase-commit: 6229da992e43f4a2d6f4ccaccf2dbbdf11bd5a4a\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-801%2Fdscho%2Freftable-on-windows-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-801/dscho/reftable-on-windows-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/801\n-- \ngitgitgadget\n"},{"id":"410946","messageId":"5c3bdc212962a24f8f23207c6b1b3504c53903ad.1606545878.git.gitgitgadget@gmail.com","threadId":"54727","inReplyTo":"pull.801.git.1606545878.gitgitgadget@gmail.com","subject":"[PATCH 4/6] fixup! reftable: rest of library","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-11-28T06:44:36Z","receivedAt":"2020-11-28T22:16:58Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin <johannes.schindelin@gmx.de>\n\nAvoid using `getopt()`: it might be POSIX, but Git's audience is much\nlarger than POSIX. MSVC, for example, does not support `getopt()`.\n\nWithout this patch, the `vs-build` job of our CI/PR builds would simply\nfail completely.\n\nSigned-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n---\n reftable/dump.c | 32 +++++++++++++-------------------\n 1 file changed, 13 insertions(+), 19 deletions(-)\n\ndiff --git a/reftable/dump.c b/reftable/dump.c\nindex 0bd7a1cfa6..e990bffe0c 100644\n--- a/reftable/dump.c\n+++ b/reftable/dump.c\n@@ -167,40 +167,34 @@ static void print_help(void)\n int reftable_dump_main(int argc, char *const *argv)\n {\n \tint err = 0;\n-\tint opt;\n \tint opt_dump_table = 0;\n \tint opt_dump_stack = 0;\n \tint opt_compact = 0;\n-\tconst char *arg = NULL;\n-\twhile ((opt = getopt(argc, argv, \"2chts\")) != -1) {\n-\t\tswitch (opt) {\n-\t\tcase '2':\n-\t\t\thash_id = 0x73323536;\n+\tconst char *arg = NULL, *argv0 = argv[0];\n+\n+\tfor (; argc > 1; argv++, argc--)\n+\t\tif (*argv[1] != '-')\n \t\t\tbreak;\n-\t\tcase 't':\n+\t\telse if (!strcmp(\"-2\", argv[1]))\n+\t\t\thash_id = 0x73323536;\n+\t\telse if (!strcmp(\"-t\", argv[1]))\n \t\t\topt_dump_table = 1;\n-\t\t\tbreak;\n-\t\tcase 's':\n+\t\telse if (!strcmp(\"-s\", argv[1]))\n \t\t\topt_dump_stack = 1;\n-\t\t\tbreak;\n-\t\tcase 'c':\n+\t\telse if (!strcmp(\"-c\", argv[1]))\n \t\t\topt_compact = 1;\n-\t\t\tbreak;\n-\t\tcase '?':\n-\t\tcase 'h':\n+\t\telse if (!strcmp(\"-?\", argv[1]) || !strcmp(\"-h\", argv[1])) {\n \t\t\tprint_help();\n \t\t\treturn 2;\n-\t\t\tbreak;\n \t\t}\n-\t}\n \n-\tif (argv[optind] == NULL) {\n+\tif (argc != 2) {\n \t\tfprintf(stderr, \"need argument\\n\");\n \t\tprint_help();\n \t\treturn 2;\n \t}\n \n-\targ = argv[optind];\n+\targ = argv[1];\n \n \tif (opt_dump_table) {\n \t\terr = dump_table(arg);\n@@ -211,7 +205,7 @@ int reftable_dump_main(int argc, char *const *argv)\n \t}\n \n \tif (err < 0) {\n-\t\tfprintf(stderr, \"%s: %s: %s\\n\", argv[0], arg,\n+\t\tfprintf(stderr, \"%s: %s: %s\\n\", argv0, arg,\n \t\t\treftable_error_str(err));\n \t\treturn 1;\n \t}\n-- \ngitgitgadget\n\n"},{"id":"410947","messageId":"94719a3462ce77e490188b5b96211aec7d67dada.1606545878.git.gitgitgadget@gmail.com","threadId":"54727","inReplyTo":"pull.801.git.1606545878.gitgitgadget@gmail.com","subject":"[PATCH 2/6] fixup! reftable: utility functions","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-11-28T06:44:34Z","receivedAt":"2020-11-28T22:16:58Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin <johannes.schindelin@gmx.de>\n\nLet's not forget our CMake configuration.\n\nSigned-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n---\n contrib/buildsystems/CMakeLists.txt | 14 ++++++++++++--\n 1 file changed, 12 insertions(+), 2 deletions(-)\n\ndiff --git a/contrib/buildsystems/CMakeLists.txt b/contrib/buildsystems/CMakeLists.txt\nindex df539a44fa..f3a2fd3561 100644\n--- a/contrib/buildsystems/CMakeLists.txt\n+++ b/contrib/buildsystems/CMakeLists.txt\n@@ -591,6 +591,12 @@ parse_makefile_for_sources(libxdiff_SOURCES \"XDIFF_OBJS\")\n list(TRANSFORM libxdiff_SOURCES PREPEND \"${CMAKE_SOURCE_DIR}/\")\n add_library(xdiff STATIC ${libxdiff_SOURCES})\n \n+#reftable\n+parse_makefile_for_sources(reftable_SOURCES \"REFTABLE_OBJS\")\n+\n+list(TRANSFORM reftable_SOURCES PREPEND \"${CMAKE_SOURCE_DIR}/\")\n+add_library(reftable STATIC ${reftable_SOURCES})\n+\n if(WIN32)\n \tif(NOT MSVC)#use windres when compiling with gcc and clang\n \t\tadd_custom_command(OUTPUT ${CMAKE_BINARY_DIR}/git.res\n@@ -613,7 +619,7 @@ endif()\n #link all required libraries to common-main\n add_library(common-main OBJECT ${CMAKE_SOURCE_DIR}/common-main.c)\n \n-target_link_libraries(common-main libgit xdiff ${ZLIB_LIBRARIES})\n+target_link_libraries(common-main libgit xdiff reftable ${ZLIB_LIBRARIES})\n if(Intl_FOUND)\n \ttarget_link_libraries(common-main ${Intl_LIBRARIES})\n endif()\n@@ -848,11 +854,15 @@ if(BUILD_TESTING)\n add_executable(test-fake-ssh ${CMAKE_SOURCE_DIR}/t/helper/test-fake-ssh.c)\n target_link_libraries(test-fake-ssh common-main)\n \n+#reftable-tests\n+parse_makefile_for_sources(test-reftable_SOURCES \"REFTABLE_TEST_OBJS\")\n+list(TRANSFORM test-reftable_SOURCES PREPEND \"${CMAKE_SOURCE_DIR}/\")\n+\n #test-tool\n parse_makefile_for_sources(test-tool_SOURCES \"TEST_BUILTINS_OBJS\")\n \n list(TRANSFORM test-tool_SOURCES PREPEND \"${CMAKE_SOURCE_DIR}/t/helper/\")\n-add_executable(test-tool ${CMAKE_SOURCE_DIR}/t/helper/test-tool.c ${test-tool_SOURCES})\n+add_executable(test-tool ${CMAKE_SOURCE_DIR}/t/helper/test-tool.c ${test-tool_SOURCES} ${test-reftable_SOURCES})\n target_link_libraries(test-tool common-main)\n \n set_target_properties(test-fake-ssh test-tool\n-- \ngitgitgadget\n\n"},{"id":"410948","messageId":"58f2b0394546e8da2922adcbc38bdb6b53f2b313.1606545878.git.gitgitgadget@gmail.com","threadId":"54727","inReplyTo":"pull.801.git.1606545878.gitgitgadget@gmail.com","subject":"[PATCH 3/6] fixup! reftable: rest of library","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-11-28T06:44:35Z","receivedAt":"2020-11-28T22:17:00Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin <johannes.schindelin@gmx.de>\n\n0-sized arrays are actually not portable.\n\nSigned-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n---\n reftable/stack_test.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/reftable/stack_test.c b/reftable/stack_test.c\nindex c35abd7301..cf2e32a25c 100644\n--- a/reftable/stack_test.c\n+++ b/reftable/stack_test.c\n@@ -579,11 +579,11 @@ static void test_sizes_to_segments(void)\n \n static void test_sizes_to_segments_empty(void)\n {\n-\tuint64_t sizes[0];\n+\tuint64_t sizes[1];\n \n \tint seglen = 0;\n \tstruct segment *segs =\n-\t\tsizes_to_segments(&seglen, sizes, ARRAY_SIZE(sizes));\n+\t\tsizes_to_segments(&seglen, sizes, 0);\n \tEXPECT(seglen == 0);\n \treftable_free(segs);\n }\n-- \ngitgitgadget\n\n"},{"id":"410949","messageId":"8d2ae6771b6584965920f5d21829b42b7fdba80f.1606545878.git.gitgitgadget@gmail.com","threadId":"54727","inReplyTo":"pull.801.git.1606545878.gitgitgadget@gmail.com","subject":"[PATCH 5/6] fixup! reftable: rest of library","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-11-28T06:44:37Z","receivedAt":"2020-11-28T22:17:02Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin <johannes.schindelin@gmx.de>\n\nYet another instance of `= {}` initialization.\n\nSigned-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n---\n reftable/dump.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/reftable/dump.c b/reftable/dump.c\nindex e990bffe0c..7d620a3cf0 100644\n--- a/reftable/dump.c\n+++ b/reftable/dump.c\n@@ -82,7 +82,7 @@ static int dump_table(const char *tablename)\n static int compact_stack(const char *stackdir)\n {\n \tstruct reftable_stack *stack = NULL;\n-\tstruct reftable_write_options cfg = {};\n+\tstruct reftable_write_options cfg = { 0 };\n \n \tint err = reftable_new_stack(&stack, stackdir, cfg);\n \tif (err < 0)\n@@ -101,7 +101,7 @@ static int compact_stack(const char *stackdir)\n static int dump_stack(const char *stackdir)\n {\n \tstruct reftable_stack *stack = NULL;\n-\tstruct reftable_write_options cfg = {};\n+\tstruct reftable_write_options cfg = { 0 };\n \tstruct reftable_iterator it = { NULL };\n \tstruct reftable_ref_record ref = { NULL };\n \tstruct reftable_log_record log = { NULL };\n-- \ngitgitgadget\n\n"},{"id":"410998","messageId":"CAFQ2z_M8N-9pCh=+rQvGoOJNYAxnMo1upJPEgsqMT9VxP5X=YA@mail.gmail.com","threadId":"54727","inReplyTo":"pull.801.git.1606545878.gitgitgadget@gmail.com","subject":"Re: [PATCH 0/6] Minimal patches to let reftable pass the CI builds","fromName":"Han-Wen Nienhuys","fromEmail":"hanwen@google.com","sentAt":"2020-11-30T14:26:16Z","receivedAt":"2020-11-30T14:27:11Z","isPatch":true,"sender":{"key":"hanwen@google.com","avatar":"https://avatars.githubusercontent.com/u/31547?v=4"},"body":"On Sat, Nov 28, 2020 at 7:44 AM Johannes Schindelin via GitGitGadget\n<gitgitgadget@gmail.com> wrote:\n>\n> These patches fix the PR build failures at\n> https://github.com/git/git/pull/847/checks?check_run_id=1460683728, and are\n> designed to be squashed into the 16 patches of the libreftable v3 patch\n> series as submitted at\n> https://lore.kernel.org/git/pull.847.v3.git.git.1606419752.gitgitgadget@gmail.com/\n> .\n>\n> A smaller form of the first patch was offered on the mailing list on May\n> 4th:\n> https://lore.kernel.org/git/ff60fde10919b6b8c54ecb8f38b710fac37624e3.1588599086.git.gitgitgadget@gmail.com/\n> . The next three patches were presented to the mailing list on October 2nd:\n> https://lore.kernel.org/git/nycvar.QRO.7.76.6.2010021555290.50@tvgsbejvaqbjf.bet/\n> , and\n> https://lore.kernel.org/git/nycvar.QRO.7.76.6.2010021557570.50@tvgsbejvaqbjf.bet/\n> . Some of the patches required slight adjustments to accommodate for context\n> changes.\n>\n> Going forward, I would like to avoid the impression that it is the\n> responsibility of the Git for Windows maintainer to keep the CI build\n> passing on Windows. I am happy to assist in case it is unclear how to fix\n> certain issues. I am not happy having to implement and test those fixes\n> myself. How can we ensure this doesn't happen in the future?\n\nThanks for the build system fix. I think it's OK to leave it to me to\nsolve the logic problems in the C code that you found.\n\nWould you mind if I massaged these contributions directly back into\ngithub.com/google/reftable? Google has a corporate CLA from Microsoft,\nit's OK to accept this from you. For better or worse, this is still\nwhere I'm developing reftable, until it has landed in git-core.\n\n-- \nHan-Wen Nienhuys - Google Munich\nI work 80%. Don't expect answers from me on Fridays.\n--\n\nGoogle Germany GmbH, Erika-Mann-Strasse 33, 80636 Munich\n\nRegistergericht und -nummer: Hamburg, HRB 86891\n\nSitz der Gesellschaft: Hamburg\n\nGeschäftsführer: Paul Manicle, Halimah DeLaine Prado\n"},{"id":"411051","messageId":"X8YaWySkyRU3DA87@coredump.intra.peff.net","threadId":"54727","inReplyTo":"58f2b0394546e8da2922adcbc38bdb6b53f2b313.1606545878.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 3/6] fixup! reftable: rest of library","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-12-01T10:26:35Z","receivedAt":"2020-12-01T10:27:33Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Nov 28, 2020 at 06:44:35AM +0000, Johannes Schindelin via GitGitGadget wrote:\n\n> From: Johannes Schindelin <johannes.schindelin@gmx.de>\n> \n> 0-sized arrays are actually not portable.\n\nDefinitely.\n\n>  static void test_sizes_to_segments_empty(void)\n>  {\n> -\tuint64_t sizes[0];\n> +\tuint64_t sizes[1];\n>  \n>  \tint seglen = 0;\n>  \tstruct segment *segs =\n> -\t\tsizes_to_segments(&seglen, sizes, ARRAY_SIZE(sizes));\n> +\t\tsizes_to_segments(&seglen, sizes, 0);\n>  \tEXPECT(seglen == 0);\n>  \treftable_free(segs);\n\nI think passing:\n\n  sizes_to_segments(&seglen, NULL, 0);\n\nmay make the code more obvious. Unlike system functions like memcpy(),\nwe can be assured of whether our functions avoid looking at a\nzero-length array (and size_to_segments does follow that rule).\n\n  This function, of course, is nonsense that real code would not do, and\n  is just unit-testing sizes_to_segments. I'm not wild in general about\n  having a parallel suite of C tests that does not interact with our\n  usual tests, but it may be the least bad way to benefit from the\n  unit-test coverage that reftable ships with. As a rule, I'd much\n  rather see a tool exposing functionality to the command-line, which\n  can then be driven independently. I recognize that can end up\n  complicated, though.\n\n-Peff\n"},{"id":"411052","messageId":"X8Ya24TJd87hRq7H@coredump.intra.peff.net","threadId":"54727","inReplyTo":"ded8d502d97d3d48dc0e4397b6153f4b06fa319b.1606545878.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 6/6] fixup! reftable: rest of library","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-12-01T10:28:43Z","receivedAt":"2020-12-01T10:29:41Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Nov 28, 2020 at 06:44:38AM +0000, Johannes Schindelin via GitGitGadget wrote:\n\n> From: Johannes Schindelin <johannes.schindelin@gmx.de>\n> \n> The stack_test hard-codes `/tmp/`. That is a particular problem on\n> Windows where the temp directory is never at that location.\n> \n> Let's not do that, but instead use `TMPDIR` as we do in similar\n> scenarios in the rest of Git's source code.\n\nYeah, I noticed this, as well. This seems like a good band-aid, but it\nwould probably be nice if the test tool was able to write into a\nspecified directory (or even just the current directory). I don't see it\nbeing invoked anywhere, but presumably if we were to add support to our\ntest suite, we'd have a script which invokes it within a scratch\ndirectory.\n\n-Peff\n"},{"id":"411055","messageId":"CAFQ2z_M3C=mCkiPxOLhBSrXR-PDQdb7GLPRWM2egV4cO4a-c0w@mail.gmail.com","threadId":"54727","inReplyTo":"X8YaWySkyRU3DA87@coredump.intra.peff.net","subject":"Re: [PATCH 3/6] fixup! reftable: rest of library","fromName":"Han-Wen Nienhuys","fromEmail":"hanwen@google.com","sentAt":"2020-12-01T11:10:14Z","receivedAt":"2020-12-01T11:11:14Z","isPatch":true,"sender":{"key":"hanwen@google.com","avatar":"https://avatars.githubusercontent.com/u/31547?v=4"},"body":"On Tue, Dec 1, 2020 at 11:26 AM Jeff King <peff@peff.net> wrote:\n>\n> On Sat, Nov 28, 2020 at 06:44:35AM +0000, Johannes Schindelin via GitGitGadget wrote:\n>\n> > From: Johannes Schindelin <johannes.schindelin@gmx.de>\n> >\n> > 0-sized arrays are actually not portable.\n>\n> Definitely.\n>\n> >  static void test_sizes_to_segments_empty(void)\n> >  {\n> > -     uint64_t sizes[0];\n> > +     uint64_t sizes[1];\n> >\n> >       int seglen = 0;\n> >       struct segment *segs =\n> > -             sizes_to_segments(&seglen, sizes, ARRAY_SIZE(sizes));\n> > +             sizes_to_segments(&seglen, sizes, 0);\n> >       EXPECT(seglen == 0);\n> >       reftable_free(segs);\n>\n> I think passing:\n>\n>   sizes_to_segments(&seglen, NULL, 0);\n>\n> may make the code more obvious. Unlike system functions like memcpy(),\n> we can be assured of whether our functions avoid looking at a\n> zero-length array (and size_to_segments does follow that rule).\n>\n>   This function, of course, is nonsense that real code would not do, and\n\nThis test was added because 'real' code did this. In particular, if\nyou initialize a stack of reftables, the stack has zero elements. The\ntest was for the following bugfix\n\nhttps://github.com/google/reftable/commit/b2e42ecb54e413e494c1fcc13c21e24422645007\n\nChanging the array size to 1 here also prevents Valgrind to mark a\nregression as a OOB write.\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":"411063","messageId":"X8YvlS29KKYje1Az@coredump.intra.peff.net","threadId":"54727","inReplyTo":"CAFQ2z_M3C=mCkiPxOLhBSrXR-PDQdb7GLPRWM2egV4cO4a-c0w@mail.gmail.com","subject":"Re: [PATCH 3/6] fixup! reftable: rest of library","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-12-01T11:57:09Z","receivedAt":"2020-12-01T11:58:32Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Dec 01, 2020 at 12:10:14PM +0100, Han-Wen Nienhuys wrote:\n\n> > I think passing:\n> >\n> >   sizes_to_segments(&seglen, NULL, 0);\n> >\n> > may make the code more obvious. Unlike system functions like memcpy(),\n> > we can be assured of whether our functions avoid looking at a\n> > zero-length array (and size_to_segments does follow that rule).\n> >\n> >   This function, of course, is nonsense that real code would not do, and\n> \n> This test was added because 'real' code did this. In particular, if\n> you initialize a stack of reftables, the stack has zero elements. The\n> test was for the following bugfix\n> \n> https://github.com/google/reftable/commit/b2e42ecb54e413e494c1fcc13c21e24422645007\n> \n> Changing the array size to 1 here also prevents Valgrind to mark a\n> regression as a OOB write.\n\nSorry to be unclear. I meant that real code would not allocate a static\nzero-length array (though it might do so on the heap). I agree that\nmaking sure the function behaves in the 0-segment case is quite\nreasonable.\n\nPassing NULL actually makes the test more rigorous IMHO (because it is a\nstrict superset of failure modes, and is something an actual caller\nmight do if it were dynamically growing an array that started at NULL).\n\n-Peff\n"},{"id":"411069","messageId":"nycvar.QRO.7.76.6.2012011518200.54@tvgsbejvaqbjf.bet","threadId":"54727","inReplyTo":"CAFQ2z_M8N-9pCh=+rQvGoOJNYAxnMo1upJPEgsqMT9VxP5X=YA@mail.gmail.com","subject":"Re: [PATCH 0/6] Minimal patches to let reftable pass the CI builds","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-12-01T14:18:52Z","receivedAt":"2020-12-01T14:20:36Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Han-Wen,\n\nOn Mon, 30 Nov 2020, Han-Wen Nienhuys wrote:\n\n> On Sat, Nov 28, 2020 at 7:44 AM Johannes Schindelin via GitGitGadget\n> <gitgitgadget@gmail.com> wrote:\n> >\n> > These patches fix the PR build failures at\n> > https://github.com/git/git/pull/847/checks?check_run_id=1460683728, and are\n> > designed to be squashed into the 16 patches of the libreftable v3 patch\n> > series as submitted at\n> > https://lore.kernel.org/git/pull.847.v3.git.git.1606419752.gitgitgadget@gmail.com/\n> > .\n> >\n> > A smaller form of the first patch was offered on the mailing list on May\n> > 4th:\n> > https://lore.kernel.org/git/ff60fde10919b6b8c54ecb8f38b710fac37624e3.1588599086.git.gitgitgadget@gmail.com/\n> > . The next three patches were presented to the mailing list on October 2nd:\n> > https://lore.kernel.org/git/nycvar.QRO.7.76.6.2010021555290.50@tvgsbejvaqbjf.bet/\n> > , and\n> > https://lore.kernel.org/git/nycvar.QRO.7.76.6.2010021557570.50@tvgsbejvaqbjf.bet/\n> > . Some of the patches required slight adjustments to accommodate for context\n> > changes.\n> >\n> > Going forward, I would like to avoid the impression that it is the\n> > responsibility of the Git for Windows maintainer to keep the CI build\n> > passing on Windows. I am happy to assist in case it is unclear how to fix\n> > certain issues. I am not happy having to implement and test those fixes\n> > myself. How can we ensure this doesn't happen in the future?\n>\n> Thanks for the build system fix. I think it's OK to leave it to me to\n> solve the logic problems in the C code that you found.\n>\n> Would you mind if I massaged these contributions directly back into\n> github.com/google/reftable?\n\nWhatever works best for you.\n\n> Google has a corporate CLA from Microsoft, it's OK to accept this from\n> you. For better or worse, this is still where I'm developing reftable,\n> until it has landed in git-core.\n\nCiao,\nDscho\n"},{"id":"411070","messageId":"nycvar.QRO.7.76.6.2012011519340.54@tvgsbejvaqbjf.bet","threadId":"54727","inReplyTo":"X8Ya24TJd87hRq7H@coredump.intra.peff.net","subject":"Re: [PATCH 6/6] fixup! reftable: rest of library","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-12-01T14:24:01Z","receivedAt":"2020-12-01T14:25:43Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Peff,\n\nOn Tue, 1 Dec 2020, Jeff King wrote:\n\n> On Sat, Nov 28, 2020 at 06:44:38AM +0000, Johannes Schindelin via GitGitGadget wrote:\n>\n> > From: Johannes Schindelin <johannes.schindelin@gmx.de>\n> >\n> > The stack_test hard-codes `/tmp/`. That is a particular problem on\n> > Windows where the temp directory is never at that location.\n> >\n> > Let's not do that, but instead use `TMPDIR` as we do in similar\n> > scenarios in the rest of Git's source code.\n>\n> Yeah, I noticed this, as well. This seems like a good band-aid, but it\n> would probably be nice if the test tool was able to write into a\n> specified directory (or even just the current directory).\n\nYou can force it to do that by setting `TMPDIR` to the path of the current\ndirectory...\n\n> I don't see it being invoked anywhere,\n\nIt is invoked in `t/t0032-reftable-unittest.sh`:\nhttps://github.com/dscho/git/blob/reftable-on-windows/t/t0032-reftable-unittest.sh\n\n> but presumably if we were to add support to our test suite, we'd have a\n> script which invokes it within a scratch directory.\n\nI agree that it would make most sense for t0032 to prefix the call to\n`test-tool` with `TMPDIR=$PWD`.\n\nCiao,\nDscho\n"},{"id":"411071","messageId":"CAFQ2z_N=sZuYorZxNLn-OoWjb6y=1zrc7sCPuKzZHQODi08s+Q@mail.gmail.com","threadId":"54727","inReplyTo":"878bffcdfe5ca7657f839de8f7993d9098726636.1606545878.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/6] fixup! reftable: rest of library","fromName":"Han-Wen Nienhuys","fromEmail":"hanwen@google.com","sentAt":"2020-12-01T14:32:06Z","receivedAt":"2020-12-01T14:33:01Z","isPatch":true,"sender":{"key":"hanwen@google.com","avatar":"https://avatars.githubusercontent.com/u/31547?v=4"},"body":"On Sat, Nov 28, 2020 at 7:44 AM Johannes Schindelin via GitGitGadget\n<gitgitgadget@gmail.com> wrote:\n>\n> From: Johannes Schindelin <johannes.schindelin@gmx.de>\n>\n> Close the file descriptors to obsolete files before trying to delete or\n> rename them. This is actually required on Windows.\n>\n> Note: this patch is just a band-aid to get the tests pass on Windows.\n> The fact that it is needed raises concerns about the overall resource\n> handling: are file descriptors closed properly whenever appropriate, or\n> are they closed much later (which can lead to rename() problems on\n> Windows, and risks running into ulimits)?\n>\n> Also, a `reftable_stack_destroy()` call had to be moved in\n> `test_reftable_stack_uptodate()` to avoid the prompt complaining that a\n> `.ref` file could not be deleted on Windows. This raises the question\n> whether the code does the right thing when two concurrent processes want\n> to access the reftable, and one wants to compact it. At the moment, it\n> does not appear to fail gracefully.\n\nThanks for the report; I have to look more closely at these fixes; I\nfear they might be incorrect.\n\nThe reftable spec doesn't treat this case in depth, and I think it was\nrather written for Unix-like semantics. In the Unix flavor, a process\nthat wants to read can keep file descriptors open to keep reading from\nthe ref DB at a consistent snapshot.\n\nWhat is the approach that the rest of Git on Windows takes in these\ncircumstances?\n\nConsider processes P1 and P2, and the following sequence of actions\n\nP1 opens ref DB (ie. opens a set of *.ref files for read)\nP2 opens ref DB, executes a transaction. Post-transaction, it compacts\nthe reftable stack.\nP2 exits\nP1 exits\n\nCurrently, the compaction in P2 tries to delete the files obviated by\nthe compaction. On Windows this currently fails, because you can't\ndelete open files.\n\nThere are several options:\n\n1) P2 should fail the compaction. This is bad because it will lead to\ndegraded performance over time, and it's not obvious if you can\nanticipate that the deletion doesn't work.\n2) P2 should retry deleting until it succeeds. This is bad, because a\nreader can starve writers.\n3) on exit, P1 should check if its *.ref files are still in use, and\ndelete them. This smells bad, because P1 is a read-only process, yet\nit executes writes. Also, do we have good on-exit hooks in Git?\n4) On exit, P1 does nothing. Stale *.ref files are left behind. Some\nsort of GC process cleans things up asynchronously.\n5) The ref DB should not keep files open, and should rather open and\nclose files as needed; this means P1 doesn't keep files open for long,\nand P2 can retry safely.\n\nI think 3) seems the cleanest to me (even though deleting in read\nprocess feels weird), but perhaps we could fallback to 5) on windows\nas well.\n\nThoughts?\n\nWhat errno code does deleting an in-use file on Windows produce?\n\n\n> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n> ---\n>  reftable/stack.c      | 37 ++++++++++++++++++++++++++++---------\n>  reftable/stack_test.c |  2 +-\n>  2 files changed, 29 insertions(+), 10 deletions(-)\n>\n> diff --git a/reftable/stack.c b/reftable/stack.c\n> index 1d632937d7..02c6a370ba 100644\n> --- a/reftable/stack.c\n> +++ b/reftable/stack.c\n> @@ -212,7 +212,6 @@ static int reftable_stack_reload_once(struct reftable_stack *st, char **names,\n>                 goto done;\n>\n>         new_tables = NULL;\n> -       st->readers_len = new_readers_len;\n>         if (st->merged != NULL) {\n>                 merged_table_release(st->merged);\n>                 reftable_merged_table_free(st->merged);\n> @@ -220,6 +219,7 @@ static int reftable_stack_reload_once(struct reftable_stack *st, char **names,\n>         if (st->readers != NULL) {\n>                 reftable_free(st->readers);\n>         }\n> +       st->readers_len = new_readers_len;\n>         st->readers = new_readers;\n>         new_readers = NULL;\n>         new_readers_len = 0;\n> @@ -939,14 +939,6 @@ static int stack_compact_range(struct reftable_stack *st, int first, int last,\n>         strbuf_addstr(&new_table_path, \"/\");\n>         strbuf_addbuf(&new_table_path, &new_table_name);\n>\n> -       if (!is_empty_table) {\n> -               err = rename(temp_tab_file_name.buf, new_table_path.buf);\n> -               if (err < 0) {\n> -                       err = REFTABLE_IO_ERROR;\n> -                       goto done;\n> -               }\n> -       }\n> -\n>         for (i = 0; i < first; i++) {\n>                 strbuf_addstr(&ref_list_contents, st->readers[i]->name);\n>                 strbuf_addstr(&ref_list_contents, \"\\n\");\n> @@ -960,6 +952,32 @@ static int stack_compact_range(struct reftable_stack *st, int first, int last,\n>                 strbuf_addstr(&ref_list_contents, \"\\n\");\n>         }\n>\n> +       /*\n> +        * Now release the merged tables and readers\n> +        */\n> +       if (st->merged != NULL) {\n> +               reftable_merged_table_free(st->merged);\n> +               st->merged = NULL;\n> +       }\n> +\n> +       if (st->readers != NULL) {\n> +               int i = 0;\n> +               for (i = 0; i < st->readers_len; i++) {\n> +                       reader_close(st->readers[i]);\n> +                       reftable_reader_free(st->readers[i]);\n> +               }\n> +               st->readers_len = 0;\n> +               FREE_AND_NULL(st->readers);\n> +       }\n> +\n> +       if (!is_empty_table) {\n> +               err = rename(temp_tab_file_name.buf, new_table_path.buf);\n> +               if (err < 0) {\n> +                       err = REFTABLE_IO_ERROR;\n> +                       goto done;\n> +               }\n> +       }\n> +\n>         err = write(lock_file_fd, ref_list_contents.buf, ref_list_contents.len);\n>         if (err < 0) {\n>                 err = REFTABLE_IO_ERROR;\n> @@ -1242,6 +1260,7 @@ static int stack_check_addition(struct reftable_stack *st,\n>\n>         free(refs);\n>         reftable_iterator_destroy(&it);\n> +       reader_close(rd);\n>         reftable_reader_free(rd);\n>         return err;\n>  }\n> diff --git a/reftable/stack_test.c b/reftable/stack_test.c\n> index 11d3d30799..c35abd7301 100644\n> --- a/reftable/stack_test.c\n> +++ b/reftable/stack_test.c\n> @@ -159,12 +159,12 @@ static void test_reftable_stack_uptodate(void)\n>         err = reftable_stack_add(st2, &write_test_ref, &ref2);\n>         EXPECT(err == REFTABLE_LOCK_ERROR);\n>\n> +       reftable_stack_destroy(st1);\n>         err = reftable_stack_reload(st2);\n>         EXPECT_ERR(err);\n>\n>         err = reftable_stack_add(st2, &write_test_ref, &ref2);\n>         EXPECT_ERR(err);\n> -       reftable_stack_destroy(st1);\n>         reftable_stack_destroy(st2);\n>         clear_dir(dir);\n>  }\n> --\n> gitgitgadget\n>\n\n\n-- \nHan-Wen Nienhuys - Google Munich\nI work 80%. Don't expect answers from me on Fridays.\n--\n\nGoogle Germany GmbH, Erika-Mann-Strasse 33, 80636 Munich\n\nRegistergericht und -nummer: Hamburg, HRB 86891\n\nSitz der Gesellschaft: Hamburg\n\nGeschäftsführer: Paul Manicle, Halimah DeLaine Prado\n"},{"id":"411130","messageId":"X8by+j24ccvt0F04@coredump.intra.peff.net","threadId":"54727","inReplyTo":"nycvar.QRO.7.76.6.2012011519340.54@tvgsbejvaqbjf.bet","subject":"Re: [PATCH 6/6] fixup! reftable: rest of library","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-12-02T01:50:50Z","receivedAt":"2020-12-02T01:51:32Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Dec 01, 2020 at 03:24:01PM +0100, Johannes Schindelin wrote:\n\n> > I don't see it being invoked anywhere,\n> \n> It is invoked in `t/t0032-reftable-unittest.sh`:\n> https://github.com/dscho/git/blob/reftable-on-windows/t/t0032-reftable-unittest.sh\n\nAh, thank you. I was looking at what Junio has queued in hn/reftable.\n\n> > but presumably if we were to add support to our test suite, we'd have a\n> > script which invokes it within a scratch directory.\n> \n> I agree that it would make most sense for t0032 to prefix the call to\n> `test-tool` with `TMPDIR=$PWD`.\n\nYep, that would be fine.\n\nI'm not sure if the long-term goal is to have this opaque unit-test\nprogram or not. If it is, I was likewise going to suggest that its\nad-hoc output be replaced with TAP. But it looks like on your branch\nthat \"test-tool reftable\" does not produce output at all. So I may be a\nbit behind on what the current state and forward plans are...\n\n-Peff\n"},{"id":"411152","messageId":"CAFQ2z_P3WmSyBJvhCQi-rOQRvUwbK9RH6CGvS2Br0zW-=xwa4A@mail.gmail.com","threadId":"54727","inReplyTo":"X8by+j24ccvt0F04@coredump.intra.peff.net","subject":"Re: [PATCH 6/6] fixup! reftable: rest of library","fromName":"Han-Wen Nienhuys","fromEmail":"hanwen@google.com","sentAt":"2020-12-02T11:01:49Z","receivedAt":"2020-12-02T11:02:43Z","isPatch":true,"sender":{"key":"hanwen@google.com","avatar":"https://avatars.githubusercontent.com/u/31547?v=4"},"body":"On Wed, Dec 2, 2020 at 2:50 AM Jeff King <peff@peff.net> wrote:\n> > > I don't see it being invoked anywhere,\n> >\n> > It is invoked in `t/t0032-reftable-unittest.sh`:\n> > https://github.com/dscho/git/blob/reftable-on-windows/t/t0032-reftable-unittest.sh\n>\n> Ah, thank you. I was looking at what Junio has queued in hn/reftable.\n>\n> > > but presumably if we were to add support to our test suite, we'd have a\n> > > script which invokes it within a scratch directory.\n> >\n> > I agree that it would make most sense for t0032 to prefix the call to\n> > `test-tool` with `TMPDIR=$PWD`.\n>\n> Yep, that would be fine.\n>\n> I'm not sure if the long-term goal is to have this opaque unit-test\n> program or not. If it is, I was likewise going to suggest that its\n> ad-hoc output be replaced with TAP. But it looks like on your branch\n> that \"test-tool reftable\" does not produce output at all. So I may be a\n> bit behind on what the current state and forward plans are...\n\nThe most important requirement is that something fails if the\nunittests don't work. I surmised that that meant running tests from\ntest-helper in some way, so this is what happens now. Looking for\n\"unit.?test\" across the git codebase didn't turn up much info. Happy\nto explore other solutions if you can give me pointers.\n\n-- \nHan-Wen Nienhuys - Google Munich\nI work 80%. Don't expect answers from me on Fridays.\n--\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":"411156","messageId":"nycvar.QRO.7.76.6.2012021149030.25979@tvgsbejvaqbjf.bet","threadId":"54727","inReplyTo":"CAFQ2z_N=sZuYorZxNLn-OoWjb6y=1zrc7sCPuKzZHQODi08s+Q@mail.gmail.com","subject":"Re: [PATCH 1/6] fixup! reftable: rest of library","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-12-02T10:57:22Z","receivedAt":"2020-12-02T12:34:21Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Han-Wen,\n\nOn Tue, 1 Dec 2020, Han-Wen Nienhuys wrote:\n\n> On Sat, Nov 28, 2020 at 7:44 AM Johannes Schindelin via GitGitGadget\n> <gitgitgadget@gmail.com> wrote:\n> >\n> > From: Johannes Schindelin <johannes.schindelin@gmx.de>\n> >\n> > Close the file descriptors to obsolete files before trying to delete or\n> > rename them. This is actually required on Windows.\n> >\n> > Note: this patch is just a band-aid to get the tests pass on Windows.\n> > The fact that it is needed raises concerns about the overall resource\n> > handling: are file descriptors closed properly whenever appropriate, or\n> > are they closed much later (which can lead to rename() problems on\n> > Windows, and risks running into ulimits)?\n> >\n> > Also, a `reftable_stack_destroy()` call had to be moved in\n> > `test_reftable_stack_uptodate()` to avoid the prompt complaining that a\n> > `.ref` file could not be deleted on Windows. This raises the question\n> > whether the code does the right thing when two concurrent processes want\n> > to access the reftable, and one wants to compact it. At the moment, it\n> > does not appear to fail gracefully.\n>\n> Thanks for the report; I have to look more closely at these fixes; I\n> fear they might be incorrect.\n\nThey might be incorrect, but less so than the previous state, as testified\nby the previously failing PR build.\n\n> The reftable spec doesn't treat this case in depth, and I think it was\n> rather written for Unix-like semantics. In the Unix flavor, a process\n> that wants to read can keep file descriptors open to keep reading from\n> the ref DB at a consistent snapshot.\n\nThanks for the explanation. I actually knew that.\n\n> What is the approach that the rest of Git on Windows takes in these\n> circumstances?\n\nThe rest of Git (whether on Windows or not) treats this as a no-no. You\ncannot keep a handle open to a file that is deleted.\n\n> Consider processes P1 and P2, and the following sequence of actions\n>\n> P1 opens ref DB (ie. opens a set of *.ref files for read)\n> P2 opens ref DB, executes a transaction. Post-transaction, it compacts\n> the reftable stack.\n> P2 exits\n> P1 exits\n>\n> Currently, the compaction in P2 tries to delete the files obviated by\n> the compaction. On Windows this currently fails, because you can't\n> delete open files.\n\nIndeed. So the design needs to be fixed, if it fails.\n\n> There are several options:\n>\n> 1) P2 should fail the compaction. This is bad because it will lead to\n> degraded performance over time, and it's not obvious if you can\n> anticipate that the deletion doesn't work.\n> 2) P2 should retry deleting until it succeeds. This is bad, because a\n> reader can starve writers.\n> 3) on exit, P1 should check if its *.ref files are still in use, and\n> delete them. This smells bad, because P1 is a read-only process, yet\n> it executes writes. Also, do we have good on-exit hooks in Git?\n> 4) On exit, P1 does nothing. Stale *.ref files are left behind. Some\n> sort of GC process cleans things up asynchronously.\n> 5) The ref DB should not keep files open, and should rather open and\n> close files as needed; this means P1 doesn't keep files open for long,\n> and P2 can retry safely.\n>\n> I think 3) seems the cleanest to me (even though deleting in read\n> process feels weird), but perhaps we could fallback to 5) on windows\n> as well.\n\nTraditionally, Git would fail gracefully (i.e. with a warning) to delete\nthe stale files, and try again at a later stage (during `git gc --auto`,\nfor example, or after the next compaction step).\n\n> What errno code does deleting an in-use file on Windows produce?\n\nI believe it would be `EACCES`. See\nhttps://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/unlink-wunlink?view=msvc-160\nfor the documented behavior (I believe that an in-use file is treated the\nsame way as a read-only file in this instance).\n\nCiao,\nDscho\n"},{"id":"411158","messageId":"X8eMAI4703sDpTzM@coredump.intra.peff.net","threadId":"54727","inReplyTo":"CAFQ2z_P3WmSyBJvhCQi-rOQRvUwbK9RH6CGvS2Br0zW-=xwa4A@mail.gmail.com","subject":"Re: [PATCH 6/6] fixup! reftable: rest of library","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-12-02T12:43:44Z","receivedAt":"2020-12-02T12:44:38Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Dec 02, 2020 at 12:01:49PM +0100, Han-Wen Nienhuys wrote:\n\n> > I'm not sure if the long-term goal is to have this opaque unit-test\n> > program or not. If it is, I was likewise going to suggest that its\n> > ad-hoc output be replaced with TAP. But it looks like on your branch\n> > that \"test-tool reftable\" does not produce output at all. So I may be a\n> > bit behind on what the current state and forward plans are...\n> \n> The most important requirement is that something fails if the\n> unittests don't work. I surmised that that meant running tests from\n> test-helper in some way, so this is what happens now. Looking for\n> \"unit.?test\" across the git codebase didn't turn up much info. Happy\n> to explore other solutions if you can give me pointers.\n\nNormally we do a combination of:\n\n  - git plumbing exposes scriptable commands, and we make sure those\n    work. This is a much coarser-grained unit than testing individual C\n    functions, but produces resilient tests because that interface is\n    user-visible and stable (and in fact seeing test breakages is often\n    a sign that one will be breaking real users).\n\n    These are obviously driven by shell script tests emulating what\n    users might run.\n\n  - for unit tests of individual data types where it's not appropriate\n    to have a user-facing command, we often add a test-tool helper that\n    exposes specific functions (e.g., t/helper/test-date.c exposes date\n    parsing and formatting routines, test-oid-array.c exercises methods\n    of that data type, etc).\n\n    The C parts of these are usually generic, and then are driven by\n    shell scripts providing the actual data in individual tests (and\n    handling success/failure, skipping tests, etc).\n\n    This is still coarser than you might get unit-testing inside C.\n    E.g., you could not generally check the difference between passing\n    an empty array vs NULL to a function. But our philosophy has\n    generally been _not_ to test at that level. The C interfaces are\n    internal, and Git is not that big a project. If there's a function\n    whose caller does something unexpected, it's usually easier to fix\n    the caller and add a test that triggers the caller's code path.\n\nI agree that a test that simply runs a bunch of C code and either exits\nwith failure or success is better than nothing, in the sense of finding\ntests. And wrapping that with a single test_expect_success does that.\nBut it's unfortunate that we get none of the fine-grained control that\nthe test suite provides, nor much support in debugging failing tests.\n\nOne middle ground would be for a battery of C tests to output\nTAP-compatible output (outputting \"ok 1 - foo works\", and \"not ok 2 -\nbar does not work\", etc). That at least gives more info on what fails,\nand does it in a way that the rest of the test suite can interpret\n(though not manipulate).\n\n-Peff\n"},{"id":"411176","messageId":"CAFQ2z_PNbMjrhVBaDsDWZbrgCFSQn=VBxxiyh=wpy+JZADmHcQ@mail.gmail.com","threadId":"54727","inReplyTo":"nycvar.QRO.7.76.6.2012021149030.25979@tvgsbejvaqbjf.bet","subject":"Reftable locking on Windows (Re: [PATCH 1/6] fixup! reftable: rest of library)","fromName":"Han-Wen Nienhuys","fromEmail":"hanwen@google.com","sentAt":"2020-12-02T18:31:57Z","receivedAt":"2020-12-02T18:32:55Z","isPatch":true,"sender":{"key":"hanwen@google.com","avatar":"https://avatars.githubusercontent.com/u/31547?v=4"},"body":"On Wed, Dec 2, 2020 at 1:32 PM Johannes Schindelin\n<Johannes.Schindelin@gmx.de> wrote:\n> > Consider processes P1 and P2, and the following sequence of actions\n> >\n> > P1 opens ref DB (ie. opens a set of *.ref files for read)\n> > P2 opens ref DB, executes a transaction. Post-transaction, it compacts\n> > the reftable stack.\n> > P2 exits\n> > P1 exits\n> >\n> > Currently, the compaction in P2 tries to delete the files obviated by\n> > the compaction. On Windows this currently fails, because you can't\n> > delete open files.\n>\n> Indeed. So the design needs to be fixed, if it fails.\n>\n> > There are several options:\n> >\n> > 1) P2 should fail the compaction. This is bad because it will lead to\n> > degraded performance over time, and it's not obvious if you can\n> > anticipate that the deletion doesn't work.\n> > 2) P2 should retry deleting until it succeeds. This is bad, because a\n> > reader can starve writers.\n> > 3) on exit, P1 should check if its *.ref files are still in use, and\n> > delete them. This smells bad, because P1 is a read-only process, yet\n> > it executes writes. Also, do we have good on-exit hooks in Git?\n> > 4) On exit, P1 does nothing. Stale *.ref files are left behind. Some\n> > sort of GC process cleans things up asynchronously.\n> > 5) The ref DB should not keep files open, and should rather open and\n> > close files as needed; this means P1 doesn't keep files open for long,\n> > and P2 can retry safely.\n> >\n> > I think 3) seems the cleanest to me (even though deleting in read\n> > process feels weird), but perhaps we could fallback to 5) on windows\n> > as well.\n>\n> Traditionally, Git would fail gracefully (i.e. with a warning) to delete\n> the stale files, and try again at a later stage (during `git gc --auto`,\n> for example, or after the next compaction step).\n\nSo, how does this sound:\n\n* add a void\n\n  set_warn_handler(void(*handler)(char const*))\n\nThe handler is called for non-fatal errors (eg. deleting an in-use\n.ref file during compaction), and can provide the warning.\n\n* all .ref files will be prefixed with an 8-byte random string, to\navoid that new *.ref files collide with unreferenced, stale *.ref\nfiles.\n\n* in reftable_stack_close(), after closing files, we check if *.ref\nfiles we were reading have gone out of use. If so, delete those .ref\nfiles, printing a warning on EACCESS.\n\n* an on-exit handler in git calls reftable_stack_close()\n\n* provide a reftable_stack_gc(), which loads tables.list and then\ndeletes all unused *.ref files that are older than tables.list.\n\n> > What errno code does deleting an in-use file on Windows produce?\n>\n> I believe it would be `EACCES`. See\n> https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/unlink-wunlink?view=msvc-160\n> for the documented behavior (I believe that an in-use file is treated the\n> same way as a read-only file in this instance).\n\nyour commit message says \"to avoid the prompt complaining that a\n`.ref` file could not be deleted on Windows.\" AFAICT, this prompt is\ncoming from windows itself, because the reftable library doesn't issue\nthis type of warning. Is there a way to delete a file that just\nreturns EACCESS if it fails, without triggering a prompt? Or is the\nprompt part of the \"failing gracefully\" behavior?\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":"411217","messageId":"87k0tzulf1.fsf@evledraar.gmail.com","threadId":"54727","inReplyTo":"CAFQ2z_PNbMjrhVBaDsDWZbrgCFSQn=VBxxiyh=wpy+JZADmHcQ@mail.gmail.com","subject":"Re: Reftable locking on Windows (Re: [PATCH 1/6] fixup! reftable: rest of library)","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2020-12-03T12:24:50Z","receivedAt":"2020-12-03T12:25:42Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Dec 02 2020, Han-Wen Nienhuys wrote:\n\n> On Wed, Dec 2, 2020 at 1:32 PM Johannes Schindelin\n> <Johannes.Schindelin@gmx.de> wrote:\n>> > Consider processes P1 and P2, and the following sequence of actions\n>> >\n>> > P1 opens ref DB (ie. opens a set of *.ref files for read)\n>> > P2 opens ref DB, executes a transaction. Post-transaction, it compacts\n>> > the reftable stack.\n>> > P2 exits\n>> > P1 exits\n>> >\n>> > Currently, the compaction in P2 tries to delete the files obviated by\n>> > the compaction. On Windows this currently fails, because you can't\n>> > delete open files.\n>>\n>> Indeed. So the design needs to be fixed, if it fails.\n>>\n>> > There are several options:\n>> >\n>> > 1) P2 should fail the compaction. This is bad because it will lead to\n>> > degraded performance over time, and it's not obvious if you can\n>> > anticipate that the deletion doesn't work.\n>> > 2) P2 should retry deleting until it succeeds. This is bad, because a\n>> > reader can starve writers.\n>> > 3) on exit, P1 should check if its *.ref files are still in use, and\n>> > delete them. This smells bad, because P1 is a read-only process, yet\n>> > it executes writes. Also, do we have good on-exit hooks in Git?\n>> > 4) On exit, P1 does nothing. Stale *.ref files are left behind. Some\n>> > sort of GC process cleans things up asynchronously.\n>> > 5) The ref DB should not keep files open, and should rather open and\n>> > close files as needed; this means P1 doesn't keep files open for long,\n>> > and P2 can retry safely.\n>> >\n>> > I think 3) seems the cleanest to me (even though deleting in read\n>> > process feels weird), but perhaps we could fallback to 5) on windows\n>> > as well.\n>>\n>> Traditionally, Git would fail gracefully (i.e. with a warning) to delete\n>> the stale files, and try again at a later stage (during `git gc --auto`,\n>> for example, or after the next compaction step).\n>\n> So, how does this sound:\n>\n> * add a void\n>\n>   set_warn_handler(void(*handler)(char const*))\n>\n> The handler is called for non-fatal errors (eg. deleting an in-use\n> .ref file during compaction), and can provide the warning.\n>\n> * all .ref files will be prefixed with an 8-byte random string, to\n> avoid that new *.ref files collide with unreferenced, stale *.ref\n> files.\n\nJust trying to follow along here. Do you mean prefix the file name or\nthe content of those *.ref files? In any case isn't this synonymous with\nproposing moving beyond reftable v1 (to the WIP v2, or a v1.1 with just\nthis change?). The current spec seems to preclude prefixing either the\nfile content or filename, but maybe I'm misreading it:\n\nhttps://googlers.googlesource.com/sop/jgit/+/reftable/Documentation/technical/reftable.md#update-transactions\nhttps://googlers.googlesource.com/sop/jgit/+/reftable/Documentation/technical/reftable.md#header\n\nWhen the reftable format was initially being discussed I suggested\nsomewhat flippantly (why not SQLite?). Partially because in trying to\ninvent what's essentially a portable and concurrently updated database\nformat we'd be likely to reinvent much of the same wheel.\n\nSo not to drag that whole thing up again as a proposal for a format\nreplacement, but I wonder what SQLite would do in this scenario & others\nwhere desired DB semantics / transactions and FS/portability semantics\nclash. AFAIK the reftable has only been battle-tested in production\nunder JGit so far (presumably Linux-only). Whereas SQLite has been\nported to and widely used on Windows, HP/UX and probably other systems\nwhere Linux-specific assumptions don't apply.\n\n> * in reftable_stack_close(), after closing files, we check if *.ref\n> files we were reading have gone out of use. If so, delete those .ref\n> files, printing a warning on EACCESS.\n>\n> * an on-exit handler in git calls reftable_stack_close()\n>\n> * provide a reftable_stack_gc(), which loads tables.list and then\n> deletes all unused *.ref files that are older than tables.list.\n>\n>> > What errno code does deleting an in-use file on Windows produce?\n>>\n>> I believe it would be `EACCES`. See\n>> https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/unlink-wunlink?view=msvc-160\n>> for the documented behavior (I believe that an in-use file is treated the\n>> same way as a read-only file in this instance).\n>\n> your commit message says \"to avoid the prompt complaining that a\n> `.ref` file could not be deleted on Windows.\" AFAICT, this prompt is\n> coming from windows itself, because the reftable library doesn't issue\n> this type of warning. Is there a way to delete a file that just\n> returns EACCESS if it fails, without triggering a prompt? Or is the\n> prompt part of the \"failing gracefully\" behavior?\n>\n> -- \n> Han-Wen Nienhuys - Google Munich\n> I work 80%. Don't expect answers from me on Fridays.\n\n"},{"id":"411220","messageId":"CAFQ2z_PBk+fHLQ_hP5Lo1qJEX+eHnfWV_pbzkijajdMJDauW-A@mail.gmail.com","threadId":"54727","inReplyTo":"87k0tzulf1.fsf@evledraar.gmail.com","subject":"Re: Reftable locking on Windows (Re: [PATCH 1/6] fixup! reftable: rest of library)","fromName":"Han-Wen Nienhuys","fromEmail":"hanwen@google.com","sentAt":"2020-12-03T13:56:43Z","receivedAt":"2020-12-03T13:57:53Z","isPatch":true,"sender":{"key":"hanwen@google.com","avatar":"https://avatars.githubusercontent.com/u/31547?v=4"},"body":"On Thu, Dec 3, 2020 at 1:24 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:\n> >> Indeed. So the design needs to be fixed, if it fails.\n> >>\n> >> > There are several options:\n> >> >\n> >> > 1) P2 should fail the compaction. This is bad because it will lead to\n> >> > degraded performance over time, and it's not obvious if you can\n> >> > anticipate that the deletion doesn't work.\n> >> > 2) P2 should retry deleting until it succeeds. This is bad, because a\n> >> > reader can starve writers.\n> >> > 3) on exit, P1 should check if its *.ref files are still in use, and\n> >> > delete them. This smells bad, because P1 is a read-only process, yet\n> >> > it executes writes. Also, do we have good on-exit hooks in Git?\n> >> > 4) On exit, P1 does nothing. Stale *.ref files are left behind. Some\n> >> > sort of GC process cleans things up asynchronously.\n> >> > 5) The ref DB should not keep files open, and should rather open and\n> >> > close files as needed; this means P1 doesn't keep files open for long,\n> >> > and P2 can retry safely.\n> >> >\n> >> > I think 3) seems the cleanest to me (even though deleting in read\n> >> > process feels weird), but perhaps we could fallback to 5) on windows\n> >> > as well.\n> >>\n> >> Traditionally, Git would fail gracefully (i.e. with a warning) to delete\n> >> the stale files, and try again at a later stage (during `git gc --auto`,\n> >> for example, or after the next compaction step).\n> >\n> > So, how does this sound:\n> >\n> > * add a void\n> >\n> >   set_warn_handler(void(*handler)(char const*))\n> >\n> > The handler is called for non-fatal errors (eg. deleting an in-use\n> > .ref file during compaction), and can provide the warning.\n> >\n> > * all .ref files will be prefixed with an 8-byte random string, to\n> > avoid that new *.ref files collide with unreferenced, stale *.ref\n> > files.\n>\n> Just trying to follow along here. Do you mean prefix the file name or\n> the content of those *.ref files? In any case isn't this synonymous with\n> proposing moving beyond reftable v1 (to the WIP v2, or a v1.1 with just\n> this change?). The current spec seems to preclude prefixing either the\n> file content or filename, but maybe I'm misreading it:\n\nThis is about prefixing the filename with an uniquifying prefix. This\nis because the range of timestamps encompassed by the table doesn't\nuniquely identify the table.\n\nConsider the following case:\n  State: 1 table \"00001.ref\"\n  Transaction 1: add ref xyz at timestamp 2\n     add ref abc at timestamp 2\n     => 00002.ref\n  Transaction 2: add delete ref xyz at timestamp 3\n     => 00003.ref\n\nOn compacting 00002 and 00003 together, you'll end up with a table for\nonly  \"ref abc at timestamp 2\". If you call that result 00002.ref\nagain, there are two versions 00002.ref.\n\nIn the current spec with unix semantics, that's not a problem, because\nout of date tables are removed, and disappear once the last reader closes\nthe file, but if we have to cater for a system\nwhere removal that doesn't always work, we'd have to fix up things\nafterwards. That means we have to be able to tell apart these two\nversions of 00002.ref, and this is where a random prefix would help.\n\nThe exact naming of the tables is not central to the spec, as the\ntable names to read are listed in tables.list, and systems can write\nfilenames as they please. Maybe recommendations around filenames\nshould be phrased as a \"SHOULD\" clause, with a \"MUST\" that each table\nhas a globally unique name.\n\n> https://googlers.googlesource.com/sop/jgit/+/reftable/Documentation/technical/reftable.md#update-transactions\n> https://googlers.googlesource.com/sop/jgit/+/reftable/Documentation/technical/reftable.md#header\n>\n> When the reftable format was initially being discussed I suggested\n> somewhat flippantly (why not SQLite?). Partially because in trying to\n> invent what's essentially a portable and concurrently updated database\n> format we'd be likely to reinvent much of the same wheel.\n>\n> So not to drag that whole thing up again as a proposal for a format\n> replacement, but I wonder what SQLite would do in this scenario & others\n> where desired DB semantics / transactions and FS/portability semantics\n> clash. AFAIK the reftable has only been battle-tested in production\n> under JGit so far (presumably Linux-only). Whereas SQLite has been\n> ported to and widely used on Windows, HP/UX and probably other systems\n> where Linux-specific assumptions don't apply.\n\nI think it would be an interesting exercise to build a ref database on\nSQLite, and I wish it had been done before, because large part of the\nwork of getting reftable to work with git proper, is disentangling\nthe code with assumptions about the underlying ref storage system.\n\nI agree that some of this is reinventing the wheel, and I wish that\nreftable had been based on another format (say, LevelDB) to get away\nfrom bit mangling, but I think SQlite is a tricky proposition, because\nSQLite is a system with considerable complexity of its own. You'd\nforce all the other Git implementations (libgit2, Microsoft's C#,\ngo-git, JGit, Dulwhich etc.) to find a way to implement SQLite\nsupport. That will be a difficult proposition for the implementations that\ntry to avoid FFI.\n\nYou could also look at this from the positive side, which is that the\ntransaction semantics of reftable are much easier to understand and\nverify than those of the files backend.\n\n--\nHan-Wen Nienhuys - Google Munich\nI work 80%. Don't expect answers from me on Fridays.\n--\n\nGoogle Germany GmbH, Erika-Mann-Strasse 33, 80636 Munich\n\nRegistergericht und -nummer: Hamburg, HRB 86891\n\nSitz der Gesellschaft: Hamburg\n\nGeschäftsführer: Paul Manicle, Halimah DeLaine Prado\n--\n\nGoogle Germany GmbH, Erika-Mann-Strasse 33, 80636 Munich\n\nRegistergericht und -nummer: Hamburg, HRB 86891\n\nSitz der Gesellschaft: Hamburg\n\nGeschäftsführer: Paul Manicle, Halimah DeLaine Prado\n"}]}