Minimal patches to let reftable pass the CI builds

These patches fix the PR build failures at
https://github.com/git/git/pull/847/checks?check_run_id=1460683728, and are designed to be squashed into the 16 patches of the libreftable v3 patch series as submitted at
https://lore.kernel.org/git/[email protected]/
.

A smaller form of the first patch was offered on the mailing list on May 4th:
https://lore.kernel.org/git/ff60fde10919b6b8c54ecb8f38b710fac37624e3.1588599086.git.gitgitgadget@gmail.com/
. The next three patches were presented to the mailing list on October 2nd:
https://lore.kernel.org/git/[email protected]/
, and
https://lore.kernel.org/git/[email protected]/
. Some of the patches required slight adjustments to accommodate for context changes.

Going forward, I would like to avoid the impression that it is the responsibility of the Git for Windows maintainer to keep the CI build passing on Windows. I am happy to assist in case it is unclear how to fix certain issues. I am not happy having to implement and test those fixes myself. How can we ensure this doesn't happen in the future?

From: Johannes Schindelin <[email protected]>

0-sized arrays are actually not portable.

From: Johannes Schindelin <[email protected]>

The stack_test hard-codes /tmp/. That is a particular problem on Windows where the temp directory is never at that location.

Let's not do that, but instead use TMPDIR as we do in similar scenarios in the rest of Git's source code.

Quoted 3 lines · Johannes Schindelin via GitGitGadget

From: Johannes Schindelin <[email protected]>

0-sized arrays are actually not portable.

Definitely.

reftable/stack_test.c static void test_sizes_to_segments(void) · line 588

Quoted 11 lines

static void test_sizes_to_segments_empty(void)
{

reftable/stack_test.c +2 -2

-     uint64_t sizes[0];
+     uint64_t sizes[1];
 
      int seglen = 0;
      struct segment *segs =
-             sizes_to_segments(&seglen, sizes, ARRAY_SIZE(sizes));
+             sizes_to_segments(&seglen, sizes, 0);
      EXPECT(seglen == 0);
      reftable_free(segs);

I think passing:

  sizes_to_segments(&seglen, NULL, 0);

may make the code more obvious. Unlike system functions like memcpy(), we can be assured of whether our functions avoid looking at a zero-length array (and size_to_segments does follow that rule).

This function, of course, is nonsense that real code would not do, and

Quoted 9 lines

I think passing:

  sizes_to_segments(&seglen, NULL, 0);

may make the code more obvious. Unlike system functions like memcpy(), we can be assured of whether our functions avoid looking at a zero-length array (and size_to_segments does follow that rule).

This function, of course, is nonsense that real code would not do, and

This test was added because 'real' code did this. In particular, if you initialize a stack of reftables, the stack has zero elements. The test was for the following bugfix

https://github.com/google/reftable/commit/b2e42ecb54e413e494c1fcc13c21e24422645007

Changing the array size to 1 here also prevents Valgrind to mark a regression as a OOB write.

Quoted 18 lines · Johannes Schindelin via GitGitGadget

These patches fix the PR build failures at
https://github.com/git/git/pull/847/checks?check_run_id=1460683728, and are designed to be squashed into the 16 patches of the libreftable v3 patch series as submitted at
https://lore.kernel.org/git/[email protected]/
.

A smaller form of the first patch was offered on the mailing list on May 4th:
https://lore.kernel.org/git/ff60fde10919b6b8c54ecb8f38b710fac37624e3.1588599086.git.gitgitgadget@gmail.com/
. The next three patches were presented to the mailing list on October 2nd:
https://lore.kernel.org/git/[email protected]/
, and
https://lore.kernel.org/git/[email protected]/
. Some of the patches required slight adjustments to accommodate for context changes.

Quoted 5 lines

Going forward, I would like to avoid the impression that it is the responsibility of the Git for Windows maintainer to keep the CI build passing on Windows. I am happy to assist in case it is unclear how to fix certain issues. I am not happy having to implement and test those fixes myself. How can we ensure this doesn't happen in the future?

Thanks for the build system fix. I think it's OK to leave it to me to solve the logic problems in the C code that you found.

Would you mind if I massaged these contributions directly back into github.com/google/reftable?

Google has a corporate CLA from Microsoft, it's OK to accept this from you. For better or worse, this is still where I'm developing reftable, until it has landed in git-core.

Quoted 5 lines · Johannes Schindelin via GitGitGadget

From: Johannes Schindelin <[email protected]>

The stack_test hard-codes /tmp/. That is a particular problem on Windows where the temp directory is never at that location.

Quoted 2 lines

Let's not do that, but instead use TMPDIR as we do in similar scenarios in the rest of Git's source code.

Yeah, I noticed this, as well. This seems like a good band-aid, but it would probably be nice if the test tool was able to write into a specified directory (or even just the current directory).

I don't see it being invoked anywhere,

but presumably if we were to add support to our test suite, we'd have a script which invokes it within a scratch directory.

From: Johannes Schindelin <[email protected]>

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 <[email protected]>

---
 reftable/stack.c      | 37 ++++++++++++++++++++++++++++---------
 reftable/stack_test.c |  2 +-
 2 files changed, 29 insertions(+), 10 deletions(-)

reftable/stack.c +28 -9

index 1d632937d7..02c6a370ba 100644
@@ -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;
 }

