[PATCH 1/6] fixup! reftable: rest of library
- From
Johannes Schindelin via GitGitGadget <gitgitgadget@gmail.com>
- Date
- Nov 28, 2020, 06:44 UTC
- Message-ID
- <878bffcdfe5ca7657f839de8f7993d9098726636.1606545878.git.gitgitgadget@gmail.com>
- In-Reply-To
- <pull.801.git.1606545878.gitgitgadget@gmail.com>
From: Johannes Schindelin <johannes.schindelin@gmx.de>
Close the file descriptors to obsolete files before trying to delete or rename them. This is actually required on Windows.
Note: this patch is just a band-aid to get the tests pass on Windows. The fact that it is needed raises concerns about the overall resource handling: are file descriptors closed properly whenever appropriate, or are they closed much later (which can lead to rename() problems on Windows, and risks running into ulimits)?
Also, a `reftable_stack_destroy()` call had to be moved in `test_reftable_stack_uptodate()` to avoid the prompt complaining that a `.ref` file could not be deleted on Windows. This raises the question whether the code does the right thing when two concurrent processes want to access the reftable, and one wants to compact it. At the moment, it does not appear to fail gracefully.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> --- reftable/stack.c | 37 ++++++++++++++++++++++++++++--------- reftable/stack_test.c | 2 +- 2 files changed, 29 insertions(+), 10 deletions(-)
diff --git a/reftable/stack.c b/reftable/stack.c index 1d632937d7..02c6a370ba 100644 --- a/reftable/stack.c +++ b/reftable/stack.c @@ -212,7 +212,6 @@ static int reftable_stack_reload_once(struct reftable_stack *st, char **names, goto done; new_tables = NULL; - st->readers_len = new_readers_len; if (st->merged != NULL) { merged_table_release(st->merged); reftable_merged_table_free(st->merged); @@ -220,6 +219,7 @@ static int reftable_stack_reload_once(struct reftable_stack *st, char **names, if (st->readers != NULL) { reftable_free(st->readers); } + st->readers_len = new_readers_len; st->readers = new_readers; new_readers = NULL; new_readers_len = 0; @@ -939,14 +939,6 @@ static int stack_compact_range(struct reftable_stack *st, int first, int last, strbuf_addstr(&new_table_path, "/"); strbuf_addbuf(&new_table_path, &new_table_name); - if (!is_empty_table) { - err = rename(temp_tab_file_name.buf, new_table_path.buf); - if (err < 0) { - err = REFTABLE_IO_ERROR; - goto done; - } - } - for (i = 0; i < first; i++) { strbuf_addstr(&ref_list_contents, st->readers[i]->name); strbuf_addstr(&ref_list_contents, "\n"); @@ -960,6 +952,32 @@ static int stack_compact_range(struct reftable_stack *st, int first, int last, strbuf_addstr(&ref_list_contents, "\n"); } + /* + * Now release the merged tables and readers + */ + if (st->merged != NULL) { + reftable_merged_table_free(st->merged); + st->merged = NULL; + } + + if (st->readers != NULL) { + int i = 0; + for (i = 0; i < st->readers_len; i++) { + reader_close(st->readers[i]); + reftable_reader_free(st->readers[i]); + } + st->readers_len = 0; + FREE_AND_NULL(st->readers); + } + + if (!is_empty_table) { + err = rename(temp_tab_file_name.buf, new_table_path.buf); + if (err < 0) { + err = REFTABLE_IO_ERROR; + goto done; + } + } + err = write(lock_file_fd, ref_list_contents.buf, ref_list_contents.len); if (err < 0) { err = REFTABLE_IO_ERROR; @@ -1242,6 +1260,7 @@ static int stack_check_addition(struct reftable_stack *st, free(refs); reftable_iterator_destroy(&it); + reader_close(rd); reftable_reader_free(rd); return err; } diff --git a/reftable/stack_test.c b/reftable/stack_test.c index 11d3d30799..c35abd7301 100644 --- a/reftable/stack_test.c +++ b/reftable/stack_test.c @@ -159,12 +159,12 @@ static void test_reftable_stack_uptodate(void) err = reftable_stack_add(st2, &write_test_ref, &ref2); EXPECT(err == REFTABLE_LOCK_ERROR); + reftable_stack_destroy(st1); err = reftable_stack_reload(st2); EXPECT_ERR(err); err = reftable_stack_add(st2, &write_test_ref, &ref2); EXPECT_ERR(err); - reftable_stack_destroy(st1); reftable_stack_destroy(st2); clear_dir(dir); }
-- gitgitgadget