From 4a2854f77cc913fafc7f3191a61b343280979643 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Wed, 24 Jan 2018 12:14:11 +0100 Subject: struct snapshot: store `start` rather than `header_len` Store a pointer to the start of the actual references within the `packed-refs` contents rather than storing the length of the header. This is more convenient for most users of this field. Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano --- refs/packed-backend.c | 64 ++++++++++++++++++++++++++------------------------- 1 file changed, 33 insertions(+), 31 deletions(-) (limited to 'refs/packed-backend.c') diff --git a/refs/packed-backend.c b/refs/packed-backend.c index dab8a85d9a..26b890118c 100644 --- a/refs/packed-backend.c +++ b/refs/packed-backend.c @@ -68,17 +68,21 @@ struct snapshot { int mmapped; /* - * The contents of the `packed-refs` file. If the file was - * already sorted, this points at the mmapped contents of the - * file. If not, this points at heap-allocated memory - * containing the contents, sorted. If there were no contents - * (e.g., because the file didn't exist), `buf` and `eof` are - * both NULL. + * The contents of the `packed-refs` file: + * + * - buf -- a pointer to the start of the memory + * - start -- a pointer to the first byte of actual references + * (i.e., after the header line, if one is present) + * - eof -- a pointer just past the end of the reference + * contents + * + * If the `packed-refs` file was already sorted, `buf` points + * at the mmapped contents of the file. If not, it points at + * heap-allocated memory containing the contents, sorted. If + * there were no contents (e.g., because the file didn't + * exist), `buf`, `start`, and `eof` are all NULL. */ - char *buf, *eof; - - /* The size of the header line, if any; otherwise, 0: */ - size_t header_len; + char *buf, *start, *eof; /* * What is the peeled state of the `packed-refs` file that @@ -169,8 +173,7 @@ static void clear_snapshot_buffer(struct snapshot *snapshot) } else { free(snapshot->buf); } - snapshot->buf = snapshot->eof = NULL; - snapshot->header_len = 0; + snapshot->buf = snapshot->start = snapshot->eof = NULL; } /* @@ -319,13 +322,14 @@ static void sort_snapshot(struct snapshot *snapshot) size_t len, i; char *new_buffer, *dst; - pos = snapshot->buf + snapshot->header_len; + pos = snapshot->start; eof = snapshot->eof; - len = eof - pos; - if (!len) + if (pos == eof) return; + len = eof - pos; + /* * Initialize records based on a crude estimate of the number * of references in the file (we'll grow it below if needed): @@ -391,9 +395,8 @@ static void sort_snapshot(struct snapshot *snapshot) * place: */ clear_snapshot_buffer(snapshot); - snapshot->buf = new_buffer; + snapshot->buf = snapshot->start = new_buffer; snapshot->eof = new_buffer + len; - snapshot->header_len = 0; cleanup: free(records); @@ -442,14 +445,14 @@ static const char *find_end_of_record(const char *p, const char *end) */ static void verify_buffer_safe(struct snapshot *snapshot) { - const char *buf = snapshot->buf + snapshot->header_len; + const char *start = snapshot->start; const char *eof = snapshot->eof; const char *last_line; - if (buf == eof) + if (start == eof) return; - last_line = find_start_of_record(buf, eof - 1); + last_line = find_start_of_record(start, eof - 1); if (*(eof - 1) != '\n' || eof - last_line < GIT_SHA1_HEXSZ + 2) die_invalid_line(snapshot->refs->path, last_line, eof - last_line); @@ -495,18 +498,19 @@ static int load_contents(struct snapshot *snapshot) bytes_read = read_in_full(fd, snapshot->buf, size); if (bytes_read < 0 || bytes_read != size) die_errno("couldn't read %s", snapshot->refs->path); - snapshot->eof = snapshot->buf + size; snapshot->mmapped = 0; break; case MMAP_TEMPORARY: case MMAP_OK: snapshot->buf = xmmap(NULL, size, PROT_READ, MAP_PRIVATE, fd, 0); - snapshot->eof = snapshot->buf + size; snapshot->mmapped = 1; break; } close(fd); + snapshot->start = snapshot->buf; + snapshot->eof = snapshot->buf + size; + return 1; } @@ -539,7 +543,7 @@ static const char *find_reference_location(struct snapshot *snapshot, * preceding records all have reference names that come * *before* `refname`. */ - const char *lo = snapshot->buf + snapshot->header_len; + const char *lo = snapshot->start; /* * A pointer to a the first character of a record whose @@ -617,8 +621,7 @@ static struct snapshot *create_snapshot(struct packed_ref_store *refs) /* If the file has a header line, process it: */ if (snapshot->buf < snapshot->eof && *snapshot->buf == '#') { struct strbuf tmp = STRBUF_INIT; - char *p; - const char *eol; + char *p, *eol; struct string_list traits = STRING_LIST_INIT_NODUP; eol = memchr(snapshot->buf, '\n', @@ -647,7 +650,7 @@ static struct snapshot *create_snapshot(struct packed_ref_store *refs) /* perhaps other traits later as well */ /* The "+ 1" is for the LF character. */ - snapshot->header_len = eol + 1 - snapshot->buf; + snapshot->start = eol + 1; string_list_clear(&traits, 0); strbuf_release(&tmp); @@ -671,13 +674,12 @@ static struct snapshot *create_snapshot(struct packed_ref_store *refs) * We don't want to leave the file mmapped, so we are * forced to make a copy now: */ - size_t size = snapshot->eof - - (snapshot->buf + snapshot->header_len); + size_t size = snapshot->eof - snapshot->start; char *buf_copy = xmalloc(size); - memcpy(buf_copy, snapshot->buf + snapshot->header_len, size); + memcpy(buf_copy, snapshot->start, size); clear_snapshot_buffer(snapshot); - snapshot->buf = buf_copy; + snapshot->buf = snapshot->start = buf_copy; snapshot->eof = buf_copy + size; } @@ -937,7 +939,7 @@ static struct ref_iterator *packed_ref_iterator_begin( if (prefix && *prefix) start = find_reference_location(snapshot, prefix, 0); else - start = snapshot->buf + snapshot->header_len; + start = snapshot->start; iter->pos = start; iter->eof = snapshot->eof; -- cgit v1.2.3 From 27a41841ec7f83b3b1078c400f149bc536e796a4 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Wed, 24 Jan 2018 12:14:12 +0100 Subject: create_snapshot(): use `xmemdupz()` rather than a strbuf It's lighter weight. Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano --- refs/packed-backend.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) (limited to 'refs/packed-backend.c') diff --git a/refs/packed-backend.c b/refs/packed-backend.c index 26b890118c..8beb99652f 100644 --- a/refs/packed-backend.c +++ b/refs/packed-backend.c @@ -620,8 +620,7 @@ static struct snapshot *create_snapshot(struct packed_ref_store *refs) /* If the file has a header line, process it: */ if (snapshot->buf < snapshot->eof && *snapshot->buf == '#') { - struct strbuf tmp = STRBUF_INIT; - char *p, *eol; + char *tmp, *p, *eol; struct string_list traits = STRING_LIST_INIT_NODUP; eol = memchr(snapshot->buf, '\n', @@ -631,9 +630,9 @@ static struct snapshot *create_snapshot(struct packed_ref_store *refs) snapshot->buf, snapshot->eof - snapshot->buf); - strbuf_add(&tmp, snapshot->buf, eol - snapshot->buf); + tmp = xmemdupz(snapshot->buf, eol - snapshot->buf); - if (!skip_prefix(tmp.buf, "# pack-refs with:", (const char **)&p)) + if (!skip_prefix(tmp, "# pack-refs with:", (const char **)&p)) die_invalid_line(refs->path, snapshot->buf, snapshot->eof - snapshot->buf); @@ -653,7 +652,7 @@ static struct snapshot *create_snapshot(struct packed_ref_store *refs) snapshot->start = eol + 1; string_list_clear(&traits, 0); - strbuf_release(&tmp); + free(tmp); } verify_buffer_safe(snapshot); -- cgit v1.2.3 From 4a14f8d093138a313070fd6a50204eda66c1f9eb Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Wed, 24 Jan 2018 12:14:13 +0100 Subject: find_reference_location(): make function safe for empty snapshots This function had two problems if called for an empty snapshot (i.e., `snapshot->start == snapshot->eof == NULL`): * It checked `NULL < NULL`, which is undefined by C (albeit highly unlikely to fail in the real world). * (Assuming the above comparison behaved as expected), it returned NULL when `mustexist` was false, contrary to its docstring. Change the check and fix the docstring. Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano --- refs/packed-backend.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) (limited to 'refs/packed-backend.c') diff --git a/refs/packed-backend.c b/refs/packed-backend.c index 8beb99652f..fffface428 100644 --- a/refs/packed-backend.c +++ b/refs/packed-backend.c @@ -519,9 +519,11 @@ static int load_contents(struct snapshot *snapshot) * `refname` starts. If `mustexist` is true and the reference doesn't * exist, then return NULL. If `mustexist` is false and the reference * doesn't exist, then return the point where that reference would be - * inserted. In the latter mode, `refname` doesn't have to be a proper - * reference name; for example, one could search for "refs/replace/" - * to find the start of any replace references. + * inserted, or `snapshot->eof` (which might be NULL) if it would be + * inserted at the end of the file. In the latter mode, `refname` + * doesn't have to be a proper reference name; for example, one could + * search for "refs/replace/" to find the start of any replace + * references. * * The record is sought using a binary search, so `snapshot->buf` must * be sorted. @@ -551,7 +553,7 @@ static const char *find_reference_location(struct snapshot *snapshot, */ const char *hi = snapshot->eof; - while (lo < hi) { + while (lo != hi) { const char *mid, *rec; int cmp; -- cgit v1.2.3 From f34242975fae1468dd94d31289d27f68853a28fb Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Wed, 24 Jan 2018 12:14:14 +0100 Subject: packed_ref_iterator_begin(): make optimization more general We can return an empty iterator not only if the `packed-refs` file is missing, but also if it is empty or if there are no references whose names succeed `prefix`. Optimize away those cases as well by moving the call to `find_reference_location()` higher in the function and checking whether the determined start position is the same as `snapshot->eof`. (This is possible now because the previous commit made `find_reference_location()` robust against empty snapshots.) Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano --- refs/packed-backend.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) (limited to 'refs/packed-backend.c') diff --git a/refs/packed-backend.c b/refs/packed-backend.c index fffface428..c2efe912cc 100644 --- a/refs/packed-backend.c +++ b/refs/packed-backend.c @@ -927,7 +927,12 @@ static struct ref_iterator *packed_ref_iterator_begin( */ snapshot = get_snapshot(refs); - if (!snapshot->buf) + if (prefix && *prefix) + start = find_reference_location(snapshot, prefix, 0); + else + start = snapshot->start; + + if (start == snapshot->eof) return empty_ref_iterator_begin(); iter = xcalloc(1, sizeof(*iter)); @@ -937,11 +942,6 @@ static struct ref_iterator *packed_ref_iterator_begin( iter->snapshot = snapshot; acquire_snapshot(snapshot); - if (prefix && *prefix) - start = find_reference_location(snapshot, prefix, 0); - else - start = snapshot->start; - iter->pos = start; iter->eof = snapshot->eof; strbuf_init(&iter->refname_buf, 0); -- cgit v1.2.3 From 01caf20d57aea73e67337ba1d396dd80a76d9dc3 Mon Sep 17 00:00:00 2001 From: Michael Haggerty Date: Wed, 24 Jan 2018 12:14:15 +0100 Subject: load_contents(): don't try to mmap an empty file We don't actually create zero-length `packed-refs` files, but they are valid and we should handle them correctly. The old code `xmmap()`ed such files, which led to an error when `munmap()` was called. So, if the `packed-refs` file is empty, leave the snapshot at its zero values and return 0 without trying to read or mmap the file. Returning 0 also makes `create_snapshot()` exit early, which avoids the technically undefined comparison `NULL < NULL`. Reported-by: Kim Gybels Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano --- refs/packed-backend.c | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) (limited to 'refs/packed-backend.c') diff --git a/refs/packed-backend.c b/refs/packed-backend.c index c2efe912cc..e30c233970 100644 --- a/refs/packed-backend.c +++ b/refs/packed-backend.c @@ -461,7 +461,8 @@ static void verify_buffer_safe(struct snapshot *snapshot) /* * Depending on `mmap_strategy`, either mmap or read the contents of * the `packed-refs` file into the snapshot. Return 1 if the file - * existed and was read, or 0 if the file was absent. Die on errors. + * existed and was read, or 0 if the file was absent or empty. Die on + * errors. */ static int load_contents(struct snapshot *snapshot) { @@ -492,19 +493,17 @@ static int load_contents(struct snapshot *snapshot) die_errno("couldn't stat %s", snapshot->refs->path); size = xsize_t(st.st_size); - switch (mmap_strategy) { - case MMAP_NONE: + if (!size) { + return 0; + } else if (mmap_strategy == MMAP_NONE) { snapshot->buf = xmalloc(size); bytes_read = read_in_full(fd, snapshot->buf, size); if (bytes_read < 0 || bytes_read != size) die_errno("couldn't read %s", snapshot->refs->path); snapshot->mmapped = 0; - break; - case MMAP_TEMPORARY: - case MMAP_OK: + } else { snapshot->buf = xmmap(NULL, size, PROT_READ, MAP_PRIVATE, fd, 0); snapshot->mmapped = 1; - break; } close(fd); -- cgit v1.2.3 From ba41a8b600871b68680ff33de08012f4887133a2 Mon Sep 17 00:00:00 2001 From: Kim Gybels Date: Wed, 24 Jan 2018 12:14:16 +0100 Subject: packed_ref_cache: don't use mmap() for small files Take a hint from commit ea68b0ce9f8 (hash-object: don't use mmap() for small files, 2010-02-21) and use read() instead of mmap() for small packed-refs files. Signed-off-by: Kim Gybels Signed-off-by: Junio C Hamano Signed-off-by: Michael Haggerty Signed-off-by: Junio C Hamano --- refs/packed-backend.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) (limited to 'refs/packed-backend.c') diff --git a/refs/packed-backend.c b/refs/packed-backend.c index e30c233970..ea7ad554e6 100644 --- a/refs/packed-backend.c +++ b/refs/packed-backend.c @@ -458,6 +458,8 @@ static void verify_buffer_safe(struct snapshot *snapshot) last_line, eof - last_line); } +#define SMALL_FILE_SIZE (32*1024) + /* * Depending on `mmap_strategy`, either mmap or read the contents of * the `packed-refs` file into the snapshot. Return 1 if the file @@ -495,7 +497,7 @@ static int load_contents(struct snapshot *snapshot) if (!size) { return 0; - } else if (mmap_strategy == MMAP_NONE) { + } else if (mmap_strategy == MMAP_NONE || size <= SMALL_FILE_SIZE) { snapshot->buf = xmalloc(size); bytes_read = read_in_full(fd, snapshot->buf, size); if (bytes_read < 0 || bytes_read != size) -- cgit v1.2.3