reftable/stack_test.c +1 -2

index 11d3d30799..c35abd7301 100644
@@ -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

Quoted 1 line

I don't see it being invoked anywhere,

It is invoked in t/t0032-reftable-unittest.sh:
https://github.com/dscho/git/blob/reftable-on-windows/t/t0032-reftable-unittest.sh

Quoted 2 lines

but presumably if we were to add support to our test suite, we'd have a script which invokes it within a scratch directory.

I agree that it would make most sense for t0032 to prefix the call to test-tool with TMPDIR=$PWD.

Quoted 4 lines

Quoted 1 line

I don't see it being invoked anywhere,

It is invoked in t/t0032-reftable-unittest.sh:
https://github.com/dscho/git/blob/reftable-on-windows/t/t0032-reftable-unittest.sh

Ah, thank you. I was looking at what Junio has queued in hn/reftable.

Quoted 5 lines

Quoted 2 lines

but presumably if we were to add support to our test suite, we'd have a script which invokes it within a scratch directory.

I agree that it would make most sense for t0032 to prefix the call to test-tool with TMPDIR=$PWD.

Yep, that would be fine.

I'm not sure if the long-term goal is to have this opaque unit-test program or not. If it is, I was likewise going to suggest that its ad-hoc output be replaced with TAP. But it looks like on your branch that "test-tool reftable" does not produce output at all. So I may be a bit behind on what the current state and forward plans are...

Quoted 12 lines · Johannes Schindelin via GitGitGadget

From: Johannes Schindelin <[email protected]>

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)?

Quoted 6 lines

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.

Thanks for the report; I have to look more closely at these fixes; I fear they might be incorrect.

The reftable spec doesn't treat this case in depth, and I think it was rather written for Unix-like semantics. In the Unix flavor, a process that wants to read can keep file descriptors open to keep reading from the ref DB at a consistent snapshot.

What is the approach that the rest of Git on Windows takes in these circumstances?

Consider processes P1 and P2, and the following sequence of actions

P1 opens ref DB (ie. opens a set of *.ref files for read) P2 opens ref DB, executes a transaction. Post-transaction, it compacts the reftable stack.
P2 exits
P1 exits

Currently, the compaction in P2 tries to delete the files obviated by the compaction. On Windows this currently fails, because you can't delete open files.

There are several options:

1) P2 should fail the compaction. This is bad because it will lead to degraded performance over time, and it's not obvious if you can anticipate that the deletion doesn't work.
2) P2 should retry deleting until it succeeds. This is bad, because a reader can starve writers.
3) on exit, P1 should check if its *.ref files are still in use, and delete them. This smells bad, because P1 is a read-only process, yet it executes writes. Also, do we have good on-exit hooks in Git?
4) On exit, P1 does nothing. Stale *.ref files are left behind. Some sort of GC process cleans things up asynchronously.
5) The ref DB should not keep files open, and should rather open and close files as needed; this means P1 doesn't keep files open for long, and P2 can retry safely.

