Re: [PATCH 00/20] Read `packed-refs` using mmap()

From: Michael Haggerty <mhagger@alum.mit.edu>
Date: Sep 14, 2017, 16:12 UTC
Message-ID: <ef919cc7-7e41-afb6-38b2-e2fae4efcbf1@alum.mit.edu>
In-Reply-To: <cover.1505319366.git.mhagger@alum.mit.edu>

On 09/13/2017 07:15 PM, Michael Haggerty wrote:

> [...]
> * `mmap()` the whole file rather than `read()`ing it.
> [...]

Apparently this doesn't work on Windows, because the `snapshot` is keeping the `packed-refs` file open too long, so the new file can't be renamed on top of it.

I didn't realize that this is even allowed, but TIL that you can close a file while keeping it mmapped. Does that technique work on Windows? If so, I'll change v2 to do it as sketched below.

Michael

diff --git a/refs/packed-backend.c b/refs/packed-backend.c
index 8235ac8506..95c1cd2a27 100644
--- a/refs/packed-backend.c
+++ b/refs/packed-backend.c
@@ -35,11 +35,8 @@ struct snapshot {
         */
        struct packed_ref_store *refs;

-       /*
-        * The file descriptor of the `packed-refs` file (open in
-        * read-only mode), or -1 if it is not open.
-        */
-       int fd;
+       /* Is the `packed-refs` file currently mmapped? */
+       int mmapped;

        /*
         * The contents of the `packed-refs` file. If the file was
@@ -135,12 +132,11 @@ static void acquire_snapshot(struct snapshot
*snapshot)
  */
 static void clear_snapshot_buffer(struct snapshot *snapshot)
 {
-       if (snapshot->fd >= 0) {
+       if (snapshot->mmapped) {
                if (munmap(snapshot->buf, snapshot->eof - snapshot->buf))
                        die_errno("error ummapping packed-refs file %s",
                                  snapshot->refs->path);
-               close(snapshot->fd);
-               snapshot->fd = -1;
+               snapshot->mmapped = 0;
        } else {
                free(snapshot->buf);
        }
@@ -525,6 +521,7 @@ static const char *find_reference_location(struct
snapshot *snapshot,
 static struct snapshot *create_snapshot(struct packed_ref_store *refs)
 {
        struct snapshot *snapshot = xcalloc(1, sizeof(*snapshot));
+       int fd;
        struct stat st;
        size_t size;
        int sorted = 0;
@@ -533,8 +530,8 @@ static struct snapshot *create_snapshot(struct
packed_ref_store *refs)
        acquire_snapshot(snapshot);
        snapshot->peeled = PEELED_NONE;

-       snapshot->fd = open(refs->path, O_RDONLY);
-       if (snapshot->fd < 0) {
+       fd = open(refs->path, O_RDONLY);
+       if (fd < 0) {
                if (errno == ENOENT) {
                        /*
                         * This is OK; it just means that no
@@ -549,15 +546,16 @@ static struct snapshot *create_snapshot(struct
packed_ref_store *refs)
                }
        }

-       stat_validity_update(&snapshot->validity, snapshot->fd);
+       stat_validity_update(&snapshot->validity, fd);

-       if (fstat(snapshot->fd, &st) < 0)
+       if (fstat(fd, &st) < 0)
                die_errno("couldn't stat %s", refs->path);

        size = xsize_t(st.st_size);
-       snapshot->buf = xmmap(NULL, size, PROT_READ, MAP_PRIVATE,
-                             snapshot->fd, 0);
+       snapshot->buf = xmmap(NULL, size, PROT_READ, MAP_PRIVATE, fd, 0);
        snapshot->eof = snapshot->buf + size;
+       snapshot->mmapped = 1;
+       close(fd);

        /* If the file has a header line, process it: */
        if (snapshot->buf < snapshot->eof && *snapshot->buf == '#') {