I think 3) seems the cleanest to me (even though deleting in read process feels weird), but perhaps we could fallback to 5) on windows as well.

What errno code does deleting an in-use file on Windows produce?

Quoted 5 lines

I'm not sure if the long-term goal is to have this opaque unit-test program or not. If it is, I was likewise going to suggest that its ad-hoc output be replaced with TAP. But it looks like on your branch that "test-tool reftable" does not produce output at all. So I may be a bit behind on what the current state and forward plans are...

The most important requirement is that something fails if the unittests don't work. I surmised that that meant running tests from test-helper in some way, so this is what happens now. Looking for "unit.?test" across the git codebase didn't turn up much info. Happy to explore other solutions if you can give me pointers.

Quoted 11 lines

Consider processes P1 and P2, and the following sequence of actions

P1 opens ref DB (ie. opens a set of *.ref files for read) P2 opens ref DB, executes a transaction. Post-transaction, it compacts the reftable stack.
P2 exits
P1 exits

Currently, the compaction in P2 tries to delete the files obviated by the compaction. On Windows this currently fails, because you can't delete open files.

Indeed. So the design needs to be fixed, if it fails.

Quoted 16 lines

There are several options:

1) P2 should fail the compaction. This is bad because it will lead to degraded performance over time, and it's not obvious if you can anticipate that the deletion doesn't work.
2) P2 should retry deleting until it succeeds. This is bad, because a reader can starve writers.
3) on exit, P1 should check if its *.ref files are still in use, and delete them. This smells bad, because P1 is a read-only process, yet it executes writes. Also, do we have good on-exit hooks in Git?
4) On exit, P1 does nothing. Stale *.ref files are left behind. Some sort of GC process cleans things up asynchronously.
5) The ref DB should not keep files open, and should rather open and close files as needed; this means P1 doesn't keep files open for long, and P2 can retry safely.

Quoted 3 lines

I think 3) seems the cleanest to me (even though deleting in read process feels weird), but perhaps we could fallback to 5) on windows as well.

Traditionally, Git would fail gracefully (i.e. with a warning) to delete the stale files, and try again at a later stage (during git gc --auto, for example, or after the next compaction step).

Quoted 1 line

What errno code does deleting an in-use file on Windows produce?

I believe it would be EACCES. See
https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/unlink-wunlink?view=msvc-160
for the documented behavior (I believe that an in-use file is treated the same way as a read-only file in this instance).

Quoted 30 lines · Johannes Schindelin

Quoted 11 lines

Consider processes P1 and P2, and the following sequence of actions

P1 opens ref DB (ie. opens a set of *.ref files for read) P2 opens ref DB, executes a transaction. Post-transaction, it compacts the reftable stack.
P2 exits
P1 exits

Currently, the compaction in P2 tries to delete the files obviated by the compaction. On Windows this currently fails, because you can't delete open files.

Indeed. So the design needs to be fixed, if it fails.

Quoted 16 lines

There are several options:

1) P2 should fail the compaction. This is bad because it will lead to degraded performance over time, and it's not obvious if you can anticipate that the deletion doesn't work.
2) P2 should retry deleting until it succeeds. This is bad, because a reader can starve writers.
3) on exit, P1 should check if its *.ref files are still in use, and delete them. This smells bad, because P1 is a read-only process, yet it executes writes. Also, do we have good on-exit hooks in Git?
4) On exit, P1 does nothing. Stale *.ref files are left behind. Some sort of GC process cleans things up asynchronously.
5) The ref DB should not keep files open, and should rather open and close files as needed; this means P1 doesn't keep files open for long, and P2 can retry safely.

Quoted 7 lines

Quoted 3 lines

I think 3) seems the cleanest to me (even though deleting in read process feels weird), but perhaps we could fallback to 5) on windows as well.

Traditionally, Git would fail gracefully (i.e. with a warning) to delete the stale files, and try again at a later stage (during git gc --auto, for example, or after the next compaction step).

So, how does this sound:

* add a void

set_warn_handler(void(*handler)(char const*))

The handler is called for non-fatal errors (eg. deleting an in-use .ref file during compaction), and can provide the warning.

* all .ref files will be prefixed with an 8-byte random string, to avoid that new *.ref files collide with unreferenced, stale *.ref files.

* in reftable_stack_close(), after closing files, we check if *.ref files we were reading have gone out of use. If so, delete those .ref files, printing a warning on EACCESS.

* an on-exit handler in git calls reftable_stack_close()

* provide a reftable_stack_gc(), which loads tables.list and then deletes all unused *.ref files that are older than tables.list.

Quoted 6 lines

Quoted 1 line

What errno code does deleting an in-use file on Windows produce?

I believe it would be EACCES. See
https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/unlink-wunlink?view=msvc-160
for the documented behavior (I believe that an in-use file is treated the same way as a read-only file in this instance).

your commit message says "to avoid the prompt complaining that a .ref file could not be deleted on Windows." AFAICT, this prompt is coming from windows itself, because the reftable library doesn't issue this type of warning. Is there a way to delete a file that just returns EACCESS if it fails, without triggering a prompt? Or is the prompt part of the "failing gracefully" behavior?

Signature · 2 lines

Han-Wen Nienhuys - Google Munich
I work 80%. Don't expect answers from me on Fridays.

Quoted 30 lines

Quoted 18 lines

Indeed. So the design needs to be fixed, if it fails.

Quoted 16 lines

There are several options:

1) P2 should fail the compaction. This is bad because it will lead to degraded performance over time, and it's not obvious if you can anticipate that the deletion doesn't work.
2) P2 should retry deleting until it succeeds. This is bad, because a reader can starve writers.
3) on exit, P1 should check if its *.ref files are still in use, and delete them. This smells bad, because P1 is a read-only process, yet it executes writes. Also, do we have good on-exit hooks in Git?
4) On exit, P1 does nothing. Stale *.ref files are left behind. Some sort of GC process cleans things up asynchronously.
5) The ref DB should not keep files open, and should rather open and close files as needed; this means P1 doesn't keep files open for long, and P2 can retry safely.

Quoted 7 lines

Quoted 3 lines

I think 3) seems the cleanest to me (even though deleting in read process feels weird), but perhaps we could fallback to 5) on windows as well.

Traditionally, Git would fail gracefully (i.e. with a warning) to delete the stale files, and try again at a later stage (during git gc --auto, for example, or after the next compaction step).

So, how does this sound:

* add a void

Quoted 8 lines

set_warn_handler(void(*handler)(char const*))

The handler is called for non-fatal errors (eg. deleting an in-use .ref file during compaction), and can provide the warning.

* all .ref files will be prefixed with an 8-byte random string, to avoid that new *.ref files collide with unreferenced, stale *.ref files.

Just trying to follow along here. Do you mean prefix the file name or the content of those *.ref files? In any case isn't this synonymous with proposing moving beyond reftable v1 (to the WIP v2, or a v1.1 with just this change?). The current spec seems to preclude prefixing either the file content or filename, but maybe I'm misreading it:

https://googlers.googlesource.com/sop/jgit/+/reftable/Documentation/technical/reftable.md#update-transactions
https://googlers.googlesource.com/sop/jgit/+/reftable/Documentation/technical/reftable.md#header

When the reftable format was initially being discussed I suggested somewhat flippantly (why not SQLite?). Partially because in trying to invent what's essentially a portable and concurrently updated database format we'd be likely to reinvent much of the same wheel.

So not to drag that whole thing up again as a proposal for a format replacement, but I wonder what SQLite would do in this scenario & others where desired DB semantics / transactions and FS/portability semantics clash. AFAIK the reftable has only been battle-tested in production under JGit so far (presumably Linux-only). Whereas SQLite has been ported to and widely used on Windows, HP/UX and probably other systems where Linux-specific assumptions don't apply.