{"thread":{"id":"46744","subject":"[PATCH 00/20] Read `packed-refs` using mmap()","startedAt":"2017-09-13T17:16:28Z","lastAt":"2017-09-15T04:21:15Z","messageCount":27,"participants":["Michael Haggerty","Junio C Hamano","Stefan Beller","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":20},"messages":[{"id":"327952","messageId":"cover.1505319366.git.mhagger@alum.mit.edu","threadId":"46744","inReplyTo":null,"subject":"[PATCH 00/20] Read `packed-refs` using mmap()","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-09-13T17:15:54Z","receivedAt":"2017-09-13T17:16:28Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Previously, any time we wanted to read even a single reference from\nthe `packed-refs` file, we parsed the whole file and stored it in an\nelaborate structure in memory called a `ref_cache`. Subsequent\nreference lookups or iterations over some or all of the references\ncould be done by reading from the `ref_cache`.\n\nBut for large `packed-refs` files, the time needed to parse the file,\nand the memory needed to cache its contents, can be quite significant.\nThis is partly because building the cache costs lots of little memory\nallocations. (And lest you think that most Git commands can be\nexecuted by reading at most a couple of loose references, remember\nthat almost any command that reads objects has to look for replace\nreferences (unless they are turned off in the config), and *that*\nnecessarily entails reading packed references.)\n\nFollowing lots of work to extract the `packed_ref_store` into a\nseparate module and decouple it from the `files_ref_store`, it is now\npossible to fundamentally change how the `packed-refs` file is read.\n\n* `mmap()` the whole file rather than `read()`ing it.\n\n* Instead of parsing the complete file at once into a `ref_cache`,\n parse the references out of the file contents on demand.\n\n* Use a binary search to find, very quickly, the reference or group of\n references that needs to be read. Parse *only* those references out\n of the file contents, without creating in-memory data structures at\n all.\n\nIn rare cases this change might force parts of the `packed-refs` file\nto be read multiple times, but that cost is far outweighed by the fact\nthat usually most of the `packed-refs` file doesn't have to be read\n*at all*.\n\nNote that the binary search optimization requires the `packed-refs`\nfile to be sorted by reference name. We have always written them\nsorted, but just in case there are clients that don't, we assume the\nfile is unsorted unless its header lists a `sorted` trait. From now\non, we write the file with that trait. If the file is not sorted, it\nis sorted on the fly in memory.\n\nFor a repository with only a couple thousand references and a warm\ndisk cache, this change doesn't make a very significant difference.\nBut for repositories with very large numbers of references, the\ndifference start to be significant:\n\nA repository with 54k references (warm cache):\n\n git 2.13.1 this branch\ngit for-each-ref 464 ms 452 ms\ngit for-each-ref (no output) 66 ms 47 ms\ngit for-each-ref (0.6% of refs) 47 ms 9 ms\ngit for-each-ref (0.6%, no output) 41 ms 2 ms\ngit rev-parse 32 ms 2 ms\n\nA repository (admittedly insane, but a real-life example) with 60M\nreferences (warm cache):\n\n git 2.13.1 this branch\ngit for-each-ref (no output) 84000 ms 61000 ms\ngit rev-parse 40000 ms 2 ms\n\nThis branch applies on top of mh/packed-ref-transactions. It can also\nbe obtained from my git fork [1] as branch `mmap-packed-refs`.\n\nMichael\n\n[1] https://github.com/mhagger/git\n\nJeff King (1):\n prefix_ref_iterator: break when we leave the prefix\n\nMichael Haggerty (19):\n ref_iterator: keep track of whether the iterator output is ordered\n packed_ref_cache: add a backlink to the associated `packed_ref_store`\n die_unterminated_line(), die_invalid_line(): new functions\n read_packed_refs(): use mmap to read the `packed-refs` file\n read_packed_refs(): only check for a header at the top of the file\n read_packed_refs(): make parsing of the header line more robust\n read_packed_refs(): read references with minimal copying\n packed_ref_cache: remember the file-wide peeling state\n mmapped_ref_iterator: add iterator over a packed-refs file\n mmapped_ref_iterator_advance(): no peeled value for broken refs\n packed_ref_cache: keep the `packed-refs` file open and mmapped\n read_packed_refs(): ensure that references are ordered when read\n packed_ref_iterator_begin(): iterate using `mmapped_ref_iterator`\n packed_read_raw_ref(): read the reference from the mmapped buffer\n ref_store: implement `refs_peel_ref()` generically\n packed_ref_store: get rid of the `ref_cache` entirely\n ref_cache: remove support for storing peeled values\n mmapped_ref_iterator: inline into `packed_ref_iterator`\n packed-backend.c: rename a bunch of things and update comments\n\n refs.c | 22 +-\n refs/files-backend.c | 54 +--\n refs/iterator.c | 47 ++-\n refs/packed-backend.c | 896 +++++++++++++++++++++++++++++++++++++-------------\n refs/ref-cache.c | 44 +--\n refs/ref-cache.h | 35 +-\n refs/refs-internal.h | 26 +-\n 7 files changed, 761 insertions(+), 363 deletions(-)\n\n-- \n2.14.1\n\n"},{"id":"327953","messageId":"cbad77c8cf579651c4448b1c6a5a7a703723eb9b.1505319366.git.mhagger@alum.mit.edu","threadId":"46744","inReplyTo":"cover.1505319366.git.mhagger@alum.mit.edu","subject":"[PATCH 01/20] ref_iterator: keep track of whether the iterator output is ordered","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-09-13T17:15:55Z","receivedAt":"2017-09-13T17:16:32Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"References are iterated over in order by refname, but reflogs are not.\nSome consumers of reference iteration care about the difference. Teach\neach `ref_iterator` to keep track of whether its output is ordered.\n\n`overlay_ref_iterator` is one of the picky consumers. Add a sanity\ncheck in `overlay_ref_iterator_begin()` to verify that its inputs are\nordered.\n\nSigned-off-by: Michael Haggerty \n---\n refs.c | 4 ++++\n refs/files-backend.c | 16 +++++++++-------\n refs/iterator.c | 15 ++++++++++-----\n refs/packed-backend.c | 2 +-\n refs/ref-cache.c | 2 +-\n refs/ref-cache.h | 3 ++-\n refs/refs-internal.h | 23 +++++++++++++++++++----\n 7 files changed, 46 insertions(+), 19 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex b0106b8162..101c107ee8 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1309,6 +1309,10 @@ struct ref_iterator *refs_ref_iterator_begin(\n \tif (trim)\n \t\titer = prefix_ref_iterator_begin(iter, \"\", trim);\n \n+\t/* Sanity check for subclasses: */\n+\tif (!iter->ordered)\n+\t\tBUG(\"reference iterator is not ordered\");\n+\n \treturn iter;\n }\n \ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 961424a4ea..35648c89fc 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -762,7 +762,7 @@ static struct ref_iterator *files_ref_iterator_begin(\n \t\tconst char *prefix, unsigned int flags)\n {\n \tstruct files_ref_store *refs;\n-\tstruct ref_iterator *loose_iter, *packed_iter;\n+\tstruct ref_iterator *loose_iter, *packed_iter, *overlay_iter;\n \tstruct files_ref_iterator *iter;\n \tstruct ref_iterator *ref_iterator;\n \tunsigned int required_flags = REF_STORE_READ;\n@@ -772,10 +772,6 @@ static struct ref_iterator *files_ref_iterator_begin(\n \n \trefs = files_downcast(ref_store, required_flags, \"ref_iterator_begin\");\n \n-\titer = xcalloc(1, sizeof(*iter));\n-\tref_iterator = &iter->base;\n-\tbase_ref_iterator_init(ref_iterator, &files_ref_iterator_vtable);\n-\n \t/*\n \t * We must make sure that all loose refs are read before\n \t * accessing the packed-refs file; this avoids a race\n@@ -811,7 +807,13 @@ static struct ref_iterator *files_ref_iterator_begin(\n \t\t\trefs->packed_ref_store, prefix, 0,\n \t\t\tDO_FOR_EACH_INCLUDE_BROKEN);\n \n-\titer->iter0 = overlay_ref_iterator_begin(loose_iter, packed_iter);\n+\toverlay_iter = overlay_ref_iterator_begin(loose_iter, packed_iter);\n+\n+\titer = xcalloc(1, sizeof(*iter));\n+\tref_iterator = &iter->base;\n+\tbase_ref_iterator_init(ref_iterator, &files_ref_iterator_vtable,\n+\t\t\t overlay_iter->ordered);\n+\titer->iter0 = overlay_iter;\n \titer->flags = flags;\n \n \treturn ref_iterator;\n@@ -2084,7 +2086,7 @@ static struct ref_iterator *files_reflog_iterator_begin(struct ref_store *ref_st\n \tstruct ref_iterator *ref_iterator = &iter->base;\n \tstruct strbuf sb = STRBUF_INIT;\n \n-\tbase_ref_iterator_init(ref_iterator, &files_reflog_iterator_vtable);\n+\tbase_ref_iterator_init(ref_iterator, &files_reflog_iterator_vtable, 0);\n \tfiles_reflog_path(refs, &sb, NULL);\n \titer->dir_iterator = dir_iterator_begin(sb.buf);\n \titer->ref_store = ref_store;\ndiff --git a/refs/iterator.c b/refs/iterator.c\nindex 4cf449ef66..c475360f0a 100644\n--- a/refs/iterator.c\n+++ b/refs/iterator.c\n@@ -25,9 +25,11 @@ int ref_iterator_abort(struct ref_iterator *ref_iterator)\n }\n \n void base_ref_iterator_init(struct ref_iterator *iter,\n-\t\t\t struct ref_iterator_vtable *vtable)\n+\t\t\t struct ref_iterator_vtable *vtable,\n+\t\t\t int ordered)\n {\n \titer->vtable = vtable;\n+\titer->ordered = !!ordered;\n \titer->refname = NULL;\n \titer->oid = NULL;\n \titer->flags = 0;\n@@ -72,7 +74,7 @@ struct ref_iterator *empty_ref_iterator_begin(void)\n \tstruct empty_ref_iterator *iter = xcalloc(1, sizeof(*iter));\n \tstruct ref_iterator *ref_iterator = &iter->base;\n \n-\tbase_ref_iterator_init(ref_iterator, &empty_ref_iterator_vtable);\n+\tbase_ref_iterator_init(ref_iterator, &empty_ref_iterator_vtable, 1);\n \treturn ref_iterator;\n }\n \n@@ -205,6 +207,7 @@ static struct ref_iterator_vtable merge_ref_iterator_vtable = {\n };\n \n struct ref_iterator *merge_ref_iterator_begin(\n+\t\tint ordered,\n \t\tstruct ref_iterator *iter0, struct ref_iterator *iter1,\n \t\tref_iterator_select_fn *select, void *cb_data)\n {\n@@ -219,7 +222,7 @@ struct ref_iterator *merge_ref_iterator_begin(\n \t * references through only if they exist in both iterators.\n \t */\n \n-\tbase_ref_iterator_init(ref_iterator, &merge_ref_iterator_vtable);\n+\tbase_ref_iterator_init(ref_iterator, &merge_ref_iterator_vtable, ordered);\n \titer->iter0 = iter0;\n \titer->iter1 = iter1;\n \titer->select = select;\n@@ -268,9 +271,11 @@ struct ref_iterator *overlay_ref_iterator_begin(\n \t} else if (is_empty_ref_iterator(back)) {\n \t\tref_iterator_abort(back);\n \t\treturn front;\n+\t} else if (!front->ordered || !back->ordered) {\n+\t\tBUG(\"overlay_ref_iterator requires ordered inputs\");\n \t}\n \n-\treturn merge_ref_iterator_begin(front, back,\n+\treturn merge_ref_iterator_begin(1, front, back,\n \t\t\t\t\toverlay_iterator_select, NULL);\n }\n \n@@ -361,7 +366,7 @@ struct ref_iterator *prefix_ref_iterator_begin(struct ref_iterator *iter0,\n \titer = xcalloc(1, sizeof(*iter));\n \tref_iterator = &iter->base;\n \n-\tbase_ref_iterator_init(ref_iterator, &prefix_ref_iterator_vtable);\n+\tbase_ref_iterator_init(ref_iterator, &prefix_ref_iterator_vtable, iter0->ordered);\n \n \titer->iter0 = iter0;\n \titer->prefix = xstrdup(prefix);\ndiff --git a/refs/packed-backend.c b/refs/packed-backend.c\nindex 0279aeceea..e411501871 100644\n--- a/refs/packed-backend.c\n+++ b/refs/packed-backend.c\n@@ -437,7 +437,7 @@ static struct ref_iterator *packed_ref_iterator_begin(\n \n \titer = xcalloc(1, sizeof(*iter));\n \tref_iterator = &iter->base;\n-\tbase_ref_iterator_init(ref_iterator, &packed_ref_iterator_vtable);\n+\tbase_ref_iterator_init(ref_iterator, &packed_ref_iterator_vtable, 1);\n \n \t/*\n \t * Note that get_packed_ref_cache() internally checks whether\ndiff --git a/refs/ref-cache.c b/refs/ref-cache.c\nindex 76bb723c86..54dfb5218c 100644\n--- a/refs/ref-cache.c\n+++ b/refs/ref-cache.c\n@@ -574,7 +574,7 @@ struct ref_iterator *cache_ref_iterator_begin(struct ref_cache *cache,\n \n \titer = xcalloc(1, sizeof(*iter));\n \tref_iterator = &iter->base;\n-\tbase_ref_iterator_init(ref_iterator, &cache_ref_iterator_vtable);\n+\tbase_ref_iterator_init(ref_iterator, &cache_ref_iterator_vtable, 1);\n \tALLOC_GROW(iter->levels, 10, iter->levels_alloc);\n \n \titer->levels_nr = 1;\ndiff --git a/refs/ref-cache.h b/refs/ref-cache.h\nindex 794f000fd3..a082bfb06c 100644\n--- a/refs/ref-cache.h\n+++ b/refs/ref-cache.h\n@@ -245,7 +245,8 @@ struct ref_entry *find_ref_entry(struct ref_dir *dir, const char *refname);\n * Start iterating over references in `cache`. If `prefix` is\n * specified, only include references whose names start with that\n * prefix. If `prime_dir` is true, then fill any incomplete\n- * directories before beginning the iteration.\n+ * directories before beginning the iteration. The output is ordered\n+ * by refname.\n */\n struct ref_iterator *cache_ref_iterator_begin(struct ref_cache *cache,\n \t\t\t\t\t const char *prefix,\ndiff --git a/refs/refs-internal.h b/refs/refs-internal.h\nindex d7d344de73..d7f233beba 100644\n--- a/refs/refs-internal.h\n+++ b/refs/refs-internal.h\n@@ -329,6 +329,13 @@ int refs_rename_ref_available(struct ref_store *refs,\n */\n struct ref_iterator {\n \tstruct ref_iterator_vtable *vtable;\n+\n+\t/*\n+\t * Does this `ref_iterator` iterate over references in order\n+\t * by refname?\n+\t */\n+\tunsigned int ordered : 1;\n+\n \tconst char *refname;\n \tconst struct object_id *oid;\n \tunsigned int flags;\n@@ -374,7 +381,7 @@ int is_empty_ref_iterator(struct ref_iterator *ref_iterator);\n * which the refname begins with prefix. If trim is non-zero, then\n * trim that many characters off the beginning of each refname. flags\n * can be DO_FOR_EACH_INCLUDE_BROKEN to include broken references in\n- * the iteration.\n+ * the iteration. The output is ordered by refname.\n */\n struct ref_iterator *refs_ref_iterator_begin(\n \t\tstruct ref_store *refs,\n@@ -400,9 +407,11 @@ typedef enum iterator_selection ref_iterator_select_fn(\n * Iterate over the entries from iter0 and iter1, with the values\n * interleaved as directed by the select function. The iterator takes\n * ownership of iter0 and iter1 and frees them when the iteration is\n- * over.\n+ * over. A derived class should set `ordered` to 1 or 0 based on\n+ * whether it generates its output in order by reference name.\n */\n struct ref_iterator *merge_ref_iterator_begin(\n+\t\tint ordered,\n \t\tstruct ref_iterator *iter0, struct ref_iterator *iter1,\n \t\tref_iterator_select_fn *select, void *cb_data);\n \n@@ -431,6 +440,8 @@ struct ref_iterator *overlay_ref_iterator_begin(\n * As an convenience to callers, if prefix is the empty string and\n * trim is zero, this function returns iter0 directly, without\n * wrapping it.\n+ *\n+ * The resulting ref_iterator is ordered if iter0 is.\n */\n struct ref_iterator *prefix_ref_iterator_begin(struct ref_iterator *iter0,\n \t\t\t\t\t const char *prefix,\n@@ -441,11 +452,14 @@ struct ref_iterator *prefix_ref_iterator_begin(struct ref_iterator *iter0,\n /*\n * Base class constructor for ref_iterators. Initialize the\n * ref_iterator part of iter, setting its vtable pointer as specified.\n+ * `ordered` should be set to 1 if the iterator will iterate over\n+ * references in order by refname; otherwise it should be set to 0.\n * This is meant to be called only by the initializers of derived\n * classes.\n */\n void base_ref_iterator_init(struct ref_iterator *iter,\n-\t\t\t struct ref_iterator_vtable *vtable);\n+\t\t\t struct ref_iterator_vtable *vtable,\n+\t\t\t int ordered);\n \n /*\n * Base class destructor for ref_iterators. Destroy the ref_iterator\n@@ -564,7 +578,8 @@ typedef int rename_ref_fn(struct ref_store *ref_store,\n * Iterate over the references in `ref_store` whose names start with\n * `prefix`. `prefix` is matched as a literal string, without regard\n * for path separators. If prefix is NULL or the empty string, iterate\n- * over all references in `ref_store`.\n+ * over all references in `ref_store`. The output is ordered by\n+ * refname.\n */\n typedef struct ref_iterator *ref_iterator_begin_fn(\n \t\tstruct ref_store *ref_store,\n-- \n2.14.1\n\n"},{"id":"327954","messageId":"2bb2e8ccb57eef8acbea5004167751a007a1bd2f.1505319366.git.mhagger@alum.mit.edu","threadId":"46744","inReplyTo":"cover.1505319366.git.mhagger@alum.mit.edu","subject":"[PATCH 02/20] prefix_ref_iterator: break when we leave the prefix","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-09-13T17:15:56Z","receivedAt":"2017-09-13T17:16:35Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"From: Jeff King \n\nIf the underlying iterator is ordered, then `prefix_ref_iterator` can\nstop as soon as it sees a refname that comes after the prefix. This\nwill rarely make a big difference now, because `ref_cache_iterator`\nonly iterates over the directory containing the prefix (and usually\nthe prefix will span a whole directory anyway). But if *hint, hint* a\nfuture reference backend doesn't itself know where to stop the\niteration, then this optimization will be a big win.\n\nNote that there is no guarantee that the underlying iterator doesn't\ninclude output preceding the prefix, so we have to skip over any\nunwanted references before we get to the ones that we want.\n\nSigned-off-by: Jeff King \nSigned-off-by: Michael Haggerty \n---\nNote that the implementation of `compare_prefix()` here is a bit\ndifferent than Peff's original version.\n\n refs/iterator.c | 32 +++++++++++++++++++++++++++++++-\n 1 file changed, 31 insertions(+), 1 deletion(-)\n\ndiff --git a/refs/iterator.c b/refs/iterator.c\nindex c475360f0a..bd35da4e62 100644\n--- a/refs/iterator.c\n+++ b/refs/iterator.c\n@@ -287,6 +287,20 @@ struct prefix_ref_iterator {\n \tint trim;\n };\n \n+/* Return -1, 0, 1 if refname is before, inside, or after the prefix. */\n+static int compare_prefix(const char *refname, const char *prefix)\n+{\n+\twhile (*prefix) {\n+\t\tif (*refname != *prefix)\n+\t\t\treturn ((unsigned char)*refname < (unsigned char)*prefix) ? -1 : +1;\n+\n+\t\trefname++;\n+\t\tprefix++;\n+\t}\n+\n+\treturn 0;\n+}\n+\n static int prefix_ref_iterator_advance(struct ref_iterator *ref_iterator)\n {\n \tstruct prefix_ref_iterator *iter =\n@@ -294,9 +308,25 @@ static int prefix_ref_iterator_advance(struct ref_iterator *ref_iterator)\n \tint ok;\n \n \twhile ((ok = ref_iterator_advance(iter->iter0)) == ITER_OK) {\n-\t\tif (!starts_with(iter->iter0->refname, iter->prefix))\n+\t\tint cmp = compare_prefix(iter->iter0->refname, iter->prefix);\n+\n+\t\tif (cmp < 0)\n \t\t\tcontinue;\n \n+\t\tif (cmp > 0) {\n+\t\t\t/*\n+\t\t\t * If the source iterator is ordered, then we\n+\t\t\t * can stop the iteration as soon as we see a\n+\t\t\t * refname that comes after the prefix:\n+\t\t\t */\n+\t\t\tif (iter->iter0->ordered) {\n+\t\t\t\tok = ref_iterator_abort(iter->iter0);\n+\t\t\t\tbreak;\n+\t\t\t} else {\n+\t\t\t\tcontinue;\n+\t\t\t}\n+\t\t}\n+\n \t\tif (iter->trim) {\n \t\t\t/*\n \t\t\t * It is nonsense to trim off characters that\n-- \n2.14.1\n\n"},{"id":"327955","messageId":"3f231e673b9aa67347dd71c3d1c0eb813fe3a761.1505319366.git.mhagger@alum.mit.edu","threadId":"46744","inReplyTo":"cover.1505319366.git.mhagger@alum.mit.edu","subject":"[PATCH 03/20] packed_ref_cache: add a backlink to the associated `packed_ref_store`","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-09-13T17:15:57Z","receivedAt":"2017-09-13T17:16:37Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"It will prove convenient in upcoming patches.\n\nSigned-off-by: Michael Haggerty \n---\n refs/packed-backend.c | 23 ++++++++++++++++-------\n 1 file changed, 16 insertions(+), 7 deletions(-)\n\ndiff --git a/refs/packed-backend.c b/refs/packed-backend.c\nindex e411501871..a3d9210cb0 100644\n--- a/refs/packed-backend.c\n+++ b/refs/packed-backend.c\n@@ -7,7 +7,15 @@\n #include \"../iterator.h\"\n #include \"../lockfile.h\"\n \n+struct packed_ref_store;\n+\n struct packed_ref_cache {\n+\t/*\n+\t * A back-pointer to the packed_ref_store with which this\n+\t * cache is associated:\n+\t */\n+\tstruct packed_ref_store *refs;\n+\n \tstruct ref_cache *cache;\n \n \t/*\n@@ -154,7 +162,7 @@ static const char *parse_ref_line(struct strbuf *line, struct object_id *oid)\n }\n \n /*\n- * Read from `packed_refs_file` into a newly-allocated\n+ * Read from the `packed-refs` file into a newly-allocated\n * `packed_ref_cache` and return it. The return value will already\n * have its reference count incremented.\n *\n@@ -182,7 +190,7 @@ static const char *parse_ref_line(struct strbuf *line, struct object_id *oid)\n * compatibility with older clients, but we do not require it\n * (i.e., \"peeled\" is a no-op if \"fully-peeled\" is set).\n */\n-static struct packed_ref_cache *read_packed_refs(const char *packed_refs_file)\n+static struct packed_ref_cache *read_packed_refs(struct packed_ref_store *refs)\n {\n \tFILE *f;\n \tstruct packed_ref_cache *packed_refs = xcalloc(1, sizeof(*packed_refs));\n@@ -191,11 +199,12 @@ static struct packed_ref_cache *read_packed_refs(const char *packed_refs_file)\n \tenum { PEELED_NONE, PEELED_TAGS, PEELED_FULLY } peeled = PEELED_NONE;\n \tstruct ref_dir *dir;\n \n+\tpacked_refs->refs = refs;\n \tacquire_packed_ref_cache(packed_refs);\n \tpacked_refs->cache = create_ref_cache(NULL, NULL);\n \tpacked_refs->cache->root->flag &= ~REF_INCOMPLETE;\n \n-\tf = fopen(packed_refs_file, \"r\");\n+\tf = fopen(refs->path, \"r\");\n \tif (!f) {\n \t\tif (errno == ENOENT) {\n \t\t\t/*\n@@ -205,7 +214,7 @@ static struct packed_ref_cache *read_packed_refs(const char *packed_refs_file)\n \t\t\t */\n \t\t\treturn packed_refs;\n \t\t} else {\n-\t\t\tdie_errno(\"couldn't read %s\", packed_refs_file);\n+\t\t\tdie_errno(\"couldn't read %s\", refs->path);\n \t\t}\n \t}\n \n@@ -218,7 +227,7 @@ static struct packed_ref_cache *read_packed_refs(const char *packed_refs_file)\n \t\tconst char *traits;\n \n \t\tif (!line.len || line.buf[line.len - 1] != '\\n')\n-\t\t\tdie(\"unterminated line in %s: %s\", packed_refs_file, line.buf);\n+\t\t\tdie(\"unterminated line in %s: %s\", refs->path, line.buf);\n \n \t\tif (skip_prefix(line.buf, \"# pack-refs with:\", &traits)) {\n \t\t\tif (strstr(traits, \" fully-peeled \"))\n@@ -258,7 +267,7 @@ static struct packed_ref_cache *read_packed_refs(const char *packed_refs_file)\n \t\t\tlast->flag |= REF_KNOWS_PEELED;\n \t\t} else {\n \t\t\tstrbuf_setlen(&line, line.len - 1);\n-\t\t\tdie(\"unexpected line in %s: %s\", packed_refs_file, line.buf);\n+\t\t\tdie(\"unexpected line in %s: %s\", refs->path, line.buf);\n \t\t}\n \t}\n \n@@ -293,7 +302,7 @@ static struct packed_ref_cache *get_packed_ref_cache(struct packed_ref_store *re\n \t\tvalidate_packed_ref_cache(refs);\n \n \tif (!refs->cache)\n-\t\trefs->cache = read_packed_refs(refs->path);\n+\t\trefs->cache = read_packed_refs(refs);\n \n \treturn refs->cache;\n }\n-- \n2.14.1\n\n"},{"id":"327956","messageId":"67c4bf861beb095a63e8db72f1d37d6bfac39cd0.1505319366.git.mhagger@alum.mit.edu","threadId":"46744","inReplyTo":"cover.1505319366.git.mhagger@alum.mit.edu","subject":"[PATCH 05/20] read_packed_refs(): use mmap to read the `packed-refs` file","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-09-13T17:15:59Z","receivedAt":"2017-09-13T17:16:41Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"It's still done in a pretty stupid way, involving more data copying\nthan necessary. That will improve in future commits.\n\nSigned-off-by: Michael Haggerty \n---\n refs/packed-backend.c | 42 ++++++++++++++++++++++++++++++++----------\n 1 file changed, 32 insertions(+), 10 deletions(-)\n\ndiff --git a/refs/packed-backend.c b/refs/packed-backend.c\nindex 5c50c223ef..154abbd83a 100644\n--- a/refs/packed-backend.c\n+++ b/refs/packed-backend.c\n@@ -215,8 +215,12 @@ static NORETURN void die_invalid_line(const char *path,\n */\n static struct packed_ref_cache *read_packed_refs(struct packed_ref_store *refs)\n {\n-\tFILE *f;\n \tstruct packed_ref_cache *packed_refs = xcalloc(1, sizeof(*packed_refs));\n+\tint fd;\n+\tstruct stat st;\n+\tsize_t size;\n+\tchar *buf;\n+\tconst char *pos, *eol, *eof;\n \tstruct ref_entry *last = NULL;\n \tstruct strbuf line = STRBUF_INIT;\n \tenum { PEELED_NONE, PEELED_TAGS, PEELED_FULLY } peeled = PEELED_NONE;\n@@ -227,8 +231,8 @@ static struct packed_ref_cache *read_packed_refs(struct packed_ref_store *refs)\n \tpacked_refs->cache = create_ref_cache(NULL, NULL);\n \tpacked_refs->cache->root->flag &= ~REF_INCOMPLETE;\n \n-\tf = fopen(refs->path, \"r\");\n-\tif (!f) {\n+\tfd = open(refs->path, O_RDONLY);\n+\tif (fd < 0) {\n \t\tif (errno == ENOENT) {\n \t\t\t/*\n \t\t\t * This is OK; it just means that no\n@@ -241,16 +245,27 @@ static struct packed_ref_cache *read_packed_refs(struct packed_ref_store *refs)\n \t\t}\n \t}\n \n-\tstat_validity_update(&packed_refs->validity, fileno(f));\n+\tstat_validity_update(&packed_refs->validity, fd);\n+\n+\tif (fstat(fd, &st) < 0)\n+\t\tdie_errno(\"couldn't stat %s\", refs->path);\n+\n+\tsize = xsize_t(st.st_size);\n+\tbuf = xmmap(NULL, size, PROT_READ, MAP_PRIVATE, fd, 0);\n+\tpos = buf;\n+\teof = buf + size;\n \n \tdir = get_ref_dir(packed_refs->cache->root);\n-\twhile (strbuf_getwholeline(&line, f, '\\n') != EOF) {\n+\twhile (pos < eof) {\n \t\tstruct object_id oid;\n \t\tconst char *refname;\n \t\tconst char *traits;\n \n-\t\tif (!line.len || line.buf[line.len - 1] != '\\n')\n-\t\t\tdie_unterminated_line(refs->path, line.buf, line.len);\n+\t\teol = memchr(pos, '\\n', eof - pos);\n+\t\tif (!eol)\n+\t\t\tdie_unterminated_line(refs->path, pos, eof - pos);\n+\n+\t\tstrbuf_add(&line, pos, eol + 1 - pos);\n \n \t\tif (skip_prefix(line.buf, \"# pack-refs with:\", &traits)) {\n \t\t\tif (strstr(traits, \" fully-peeled \"))\n@@ -258,7 +273,7 @@ static struct packed_ref_cache *read_packed_refs(struct packed_ref_store *refs)\n \t\t\telse if (strstr(traits, \" peeled \"))\n \t\t\t\tpeeled = PEELED_TAGS;\n \t\t\t/* perhaps other traits later as well */\n-\t\t\tcontinue;\n+\t\t\tgoto next_line;\n \t\t}\n \n \t\trefname = parse_ref_line(&line, &oid);\n@@ -291,11 +306,18 @@ static struct packed_ref_cache *read_packed_refs(struct packed_ref_store *refs)\n \t\t} else {\n \t\t\tdie_invalid_line(refs->path, line.buf, line.len);\n \t\t}\n+\n+\tnext_line:\n+\t\t/* The \"+ 1\" is for the LF character. */\n+\t\tpos = eol + 1;\n+\t\tstrbuf_reset(&line);\n \t}\n \n-\tfclose(f);\n-\tstrbuf_release(&line);\n+\tif (munmap(buf, size))\n+\t\tdie_errno(\"error ummapping packed-refs file\");\n+\tclose(fd);\n \n+\tstrbuf_release(&line);\n \treturn packed_refs;\n }\n \n-- \n2.14.1\n\n"},{"id":"327958","messageId":"a1d50f3a452873f3d845742d378887aac14f7cb6.1505319366.git.mhagger@alum.mit.edu","threadId":"46744","inReplyTo":"cover.1505319366.git.mhagger@alum.mit.edu","subject":"[PATCH 06/20] read_packed_refs(): only check for a header at the top of the file","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-09-13T17:16:00Z","receivedAt":"2017-09-13T17:16:46Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"This tightens up the parsing a bit; previously, stray header-looking\nlines would have been processed.\n\nSigned-off-by: Michael Haggerty \n---\n refs/packed-backend.c | 35 ++++++++++++++++++++++++-----------\n 1 file changed, 24 insertions(+), 11 deletions(-)\n\ndiff --git a/refs/packed-backend.c b/refs/packed-backend.c\nindex 154abbd83a..141f02b9c8 100644\n--- a/refs/packed-backend.c\n+++ b/refs/packed-backend.c\n@@ -255,11 +255,34 @@ static struct packed_ref_cache *read_packed_refs(struct packed_ref_store *refs)\n \tpos = buf;\n \teof = buf + size;\n \n+\t/* If the file has a header line, process it: */\n+\tif (pos < eof && *pos == '#') {\n+\t\tconst char *traits;\n+\n+\t\teol = memchr(pos, '\\n', eof - pos);\n+\t\tif (!eol)\n+\t\t\tdie_unterminated_line(refs->path, pos, eof - pos);\n+\n+\t\tstrbuf_add(&line, pos, eol + 1 - pos);\n+\n+\t\tif (!skip_prefix(line.buf, \"# pack-refs with:\", &traits))\n+\t\t\tdie_invalid_line(refs->path, pos, eof - pos);\n+\n+\t\tif (strstr(traits, \" fully-peeled \"))\n+\t\t\tpeeled = PEELED_FULLY;\n+\t\telse if (strstr(traits, \" peeled \"))\n+\t\t\tpeeled = PEELED_TAGS;\n+\t\t/* perhaps other traits later as well */\n+\n+\t\t/* The \"+ 1\" is for the LF character. */\n+\t\tpos = eol + 1;\n+\t\tstrbuf_reset(&line);\n+\t}\n+\n \tdir = get_ref_dir(packed_refs->cache->root);\n \twhile (pos < eof) {\n \t\tstruct object_id oid;\n \t\tconst char *refname;\n-\t\tconst char *traits;\n \n \t\teol = memchr(pos, '\\n', eof - pos);\n \t\tif (!eol)\n@@ -267,15 +290,6 @@ static struct packed_ref_cache *read_packed_refs(struct packed_ref_store *refs)\n \n \t\tstrbuf_add(&line, pos, eol + 1 - pos);\n \n-\t\tif (skip_prefix(line.buf, \"# pack-refs with:\", &traits)) {\n-\t\t\tif (strstr(traits, \" fully-peeled \"))\n-\t\t\t\tpeeled = PEELED_FULLY;\n-\t\t\telse if (strstr(traits, \" peeled \"))\n-\t\t\t\tpeeled = PEELED_TAGS;\n-\t\t\t/* perhaps other traits later as well */\n-\t\t\tgoto next_line;\n-\t\t}\n-\n \t\trefname = parse_ref_line(&line, &oid);\n \t\tif (refname) {\n \t\t\tint flag = REF_ISPACKED;\n@@ -307,7 +321,6 @@ static struct packed_ref_cache *read_packed_refs(struct packed_ref_store *refs)\n \t\t\tdie_invalid_line(refs->path, line.buf, line.len);\n \t\t}\n \n-\tnext_line:\n \t\t/* The \"+ 1\" is for the LF character. */\n \t\tpos = eol + 1;\n \t\tstrbuf_reset(&line);\n-- \n2.14.1\n\n"},{"id":"327959","messageId":"0cc2a9af0eeeb2ffbf1942d19fbcceb0c0cf6d5c.1505319366.git.mhagger@alum.mit.edu","threadId":"46744","inReplyTo":"cover.1505319366.git.mhagger@alum.mit.edu","subject":"[PATCH 07/20] read_packed_refs(): make parsing of the header line more robust","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-09-13T17:16:01Z","receivedAt":"2017-09-13T17:16:48Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"The old code parsed the traits in the `packed-refs` header by looking\nfor the string \" trait \" (i.e., the name of the trait with a space on\neither side) in the header line. This is fragile, because if any other\nimplementation of Git forgets to write the trailing space, the last\ntrait would silently be ignored (and the error might never be\nnoticed).\n\nSo instead, use `string_list_split_in_place()` to split the traits\ninto tokens then use `unsorted_string_list_has_string()` to look for\nthe tokens we are interested in. This means that we can read the\ntraits correctly even if the header line is missing a trailing\nspace (or indeed, if it is missing the space after the colon, or if it\nhas multiple spaces somewhere).\n\nHowever, older Git clients (and perhaps other Git implementations)\nstill require the surrounding spaces, so we still have to output the\nheader with a trailing space.\n\nSigned-off-by: Michael Haggerty \n---\n refs/packed-backend.c | 21 +++++++++++++++------\n 1 file changed, 15 insertions(+), 6 deletions(-)\n\ndiff --git a/refs/packed-backend.c b/refs/packed-backend.c\nindex 141f02b9c8..a45e3ff92f 100644\n--- a/refs/packed-backend.c\n+++ b/refs/packed-backend.c\n@@ -257,25 +257,30 @@ static struct packed_ref_cache *read_packed_refs(struct packed_ref_store *refs)\n \n \t/* If the file has a header line, process it: */\n \tif (pos < eof && *pos == '#') {\n-\t\tconst char *traits;\n+\t\tchar *p;\n+\t\tstruct string_list traits = STRING_LIST_INIT_NODUP;\n \n \t\teol = memchr(pos, '\\n', eof - pos);\n \t\tif (!eol)\n \t\t\tdie_unterminated_line(refs->path, pos, eof - pos);\n \n-\t\tstrbuf_add(&line, pos, eol + 1 - pos);\n+\t\tstrbuf_add(&line, pos, eol - pos);\n \n-\t\tif (!skip_prefix(line.buf, \"# pack-refs with:\", &traits))\n+\t\tif (!skip_prefix(line.buf, \"# pack-refs with:\", (const char **)&p))\n \t\t\tdie_invalid_line(refs->path, pos, eof - pos);\n \n-\t\tif (strstr(traits, \" fully-peeled \"))\n+\t\tstring_list_split_in_place(&traits, p, ' ', -1);\n+\n+\t\tif (unsorted_string_list_has_string(&traits, \"fully-peeled\"))\n \t\t\tpeeled = PEELED_FULLY;\n-\t\telse if (strstr(traits, \" peeled \"))\n+\t\telse if (unsorted_string_list_has_string(&traits, \"peeled\"))\n \t\t\tpeeled = PEELED_TAGS;\n \t\t/* perhaps other traits later as well */\n \n \t\t/* The \"+ 1\" is for the LF character. */\n \t\tpos = eol + 1;\n+\n+\t\tstring_list_clear(&traits, 0);\n \t\tstrbuf_reset(&line);\n \t}\n \n@@ -610,7 +615,11 @@ int packed_refs_is_locked(struct ref_store *ref_store)\n \n /*\n * The packed-refs header line that we write out. Perhaps other\n- * traits will be added later. The trailing space is required.\n+ * traits will be added later.\n+ *\n+ * Note that earlier versions of Git used to parse these traits by\n+ * looking for \" trait \" in the line. For this reason, the space after\n+ * the colon and the trailing space are required.\n */\n static const char PACKED_REFS_HEADER[] =\n \t\"# pack-refs with: peeled fully-peeled \\n\";\n-- \n2.14.1\n\n"},{"id":"327960","messageId":"6b7c9b6bd4160635cc5bebf5c0d6e298f05d4af4.1505319366.git.mhagger@alum.mit.edu","threadId":"46744","inReplyTo":"cover.1505319366.git.mhagger@alum.mit.edu","subject":"[PATCH 09/20] packed_ref_cache: remember the file-wide peeling state","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-09-13T17:16:03Z","receivedAt":"2017-09-13T17:16:52Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Rather than store the peeling state (i.e., the one defined by traits\nin the `packed-refs` file header line) in a local variable in\n`read_packed_refs()`, store it permanently in `packed_ref_cache`. This\nwill be needed when we stop reading all packed refs at once.\n\nSigned-off-by: Michael Haggerty \n---\n refs/packed-backend.c | 17 ++++++++++++-----\n 1 file changed, 12 insertions(+), 5 deletions(-)\n\ndiff --git a/refs/packed-backend.c b/refs/packed-backend.c\nindex 2b80f244c8..ae276f3445 100644\n--- a/refs/packed-backend.c\n+++ b/refs/packed-backend.c\n@@ -18,6 +18,12 @@ struct packed_ref_cache {\n \n \tstruct ref_cache *cache;\n \n+\t/*\n+\t * What is the peeled state of this cache? (This is usually\n+\t * determined from the header of the \"packed-refs\" file.)\n+\t */\n+\tenum { PEELED_NONE, PEELED_TAGS, PEELED_FULLY } peeled;\n+\n \t/*\n \t * Count of references to the data structure in this instance,\n \t * including the pointer from files_ref_store::packed if any.\n@@ -195,13 +201,13 @@ static struct packed_ref_cache *read_packed_refs(struct packed_ref_store *refs)\n \tchar *buf;\n \tconst char *pos, *eol, *eof;\n \tstruct strbuf tmp = STRBUF_INIT;\n-\tenum { PEELED_NONE, PEELED_TAGS, PEELED_FULLY } peeled = PEELED_NONE;\n \tstruct ref_dir *dir;\n \n \tpacked_refs->refs = refs;\n \tacquire_packed_ref_cache(packed_refs);\n \tpacked_refs->cache = create_ref_cache(NULL, NULL);\n \tpacked_refs->cache->root->flag &= ~REF_INCOMPLETE;\n+\tpacked_refs->peeled = PEELED_NONE;\n \n \tfd = open(refs->path, O_RDONLY);\n \tif (fd < 0) {\n@@ -244,9 +250,9 @@ static struct packed_ref_cache *read_packed_refs(struct packed_ref_store *refs)\n \t\tstring_list_split_in_place(&traits, p, ' ', -1);\n \n \t\tif (unsorted_string_list_has_string(&traits, \"fully-peeled\"))\n-\t\t\tpeeled = PEELED_FULLY;\n+\t\t\tpacked_refs->peeled = PEELED_FULLY;\n \t\telse if (unsorted_string_list_has_string(&traits, \"peeled\"))\n-\t\t\tpeeled = PEELED_TAGS;\n+\t\t\tpacked_refs->peeled = PEELED_TAGS;\n \t\t/* perhaps other traits later as well */\n \n \t\t/* The \"+ 1\" is for the LF character. */\n@@ -282,8 +288,9 @@ static struct packed_ref_cache *read_packed_refs(struct packed_ref_store *refs)\n \t\t\toidclr(&oid);\n \t\t\tflag |= REF_BAD_NAME | REF_ISBROKEN;\n \t\t}\n-\t\tif (peeled == PEELED_FULLY ||\n-\t\t (peeled == PEELED_TAGS && starts_with(refname, \"refs/tags/\")))\n+\t\tif (packed_refs->peeled == PEELED_FULLY ||\n+\t\t (packed_refs->peeled == PEELED_TAGS &&\n+\t\t starts_with(refname, \"refs/tags/\")))\n \t\t\tflag |= REF_KNOWS_PEELED;\n \t\tentry = create_ref_entry(refname, &oid, flag);\n \t\tadd_ref_entry(dir, entry);\n-- \n2.14.1\n\n"},{"id":"327961","messageId":"d34d2cbde67e9ef0b0b11910de19c7f722c2314c.1505319366.git.mhagger@alum.mit.edu","threadId":"46744","inReplyTo":"cover.1505319366.git.mhagger@alum.mit.edu","subject":"[PATCH 11/20] mmapped_ref_iterator_advance(): no peeled value for broken refs","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-09-13T17:16:05Z","receivedAt":"2017-09-13T17:16:56Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"If a reference is broken, suppress its peeled value.\n\nSigned-off-by: Michael Haggerty \n---\n refs/packed-backend.c | 10 ++++++++--\n 1 file changed, 8 insertions(+), 2 deletions(-)\n\ndiff --git a/refs/packed-backend.c b/refs/packed-backend.c\nindex 312116a99d..724c88631d 100644\n--- a/refs/packed-backend.c\n+++ b/refs/packed-backend.c\n@@ -234,9 +234,15 @@ static int mmapped_ref_iterator_advance(struct ref_iterator *ref_iterator)\n \n \t\t/*\n \t\t * Regardless of what the file header said, we\n-\t\t * definitely know the value of *this* reference:\n+\t\t * definitely know the value of *this* reference. But\n+\t\t * we suppress it if the reference is broken:\n \t\t */\n-\t\titer->base.flags |= REF_KNOWS_PEELED;\n+\t\tif ((iter->base.flags & REF_ISBROKEN)) {\n+\t\t\toidclr(&iter->peeled);\n+\t\t\titer->base.flags &= ~REF_KNOWS_PEELED;\n+\t\t} else {\n+\t\t\titer->base.flags |= REF_KNOWS_PEELED;\n+\t\t}\n \t} else {\n \t\toidclr(&iter->peeled);\n \t}\n-- \n2.14.1\n\n"},{"id":"327962","messageId":"c3a96718f2d082b2384b00e869ec00ef364d40bb.1505319366.git.mhagger@alum.mit.edu","threadId":"46744","inReplyTo":"cover.1505319366.git.mhagger@alum.mit.edu","subject":"[PATCH 12/20] packed_ref_cache: keep the `packed-refs` file open and mmapped","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-09-13T17:16:06Z","receivedAt":"2017-09-13T17:16:57Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"As long as a `packed_ref_cache` object is in use, keep the\ncorresponding `packed-refs` file open and mmapped.\n\nThis is still pointless, because we only read the file immediately\nafter instantiating the `packed_ref_cache`. But that will soon change.\n\nSigned-off-by: Michael Haggerty \n---\n refs/packed-backend.c | 137 ++++++++++++++++++++++++++++++++------------------\n 1 file changed, 89 insertions(+), 48 deletions(-)\n\ndiff --git a/refs/packed-backend.c b/refs/packed-backend.c\nindex 724c88631d..d209c8e5a0 100644\n--- a/refs/packed-backend.c\n+++ b/refs/packed-backend.c\n@@ -18,6 +18,22 @@ struct packed_ref_cache {\n \n \tstruct ref_cache *cache;\n \n+\t/*\n+\t * The file descriptor of the `packed-refs` file (open in\n+\t * read-only mode), or -1 if it is not open.\n+\t */\n+\tint fd;\n+\n+\t/*\n+\t * The range of memory to which the `packed-refs` file is\n+\t * mmapped. If there were no contents (e.g., because the file\n+\t * didn't exist), `buf` and `eof` are both NULL.\n+\t */\n+\tchar *buf, *eof;\n+\n+\t/* The size of the header line, if any; otherwise, 0: */\n+\tsize_t header_len;\n+\n \t/*\n \t * What is the peeled state of this cache? (This is usually\n \t * determined from the header of the \"packed-refs\" file.)\n@@ -36,30 +52,6 @@ struct packed_ref_cache {\n \tstruct stat_validity validity;\n };\n \n-/*\n- * Increment the reference count of *packed_refs.\n- */\n-static void acquire_packed_ref_cache(struct packed_ref_cache *packed_refs)\n-{\n-\tpacked_refs->referrers++;\n-}\n-\n-/*\n- * Decrease the reference count of *packed_refs. If it goes to zero,\n- * free *packed_refs and return true; otherwise return false.\n- */\n-static int release_packed_ref_cache(struct packed_ref_cache *packed_refs)\n-{\n-\tif (!--packed_refs->referrers) {\n-\t\tfree_ref_cache(packed_refs->cache);\n-\t\tstat_validity_clear(&packed_refs->validity);\n-\t\tfree(packed_refs);\n-\t\treturn 1;\n-\t} else {\n-\t\treturn 0;\n-\t}\n-}\n-\n /*\n * A container for `packed-refs`-related data. It is not (yet) a\n * `ref_store`.\n@@ -92,6 +84,50 @@ struct packed_ref_store {\n \tstruct tempfile tempfile;\n };\n \n+/*\n+ * Increment the reference count of *packed_refs.\n+ */\n+static void acquire_packed_ref_cache(struct packed_ref_cache *packed_refs)\n+{\n+\tpacked_refs->referrers++;\n+}\n+\n+/*\n+ * If the buffer in `packed_refs` is active, munmap the memory, close the\n+ * file, and set the buffer pointers to NULL.\n+ */\n+static void release_packed_ref_buffer(struct packed_ref_cache *packed_refs)\n+{\n+\tif (packed_refs->buf) {\n+\t\tif (munmap(packed_refs->buf,\n+\t\t\t packed_refs->eof - packed_refs->buf))\n+\t\t\tdie_errno(\"error ummapping packed-refs file %s\",\n+\t\t\t\t packed_refs->refs->path);\n+\t\tpacked_refs->buf = packed_refs->eof = NULL;\n+\t\tpacked_refs->header_len = 0;\n+\t}\n+\n+\tif (packed_refs->fd >= 0)\n+\t\tclose(packed_refs->fd);\n+}\n+\n+/*\n+ * Decrease the reference count of *packed_refs. If it goes to zero,\n+ * free *packed_refs and return true; otherwise return false.\n+ */\n+static int release_packed_ref_cache(struct packed_ref_cache *packed_refs)\n+{\n+\tif (!--packed_refs->referrers) {\n+\t\tfree_ref_cache(packed_refs->cache);\n+\t\tstat_validity_clear(&packed_refs->validity);\n+\t\trelease_packed_ref_buffer(packed_refs);\n+\t\tfree(packed_refs);\n+\t\treturn 1;\n+\t} else {\n+\t\treturn 0;\n+\t}\n+}\n+\n struct ref_store *packed_ref_store_create(const char *path,\n \t\t\t\t\t unsigned int store_flags)\n {\n@@ -284,13 +320,15 @@ static struct ref_iterator_vtable mmapped_ref_iterator_vtable = {\n };\n \n struct ref_iterator *mmapped_ref_iterator_begin(\n-\t\tconst char *packed_refs_file,\n \t\tstruct packed_ref_cache *packed_refs,\n \t\tconst char *pos, const char *eof)\n {\n \tstruct mmapped_ref_iterator *iter = xcalloc(1, sizeof(*iter));\n \tstruct ref_iterator *ref_iterator = &iter->base;\n \n+\tif (!packed_refs->buf)\n+\t\treturn empty_ref_iterator_begin();\n+\n \tbase_ref_iterator_init(ref_iterator, &mmapped_ref_iterator_vtable, 0);\n \n \titer->packed_refs = packed_refs;\n@@ -336,11 +374,8 @@ struct ref_iterator *mmapped_ref_iterator_begin(\n static struct packed_ref_cache *read_packed_refs(struct packed_ref_store *refs)\n {\n \tstruct packed_ref_cache *packed_refs = xcalloc(1, sizeof(*packed_refs));\n-\tint fd;\n \tstruct stat st;\n \tsize_t size;\n-\tchar *buf;\n-\tconst char *pos, *eof;\n \tstruct ref_dir *dir;\n \tstruct ref_iterator *iter;\n \tint ok;\n@@ -351,13 +386,15 @@ static struct packed_ref_cache *read_packed_refs(struct packed_ref_store *refs)\n \tpacked_refs->cache->root->flag &= ~REF_INCOMPLETE;\n \tpacked_refs->peeled = PEELED_NONE;\n \n-\tfd = open(refs->path, O_RDONLY);\n-\tif (fd < 0) {\n+\tpacked_refs->fd = open(refs->path, O_RDONLY);\n+\tif (packed_refs->fd < 0) {\n \t\tif (errno == ENOENT) {\n \t\t\t/*\n \t\t\t * This is OK; it just means that no\n \t\t\t * \"packed-refs\" file has been written yet,\n-\t\t\t * which is equivalent to it being empty.\n+\t\t\t * which is equivalent to it being empty,\n+\t\t\t * which is its state when initialized with\n+\t\t\t * zeros.\n \t\t\t */\n \t\t\treturn packed_refs;\n \t\t} else {\n@@ -365,31 +402,37 @@ static struct packed_ref_cache *read_packed_refs(struct packed_ref_store *refs)\n \t\t}\n \t}\n \n-\tstat_validity_update(&packed_refs->validity, fd);\n+\tstat_validity_update(&packed_refs->validity, packed_refs->fd);\n \n-\tif (fstat(fd, &st) < 0)\n+\tif (fstat(packed_refs->fd, &st) < 0)\n \t\tdie_errno(\"couldn't stat %s\", refs->path);\n \n \tsize = xsize_t(st.st_size);\n-\tbuf = xmmap(NULL, size, PROT_READ, MAP_PRIVATE, fd, 0);\n-\tpos = buf;\n-\teof = buf + size;\n+\tpacked_refs->buf = xmmap(NULL, size,\n+\t\t\t\t PROT_READ, MAP_PRIVATE,\n+\t\t\t\t packed_refs->fd, 0);\n+\tpacked_refs->eof = packed_refs->buf + size;\n \n \t/* If the file has a header line, process it: */\n-\tif (pos < eof && *pos == '#') {\n+\tif (packed_refs->buf < packed_refs->eof && *packed_refs->buf == '#') {\n \t\tstruct strbuf tmp = STRBUF_INIT;\n \t\tchar *p;\n \t\tconst char *eol;\n \t\tstruct string_list traits = STRING_LIST_INIT_NODUP;\n \n-\t\teol = memchr(pos, '\\n', eof - pos);\n+\t\teol = memchr(packed_refs->buf, '\\n',\n+\t\t\t packed_refs->eof - packed_refs->buf);\n \t\tif (!eol)\n-\t\t\tdie_unterminated_line(refs->path, pos, eof - pos);\n+\t\t\tdie_unterminated_line(refs->path,\n+\t\t\t\t\t packed_refs->buf,\n+\t\t\t\t\t packed_refs->eof - packed_refs->buf);\n \n-\t\tstrbuf_add(&tmp, pos, eol - pos);\n+\t\tstrbuf_add(&tmp, packed_refs->buf, eol - packed_refs->buf);\n \n \t\tif (!skip_prefix(tmp.buf, \"# pack-refs with:\", (const char **)&p))\n-\t\t\tdie_invalid_line(refs->path, pos, eof - pos);\n+\t\t\tdie_invalid_line(refs->path,\n+\t\t\t\t\t packed_refs->buf,\n+\t\t\t\t\t packed_refs->eof - packed_refs->buf);\n \n \t\tstring_list_split_in_place(&traits, p, ' ', -1);\n \n@@ -400,14 +443,17 @@ static struct packed_ref_cache *read_packed_refs(struct packed_ref_store *refs)\n \t\t/* perhaps other traits later as well */\n \n \t\t/* The \"+ 1\" is for the LF character. */\n-\t\tpos = eol + 1;\n+\t\tpacked_refs->header_len = eol + 1 - packed_refs->buf;\n \n \t\tstring_list_clear(&traits, 0);\n \t\tstrbuf_release(&tmp);\n \t}\n \n \tdir = get_ref_dir(packed_refs->cache->root);\n-\titer = mmapped_ref_iterator_begin(refs->path, packed_refs, pos, eof);\n+\titer = mmapped_ref_iterator_begin(\n+\t\t\tpacked_refs,\n+\t\t\tpacked_refs->buf + packed_refs->header_len,\n+\t\t\tpacked_refs->eof);\n \twhile ((ok = ref_iterator_advance(iter)) == ITER_OK) {\n \t\tstruct ref_entry *entry =\n \t\t\tcreate_ref_entry(iter->refname, iter->oid, iter->flags);\n@@ -420,11 +466,6 @@ static struct packed_ref_cache *read_packed_refs(struct packed_ref_store *refs)\n \tif (ok != ITER_DONE)\n \t\tdie(\"error reading packed-refs file %s\", refs->path);\n \n-\tif (munmap(buf, size))\n-\t\tdie_errno(\"error ummapping packed-refs file %s\", refs->path);\n-\n-\tclose(fd);\n-\n \treturn packed_refs;\n }\n \n-- \n2.14.1\n\n"},{"id":"327963","messageId":"e556cd698a9106a4b796069dba489f0ac0e0f3e7.1505319366.git.mhagger@alum.mit.edu","threadId":"46744","inReplyTo":"cover.1505319366.git.mhagger@alum.mit.edu","subject":"[PATCH 17/20] packed_ref_store: get rid of the `ref_cache` entirely","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-09-13T17:16:11Z","receivedAt":"2017-09-13T17:17:09Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Now that everything has been changed to read what it needs directly\nout of the `packed-refs` file, `packed_ref_store` doesn't need to\nmaintain a `ref_cache` at all. So get rid of it.\n\nFirst of all, this will save a lot of memory and lots of little\nallocations. Instead of needing to store complicated parsed data\nstructures in memory, we just mmap the file (potentially sharing\nmemory with other processes) and parse only what we need.\n\nMoreover, since the mmapped access to the file reads only the parts of\nthe file that it needs, this might save reading all of the data from\ndisk at all (at least if the file starts out sorted).\n\nSigned-off-by: Michael Haggerty \n---\n refs/packed-backend.c | 29 ++---------------------------\n 1 file changed, 2 insertions(+), 27 deletions(-)\n\ndiff --git a/refs/packed-backend.c b/refs/packed-backend.c\nindex 88242a49fe..9868a5c198 100644\n--- a/refs/packed-backend.c\n+++ b/refs/packed-backend.c\n@@ -16,8 +16,6 @@ struct packed_ref_cache {\n \t */\n \tstruct packed_ref_store *refs;\n \n-\tstruct ref_cache *cache;\n-\n \t/*\n \t * The file descriptor of the `packed-refs` file (open in\n \t * read-only mode), or -1 if it is not open.\n@@ -123,7 +121,6 @@ static void release_packed_ref_buffer(struct packed_ref_cache *packed_refs)\n static int release_packed_ref_cache(struct packed_ref_cache *packed_refs)\n {\n \tif (!--packed_refs->referrers) {\n-\t\tfree_ref_cache(packed_refs->cache);\n \t\tstat_validity_clear(&packed_refs->validity);\n \t\trelease_packed_ref_buffer(packed_refs);\n \t\tfree(packed_refs);\n@@ -640,15 +637,10 @@ static struct packed_ref_cache *read_packed_refs(struct packed_ref_store *refs)\n \tstruct packed_ref_cache *packed_refs = xcalloc(1, sizeof(*packed_refs));\n \tstruct stat st;\n \tsize_t size;\n-\tstruct ref_dir *dir;\n-\tstruct ref_iterator *iter;\n \tint sorted = 0;\n-\tint ok;\n \n \tpacked_refs->refs = refs;\n \tacquire_packed_ref_cache(packed_refs);\n-\tpacked_refs->cache = create_ref_cache(NULL, NULL);\n-\tpacked_refs->cache->root->flag &= ~REF_INCOMPLETE;\n \tpacked_refs->peeled = PEELED_NONE;\n \n \tpacked_refs->fd = open(refs->path, O_RDONLY);\n@@ -730,23 +722,6 @@ static struct packed_ref_cache *read_packed_refs(struct packed_ref_store *refs)\n \t\tverify_buffer_safe(packed_refs);\n \t}\n \n-\tdir = get_ref_dir(packed_refs->cache->root);\n-\titer = mmapped_ref_iterator_begin(\n-\t\t\tpacked_refs,\n-\t\t\tpacked_refs->buf + packed_refs->header_len,\n-\t\t\tpacked_refs->eof);\n-\twhile ((ok = ref_iterator_advance(iter)) == ITER_OK) {\n-\t\tstruct ref_entry *entry =\n-\t\t\tcreate_ref_entry(iter->refname, iter->oid, iter->flags);\n-\n-\t\tif ((iter->flags & REF_KNOWS_PEELED))\n-\t\t\tref_iterator_peel(iter, &entry->u.value.peeled);\n-\t\tadd_ref_entry(dir, entry);\n-\t}\n-\n-\tif (ok != ITER_DONE)\n-\t\tdie(\"error reading packed-refs file %s\", refs->path);\n-\n \treturn packed_refs;\n }\n \n@@ -905,8 +880,8 @@ static struct ref_iterator *packed_ref_iterator_begin(\n \telse\n \t\tstart = packed_refs->buf + packed_refs->header_len;\n \n-\titer->iter0 = mmapped_ref_iterator_begin(\n-\t\t\tpacked_refs, start, packed_refs->eof);\n+\titer->iter0 = mmapped_ref_iterator_begin(packed_refs,\n+\t\t\t\t\t\t start, packed_refs->eof);\n \n \titer->flags = flags;\n \n-- \n2.14.1\n\n"},{"id":"327964","messageId":"050249b4d34d9a072dba0f3cdb60e5733f0e6631.1505319366.git.mhagger@alum.mit.edu","threadId":"46744","inReplyTo":"cover.1505319366.git.mhagger@alum.mit.edu","subject":"[PATCH 19/20] mmapped_ref_iterator: inline into `packed_ref_iterator`","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-09-13T17:16:13Z","receivedAt":"2017-09-13T17:17:15Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Since `packed_ref_iterator` is now delegating to\n`mmapped_ref_iterator` rather than `cache_ref_iterator` to do the\nheavy lifting, there is no need to keep the two iterators separate. So\n\"inline\" `mmapped_ref_iterator` into `packed_ref_iterator`. This\nremoves a bunch of boilerplate.\n\nSigned-off-by: Michael Haggerty \n---\n refs/packed-backend.c | 284 ++++++++++++++++++++------------------------------\n 1 file changed, 114 insertions(+), 170 deletions(-)\n\ndiff --git a/refs/packed-backend.c b/refs/packed-backend.c\nindex d76051c7f5..a3f8a19b9b 100644\n--- a/refs/packed-backend.c\n+++ b/refs/packed-backend.c\n@@ -200,157 +200,6 @@ static NORETURN void die_invalid_line(const char *path,\n \n }\n \n-/*\n- * This value is set in `base.flags` if the peeled value of the\n- * current reference is known. In that case, `peeled` contains the\n- * correct peeled value for the reference, which might be `null_sha1`\n- * if the reference is not a tag or if it is broken.\n- */\n-#define REF_KNOWS_PEELED 0x40\n-\n-/*\n- * An iterator over a packed-refs file that is currently mmapped.\n- */\n-struct mmapped_ref_iterator {\n-\tstruct ref_iterator base;\n-\n-\tstruct packed_ref_cache *packed_refs;\n-\n-\t/* The current position in the mmapped file: */\n-\tconst char *pos;\n-\n-\t/* The end of the mmapped file: */\n-\tconst char *eof;\n-\n-\tstruct object_id oid, peeled;\n-\n-\tstruct strbuf refname_buf;\n-};\n-\n-static int mmapped_ref_iterator_advance(struct ref_iterator *ref_iterator)\n-{\n-\tstruct mmapped_ref_iterator *iter =\n-\t\t(struct mmapped_ref_iterator *)ref_iterator;\n-\tconst char *p = iter->pos, *eol;\n-\n-\tstrbuf_reset(&iter->refname_buf);\n-\n-\tif (iter->pos == iter->eof)\n-\t\treturn ref_iterator_abort(ref_iterator);\n-\n-\titer->base.flags = REF_ISPACKED;\n-\n-\tif (iter->eof - p < GIT_SHA1_HEXSZ + 2 ||\n-\t parse_oid_hex(p, &iter->oid, &p) ||\n-\t !isspace(*p++))\n-\t\tdie_invalid_line(iter->packed_refs->refs->path,\n-\t\t\t\t iter->pos, iter->eof - iter->pos);\n-\n-\teol = memchr(p, '\\n', iter->eof - p);\n-\tif (!eol)\n-\t\tdie_unterminated_line(iter->packed_refs->refs->path,\n-\t\t\t\t iter->pos, iter->eof - iter->pos);\n-\n-\tstrbuf_add(&iter->refname_buf, p, eol - p);\n-\titer->base.refname = iter->refname_buf.buf;\n-\n-\tif (check_refname_format(iter->base.refname, REFNAME_ALLOW_ONELEVEL)) {\n-\t\tif (!refname_is_safe(iter->base.refname))\n-\t\t\tdie(\"packed refname is dangerous: %s\",\n-\t\t\t iter->base.refname);\n-\t\toidclr(&iter->oid);\n-\t\titer->base.flags |= REF_BAD_NAME | REF_ISBROKEN;\n-\t}\n-\tif (iter->packed_refs->peeled == PEELED_FULLY ||\n-\t (iter->packed_refs->peeled == PEELED_TAGS &&\n-\t starts_with(iter->base.refname, \"refs/tags/\")))\n-\t\titer->base.flags |= REF_KNOWS_PEELED;\n-\n-\titer->pos = eol + 1;\n-\n-\tif (iter->pos < iter->eof && *iter->pos == '^') {\n-\t\tp = iter->pos + 1;\n-\t\tif (iter->eof - p < GIT_SHA1_HEXSZ + 1 ||\n-\t\t parse_oid_hex(p, &iter->peeled, &p) ||\n-\t\t *p++ != '\\n')\n-\t\t\tdie_invalid_line(iter->packed_refs->refs->path,\n-\t\t\t\t\t iter->pos, iter->eof - iter->pos);\n-\t\titer->pos = p;\n-\n-\t\t/*\n-\t\t * Regardless of what the file header said, we\n-\t\t * definitely know the value of *this* reference. But\n-\t\t * we suppress it if the reference is broken:\n-\t\t */\n-\t\tif ((iter->base.flags & REF_ISBROKEN)) {\n-\t\t\toidclr(&iter->peeled);\n-\t\t\titer->base.flags &= ~REF_KNOWS_PEELED;\n-\t\t} else {\n-\t\t\titer->base.flags |= REF_KNOWS_PEELED;\n-\t\t}\n-\t} else {\n-\t\toidclr(&iter->peeled);\n-\t}\n-\n-\treturn ITER_OK;\n-}\n-\n-static int mmapped_ref_iterator_peel(struct ref_iterator *ref_iterator,\n-\t\t\t\t struct object_id *peeled)\n-{\n-\tstruct mmapped_ref_iterator *iter =\n-\t\t(struct mmapped_ref_iterator *)ref_iterator;\n-\n-\tif ((iter->base.flags & REF_KNOWS_PEELED)) {\n-\t\toidcpy(peeled, &iter->peeled);\n-\t\treturn is_null_oid(&iter->peeled) ? -1 : 0;\n-\t} else if ((iter->base.flags & (REF_ISBROKEN | REF_ISSYMREF))) {\n-\t\treturn -1;\n-\t} else {\n-\t\treturn !!peel_object(iter->oid.hash, peeled->hash);\n-\t}\n-}\n-\n-static int mmapped_ref_iterator_abort(struct ref_iterator *ref_iterator)\n-{\n-\tstruct mmapped_ref_iterator *iter =\n-\t\t(struct mmapped_ref_iterator *)ref_iterator;\n-\n-\trelease_packed_ref_cache(iter->packed_refs);\n-\tstrbuf_release(&iter->refname_buf);\n-\tbase_ref_iterator_free(ref_iterator);\n-\treturn ITER_DONE;\n-}\n-\n-static struct ref_iterator_vtable mmapped_ref_iterator_vtable = {\n-\tmmapped_ref_iterator_advance,\n-\tmmapped_ref_iterator_peel,\n-\tmmapped_ref_iterator_abort\n-};\n-\n-struct ref_iterator *mmapped_ref_iterator_begin(\n-\t\tstruct packed_ref_cache *packed_refs,\n-\t\tconst char *pos, const char *eof)\n-{\n-\tstruct mmapped_ref_iterator *iter = xcalloc(1, sizeof(*iter));\n-\tstruct ref_iterator *ref_iterator = &iter->base;\n-\n-\tif (!packed_refs->buf)\n-\t\treturn empty_ref_iterator_begin();\n-\n-\tbase_ref_iterator_init(ref_iterator, &mmapped_ref_iterator_vtable, 1);\n-\n-\titer->packed_refs = packed_refs;\n-\tacquire_packed_ref_cache(iter->packed_refs);\n-\titer->pos = pos;\n-\titer->eof = eof;\n-\tstrbuf_init(&iter->refname_buf, 0);\n-\n-\titer->base.oid = &iter->oid;\n-\n-\treturn ref_iterator;\n-}\n-\n struct packed_ref_entry {\n \tconst char *start;\n \tsize_t len;\n@@ -788,38 +637,120 @@ static int packed_read_raw_ref(struct ref_store *ref_store,\n \treturn 0;\n }\n \n+/*\n+ * This value is set in `base.flags` if the peeled value of the\n+ * current reference is known. In that case, `peeled` contains the\n+ * correct peeled value for the reference, which might be `null_sha1`\n+ * if the reference is not a tag or if it is broken.\n+ */\n+#define REF_KNOWS_PEELED 0x40\n+\n+/*\n+ * An iterator over a packed-refs file that is currently mmapped.\n+ */\n struct packed_ref_iterator {\n \tstruct ref_iterator base;\n \n-\tstruct packed_ref_cache *cache;\n-\tstruct ref_iterator *iter0;\n+\tstruct packed_ref_cache *packed_refs;\n+\n+\t/* The current position in the mmapped file: */\n+\tconst char *pos;\n+\n+\t/* The end of the mmapped file: */\n+\tconst char *eof;\n+\n+\tstruct object_id oid, peeled;\n+\n+\tstruct strbuf refname_buf;\n+\n \tunsigned int flags;\n };\n \n+static int next_record(struct packed_ref_iterator *iter)\n+{\n+\tconst char *p = iter->pos, *eol;\n+\n+\tstrbuf_reset(&iter->refname_buf);\n+\n+\tif (iter->pos == iter->eof)\n+\t\treturn ITER_DONE;\n+\n+\titer->base.flags = REF_ISPACKED;\n+\n+\tif (iter->eof - p < GIT_SHA1_HEXSZ + 2 ||\n+\t parse_oid_hex(p, &iter->oid, &p) ||\n+\t !isspace(*p++))\n+\t\tdie_invalid_line(iter->packed_refs->refs->path,\n+\t\t\t\t iter->pos, iter->eof - iter->pos);\n+\n+\teol = memchr(p, '\\n', iter->eof - p);\n+\tif (!eol)\n+\t\tdie_unterminated_line(iter->packed_refs->refs->path,\n+\t\t\t\t iter->pos, iter->eof - iter->pos);\n+\n+\tstrbuf_add(&iter->refname_buf, p, eol - p);\n+\titer->base.refname = iter->refname_buf.buf;\n+\n+\tif (check_refname_format(iter->base.refname, REFNAME_ALLOW_ONELEVEL)) {\n+\t\tif (!refname_is_safe(iter->base.refname))\n+\t\t\tdie(\"packed refname is dangerous: %s\",\n+\t\t\t iter->base.refname);\n+\t\toidclr(&iter->oid);\n+\t\titer->base.flags |= REF_BAD_NAME | REF_ISBROKEN;\n+\t}\n+\tif (iter->packed_refs->peeled == PEELED_FULLY ||\n+\t (iter->packed_refs->peeled == PEELED_TAGS &&\n+\t starts_with(iter->base.refname, \"refs/tags/\")))\n+\t\titer->base.flags |= REF_KNOWS_PEELED;\n+\n+\titer->pos = eol + 1;\n+\n+\tif (iter->pos < iter->eof && *iter->pos == '^') {\n+\t\tp = iter->pos + 1;\n+\t\tif (iter->eof - p < GIT_SHA1_HEXSZ + 1 ||\n+\t\t parse_oid_hex(p, &iter->peeled, &p) ||\n+\t\t *p++ != '\\n')\n+\t\t\tdie_invalid_line(iter->packed_refs->refs->path,\n+\t\t\t\t\t iter->pos, iter->eof - iter->pos);\n+\t\titer->pos = p;\n+\n+\t\t/*\n+\t\t * Regardless of what the file header said, we\n+\t\t * definitely know the value of *this* reference. But\n+\t\t * we suppress it if the reference is broken:\n+\t\t */\n+\t\tif ((iter->base.flags & REF_ISBROKEN)) {\n+\t\t\toidclr(&iter->peeled);\n+\t\t\titer->base.flags &= ~REF_KNOWS_PEELED;\n+\t\t} else {\n+\t\t\titer->base.flags |= REF_KNOWS_PEELED;\n+\t\t}\n+\t} else {\n+\t\toidclr(&iter->peeled);\n+\t}\n+\n+\treturn ITER_OK;\n+}\n+\n static int packed_ref_iterator_advance(struct ref_iterator *ref_iterator)\n {\n \tstruct packed_ref_iterator *iter =\n \t\t(struct packed_ref_iterator *)ref_iterator;\n \tint ok;\n \n-\twhile ((ok = ref_iterator_advance(iter->iter0)) == ITER_OK) {\n+\twhile ((ok = next_record(iter)) == ITER_OK) {\n \t\tif (iter->flags & DO_FOR_EACH_PER_WORKTREE_ONLY &&\n-\t\t ref_type(iter->iter0->refname) != REF_TYPE_PER_WORKTREE)\n+\t\t ref_type(iter->base.refname) != REF_TYPE_PER_WORKTREE)\n \t\t\tcontinue;\n \n \t\tif (!(iter->flags & DO_FOR_EACH_INCLUDE_BROKEN) &&\n-\t\t !ref_resolves_to_object(iter->iter0->refname,\n-\t\t\t\t\t iter->iter0->oid,\n-\t\t\t\t\t iter->iter0->flags))\n+\t\t !ref_resolves_to_object(iter->base.refname, &iter->oid,\n+\t\t\t\t\t iter->flags))\n \t\t\tcontinue;\n \n-\t\titer->base.refname = iter->iter0->refname;\n-\t\titer->base.oid = iter->iter0->oid;\n-\t\titer->base.flags = iter->iter0->flags;\n \t\treturn ITER_OK;\n \t}\n \n-\titer->iter0 = NULL;\n \tif (ref_iterator_abort(ref_iterator) != ITER_DONE)\n \t\tok = ITER_ERROR;\n \n@@ -832,7 +763,14 @@ static int packed_ref_iterator_peel(struct ref_iterator *ref_iterator,\n \tstruct packed_ref_iterator *iter =\n \t\t(struct packed_ref_iterator *)ref_iterator;\n \n-\treturn ref_iterator_peel(iter->iter0, peeled);\n+\tif ((iter->base.flags & REF_KNOWS_PEELED)) {\n+\t\toidcpy(peeled, &iter->peeled);\n+\t\treturn is_null_oid(&iter->peeled) ? -1 : 0;\n+\t} else if ((iter->base.flags & (REF_ISBROKEN | REF_ISSYMREF))) {\n+\t\treturn -1;\n+\t} else {\n+\t\treturn !!peel_object(iter->oid.hash, peeled->hash);\n+\t}\n }\n \n static int packed_ref_iterator_abort(struct ref_iterator *ref_iterator)\n@@ -841,10 +779,8 @@ static int packed_ref_iterator_abort(struct ref_iterator *ref_iterator)\n \t\t(struct packed_ref_iterator *)ref_iterator;\n \tint ok = ITER_DONE;\n \n-\tif (iter->iter0)\n-\t\tok = ref_iterator_abort(iter->iter0);\n-\n-\trelease_packed_ref_cache(iter->cache);\n+\tstrbuf_release(&iter->refname_buf);\n+\trelease_packed_ref_cache(iter->packed_refs);\n \tbase_ref_iterator_free(ref_iterator);\n \treturn ok;\n }\n@@ -870,6 +806,11 @@ static struct ref_iterator *packed_ref_iterator_begin(\n \t\trequired_flags |= REF_STORE_ODB;\n \trefs = packed_downcast(ref_store, required_flags, \"ref_iterator_begin\");\n \n+\tpacked_refs = get_packed_ref_cache(refs);\n+\n+\tif (!packed_refs->buf)\n+\t\treturn empty_ref_iterator_begin();\n+\n \titer = xcalloc(1, sizeof(*iter));\n \tref_iterator = &iter->base;\n \tbase_ref_iterator_init(ref_iterator, &packed_ref_iterator_vtable, 1);\n@@ -879,7 +820,7 @@ static struct ref_iterator *packed_ref_iterator_begin(\n \t * the packed-ref cache is up to date with what is on disk,\n \t * and re-reads it if not.\n \t */\n-\titer->cache = packed_refs = get_packed_ref_cache(refs);\n+\titer->packed_refs = packed_refs;\n \tacquire_packed_ref_cache(packed_refs);\n \n \tif (prefix && *prefix)\n@@ -887,8 +828,11 @@ static struct ref_iterator *packed_ref_iterator_begin(\n \telse\n \t\tstart = packed_refs->buf + packed_refs->header_len;\n \n-\titer->iter0 = mmapped_ref_iterator_begin(packed_refs,\n-\t\t\t\t\t\t start, packed_refs->eof);\n+\titer->pos = start;\n+\titer->eof = packed_refs->eof;\n+\tstrbuf_init(&iter->refname_buf, 0);\n+\n+\titer->base.oid = &iter->oid;\n \n \titer->flags = flags;\n \n-- \n2.14.1\n\n"},{"id":"327965","messageId":"8844c66d7b245dda7a2fa2794cb4051b5b2b35e2.1505319366.git.mhagger@alum.mit.edu","threadId":"46744","inReplyTo":"cover.1505319366.git.mhagger@alum.mit.edu","subject":"[PATCH 20/20] packed-backend.c: rename a bunch of things and update comments","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-09-13T17:16:14Z","receivedAt":"2017-09-13T17:17:19Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"We've made huge changes to this file, and some of the old names and\ncomments are no longer very fitting. So rename a bunch of things:\n\n* `struct packed_ref_cache` → `struct snapshot`\n* `acquire_packed_ref_cache()` → `acquire_snapshot()`\n* `release_packed_ref_buffer()` → `clear_snapshot_buffer()`\n* `release_packed_ref_cache()` → `release_snapshot()`\n* `clear_packed_ref_cache()` → `clear_snapshot()`\n* `struct packed_ref_entry` → `struct snapshot_record`\n* `cmp_packed_ref_entries()` → `cmp_packed_ref_records()`\n* `cmp_entry_to_refname()` → `cmp_record_to_refname()`\n* `sort_packed_refs()` → `sort_snapshot()`\n* `read_packed_refs()` → `create_snapshot()`\n* `validate_packed_ref_cache()` → `validate_snapshot()`\n* `get_packed_ref_cache()` → `get_snapshot()`\n* Renamed local variables and struct members accordingly.\n\nAlso update a bunch of comments to reflect the renaming and the\naccumulated changes that the code has undergone.\n\nSigned-off-by: Michael Haggerty \n---\n refs/packed-backend.c | 392 ++++++++++++++++++++++++++++----------------------\n 1 file changed, 217 insertions(+), 175 deletions(-)\n\ndiff --git a/refs/packed-backend.c b/refs/packed-backend.c\nindex a3f8a19b9b..8235ac8506 100644\n--- a/refs/packed-backend.c\n+++ b/refs/packed-backend.c\n@@ -8,10 +8,30 @@\n \n struct packed_ref_store;\n \n-struct packed_ref_cache {\n+/*\n+ * A `snapshot` represents one snapshot of a `packed-refs` file.\n+ *\n+ * Normally, this will be a mmapped view of the contents of the\n+ * `packed-refs` file at the time the snapshot was created. However,\n+ * if the `packed-refs` file was not sorted, this might point at heap\n+ * memory holding the contents of the `packed-refs` file with its\n+ * records sorted by refname.\n+ *\n+ * `snapshot` instances are reference counted (via\n+ * `acquire_snapshot()` and `release_snapshot()`). This is to prevent\n+ * an instance from disappearing while an iterator is still iterating\n+ * over it. Instances are garbage collected when their `referrers`\n+ * count goes to zero.\n+ *\n+ * The most recent `snapshot`, if available, is referenced by the\n+ * `packed_ref_store`. Its freshness is checked whenever\n+ * `get_snapshot()` is called; if the existing snapshot is obsolete, a\n+ * new snapshot is taken.\n+ */\n+struct snapshot {\n \t/*\n \t * A back-pointer to the packed_ref_store with which this\n-\t * cache is associated:\n+\t * snapshot is associated:\n \t */\n \tstruct packed_ref_store *refs;\n \n@@ -35,26 +55,42 @@ struct packed_ref_cache {\n \tsize_t header_len;\n \n \t/*\n-\t * What is the peeled state of this cache? (This is usually\n-\t * determined from the header of the \"packed-refs\" file.)\n+\t * What is the peeled state of the `packed-refs` file that\n+\t * this snapshot represents? (This is usually determined from\n+\t * the file's header.)\n \t */\n \tenum { PEELED_NONE, PEELED_TAGS, PEELED_FULLY } peeled;\n \n \t/*\n-\t * Count of references to the data structure in this instance,\n-\t * including the pointer from files_ref_store::packed if any.\n-\t * The data will not be freed as long as the reference count\n-\t * is nonzero.\n+\t * Count of references to this instance, including the pointer\n+\t * from `packed_ref_store::snapshot`, if any. The instance\n+\t * will not be freed as long as the reference count is\n+\t * nonzero.\n \t */\n \tunsigned int referrers;\n \n-\t/* The metadata from when this packed-refs cache was read */\n+\t/*\n+\t * The metadata of the `packed-refs` file from which this\n+\t * snapshot was created, used to tell if the file has been\n+\t * replaced since we read it.\n+\t */\n \tstruct stat_validity validity;\n };\n \n /*\n- * A container for `packed-refs`-related data. It is not (yet) a\n- * `ref_store`.\n+ * A `ref_store` representing references stored in a `packed-refs`\n+ * file. It implements the `ref_store` interface, though it has some\n+ * limitations:\n+ *\n+ * - It cannot store symbolic references.\n+ *\n+ * - It cannot store reflogs.\n+ *\n+ * - It does not support reference renaming (though it could).\n+ *\n+ * On the other hand, it can be locked outside of a reference\n+ * transaction. In that case, it remains locked even after the\n+ * transaction is done and the new `packed-refs` file is activated.\n */\n struct packed_ref_store {\n \tstruct ref_store base;\n@@ -65,10 +101,10 @@ struct packed_ref_store {\n \tchar *path;\n \n \t/*\n-\t * A cache of the values read from the `packed-refs` file, if\n-\t * it might still be current; otherwise, NULL.\n+\t * A snapshot of the values read from the `packed-refs` file,\n+\t * if it might still be current; otherwise, NULL.\n \t */\n-\tstruct packed_ref_cache *cache;\n+\tstruct snapshot *snapshot;\n \n \t/*\n \t * Lock used for the \"packed-refs\" file. Note that this (and\n@@ -85,44 +121,43 @@ struct packed_ref_store {\n };\n \n /*\n- * Increment the reference count of *packed_refs.\n+ * Increment the reference count of `*snapshot`.\n */\n-static void acquire_packed_ref_cache(struct packed_ref_cache *packed_refs)\n+static void acquire_snapshot(struct snapshot *snapshot)\n {\n-\tpacked_refs->referrers++;\n+\tsnapshot->referrers++;\n }\n \n /*\n- * If the buffer in `packed_refs` is active, then either munmap the\n+ * If the buffer in `snapshot` is active, then either munmap the\n * memory and close the file, or free the memory. Then set the buffer\n * pointers to NULL.\n */\n-static void release_packed_ref_buffer(struct packed_ref_cache *packed_refs)\n+static void clear_snapshot_buffer(struct snapshot *snapshot)\n {\n-\tif (packed_refs->fd >= 0) {\n-\t\tif (munmap(packed_refs->buf,\n-\t\t\t packed_refs->eof - packed_refs->buf))\n+\tif (snapshot->fd >= 0) {\n+\t\tif (munmap(snapshot->buf, snapshot->eof - snapshot->buf))\n \t\t\tdie_errno(\"error ummapping packed-refs file %s\",\n-\t\t\t\t packed_refs->refs->path);\n-\t\tclose(packed_refs->fd);\n-\t\tpacked_refs->fd = -1;\n+\t\t\t\t snapshot->refs->path);\n+\t\tclose(snapshot->fd);\n+\t\tsnapshot->fd = -1;\n \t} else {\n-\t\tfree(packed_refs->buf);\n+\t\tfree(snapshot->buf);\n \t}\n-\tpacked_refs->buf = packed_refs->eof = NULL;\n-\tpacked_refs->header_len = 0;\n+\tsnapshot->buf = snapshot->eof = NULL;\n+\tsnapshot->header_len = 0;\n }\n \n /*\n- * Decrease the reference count of *packed_refs. If it goes to zero,\n- * free *packed_refs and return true; otherwise return false.\n+ * Decrease the reference count of `*snapshot`. If it goes to zero,\n+ * free `*snapshot` and return true; otherwise return false.\n */\n-static int release_packed_ref_cache(struct packed_ref_cache *packed_refs)\n+static int release_snapshot(struct snapshot *snapshot)\n {\n-\tif (!--packed_refs->referrers) {\n-\t\tstat_validity_clear(&packed_refs->validity);\n-\t\trelease_packed_ref_buffer(packed_refs);\n-\t\tfree(packed_refs);\n+\tif (!--snapshot->referrers) {\n+\t\tstat_validity_clear(&snapshot->validity);\n+\t\tclear_snapshot_buffer(snapshot);\n+\t\tfree(snapshot);\n \t\treturn 1;\n \t} else {\n \t\treturn 0;\n@@ -167,13 +202,13 @@ static struct packed_ref_store *packed_downcast(struct ref_store *ref_store,\n \treturn refs;\n }\n \n-static void clear_packed_ref_cache(struct packed_ref_store *refs)\n+static void clear_snapshot(struct packed_ref_store *refs)\n {\n-\tif (refs->cache) {\n-\t\tstruct packed_ref_cache *cache = refs->cache;\n+\tif (refs->snapshot) {\n+\t\tstruct snapshot *snapshot = refs->snapshot;\n \n-\t\trefs->cache = NULL;\n-\t\trelease_packed_ref_cache(cache);\n+\t\trefs->snapshot = NULL;\n+\t\trelease_snapshot(snapshot);\n \t}\n }\n \n@@ -200,14 +235,14 @@ static NORETURN void die_invalid_line(const char *path,\n \n }\n \n-struct packed_ref_entry {\n+struct snapshot_record {\n \tconst char *start;\n \tsize_t len;\n };\n \n-static int cmp_packed_ref_entries(const void *v1, const void *v2)\n+static int cmp_packed_ref_records(const void *v1, const void *v2)\n {\n-\tconst struct packed_ref_entry *e1 = v1, *e2 = v2;\n+\tconst struct snapshot_record *e1 = v1, *e2 = v2;\n \tconst char *r1 = e1->start + GIT_SHA1_HEXSZ + 1;\n \tconst char *r2 = e2->start + GIT_SHA1_HEXSZ + 1;\n \n@@ -226,10 +261,10 @@ static int cmp_packed_ref_entries(const void *v1, const void *v2)\n }\n \n /*\n- * Compare a packed-refs record pointed to by `rec` to the specified\n- * NUL-terminated refname.\n+ * Compare a snapshot record at `rec` to the specified NUL-terminated\n+ * refname.\n */\n-static int cmp_entry_to_refname(const char *rec, const char *refname)\n+static int cmp_record_to_refname(const char *rec, const char *refname)\n {\n \tconst char *r1 = rec + GIT_SHA1_HEXSZ + 1;\n \tconst char *r2 = refname;\n@@ -247,31 +282,30 @@ static int cmp_entry_to_refname(const char *rec, const char *refname)\n }\n \n /*\n- * `packed_refs->buf` is not known to be sorted. Check whether it is,\n- * and if not, sort it into new memory and munmap/free the old\n- * storage.\n+ * `snapshot->buf` is not known to be sorted. Check whether it is, and\n+ * if not, sort it into new memory and munmap/free the old storage.\n */\n-static void sort_packed_refs(struct packed_ref_cache *packed_refs)\n+static void sort_snapshot(struct snapshot *snapshot)\n {\n-\tstruct packed_ref_entry *entries = NULL;\n+\tstruct snapshot_record *records = NULL;\n \tsize_t alloc = 0, nr = 0;\n \tint sorted = 1;\n \tconst char *pos, *eof, *eol;\n \tsize_t len, i;\n \tchar *new_buffer, *dst;\n \n-\tpos = packed_refs->buf + packed_refs->header_len;\n-\teof = packed_refs->eof;\n+\tpos = snapshot->buf + snapshot->header_len;\n+\teof = snapshot->eof;\n \tlen = eof - pos;\n \n \tif (!len)\n \t\treturn;\n \n \t/*\n-\t * Initialize entries based on a crude estimate of the number\n+\t * Initialize records based on a crude estimate of the number\n \t * of references in the file (we'll grow it below if needed):\n \t */\n-\tALLOC_GROW(entries, len / 80 + 20, alloc);\n+\tALLOC_GROW(records, len / 80 + 20, alloc);\n \n \twhile (pos < eof) {\n \t\teol = memchr(pos, '\\n', eof - pos);\n@@ -279,7 +313,7 @@ static void sort_packed_refs(struct packed_ref_cache *packed_refs)\n \t\t\t/* The safety check should prevent this. */\n \t\t\tBUG(\"unterminated line found in packed-refs\");\n \t\tif (eol - pos < GIT_SHA1_HEXSZ + 2)\n-\t\t\tdie_invalid_line(packed_refs->refs->path,\n+\t\t\tdie_invalid_line(snapshot->refs->path,\n \t\t\t\t\t pos, eof - pos);\n \t\teol++;\n \t\tif (eol < eof && *eol == '^') {\n@@ -296,15 +330,15 @@ static void sort_packed_refs(struct packed_ref_cache *packed_refs)\n \t\t\teol++;\n \t\t}\n \n-\t\tALLOC_GROW(entries, nr + 1, alloc);\n-\t\tentries[nr].start = pos;\n-\t\tentries[nr].len = eol - pos;\n+\t\tALLOC_GROW(records, nr + 1, alloc);\n+\t\trecords[nr].start = pos;\n+\t\trecords[nr].len = eol - pos;\n \t\tnr++;\n \n \t\tif (sorted &&\n \t\t nr > 1 &&\n-\t\t cmp_packed_ref_entries(&entries[nr - 2],\n-\t\t\t\t\t &entries[nr - 1]) >= 0)\n+\t\t cmp_packed_ref_records(&records[nr - 2],\n+\t\t\t\t\t &records[nr - 1]) >= 0)\n \t\t\tsorted = 0;\n \n \t\tpos = eol;\n@@ -313,31 +347,31 @@ static void sort_packed_refs(struct packed_ref_cache *packed_refs)\n \tif (sorted)\n \t\tgoto cleanup;\n \n-\t/* We need to sort the memory. First we sort the entries array: */\n-\tQSORT(entries, nr, cmp_packed_ref_entries);\n+\t/* We need to sort the memory. First we sort the records array: */\n+\tQSORT(records, nr, cmp_packed_ref_records);\n \n \t/*\n \t * Allocate a new chunk of memory, and copy the old memory to\n-\t * the new in the order indicated by `entries` (not bothering\n+\t * the new in the order indicated by `records` (not bothering\n \t * with the header line):\n \t */\n \tnew_buffer = xmalloc(len);\n \tfor (dst = new_buffer, i = 0; i < nr; i++) {\n-\t\tmemcpy(dst, entries[i].start, entries[i].len);\n-\t\tdst += entries[i].len;\n+\t\tmemcpy(dst, records[i].start, records[i].len);\n+\t\tdst += records[i].len;\n \t}\n \n \t/*\n \t * Now munmap the old buffer and use the sorted buffer in its\n \t * place:\n \t */\n-\trelease_packed_ref_buffer(packed_refs);\n-\tpacked_refs->buf = new_buffer;\n-\tpacked_refs->eof = new_buffer + len;\n-\tpacked_refs->header_len = 0;\n+\tclear_snapshot_buffer(snapshot);\n+\tsnapshot->buf = new_buffer;\n+\tsnapshot->eof = new_buffer + len;\n+\tsnapshot->header_len = 0;\n \n cleanup:\n-\tfree(entries);\n+\tfree(records);\n }\n \n /*\n@@ -381,10 +415,10 @@ static const char *find_end_of_record(const char *p, const char *end)\n * (GIT_SHA1_HEXSZ + 1) characters before the LF. Die if either of\n * these checks fails.\n */\n-static void verify_buffer_safe(struct packed_ref_cache *packed_refs)\n+static void verify_buffer_safe(struct snapshot *snapshot)\n {\n-\tconst char *buf = packed_refs->buf + packed_refs->header_len;\n-\tconst char *eof = packed_refs->eof;\n+\tconst char *buf = snapshot->buf + snapshot->header_len;\n+\tconst char *eof = snapshot->eof;\n \tconst char *last_line;\n \n \tif (buf == eof)\n@@ -392,12 +426,12 @@ static void verify_buffer_safe(struct packed_ref_cache *packed_refs)\n \n \tlast_line = find_start_of_record(buf, eof - 1);\n \tif (*(eof - 1) != '\\n' || eof - last_line < GIT_SHA1_HEXSZ + 2)\n-\t\tdie_invalid_line(packed_refs->refs->path,\n+\t\tdie_invalid_line(snapshot->refs->path,\n \t\t\t\t last_line, eof - last_line);\n }\n \n /*\n- * Find the place in `cache->buf` where the start of the record for\n+ * Find the place in `snapshot->buf` where the start of the record for\n * `refname` starts. If `mustexist` is true and the reference doesn't\n * exist, then return NULL. If `mustexist` is false and the reference\n * doesn't exist, then return the point where that reference would be\n@@ -405,10 +439,10 @@ static void verify_buffer_safe(struct packed_ref_cache *packed_refs)\n * reference name; for example, one could search for \"refs/replace/\"\n * to find the start of any replace references.\n *\n- * The record is sought using a binary search, so `cache->buf` must be\n- * sorted.\n+ * The record is sought using a binary search, so `snapshot->buf` must\n+ * be sorted.\n */\n-static const char *find_reference_location(struct packed_ref_cache *cache,\n+static const char *find_reference_location(struct snapshot *snapshot,\n \t\t\t\t\t const char *refname, int mustexist)\n {\n \t/*\n@@ -425,13 +459,13 @@ static const char *find_reference_location(struct packed_ref_cache *cache,\n \t * preceding records all have reference names that come\n \t * *before* `refname`.\n \t */\n-\tconst char *lo = cache->buf + cache->header_len;\n+\tconst char *lo = snapshot->buf + snapshot->header_len;\n \n \t/*\n \t * A pointer to a the first character of a record whose\n \t * reference name comes *after* `refname`.\n \t */\n-\tconst char *hi = cache->eof;\n+\tconst char *hi = snapshot->eof;\n \n \twhile (lo < hi) {\n \t\tconst char *mid, *rec;\n@@ -439,7 +473,7 @@ static const char *find_reference_location(struct packed_ref_cache *cache,\n \n \t\tmid = lo + (hi - lo) / 2;\n \t\trec = find_start_of_record(lo, mid);\n-\t\tcmp = cmp_entry_to_refname(rec, refname);\n+\t\tcmp = cmp_record_to_refname(rec, refname);\n \t\tif (cmp < 0) {\n \t\t\tlo = find_end_of_record(mid, hi);\n \t\t} else if (cmp > 0) {\n@@ -456,9 +490,9 @@ static const char *find_reference_location(struct packed_ref_cache *cache,\n }\n \n /*\n- * Read from the `packed-refs` file into a newly-allocated\n- * `packed_ref_cache` and return it. The return value will already\n- * have its reference count incremented.\n+ * Create a newly-allocated `snapshot` of the `packed-refs` file in\n+ * its current state and return it. The return value will already have\n+ * its reference count incremented.\n *\n * A comment line of the form \"# pack-refs with: \" may contain zero or\n * more traits. We interpret the traits as follows:\n@@ -488,19 +522,19 @@ static const char *find_reference_location(struct packed_ref_cache *cache,\n *\n * The references in this file are known to be sorted by refname.\n */\n-static struct packed_ref_cache *read_packed_refs(struct packed_ref_store *refs)\n+static struct snapshot *create_snapshot(struct packed_ref_store *refs)\n {\n-\tstruct packed_ref_cache *packed_refs = xcalloc(1, sizeof(*packed_refs));\n+\tstruct snapshot *snapshot = xcalloc(1, sizeof(*snapshot));\n \tstruct stat st;\n \tsize_t size;\n \tint sorted = 0;\n \n-\tpacked_refs->refs = refs;\n-\tacquire_packed_ref_cache(packed_refs);\n-\tpacked_refs->peeled = PEELED_NONE;\n+\tsnapshot->refs = refs;\n+\tacquire_snapshot(snapshot);\n+\tsnapshot->peeled = PEELED_NONE;\n \n-\tpacked_refs->fd = open(refs->path, O_RDONLY);\n-\tif (packed_refs->fd < 0) {\n+\tsnapshot->fd = open(refs->path, O_RDONLY);\n+\tif (snapshot->fd < 0) {\n \t\tif (errno == ENOENT) {\n \t\t\t/*\n \t\t\t * This is OK; it just means that no\n@@ -509,106 +543,106 @@ static struct packed_ref_cache *read_packed_refs(struct packed_ref_store *refs)\n \t\t\t * which is its state when initialized with\n \t\t\t * zeros.\n \t\t\t */\n-\t\t\treturn packed_refs;\n+\t\t\treturn snapshot;\n \t\t} else {\n \t\t\tdie_errno(\"couldn't read %s\", refs->path);\n \t\t}\n \t}\n \n-\tstat_validity_update(&packed_refs->validity, packed_refs->fd);\n+\tstat_validity_update(&snapshot->validity, snapshot->fd);\n \n-\tif (fstat(packed_refs->fd, &st) < 0)\n+\tif (fstat(snapshot->fd, &st) < 0)\n \t\tdie_errno(\"couldn't stat %s\", refs->path);\n \n \tsize = xsize_t(st.st_size);\n-\tpacked_refs->buf = xmmap(NULL, size,\n-\t\t\t\t PROT_READ, MAP_PRIVATE,\n-\t\t\t\t packed_refs->fd, 0);\n-\tpacked_refs->eof = packed_refs->buf + size;\n+\tsnapshot->buf = xmmap(NULL, size, PROT_READ, MAP_PRIVATE,\n+\t\t\t snapshot->fd, 0);\n+\tsnapshot->eof = snapshot->buf + size;\n \n \t/* If the file has a header line, process it: */\n-\tif (packed_refs->buf < packed_refs->eof && *packed_refs->buf == '#') {\n+\tif (snapshot->buf < snapshot->eof && *snapshot->buf == '#') {\n \t\tstruct strbuf tmp = STRBUF_INIT;\n \t\tchar *p;\n \t\tconst char *eol;\n \t\tstruct string_list traits = STRING_LIST_INIT_NODUP;\n \n-\t\teol = memchr(packed_refs->buf, '\\n',\n-\t\t\t packed_refs->eof - packed_refs->buf);\n+\t\teol = memchr(snapshot->buf, '\\n',\n+\t\t\t snapshot->eof - snapshot->buf);\n \t\tif (!eol)\n \t\t\tdie_unterminated_line(refs->path,\n-\t\t\t\t\t packed_refs->buf,\n-\t\t\t\t\t packed_refs->eof - packed_refs->buf);\n+\t\t\t\t\t snapshot->buf,\n+\t\t\t\t\t snapshot->eof - snapshot->buf);\n \n-\t\tstrbuf_add(&tmp, packed_refs->buf, eol - packed_refs->buf);\n+\t\tstrbuf_add(&tmp, snapshot->buf, eol - snapshot->buf);\n \n \t\tif (!skip_prefix(tmp.buf, \"# pack-refs with:\", (const char **)&p))\n \t\t\tdie_invalid_line(refs->path,\n-\t\t\t\t\t packed_refs->buf,\n-\t\t\t\t\t packed_refs->eof - packed_refs->buf);\n+\t\t\t\t\t snapshot->buf,\n+\t\t\t\t\t snapshot->eof - snapshot->buf);\n \n \t\tstring_list_split_in_place(&traits, p, ' ', -1);\n \n \t\tif (unsorted_string_list_has_string(&traits, \"fully-peeled\"))\n-\t\t\tpacked_refs->peeled = PEELED_FULLY;\n+\t\t\tsnapshot->peeled = PEELED_FULLY;\n \t\telse if (unsorted_string_list_has_string(&traits, \"peeled\"))\n-\t\t\tpacked_refs->peeled = PEELED_TAGS;\n+\t\t\tsnapshot->peeled = PEELED_TAGS;\n \n \t\tsorted = unsorted_string_list_has_string(&traits, \"sorted\");\n \n \t\t/* perhaps other traits later as well */\n \n \t\t/* The \"+ 1\" is for the LF character. */\n-\t\tpacked_refs->header_len = eol + 1 - packed_refs->buf;\n+\t\tsnapshot->header_len = eol + 1 - snapshot->buf;\n \n \t\tstring_list_clear(&traits, 0);\n \t\tstrbuf_release(&tmp);\n \t}\n \n-\tverify_buffer_safe(packed_refs);\n+\tverify_buffer_safe(snapshot);\n \n \tif (!sorted) {\n-\t\tsort_packed_refs(packed_refs);\n+\t\tsort_snapshot(snapshot);\n \n \t\t/*\n \t\t * Reordering the records might have moved a short one\n \t\t * to the end of the buffer, so verify the buffer's\n \t\t * safety again:\n \t\t */\n-\t\tverify_buffer_safe(packed_refs);\n+\t\tverify_buffer_safe(snapshot);\n \t}\n \n-\treturn packed_refs;\n+\treturn snapshot;\n }\n \n /*\n- * Check that the packed refs cache (if any) still reflects the\n- * contents of the file. If not, clear the cache.\n+ * Check that `refs->snapshot` (if present) still reflects the\n+ * contents of the `packed-refs` file. If not, clear the snapshot.\n */\n-static void validate_packed_ref_cache(struct packed_ref_store *refs)\n+static void validate_snapshot(struct packed_ref_store *refs)\n {\n-\tif (refs->cache &&\n-\t !stat_validity_check(&refs->cache->validity, refs->path))\n-\t\tclear_packed_ref_cache(refs);\n+\tif (refs->snapshot &&\n+\t !stat_validity_check(&refs->snapshot->validity, refs->path))\n+\t\tclear_snapshot(refs);\n }\n \n /*\n- * Get the packed_ref_cache for the specified packed_ref_store,\n- * creating and populating it if it hasn't been read before or if the\n- * file has been changed (according to its `validity` field) since it\n- * was last read. On the other hand, if we hold the lock, then assume\n- * that the file hasn't been changed out from under us, so skip the\n- * extra `stat()` call in `stat_validity_check()`.\n+ * Get the `snapshot` for the specified packed_ref_store, creating and\n+ * populating it if it hasn't been read before or if the file has been\n+ * changed (according to its `validity` field) since it was last read.\n+ * On the other hand, if we hold the lock, then assume that the file\n+ * hasn't been changed out from under us, so skip the extra `stat()`\n+ * call in `stat_validity_check()`. This function does *not* increase\n+ * the snapshot's reference count on behalf of the caller.\n */\n-static struct packed_ref_cache *get_packed_ref_cache(struct packed_ref_store *refs)\n+static struct snapshot *get_snapshot(struct packed_ref_store *refs)\n {\n \tif (!is_lock_file_locked(&refs->lock))\n-\t\tvalidate_packed_ref_cache(refs);\n+\t\tvalidate_snapshot(refs);\n \n-\tif (!refs->cache)\n-\t\trefs->cache = read_packed_refs(refs);\n+\tif (!refs->snapshot)\n+\t\trefs->snapshot = create_snapshot(refs);\n \n-\treturn refs->cache;\n+\treturn refs->snapshot;\n }\n \n static int packed_read_raw_ref(struct ref_store *ref_store,\n@@ -617,12 +651,12 @@ static int packed_read_raw_ref(struct ref_store *ref_store,\n {\n \tstruct packed_ref_store *refs =\n \t\tpacked_downcast(ref_store, REF_STORE_READ, \"read_raw_ref\");\n-\tstruct packed_ref_cache *packed_refs = get_packed_ref_cache(refs);\n+\tstruct snapshot *snapshot = get_snapshot(refs);\n \tconst char *rec;\n \n \t*type = 0;\n \n-\trec = find_reference_location(packed_refs, refname, 1);\n+\trec = find_reference_location(snapshot, refname, 1);\n \n \tif (!rec) {\n \t\t/* refname is not a packed reference. */\n@@ -631,7 +665,7 @@ static int packed_read_raw_ref(struct ref_store *ref_store,\n \t}\n \n \tif (get_sha1_hex(rec, sha1))\n-\t\tdie_invalid_line(refs->path, rec, packed_refs->eof - rec);\n+\t\tdie_invalid_line(refs->path, rec, snapshot->eof - rec);\n \n \t*type = REF_ISPACKED;\n \treturn 0;\n@@ -646,26 +680,33 @@ static int packed_read_raw_ref(struct ref_store *ref_store,\n #define REF_KNOWS_PEELED 0x40\n \n /*\n- * An iterator over a packed-refs file that is currently mmapped.\n+ * An iterator over a snapshot of a `packed-refs` file.\n */\n struct packed_ref_iterator {\n \tstruct ref_iterator base;\n \n-\tstruct packed_ref_cache *packed_refs;\n+\tstruct snapshot *snapshot;\n \n-\t/* The current position in the mmapped file: */\n+\t/* The current position in the snapshot's buffer: */\n \tconst char *pos;\n \n-\t/* The end of the mmapped file: */\n+\t/* The end of the part of the buffer that will be iterated over: */\n \tconst char *eof;\n \n+\t/* Scratch space for current values: */\n \tstruct object_id oid, peeled;\n-\n \tstruct strbuf refname_buf;\n \n \tunsigned int flags;\n };\n \n+/*\n+ * Move the iterator to the next record in the snapshot, without\n+ * respect for whether the record is actually required by the current\n+ * iteration. Adjust the fields in `iter` and return `ITER_OK` or\n+ * `ITER_DONE`. This function does not free the iterator in the case\n+ * of `ITER_DONE`.\n+ */\n static int next_record(struct packed_ref_iterator *iter)\n {\n \tconst char *p = iter->pos, *eol;\n@@ -680,12 +721,12 @@ static int next_record(struct packed_ref_iterator *iter)\n \tif (iter->eof - p < GIT_SHA1_HEXSZ + 2 ||\n \t parse_oid_hex(p, &iter->oid, &p) ||\n \t !isspace(*p++))\n-\t\tdie_invalid_line(iter->packed_refs->refs->path,\n+\t\tdie_invalid_line(iter->snapshot->refs->path,\n \t\t\t\t iter->pos, iter->eof - iter->pos);\n \n \teol = memchr(p, '\\n', iter->eof - p);\n \tif (!eol)\n-\t\tdie_unterminated_line(iter->packed_refs->refs->path,\n+\t\tdie_unterminated_line(iter->snapshot->refs->path,\n \t\t\t\t iter->pos, iter->eof - iter->pos);\n \n \tstrbuf_add(&iter->refname_buf, p, eol - p);\n@@ -698,8 +739,8 @@ static int next_record(struct packed_ref_iterator *iter)\n \t\toidclr(&iter->oid);\n \t\titer->base.flags |= REF_BAD_NAME | REF_ISBROKEN;\n \t}\n-\tif (iter->packed_refs->peeled == PEELED_FULLY ||\n-\t (iter->packed_refs->peeled == PEELED_TAGS &&\n+\tif (iter->snapshot->peeled == PEELED_FULLY ||\n+\t (iter->snapshot->peeled == PEELED_TAGS &&\n \t starts_with(iter->base.refname, \"refs/tags/\")))\n \t\titer->base.flags |= REF_KNOWS_PEELED;\n \n@@ -710,7 +751,7 @@ static int next_record(struct packed_ref_iterator *iter)\n \t\tif (iter->eof - p < GIT_SHA1_HEXSZ + 1 ||\n \t\t parse_oid_hex(p, &iter->peeled, &p) ||\n \t\t *p++ != '\\n')\n-\t\t\tdie_invalid_line(iter->packed_refs->refs->path,\n+\t\t\tdie_invalid_line(iter->snapshot->refs->path,\n \t\t\t\t\t iter->pos, iter->eof - iter->pos);\n \t\titer->pos = p;\n \n@@ -780,7 +821,7 @@ static int packed_ref_iterator_abort(struct ref_iterator *ref_iterator)\n \tint ok = ITER_DONE;\n \n \tstrbuf_release(&iter->refname_buf);\n-\trelease_packed_ref_cache(iter->packed_refs);\n+\trelease_snapshot(iter->snapshot);\n \tbase_ref_iterator_free(ref_iterator);\n \treturn ok;\n }\n@@ -796,7 +837,7 @@ static struct ref_iterator *packed_ref_iterator_begin(\n \t\tconst char *prefix, unsigned int flags)\n {\n \tstruct packed_ref_store *refs;\n-\tstruct packed_ref_cache *packed_refs;\n+\tstruct snapshot *snapshot;\n \tconst char *start;\n \tstruct packed_ref_iterator *iter;\n \tstruct ref_iterator *ref_iterator;\n@@ -806,30 +847,30 @@ static struct ref_iterator *packed_ref_iterator_begin(\n \t\trequired_flags |= REF_STORE_ODB;\n \trefs = packed_downcast(ref_store, required_flags, \"ref_iterator_begin\");\n \n-\tpacked_refs = get_packed_ref_cache(refs);\n+\t/*\n+\t * Note that `get_snapshot()` internally checks whether the\n+\t * snapshot is up to date with what is on disk, and re-reads\n+\t * it if not.\n+\t */\n+\tsnapshot = get_snapshot(refs);\n \n-\tif (!packed_refs->buf)\n+\tif (!snapshot->buf)\n \t\treturn empty_ref_iterator_begin();\n \n \titer = xcalloc(1, sizeof(*iter));\n \tref_iterator = &iter->base;\n \tbase_ref_iterator_init(ref_iterator, &packed_ref_iterator_vtable, 1);\n \n-\t/*\n-\t * Note that get_packed_ref_cache() internally checks whether\n-\t * the packed-ref cache is up to date with what is on disk,\n-\t * and re-reads it if not.\n-\t */\n-\titer->packed_refs = packed_refs;\n-\tacquire_packed_ref_cache(packed_refs);\n+\titer->snapshot = snapshot;\n+\tacquire_snapshot(snapshot);\n \n \tif (prefix && *prefix)\n-\t\tstart = find_reference_location(packed_refs, prefix, 0);\n+\t\tstart = find_reference_location(snapshot, prefix, 0);\n \telse\n-\t\tstart = packed_refs->buf + packed_refs->header_len;\n+\t\tstart = snapshot->buf + snapshot->header_len;\n \n \titer->pos = start;\n-\titer->eof = packed_refs->eof;\n+\titer->eof = snapshot->eof;\n \tstrbuf_init(&iter->refname_buf, 0);\n \n \titer->base.oid = &iter->oid;\n@@ -893,19 +934,19 @@ int packed_refs_lock(struct ref_store *ref_store, int flags, struct strbuf *err)\n \n \t/*\n \t * Now that we hold the `packed-refs` lock, make sure that our\n-\t * cache matches the current version of the file. Normally\n-\t * `get_packed_ref_cache()` does that for us, but that\n-\t * function assumes that when the file is locked, any existing\n-\t * cache is still valid. We've just locked the file, but it\n-\t * might have changed the moment *before* we locked it.\n+\t * snapshot matches the current version of the file. Normally\n+\t * `get_snapshot()` does that for us, but that function\n+\t * assumes that when the file is locked, any existing snapshot\n+\t * is still valid. We've just locked the file, but it might\n+\t * have changed the moment *before* we locked it.\n \t */\n-\tvalidate_packed_ref_cache(refs);\n+\tvalidate_snapshot(refs);\n \n \t/*\n \t * Now make sure that the packed-refs file as it exists in the\n-\t * locked state is loaded into the cache:\n+\t * locked state is loaded into the snapshot:\n \t */\n-\tget_packed_ref_cache(refs);\n+\tget_snapshot(refs);\n \treturn 0;\n }\n \n@@ -932,8 +973,8 @@ int packed_refs_is_locked(struct ref_store *ref_store)\n }\n \n /*\n- * The packed-refs header line that we write out. Perhaps other\n- * traits will be added later.\n+ * The packed-refs header line that we write out. Perhaps other traits\n+ * will be added later.\n *\n * Note that earlier versions of Git used to parse these traits by\n * looking for \" trait \" in the line. For this reason, the space after\n@@ -949,9 +990,9 @@ static int packed_init_db(struct ref_store *ref_store, struct strbuf *err)\n }\n \n /*\n- * Write the packed-refs from the cache to the packed-refs tempfile,\n- * incorporating any changes from `updates`. `updates` must be a\n- * sorted string list whose keys are the refnames and whose util\n+ * Write the packed refs from the current snapshot to the packed-refs\n+ * tempfile, incorporating any changes from `updates`. `updates` must\n+ * be a sorted string list whose keys are the refnames and whose util\n * values are `struct ref_update *`. On error, rollback the tempfile,\n * write an error message to `err`, and return a nonzero value.\n *\n@@ -1192,9 +1233,10 @@ static int packed_transaction_prepare(struct ref_store *ref_store,\n \t/*\n \t * Note that we *don't* skip transactions with zero updates,\n \t * because such a transaction might be executed for the side\n-\t * effect of ensuring that all of the references are peeled.\n-\t * If the caller wants to optimize away empty transactions, it\n-\t * should do so itself.\n+\t * effect of ensuring that all of the references are peeled or\n+\t * ensuring that the `packed-refs` file is sorted. If the\n+\t * caller wants to optimize away empty transactions, it should\n+\t * do so itself.\n \t */\n \n \tdata = xcalloc(1, sizeof(*data));\n@@ -1267,7 +1309,7 @@ static int packed_transaction_finish(struct ref_store *ref_store,\n \t\tgoto cleanup;\n \t}\n \n-\tclear_packed_ref_cache(refs);\n+\tclear_snapshot(refs);\n \tret = 0;\n \n cleanup:\n-- \n2.14.1\n\n"},{"id":"327966","messageId":"fa59662345ea577724d1c40fd8b7fdf7344cbd8e.1505319366.git.mhagger@alum.mit.edu","threadId":"46744","inReplyTo":"cover.1505319366.git.mhagger@alum.mit.edu","subject":"[PATCH 14/20] packed_ref_iterator_begin(): iterate using `mmapped_ref_iterator`","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-09-13T17:16:08Z","receivedAt":"2017-09-13T17:17:22Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Now that we have an efficient way to iterate, in order, over the\nmmapped contents of the `packed-refs` file, we can use that directly\nto implement reference iteration for the `packed_ref_store`, rather\nthan iterating over the `ref_cache`. This is the next step towards\ngetting rid of the `ref_cache` entirely.\n\nSigned-off-by: Michael Haggerty \n---\n refs/packed-backend.c | 109 ++++++++++++++++++++++++++++++++++++++++++++++++--\n 1 file changed, 106 insertions(+), 3 deletions(-)\n\ndiff --git a/refs/packed-backend.c b/refs/packed-backend.c\nindex 2a36dd9afb..a018a6c8ad 100644\n--- a/refs/packed-backend.c\n+++ b/refs/packed-backend.c\n@@ -372,6 +372,27 @@ static int cmp_packed_ref_entries(const void *v1, const void *v2)\n \t}\n }\n \n+/*\n+ * Compare a packed-refs record pointed to by `rec` to the specified\n+ * NUL-terminated refname.\n+ */\n+static int cmp_entry_to_refname(const char *rec, const char *refname)\n+{\n+\tconst char *r1 = rec + GIT_SHA1_HEXSZ + 1;\n+\tconst char *r2 = refname;\n+\n+\twhile (1) {\n+\t\tif (*r1 == '\\n')\n+\t\t\treturn *r2 ? -1 : 0;\n+\t\tif (!*r2)\n+\t\t\treturn 1;\n+\t\tif (*r1 != *r2)\n+\t\t\treturn (unsigned char)*r1 < (unsigned char)*r2 ? -1 : +1;\n+\t\tr1++;\n+\t\tr2++;\n+\t}\n+}\n+\n /*\n * `packed_refs->buf` is not known to be sorted. Check whether it is,\n * and if not, sort it into new memory and munmap/free the old\n@@ -478,6 +499,17 @@ static const char *find_start_of_record(const char *buf, const char *p)\n \treturn p;\n }\n \n+/*\n+ * Return a pointer to the start of the record following the record\n+ * that contains `*p`. If none is found before `end`, return `end`.\n+ */\n+static const char *find_end_of_record(const char *p, const char *end)\n+{\n+\twhile (++p < end && (p[-1] != '\\n' || p[0] == '^'))\n+\t\t;\n+\treturn p;\n+}\n+\n /*\n * We want to be able to compare mmapped reference records quickly,\n * without totally parsing them. We can do so because the records are\n@@ -511,6 +543,65 @@ static void verify_buffer_safe(struct packed_ref_cache *packed_refs)\n \t\t\t\t last_line, eof - last_line);\n }\n \n+/*\n+ * Find the place in `cache->buf` where the start of the record for\n+ * `refname` starts. If `mustexist` is true and the reference doesn't\n+ * exist, then return NULL. If `mustexist` is false and the reference\n+ * doesn't exist, then return the point where that reference would be\n+ * inserted. In the latter mode, `refname` doesn't have to be a proper\n+ * reference name; for example, one could search for \"refs/replace/\"\n+ * to find the start of any replace references.\n+ *\n+ * The record is sought using a binary search, so `cache->buf` must be\n+ * sorted.\n+ */\n+static const char *find_reference_location(struct packed_ref_cache *cache,\n+\t\t\t\t\t const char *refname, int mustexist)\n+{\n+\t/*\n+\t * This is not *quite* a garden-variety binary search, because\n+\t * the data we're searching is made up of records, and we\n+\t * always need to find the beginning of a record to do a\n+\t * comparison. A \"record\" here is one line for the reference\n+\t * itself and zero or one peel lines that start with '^'. Our\n+\t * loop invariant is described in the next two comments.\n+\t */\n+\n+\t/*\n+\t * A pointer to the character at the start of a record whose\n+\t * preceding records all have reference names that come\n+\t * *before* `refname`.\n+\t */\n+\tconst char *lo = cache->buf + cache->header_len;\n+\n+\t/*\n+\t * A pointer to a the first character of a record whose\n+\t * reference name comes *after* `refname`.\n+\t */\n+\tconst char *hi = cache->eof;\n+\n+\twhile (lo < hi) {\n+\t\tconst char *mid, *rec;\n+\t\tint cmp;\n+\n+\t\tmid = lo + (hi - lo) / 2;\n+\t\trec = find_start_of_record(lo, mid);\n+\t\tcmp = cmp_entry_to_refname(rec, refname);\n+\t\tif (cmp < 0) {\n+\t\t\tlo = find_end_of_record(mid, hi);\n+\t\t} else if (cmp > 0) {\n+\t\t\thi = rec;\n+\t\t} else {\n+\t\t\treturn rec;\n+\t\t}\n+\t}\n+\n+\tif (mustexist)\n+\t\treturn NULL;\n+\telse\n+\t\treturn lo;\n+}\n+\n /*\n * Read from the `packed-refs` file into a newly-allocated\n * `packed_ref_cache` and return it. The return value will already\n@@ -818,6 +909,8 @@ static struct ref_iterator *packed_ref_iterator_begin(\n \t\tconst char *prefix, unsigned int flags)\n {\n \tstruct packed_ref_store *refs;\n+\tstruct packed_ref_cache *packed_refs;\n+\tconst char *start;\n \tstruct packed_ref_iterator *iter;\n \tstruct ref_iterator *ref_iterator;\n \tunsigned int required_flags = REF_STORE_READ;\n@@ -835,13 +928,23 @@ static struct ref_iterator *packed_ref_iterator_begin(\n \t * the packed-ref cache is up to date with what is on disk,\n \t * and re-reads it if not.\n \t */\n+\titer->cache = packed_refs = get_packed_ref_cache(refs);\n+\tacquire_packed_ref_cache(packed_refs);\n \n-\titer->cache = get_packed_ref_cache(refs);\n-\tacquire_packed_ref_cache(iter->cache);\n-\titer->iter0 = cache_ref_iterator_begin(iter->cache->cache, prefix, 0);\n+\tif (prefix && *prefix)\n+\t\tstart = find_reference_location(packed_refs, prefix, 0);\n+\telse\n+\t\tstart = packed_refs->buf + packed_refs->header_len;\n+\n+\titer->iter0 = mmapped_ref_iterator_begin(\n+\t\t\tpacked_refs, start, packed_refs->eof);\n \n \titer->flags = flags;\n \n+\tif (prefix && *prefix)\n+\t\t/* Stop iteration after we've gone *past* prefix: */\n+\t\tref_iterator = prefix_ref_iterator_begin(ref_iterator, prefix, 0);\n+\n \treturn ref_iterator;\n }\n \n-- \n2.14.1\n\n"},{"id":"327967","messageId":"01effbfa93b3e0b99aa0c36683fa0f06f4917240.1505319366.git.mhagger@alum.mit.edu","threadId":"46744","inReplyTo":"cover.1505319366.git.mhagger@alum.mit.edu","subject":"[PATCH 18/20] ref_cache: remove support for storing peeled values","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-09-13T17:16:12Z","receivedAt":"2017-09-13T17:17:26Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Now that the `packed-refs` backend doesn't use `ref_cache`, there is\nnobody left who might want to store peeled values of references in\n`ref_cache`. So remove that feature.\n\nSigned-off-by: Michael Haggerty \n---\n refs/packed-backend.c | 9 ++++++++-\n refs/ref-cache.c | 42 +-----------------------------------------\n refs/ref-cache.h | 32 ++------------------------------\n 3 files changed, 11 insertions(+), 72 deletions(-)\n\ndiff --git a/refs/packed-backend.c b/refs/packed-backend.c\nindex 9868a5c198..d76051c7f5 100644\n--- a/refs/packed-backend.c\n+++ b/refs/packed-backend.c\n@@ -2,7 +2,6 @@\n #include \"../config.h\"\n #include \"../refs.h\"\n #include \"refs-internal.h\"\n-#include \"ref-cache.h\"\n #include \"packed-backend.h\"\n #include \"../iterator.h\"\n #include \"../lockfile.h\"\n@@ -201,6 +200,14 @@ static NORETURN void die_invalid_line(const char *path,\n \n }\n \n+/*\n+ * This value is set in `base.flags` if the peeled value of the\n+ * current reference is known. In that case, `peeled` contains the\n+ * correct peeled value for the reference, which might be `null_sha1`\n+ * if the reference is not a tag or if it is broken.\n+ */\n+#define REF_KNOWS_PEELED 0x40\n+\n /*\n * An iterator over a packed-refs file that is currently mmapped.\n */\ndiff --git a/refs/ref-cache.c b/refs/ref-cache.c\nindex 54dfb5218c..4f850e1b5c 100644\n--- a/refs/ref-cache.c\n+++ b/refs/ref-cache.c\n@@ -38,7 +38,6 @@ struct ref_entry *create_ref_entry(const char *refname,\n \n \tFLEX_ALLOC_STR(ref, name, refname);\n \toidcpy(&ref->u.value.oid, oid);\n-\toidclr(&ref->u.value.peeled);\n \tref->flag = flag;\n \treturn ref;\n }\n@@ -491,49 +490,10 @@ static int cache_ref_iterator_advance(struct ref_iterator *ref_iterator)\n \t}\n }\n \n-enum peel_status peel_entry(struct ref_entry *entry, int repeel)\n-{\n-\tenum peel_status status;\n-\n-\tif (entry->flag & REF_KNOWS_PEELED) {\n-\t\tif (repeel) {\n-\t\t\tentry->flag &= ~REF_KNOWS_PEELED;\n-\t\t\toidclr(&entry->u.value.peeled);\n-\t\t} else {\n-\t\t\treturn is_null_oid(&entry->u.value.peeled) ?\n-\t\t\t\tPEEL_NON_TAG : PEEL_PEELED;\n-\t\t}\n-\t}\n-\tif (entry->flag & REF_ISBROKEN)\n-\t\treturn PEEL_BROKEN;\n-\tif (entry->flag & REF_ISSYMREF)\n-\t\treturn PEEL_IS_SYMREF;\n-\n-\tstatus = peel_object(entry->u.value.oid.hash, entry->u.value.peeled.hash);\n-\tif (status == PEEL_PEELED || status == PEEL_NON_TAG)\n-\t\tentry->flag |= REF_KNOWS_PEELED;\n-\treturn status;\n-}\n-\n static int cache_ref_iterator_peel(struct ref_iterator *ref_iterator,\n \t\t\t\t struct object_id *peeled)\n {\n-\tstruct cache_ref_iterator *iter =\n-\t\t(struct cache_ref_iterator *)ref_iterator;\n-\tstruct cache_ref_iterator_level *level;\n-\tstruct ref_entry *entry;\n-\n-\tlevel = &iter->levels[iter->levels_nr - 1];\n-\n-\tif (level->index == -1)\n-\t\tdie(\"BUG: peel called before advance for cache iterator\");\n-\n-\tentry = level->dir->entries[level->index];\n-\n-\tif (peel_entry(entry, 0))\n-\t\treturn -1;\n-\toidcpy(peeled, &entry->u.value.peeled);\n-\treturn 0;\n+\treturn peel_object(ref_iterator->oid->hash, peeled->hash);\n }\n \n static int cache_ref_iterator_abort(struct ref_iterator *ref_iterator)\ndiff --git a/refs/ref-cache.h b/refs/ref-cache.h\nindex a082bfb06c..eda65e73ed 100644\n--- a/refs/ref-cache.h\n+++ b/refs/ref-cache.h\n@@ -38,14 +38,6 @@ struct ref_value {\n \t * referred to by the last reference in the symlink chain.\n \t */\n \tstruct object_id oid;\n-\n-\t/*\n-\t * If REF_KNOWS_PEELED, then this field holds the peeled value\n-\t * of this reference, or null if the reference is known not to\n-\t * be peelable. See the documentation for peel_ref() for an\n-\t * exact definition of \"peelable\".\n-\t */\n-\tstruct object_id peeled;\n };\n \n /*\n@@ -97,21 +89,14 @@ struct ref_dir {\n * public values; see refs.h.\n */\n \n-/*\n- * The field ref_entry->u.value.peeled of this value entry contains\n- * the correct peeled value for the reference, which might be\n- * null_sha1 if the reference is not a tag or if it is broken.\n- */\n-#define REF_KNOWS_PEELED 0x10\n-\n /* ref_entry represents a directory of references */\n-#define REF_DIR 0x20\n+#define REF_DIR 0x10\n \n /*\n * Entry has not yet been read from disk (used only for REF_DIR\n * entries representing loose references)\n */\n-#define REF_INCOMPLETE 0x40\n+#define REF_INCOMPLETE 0x20\n \n /*\n * A ref_entry represents either a reference or a \"subdirectory\" of\n@@ -252,17 +237,4 @@ struct ref_iterator *cache_ref_iterator_begin(struct ref_cache *cache,\n \t\t\t\t\t const char *prefix,\n \t\t\t\t\t int prime_dir);\n \n-/*\n- * Peel the entry (if possible) and return its new peel_status. If\n- * repeel is true, re-peel the entry even if there is an old peeled\n- * value that is already stored in it.\n- *\n- * It is OK to call this function with a packed reference entry that\n- * might be stale and might even refer to an object that has since\n- * been garbage-collected. In such a case, if the entry has\n- * REF_KNOWS_PEELED then leave the status unchanged and return\n- * PEEL_PEELED or PEEL_NON_TAG; otherwise, return PEEL_INVALID.\n- */\n-enum peel_status peel_entry(struct ref_entry *entry, int repeel);\n-\n #endif /* REFS_REF_CACHE_H */\n-- \n2.14.1\n\n"},{"id":"327968","messageId":"aab75b5a61b5b71248267d29c04254b60f4ae385.1505319366.git.mhagger@alum.mit.edu","threadId":"46744","inReplyTo":"cover.1505319366.git.mhagger@alum.mit.edu","subject":"[PATCH 10/20] mmapped_ref_iterator: add iterator over a packed-refs file","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-09-13T17:16:04Z","receivedAt":"2017-09-13T17:17:30Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Add a new `mmapped_ref_iterator`, which can iterate over the\nreferences in an mmapped `packed-refs` file directly. Use this\niterator from `read_packed_refs()` to fill the packed refs cache.\n\nNote that we are not yet willing to promise that the new iterator\ngenerates its output in order. That doesn't matter for now, because\nthe packed refs cache doesn't care what order it is filled.\n\nThis change adds a lot of boilerplate without providing any obvious\nbenefits. The benefits will come soon, when we get rid of the\n`ref_cache` for packed references altogether.\n\nSigned-off-by: Michael Haggerty \n---\n refs/packed-backend.c | 207 ++++++++++++++++++++++++++++++++++++--------------\n 1 file changed, 152 insertions(+), 55 deletions(-)\n\ndiff --git a/refs/packed-backend.c b/refs/packed-backend.c\nindex ae276f3445..312116a99d 100644\n--- a/refs/packed-backend.c\n+++ b/refs/packed-backend.c\n@@ -163,6 +163,141 @@ static NORETURN void die_invalid_line(const char *path,\n \n }\n \n+/*\n+ * An iterator over a packed-refs file that is currently mmapped.\n+ */\n+struct mmapped_ref_iterator {\n+\tstruct ref_iterator base;\n+\n+\tstruct packed_ref_cache *packed_refs;\n+\n+\t/* The current position in the mmapped file: */\n+\tconst char *pos;\n+\n+\t/* The end of the mmapped file: */\n+\tconst char *eof;\n+\n+\tstruct object_id oid, peeled;\n+\n+\tstruct strbuf refname_buf;\n+};\n+\n+static int mmapped_ref_iterator_advance(struct ref_iterator *ref_iterator)\n+{\n+\tstruct mmapped_ref_iterator *iter =\n+\t\t(struct mmapped_ref_iterator *)ref_iterator;\n+\tconst char *p = iter->pos, *eol;\n+\n+\tstrbuf_reset(&iter->refname_buf);\n+\n+\tif (iter->pos == iter->eof)\n+\t\treturn ref_iterator_abort(ref_iterator);\n+\n+\titer->base.flags = REF_ISPACKED;\n+\n+\tif (iter->eof - p < GIT_SHA1_HEXSZ + 2 ||\n+\t parse_oid_hex(p, &iter->oid, &p) ||\n+\t !isspace(*p++))\n+\t\tdie_invalid_line(iter->packed_refs->refs->path,\n+\t\t\t\t iter->pos, iter->eof - iter->pos);\n+\n+\teol = memchr(p, '\\n', iter->eof - p);\n+\tif (!eol)\n+\t\tdie_unterminated_line(iter->packed_refs->refs->path,\n+\t\t\t\t iter->pos, iter->eof - iter->pos);\n+\n+\tstrbuf_add(&iter->refname_buf, p, eol - p);\n+\titer->base.refname = iter->refname_buf.buf;\n+\n+\tif (check_refname_format(iter->base.refname, REFNAME_ALLOW_ONELEVEL)) {\n+\t\tif (!refname_is_safe(iter->base.refname))\n+\t\t\tdie(\"packed refname is dangerous: %s\",\n+\t\t\t iter->base.refname);\n+\t\toidclr(&iter->oid);\n+\t\titer->base.flags |= REF_BAD_NAME | REF_ISBROKEN;\n+\t}\n+\tif (iter->packed_refs->peeled == PEELED_FULLY ||\n+\t (iter->packed_refs->peeled == PEELED_TAGS &&\n+\t starts_with(iter->base.refname, \"refs/tags/\")))\n+\t\titer->base.flags |= REF_KNOWS_PEELED;\n+\n+\titer->pos = eol + 1;\n+\n+\tif (iter->pos < iter->eof && *iter->pos == '^') {\n+\t\tp = iter->pos + 1;\n+\t\tif (iter->eof - p < GIT_SHA1_HEXSZ + 1 ||\n+\t\t parse_oid_hex(p, &iter->peeled, &p) ||\n+\t\t *p++ != '\\n')\n+\t\t\tdie_invalid_line(iter->packed_refs->refs->path,\n+\t\t\t\t\t iter->pos, iter->eof - iter->pos);\n+\t\titer->pos = p;\n+\n+\t\t/*\n+\t\t * Regardless of what the file header said, we\n+\t\t * definitely know the value of *this* reference:\n+\t\t */\n+\t\titer->base.flags |= REF_KNOWS_PEELED;\n+\t} else {\n+\t\toidclr(&iter->peeled);\n+\t}\n+\n+\treturn ITER_OK;\n+}\n+\n+static int mmapped_ref_iterator_peel(struct ref_iterator *ref_iterator,\n+\t\t\t\t struct object_id *peeled)\n+{\n+\tstruct mmapped_ref_iterator *iter =\n+\t\t(struct mmapped_ref_iterator *)ref_iterator;\n+\n+\tif ((iter->base.flags & REF_KNOWS_PEELED)) {\n+\t\toidcpy(peeled, &iter->peeled);\n+\t\treturn is_null_oid(&iter->peeled) ? -1 : 0;\n+\t} else if ((iter->base.flags & (REF_ISBROKEN | REF_ISSYMREF))) {\n+\t\treturn -1;\n+\t} else {\n+\t\treturn !!peel_object(iter->oid.hash, peeled->hash);\n+\t}\n+}\n+\n+static int mmapped_ref_iterator_abort(struct ref_iterator *ref_iterator)\n+{\n+\tstruct mmapped_ref_iterator *iter =\n+\t\t(struct mmapped_ref_iterator *)ref_iterator;\n+\n+\trelease_packed_ref_cache(iter->packed_refs);\n+\tstrbuf_release(&iter->refname_buf);\n+\tbase_ref_iterator_free(ref_iterator);\n+\treturn ITER_DONE;\n+}\n+\n+static struct ref_iterator_vtable mmapped_ref_iterator_vtable = {\n+\tmmapped_ref_iterator_advance,\n+\tmmapped_ref_iterator_peel,\n+\tmmapped_ref_iterator_abort\n+};\n+\n+struct ref_iterator *mmapped_ref_iterator_begin(\n+\t\tconst char *packed_refs_file,\n+\t\tstruct packed_ref_cache *packed_refs,\n+\t\tconst char *pos, const char *eof)\n+{\n+\tstruct mmapped_ref_iterator *iter = xcalloc(1, sizeof(*iter));\n+\tstruct ref_iterator *ref_iterator = &iter->base;\n+\n+\tbase_ref_iterator_init(ref_iterator, &mmapped_ref_iterator_vtable, 0);\n+\n+\titer->packed_refs = packed_refs;\n+\tacquire_packed_ref_cache(iter->packed_refs);\n+\titer->pos = pos;\n+\titer->eof = eof;\n+\tstrbuf_init(&iter->refname_buf, 0);\n+\n+\titer->base.oid = &iter->oid;\n+\n+\treturn ref_iterator;\n+}\n+\n /*\n * Read from the `packed-refs` file into a newly-allocated\n * `packed_ref_cache` and return it. The return value will already\n@@ -199,9 +334,10 @@ static struct packed_ref_cache *read_packed_refs(struct packed_ref_store *refs)\n \tstruct stat st;\n \tsize_t size;\n \tchar *buf;\n-\tconst char *pos, *eol, *eof;\n-\tstruct strbuf tmp = STRBUF_INIT;\n+\tconst char *pos, *eof;\n \tstruct ref_dir *dir;\n+\tstruct ref_iterator *iter;\n+\tint ok;\n \n \tpacked_refs->refs = refs;\n \tacquire_packed_ref_cache(packed_refs);\n@@ -235,7 +371,9 @@ static struct packed_ref_cache *read_packed_refs(struct packed_ref_store *refs)\n \n \t/* If the file has a header line, process it: */\n \tif (pos < eof && *pos == '#') {\n+\t\tstruct strbuf tmp = STRBUF_INIT;\n \t\tchar *p;\n+\t\tconst char *eol;\n \t\tstruct string_list traits = STRING_LIST_INIT_NODUP;\n \n \t\teol = memchr(pos, '\\n', eof - pos);\n@@ -259,69 +397,28 @@ static struct packed_ref_cache *read_packed_refs(struct packed_ref_store *refs)\n \t\tpos = eol + 1;\n \n \t\tstring_list_clear(&traits, 0);\n-\t\tstrbuf_reset(&tmp);\n+\t\tstrbuf_release(&tmp);\n \t}\n \n \tdir = get_ref_dir(packed_refs->cache->root);\n-\twhile (pos < eof) {\n-\t\tconst char *p = pos;\n-\t\tstruct object_id oid;\n-\t\tconst char *refname;\n-\t\tint flag = REF_ISPACKED;\n-\t\tstruct ref_entry *entry = NULL;\n-\n-\t\tif (eof - pos < GIT_SHA1_HEXSZ + 2 ||\n-\t\t parse_oid_hex(p, &oid, &p) ||\n-\t\t !isspace(*p++))\n-\t\t\tdie_invalid_line(refs->path, pos, eof - pos);\n+\titer = mmapped_ref_iterator_begin(refs->path, packed_refs, pos, eof);\n+\twhile ((ok = ref_iterator_advance(iter)) == ITER_OK) {\n+\t\tstruct ref_entry *entry =\n+\t\t\tcreate_ref_entry(iter->refname, iter->oid, iter->flags);\n \n-\t\teol = memchr(p, '\\n', eof - p);\n-\t\tif (!eol)\n-\t\t\tdie_unterminated_line(refs->path, pos, eof - pos);\n-\n-\t\tstrbuf_add(&tmp, p, eol - p);\n-\t\trefname = tmp.buf;\n-\n-\t\tif (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL)) {\n-\t\t\tif (!refname_is_safe(refname))\n-\t\t\t\tdie(\"packed refname is dangerous: %s\", refname);\n-\t\t\toidclr(&oid);\n-\t\t\tflag |= REF_BAD_NAME | REF_ISBROKEN;\n-\t\t}\n-\t\tif (packed_refs->peeled == PEELED_FULLY ||\n-\t\t (packed_refs->peeled == PEELED_TAGS &&\n-\t\t starts_with(refname, \"refs/tags/\")))\n-\t\t\tflag |= REF_KNOWS_PEELED;\n-\t\tentry = create_ref_entry(refname, &oid, flag);\n+\t\tif ((iter->flags & REF_KNOWS_PEELED))\n+\t\t\tref_iterator_peel(iter, &entry->u.value.peeled);\n \t\tadd_ref_entry(dir, entry);\n-\n-\t\tpos = eol + 1;\n-\n-\t\tif (pos < eof && *pos == '^') {\n-\t\t\tp = pos + 1;\n-\t\t\tif (eof - p < GIT_SHA1_HEXSZ + 1 ||\n-\t\t\t parse_oid_hex(p, &entry->u.value.peeled, &p) ||\n-\t\t\t *p++ != '\\n')\n-\t\t\t\tdie_invalid_line(refs->path, pos, eof - pos);\n-\n-\t\t\t/*\n-\t\t\t * Regardless of what the file header said,\n-\t\t\t * we definitely know the value of *this*\n-\t\t\t * reference:\n-\t\t\t */\n-\t\t\tentry->flag |= REF_KNOWS_PEELED;\n-\n-\t\t\tpos = p;\n-\t\t}\n-\n-\t\tstrbuf_reset(&tmp);\n \t}\n \n+\tif (ok != ITER_DONE)\n+\t\tdie(\"error reading packed-refs file %s\", refs->path);\n+\n \tif (munmap(buf, size))\n-\t\tdie_errno(\"error ummapping packed-refs file\");\n+\t\tdie_errno(\"error ummapping packed-refs file %s\", refs->path);\n+\n \tclose(fd);\n \n-\tstrbuf_release(&tmp);\n \treturn packed_refs;\n }\n \n-- \n2.14.1\n\n"},{"id":"327969","messageId":"30512c0e9071af73af34fbdde8b591b07161fdc8.1505319366.git.mhagger@alum.mit.edu","threadId":"46744","inReplyTo":"cover.1505319366.git.mhagger@alum.mit.edu","subject":"[PATCH 15/20] packed_read_raw_ref(): read the reference from the mmapped buffer","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-09-13T17:16:09Z","receivedAt":"2017-09-13T17:17:34Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Instead of reading the reference from the `ref_cache`, read it\ndirectly from the mmapped buffer.\n\nSigned-off-by: Michael Haggerty \n---\n refs/packed-backend.c | 14 +++++++++-----\n 1 file changed, 9 insertions(+), 5 deletions(-)\n\ndiff --git a/refs/packed-backend.c b/refs/packed-backend.c\nindex a018a6c8ad..9b10532eb3 100644\n--- a/refs/packed-backend.c\n+++ b/refs/packed-backend.c\n@@ -806,18 +806,22 @@ static int packed_read_raw_ref(struct ref_store *ref_store,\n {\n \tstruct packed_ref_store *refs =\n \t\tpacked_downcast(ref_store, REF_STORE_READ, \"read_raw_ref\");\n-\n-\tstruct ref_entry *entry;\n+\tstruct packed_ref_cache *packed_refs = get_packed_ref_cache(refs);\n+\tconst char *rec;\n \n \t*type = 0;\n \n-\tentry = get_packed_ref(refs, refname);\n-\tif (!entry) {\n+\trec = find_reference_location(packed_refs, refname, 1);\n+\n+\tif (!rec) {\n+\t\t/* refname is not a packed reference. */\n \t\terrno = ENOENT;\n \t\treturn -1;\n \t}\n \n-\thashcpy(sha1, entry->u.value.oid.hash);\n+\tif (get_sha1_hex(rec, sha1))\n+\t\tdie_invalid_line(refs->path, rec, packed_refs->eof - rec);\n+\n \t*type = REF_ISPACKED;\n \treturn 0;\n }\n-- \n2.14.1\n\n"},{"id":"327970","messageId":"b3eba02393910890a29e683a0e840b044fd8c311.1505319366.git.mhagger@alum.mit.edu","threadId":"46744","inReplyTo":"cover.1505319366.git.mhagger@alum.mit.edu","subject":"[PATCH 16/20] ref_store: implement `refs_peel_ref()` generically","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-09-13T17:16:10Z","receivedAt":"2017-09-13T17:17:37Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"We're about to stop storing packed refs in a `ref_cache`. That means\nthat the only way we have left to optimize `peel_ref()` is by checking\nwhether the reference being peeled is the one currently being iterated\nover (in `current_ref_iter`), and if so, using `ref_iterator_peel()`.\nBut this can be done generically; it doesn't have to be implemented\nper-backend.\n\nSo implement `refs_peel_ref()` in `refs.c` and remove the `peel_ref()`\nmethod from the refs API.\n\nThis removes the last callers of a couple of functions, so delete\nthem. More cleanup to come...\n\nSigned-off-by: Michael Haggerty \n---\n refs.c | 18 +++++++++++++++++-\n refs/files-backend.c | 38 --------------------------------------\n refs/packed-backend.c | 36 ------------------------------------\n refs/refs-internal.h | 3 ---\n 4 files changed, 17 insertions(+), 78 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 101c107ee8..c5e6f79c77 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1735,7 +1735,23 @@ int refs_pack_refs(struct ref_store *refs, unsigned int flags)\n int refs_peel_ref(struct ref_store *refs, const char *refname,\n \t\t unsigned char *sha1)\n {\n-\treturn refs->be->peel_ref(refs, refname, sha1);\n+\tint flag;\n+\tunsigned char base[20];\n+\n+\tif (current_ref_iter && current_ref_iter->refname == refname) {\n+\t\tstruct object_id peeled;\n+\n+\t\tif (ref_iterator_peel(current_ref_iter, &peeled))\n+\t\t\treturn -1;\n+\t\thashcpy(sha1, peeled.hash);\n+\t\treturn 0;\n+\t}\n+\n+\tif (refs_read_ref_full(refs, refname,\n+\t\t\t RESOLVE_REF_READING, base, &flag))\n+\t\treturn -1;\n+\n+\treturn peel_object(base, sha1);\n }\n \n int peel_ref(const char *refname, unsigned char *sha1)\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 35648c89fc..7d12de88d0 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -655,43 +655,6 @@ static int lock_raw_ref(struct files_ref_store *refs,\n \treturn ret;\n }\n \n-static int files_peel_ref(struct ref_store *ref_store,\n-\t\t\t const char *refname, unsigned char *sha1)\n-{\n-\tstruct files_ref_store *refs =\n-\t\tfiles_downcast(ref_store, REF_STORE_READ | REF_STORE_ODB,\n-\t\t\t \"peel_ref\");\n-\tint flag;\n-\tunsigned char base[20];\n-\n-\tif (current_ref_iter && current_ref_iter->refname == refname) {\n-\t\tstruct object_id peeled;\n-\n-\t\tif (ref_iterator_peel(current_ref_iter, &peeled))\n-\t\t\treturn -1;\n-\t\thashcpy(sha1, peeled.hash);\n-\t\treturn 0;\n-\t}\n-\n-\tif (refs_read_ref_full(ref_store, refname,\n-\t\t\t RESOLVE_REF_READING, base, &flag))\n-\t\treturn -1;\n-\n-\t/*\n-\t * If the reference is packed, read its ref_entry from the\n-\t * cache in the hope that we already know its peeled value.\n-\t * We only try this optimization on packed references because\n-\t * (a) forcing the filling of the loose reference cache could\n-\t * be expensive and (b) loose references anyway usually do not\n-\t * have REF_KNOWS_PEELED.\n-\t */\n-\tif (flag & REF_ISPACKED &&\n-\t !refs_peel_ref(refs->packed_ref_store, refname, sha1))\n-\t\treturn 0;\n-\n-\treturn peel_object(base, sha1);\n-}\n-\n struct files_ref_iterator {\n \tstruct ref_iterator base;\n \n@@ -3012,7 +2975,6 @@ struct ref_storage_be refs_be_files = {\n \tfiles_initial_transaction_commit,\n \n \tfiles_pack_refs,\n-\tfiles_peel_ref,\n \tfiles_create_symref,\n \tfiles_delete_refs,\n \tfiles_rename_ref,\ndiff --git a/refs/packed-backend.c b/refs/packed-backend.c\nindex 9b10532eb3..88242a49fe 100644\n--- a/refs/packed-backend.c\n+++ b/refs/packed-backend.c\n@@ -780,26 +780,6 @@ static struct packed_ref_cache *get_packed_ref_cache(struct packed_ref_store *re\n \treturn refs->cache;\n }\n \n-static struct ref_dir *get_packed_ref_dir(struct packed_ref_cache *packed_ref_cache)\n-{\n-\treturn get_ref_dir(packed_ref_cache->cache->root);\n-}\n-\n-static struct ref_dir *get_packed_refs(struct packed_ref_store *refs)\n-{\n-\treturn get_packed_ref_dir(get_packed_ref_cache(refs));\n-}\n-\n-/*\n- * Return the ref_entry for the given refname from the packed\n- * references. If it does not exist, return NULL.\n- */\n-static struct ref_entry *get_packed_ref(struct packed_ref_store *refs,\n-\t\t\t\t\tconst char *refname)\n-{\n-\treturn find_ref_entry(get_packed_refs(refs), refname);\n-}\n-\n static int packed_read_raw_ref(struct ref_store *ref_store,\n \t\t\t const char *refname, unsigned char *sha1,\n \t\t\t struct strbuf *referent, unsigned int *type)\n@@ -826,21 +806,6 @@ static int packed_read_raw_ref(struct ref_store *ref_store,\n \treturn 0;\n }\n \n-static int packed_peel_ref(struct ref_store *ref_store,\n-\t\t\t const char *refname, unsigned char *sha1)\n-{\n-\tstruct packed_ref_store *refs =\n-\t\tpacked_downcast(ref_store, REF_STORE_READ | REF_STORE_ODB,\n-\t\t\t\t\"peel_ref\");\n-\tstruct ref_entry *r = get_packed_ref(refs, refname);\n-\n-\tif (!r || peel_entry(r, 0))\n-\t\treturn -1;\n-\n-\thashcpy(sha1, r->u.value.peeled.hash);\n-\treturn 0;\n-}\n-\n struct packed_ref_iterator {\n \tstruct ref_iterator base;\n \n@@ -1526,7 +1491,6 @@ struct ref_storage_be refs_be_packed = {\n \tpacked_initial_transaction_commit,\n \n \tpacked_pack_refs,\n-\tpacked_peel_ref,\n \tpacked_create_symref,\n \tpacked_delete_refs,\n \tpacked_rename_ref,\ndiff --git a/refs/refs-internal.h b/refs/refs-internal.h\nindex d7f233beba..cc6c373f59 100644\n--- a/refs/refs-internal.h\n+++ b/refs/refs-internal.h\n@@ -562,8 +562,6 @@ typedef int ref_transaction_commit_fn(struct ref_store *refs,\n \t\t\t\t struct strbuf *err);\n \n typedef int pack_refs_fn(struct ref_store *ref_store, unsigned int flags);\n-typedef int peel_ref_fn(struct ref_store *ref_store,\n-\t\t\tconst char *refname, unsigned char *sha1);\n typedef int create_symref_fn(struct ref_store *ref_store,\n \t\t\t const char *ref_target,\n \t\t\t const char *refs_heads_master,\n@@ -668,7 +666,6 @@ struct ref_storage_be {\n \tref_transaction_commit_fn *initial_transaction_commit;\n \n \tpack_refs_fn *pack_refs;\n-\tpeel_ref_fn *peel_ref;\n \tcreate_symref_fn *create_symref;\n \tdelete_refs_fn *delete_refs;\n \trename_ref_fn *rename_ref;\n-- \n2.14.1\n\n"},{"id":"327972","messageId":"7d40a319b3c41577efee8ef9f5c426b7517751ba.1505319366.git.mhagger@alum.mit.edu","threadId":"46744","inReplyTo":"cover.1505319366.git.mhagger@alum.mit.edu","subject":"[PATCH 04/20] die_unterminated_line(), die_invalid_line(): new functions","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-09-13T17:15:58Z","receivedAt":"2017-09-13T17:17:47Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Extract some helper functions for reporting errors. While we're at it,\nprevent them from spewing unlimited output to the terminal. These\nfunctions will soon have more callers.\n\nThese functions accept the problematic line as a `(ptr, len)` pair\nrather than a NUL-terminated string, and `die_invalid_line()` checks\nfor an EOL itself, because these calling conventions will be\nconvenient for future callers. (Efficiency is not a concern here\nbecause these functions are only ever called if the `packed-refs` file\nis corrupt.)\n\nSigned-off-by: Michael Haggerty \n---\n refs/packed-backend.c | 28 +++++++++++++++++++++++++---\n 1 file changed, 25 insertions(+), 3 deletions(-)\n\ndiff --git a/refs/packed-backend.c b/refs/packed-backend.c\nindex a3d9210cb0..5c50c223ef 100644\n--- a/refs/packed-backend.c\n+++ b/refs/packed-backend.c\n@@ -161,6 +161,29 @@ static const char *parse_ref_line(struct strbuf *line, struct object_id *oid)\n \treturn ref;\n }\n \n+static NORETURN void die_unterminated_line(const char *path,\n+\t\t\t\t\t const char *p, size_t len)\n+{\n+\tif (len < 80)\n+\t\tdie(\"unterminated line in %s: %.*s\", path, (int)len, p);\n+\telse\n+\t\tdie(\"unterminated line in %s: %.75s...\", path, p);\n+}\n+\n+static NORETURN void die_invalid_line(const char *path,\n+\t\t\t\t const char *p, size_t len)\n+{\n+\tconst char *eol = memchr(p, '\\n', len);\n+\n+\tif (!eol)\n+\t\tdie_unterminated_line(path, p, len);\n+\telse if (eol - p < 80)\n+\t\tdie(\"unexpected line in %s: %.*s\", path, (int)(eol - p), p);\n+\telse\n+\t\tdie(\"unexpected line in %s: %.75s...\", path, p);\n+\n+}\n+\n /*\n * Read from the `packed-refs` file into a newly-allocated\n * `packed_ref_cache` and return it. The return value will already\n@@ -227,7 +250,7 @@ static struct packed_ref_cache *read_packed_refs(struct packed_ref_store *refs)\n \t\tconst char *traits;\n \n \t\tif (!line.len || line.buf[line.len - 1] != '\\n')\n-\t\t\tdie(\"unterminated line in %s: %s\", refs->path, line.buf);\n+\t\t\tdie_unterminated_line(refs->path, line.buf, line.len);\n \n \t\tif (skip_prefix(line.buf, \"# pack-refs with:\", &traits)) {\n \t\t\tif (strstr(traits, \" fully-peeled \"))\n@@ -266,8 +289,7 @@ static struct packed_ref_cache *read_packed_refs(struct packed_ref_store *refs)\n \t\t\t */\n \t\t\tlast->flag |= REF_KNOWS_PEELED;\n \t\t} else {\n-\t\t\tstrbuf_setlen(&line, line.len - 1);\n-\t\t\tdie(\"unexpected line in %s: %s\", refs->path, line.buf);\n+\t\t\tdie_invalid_line(refs->path, line.buf, line.len);\n \t\t}\n \t}\n \n-- \n2.14.1\n\n"},{"id":"327973","messageId":"ff65c485aef01b35d8ab82fb785923384f4d68ca.1505319366.git.mhagger@alum.mit.edu","threadId":"46744","inReplyTo":"cover.1505319366.git.mhagger@alum.mit.edu","subject":"[PATCH 13/20] read_packed_refs(): ensure that references are ordered when read","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-09-13T17:16:07Z","receivedAt":"2017-09-13T17:17:51Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"It doesn't actually matter now, because the references are only\niterated over to fill the associated `ref_cache`, which itself puts\nthem in the correct order. But we want to get rid of the `ref_cache`,\nso we want to be able to iterate directly over the `packed-refs`\nbuffer, and then the iteration will need to be ordered correctly.\n\nIn fact, we already write the `packed-refs` file sorted, but it is\npossible that other Git clients don't get it right. So let's not\nassume that a `packed-refs` file is sorted unless it is explicitly\ndeclared to be so via a `sorted` trait in its header line.\n\nIf it is *not* declared to be sorted, then scan quickly through the\nfile to check. If it is found to be out of order, then sort the\nrecords into a new memory-only copy rather than using the mmapped file\ncontents. This checking and sorting is done quickly, without parsing\nthe full file contents. However, it needs a little bit of care to\navoid reading past the end of the buffer even if the `packed-refs`\nfile is corrupt.\n\nIf `packed-refs` is sorted (i.e., its file header contains the\n`sorted` trait or our check shows that it is), then use the mmapped\nfile contents directly.\n\nSince we write the file correctly sorted, include that trait when we\nwrite or rewrite a `packed-refs` file.\n\nSigned-off-by: Michael Haggerty \n---\n refs/packed-backend.c | 222 ++++++++++++++++++++++++++++++++++++++++++++++----\n 1 file changed, 206 insertions(+), 16 deletions(-)\n\ndiff --git a/refs/packed-backend.c b/refs/packed-backend.c\nindex d209c8e5a0..2a36dd9afb 100644\n--- a/refs/packed-backend.c\n+++ b/refs/packed-backend.c\n@@ -25,9 +25,12 @@ struct packed_ref_cache {\n \tint fd;\n \n \t/*\n-\t * The range of memory to which the `packed-refs` file is\n-\t * mmapped. If there were no contents (e.g., because the file\n-\t * didn't exist), `buf` and `eof` are both NULL.\n+\t * The contents of the `packed-refs` file. If the file was\n+\t * already sorted, this points at the mmapped contents of the\n+\t * file. If not, this points at heap-allocated memory\n+\t * containing the contents, sorted. If there were no contents\n+\t * (e.g., because the file didn't exist), `buf` and `eof` are\n+\t * both NULL.\n \t */\n \tchar *buf, *eof;\n \n@@ -93,22 +96,24 @@ static void acquire_packed_ref_cache(struct packed_ref_cache *packed_refs)\n }\n \n /*\n- * If the buffer in `packed_refs` is active, munmap the memory, close the\n- * file, and set the buffer pointers to NULL.\n+ * If the buffer in `packed_refs` is active, then either munmap the\n+ * memory and close the file, or free the memory. Then set the buffer\n+ * pointers to NULL.\n */\n static void release_packed_ref_buffer(struct packed_ref_cache *packed_refs)\n {\n-\tif (packed_refs->buf) {\n+\tif (packed_refs->fd >= 0) {\n \t\tif (munmap(packed_refs->buf,\n \t\t\t packed_refs->eof - packed_refs->buf))\n \t\t\tdie_errno(\"error ummapping packed-refs file %s\",\n \t\t\t\t packed_refs->refs->path);\n-\t\tpacked_refs->buf = packed_refs->eof = NULL;\n-\t\tpacked_refs->header_len = 0;\n-\t}\n-\n-\tif (packed_refs->fd >= 0)\n \t\tclose(packed_refs->fd);\n+\t\tpacked_refs->fd = -1;\n+\t} else {\n+\t\tfree(packed_refs->buf);\n+\t}\n+\tpacked_refs->buf = packed_refs->eof = NULL;\n+\tpacked_refs->header_len = 0;\n }\n \n /*\n@@ -329,7 +334,7 @@ struct ref_iterator *mmapped_ref_iterator_begin(\n \tif (!packed_refs->buf)\n \t\treturn empty_ref_iterator_begin();\n \n-\tbase_ref_iterator_init(ref_iterator, &mmapped_ref_iterator_vtable, 0);\n+\tbase_ref_iterator_init(ref_iterator, &mmapped_ref_iterator_vtable, 1);\n \n \titer->packed_refs = packed_refs;\n \tacquire_packed_ref_cache(iter->packed_refs);\n@@ -342,6 +347,170 @@ struct ref_iterator *mmapped_ref_iterator_begin(\n \treturn ref_iterator;\n }\n \n+struct packed_ref_entry {\n+\tconst char *start;\n+\tsize_t len;\n+};\n+\n+static int cmp_packed_ref_entries(const void *v1, const void *v2)\n+{\n+\tconst struct packed_ref_entry *e1 = v1, *e2 = v2;\n+\tconst char *r1 = e1->start + GIT_SHA1_HEXSZ + 1;\n+\tconst char *r2 = e2->start + GIT_SHA1_HEXSZ + 1;\n+\n+\twhile (1) {\n+\t\tif (*r1 == '\\n')\n+\t\t\treturn *r2 == '\\n' ? 0 : -1;\n+\t\tif (*r1 != *r2) {\n+\t\t\tif (*r2 == '\\n')\n+\t\t\t\treturn 1;\n+\t\t\telse\n+\t\t\t\treturn (unsigned char)*r1 < (unsigned char)*r2 ? -1 : +1;\n+\t\t}\n+\t\tr1++;\n+\t\tr2++;\n+\t}\n+}\n+\n+/*\n+ * `packed_refs->buf` is not known to be sorted. Check whether it is,\n+ * and if not, sort it into new memory and munmap/free the old\n+ * storage.\n+ */\n+static void sort_packed_refs(struct packed_ref_cache *packed_refs)\n+{\n+\tstruct packed_ref_entry *entries = NULL;\n+\tsize_t alloc = 0, nr = 0;\n+\tint sorted = 1;\n+\tconst char *pos, *eof, *eol;\n+\tsize_t len, i;\n+\tchar *new_buffer, *dst;\n+\n+\tpos = packed_refs->buf + packed_refs->header_len;\n+\teof = packed_refs->eof;\n+\tlen = eof - pos;\n+\n+\tif (!len)\n+\t\treturn;\n+\n+\t/*\n+\t * Initialize entries based on a crude estimate of the number\n+\t * of references in the file (we'll grow it below if needed):\n+\t */\n+\tALLOC_GROW(entries, len / 80 + 20, alloc);\n+\n+\twhile (pos < eof) {\n+\t\teol = memchr(pos, '\\n', eof - pos);\n+\t\tif (!eol)\n+\t\t\t/* The safety check should prevent this. */\n+\t\t\tBUG(\"unterminated line found in packed-refs\");\n+\t\tif (eol - pos < GIT_SHA1_HEXSZ + 2)\n+\t\t\tdie_invalid_line(packed_refs->refs->path,\n+\t\t\t\t\t pos, eof - pos);\n+\t\teol++;\n+\t\tif (eol < eof && *eol == '^') {\n+\t\t\t/*\n+\t\t\t * Keep any peeled line together with its\n+\t\t\t * reference:\n+\t\t\t */\n+\t\t\tconst char *peeled_start = eol;\n+\n+\t\t\teol = memchr(peeled_start, '\\n', eof - peeled_start);\n+\t\t\tif (!eol)\n+\t\t\t\t/* The safety check should prevent this. */\n+\t\t\t\tBUG(\"unterminated peeled line found in packed-refs\");\n+\t\t\teol++;\n+\t\t}\n+\n+\t\tALLOC_GROW(entries, nr + 1, alloc);\n+\t\tentries[nr].start = pos;\n+\t\tentries[nr].len = eol - pos;\n+\t\tnr++;\n+\n+\t\tif (sorted &&\n+\t\t nr > 1 &&\n+\t\t cmp_packed_ref_entries(&entries[nr - 2],\n+\t\t\t\t\t &entries[nr - 1]) >= 0)\n+\t\t\tsorted = 0;\n+\n+\t\tpos = eol;\n+\t}\n+\n+\tif (sorted)\n+\t\tgoto cleanup;\n+\n+\t/* We need to sort the memory. First we sort the entries array: */\n+\tQSORT(entries, nr, cmp_packed_ref_entries);\n+\n+\t/*\n+\t * Allocate a new chunk of memory, and copy the old memory to\n+\t * the new in the order indicated by `entries` (not bothering\n+\t * with the header line):\n+\t */\n+\tnew_buffer = xmalloc(len);\n+\tfor (dst = new_buffer, i = 0; i < nr; i++) {\n+\t\tmemcpy(dst, entries[i].start, entries[i].len);\n+\t\tdst += entries[i].len;\n+\t}\n+\n+\t/*\n+\t * Now munmap the old buffer and use the sorted buffer in its\n+\t * place:\n+\t */\n+\trelease_packed_ref_buffer(packed_refs);\n+\tpacked_refs->buf = new_buffer;\n+\tpacked_refs->eof = new_buffer + len;\n+\tpacked_refs->header_len = 0;\n+\n+cleanup:\n+\tfree(entries);\n+}\n+\n+/*\n+ * Return a pointer to the start of the record that contains the\n+ * character `*p` (which must be within the buffer). If no other\n+ * record start is found, return `buf`.\n+ */\n+static const char *find_start_of_record(const char *buf, const char *p)\n+{\n+\twhile (p > buf && (p[-1] != '\\n' || p[0] == '^'))\n+\t\tp--;\n+\treturn p;\n+}\n+\n+/*\n+ * We want to be able to compare mmapped reference records quickly,\n+ * without totally parsing them. We can do so because the records are\n+ * LF-terminated, and the refname should start exactly (GIT_SHA1_HEXSZ\n+ * + 1) bytes past the beginning of the record.\n+ *\n+ * But what if the `packed-refs` file contains garbage? We're willing\n+ * to tolerate not detecting the problem, as long as we don't produce\n+ * totally garbled output (we can't afford to check the integrity of\n+ * the whole file during every Git invocation). But we do want to be\n+ * sure that we never read past the end of the buffer in memory and\n+ * perform an illegal memory access.\n+ *\n+ * Guarantee that minimum level of safety by verifying that the last\n+ * record in the file is LF-terminated, and that it has at least\n+ * (GIT_SHA1_HEXSZ + 1) characters before the LF. Die if either of\n+ * these checks fails.\n+ */\n+static void verify_buffer_safe(struct packed_ref_cache *packed_refs)\n+{\n+\tconst char *buf = packed_refs->buf + packed_refs->header_len;\n+\tconst char *eof = packed_refs->eof;\n+\tconst char *last_line;\n+\n+\tif (buf == eof)\n+\t\treturn;\n+\n+\tlast_line = find_start_of_record(buf, eof - 1);\n+\tif (*(eof - 1) != '\\n' || eof - last_line < GIT_SHA1_HEXSZ + 2)\n+\t\tdie_invalid_line(packed_refs->refs->path,\n+\t\t\t\t last_line, eof - last_line);\n+}\n+\n /*\n * Read from the `packed-refs` file into a newly-allocated\n * `packed_ref_cache` and return it. The return value will already\n@@ -350,19 +519,19 @@ struct ref_iterator *mmapped_ref_iterator_begin(\n * A comment line of the form \"# pack-refs with: \" may contain zero or\n * more traits. We interpret the traits as follows:\n *\n- * No traits:\n+ * Neither `peeled` nor `fully-peeled`:\n *\n * Probably no references are peeled. But if the file contains a\n * peeled value for a reference, we will use it.\n *\n- * peeled:\n+ * `peeled`:\n *\n * References under \"refs/tags/\", if they *can* be peeled, *are*\n * peeled in this file. References outside of \"refs/tags/\" are\n * probably not peeled even if they could have been, but if we find\n * a peeled value for such a reference we will use it.\n *\n- * fully-peeled:\n+ * `fully-peeled`:\n *\n * All references in the file that can be peeled are peeled.\n * Inversely (and this is more important), any references in the\n@@ -370,6 +539,10 @@ struct ref_iterator *mmapped_ref_iterator_begin(\n * trait should typically be written alongside \"peeled\" for\n * compatibility with older clients, but we do not require it\n * (i.e., \"peeled\" is a no-op if \"fully-peeled\" is set).\n+ *\n+ * `sorted`:\n+ *\n+ * The references in this file are known to be sorted by refname.\n */\n static struct packed_ref_cache *read_packed_refs(struct packed_ref_store *refs)\n {\n@@ -378,6 +551,7 @@ static struct packed_ref_cache *read_packed_refs(struct packed_ref_store *refs)\n \tsize_t size;\n \tstruct ref_dir *dir;\n \tstruct ref_iterator *iter;\n+\tint sorted = 0;\n \tint ok;\n \n \tpacked_refs->refs = refs;\n@@ -440,6 +614,9 @@ static struct packed_ref_cache *read_packed_refs(struct packed_ref_store *refs)\n \t\t\tpacked_refs->peeled = PEELED_FULLY;\n \t\telse if (unsorted_string_list_has_string(&traits, \"peeled\"))\n \t\t\tpacked_refs->peeled = PEELED_TAGS;\n+\n+\t\tsorted = unsorted_string_list_has_string(&traits, \"sorted\");\n+\n \t\t/* perhaps other traits later as well */\n \n \t\t/* The \"+ 1\" is for the LF character. */\n@@ -449,6 +626,19 @@ static struct packed_ref_cache *read_packed_refs(struct packed_ref_store *refs)\n \t\tstrbuf_release(&tmp);\n \t}\n \n+\tverify_buffer_safe(packed_refs);\n+\n+\tif (!sorted) {\n+\t\tsort_packed_refs(packed_refs);\n+\n+\t\t/*\n+\t\t * Reordering the records might have moved a short one\n+\t\t * to the end of the buffer, so verify the buffer's\n+\t\t * safety again:\n+\t\t */\n+\t\tverify_buffer_safe(packed_refs);\n+\t}\n+\n \tdir = get_ref_dir(packed_refs->cache->root);\n \titer = mmapped_ref_iterator_begin(\n \t\t\tpacked_refs,\n@@ -752,7 +942,7 @@ int packed_refs_is_locked(struct ref_store *ref_store)\n * the colon and the trailing space are required.\n */\n static const char PACKED_REFS_HEADER[] =\n-\t\"# pack-refs with: peeled fully-peeled \\n\";\n+\t\"# pack-refs with: peeled fully-peeled sorted \\n\";\n \n static int packed_init_db(struct ref_store *ref_store, struct strbuf *err)\n {\n-- \n2.14.1\n\n"},{"id":"327974","messageId":"597241da13f62d51fefd3396bfbab22a8f633a56.1505319366.git.mhagger@alum.mit.edu","threadId":"46744","inReplyTo":"cover.1505319366.git.mhagger@alum.mit.edu","subject":"[PATCH 08/20] read_packed_refs(): read references with minimal copying","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-09-13T17:16:02Z","receivedAt":"2017-09-13T17:17:54Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Instead of copying data from the `packed-refs` file one line at time\nand then processing it, process the data in place as much as possible.\n\nAlso, instead of processing one line per iteration of the main loop,\nprocess a reference line plus its corresponding peeled line (if\npresent) together.\n\nNote that this change slightly tightens up the parsing of the\n`parse-ref` file. Previously, the parser would have accepted multiple\n\"peeled\" lines for a single reference (ignoring all but the last one).\nNow it would reject that.\n\nSigned-off-by: Michael Haggerty \n---\n refs/packed-backend.c | 101 ++++++++++++++++++++------------------------------\n 1 file changed, 40 insertions(+), 61 deletions(-)\n\ndiff --git a/refs/packed-backend.c b/refs/packed-backend.c\nindex a45e3ff92f..2b80f244c8 100644\n--- a/refs/packed-backend.c\n+++ b/refs/packed-backend.c\n@@ -134,33 +134,6 @@ static void clear_packed_ref_cache(struct packed_ref_store *refs)\n \t}\n }\n \n-/* The length of a peeled reference line in packed-refs, including EOL: */\n-#define PEELED_LINE_LENGTH 42\n-\n-/*\n- * Parse one line from a packed-refs file. Write the SHA1 to sha1.\n- * Return a pointer to the refname within the line (null-terminated),\n- * or NULL if there was a problem.\n- */\n-static const char *parse_ref_line(struct strbuf *line, struct object_id *oid)\n-{\n-\tconst char *ref;\n-\n-\tif (parse_oid_hex(line->buf, oid, &ref) < 0)\n-\t\treturn NULL;\n-\tif (!isspace(*ref++))\n-\t\treturn NULL;\n-\n-\tif (isspace(*ref))\n-\t\treturn NULL;\n-\n-\tif (line->buf[line->len - 1] != '\\n')\n-\t\treturn NULL;\n-\tline->buf[--line->len] = 0;\n-\n-\treturn ref;\n-}\n-\n static NORETURN void die_unterminated_line(const char *path,\n \t\t\t\t\t const char *p, size_t len)\n {\n@@ -221,8 +194,7 @@ static struct packed_ref_cache *read_packed_refs(struct packed_ref_store *refs)\n \tsize_t size;\n \tchar *buf;\n \tconst char *pos, *eol, *eof;\n-\tstruct ref_entry *last = NULL;\n-\tstruct strbuf line = STRBUF_INIT;\n+\tstruct strbuf tmp = STRBUF_INIT;\n \tenum { PEELED_NONE, PEELED_TAGS, PEELED_FULLY } peeled = PEELED_NONE;\n \tstruct ref_dir *dir;\n \n@@ -264,9 +236,9 @@ static struct packed_ref_cache *read_packed_refs(struct packed_ref_store *refs)\n \t\tif (!eol)\n \t\t\tdie_unterminated_line(refs->path, pos, eof - pos);\n \n-\t\tstrbuf_add(&line, pos, eol - pos);\n+\t\tstrbuf_add(&tmp, pos, eol - pos);\n \n-\t\tif (!skip_prefix(line.buf, \"# pack-refs with:\", (const char **)&p))\n+\t\tif (!skip_prefix(tmp.buf, \"# pack-refs with:\", (const char **)&p))\n \t\t\tdie_invalid_line(refs->path, pos, eof - pos);\n \n \t\tstring_list_split_in_place(&traits, p, ' ', -1);\n@@ -281,61 +253,68 @@ static struct packed_ref_cache *read_packed_refs(struct packed_ref_store *refs)\n \t\tpos = eol + 1;\n \n \t\tstring_list_clear(&traits, 0);\n-\t\tstrbuf_reset(&line);\n+\t\tstrbuf_reset(&tmp);\n \t}\n \n \tdir = get_ref_dir(packed_refs->cache->root);\n \twhile (pos < eof) {\n+\t\tconst char *p = pos;\n \t\tstruct object_id oid;\n \t\tconst char *refname;\n+\t\tint flag = REF_ISPACKED;\n+\t\tstruct ref_entry *entry = NULL;\n \n-\t\teol = memchr(pos, '\\n', eof - pos);\n+\t\tif (eof - pos < GIT_SHA1_HEXSZ + 2 ||\n+\t\t parse_oid_hex(p, &oid, &p) ||\n+\t\t !isspace(*p++))\n+\t\t\tdie_invalid_line(refs->path, pos, eof - pos);\n+\n+\t\teol = memchr(p, '\\n', eof - p);\n \t\tif (!eol)\n \t\t\tdie_unterminated_line(refs->path, pos, eof - pos);\n \n-\t\tstrbuf_add(&line, pos, eol + 1 - pos);\n+\t\tstrbuf_add(&tmp, p, eol - p);\n+\t\trefname = tmp.buf;\n+\n+\t\tif (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL)) {\n+\t\t\tif (!refname_is_safe(refname))\n+\t\t\t\tdie(\"packed refname is dangerous: %s\", refname);\n+\t\t\toidclr(&oid);\n+\t\t\tflag |= REF_BAD_NAME | REF_ISBROKEN;\n+\t\t}\n+\t\tif (peeled == PEELED_FULLY ||\n+\t\t (peeled == PEELED_TAGS && starts_with(refname, \"refs/tags/\")))\n+\t\t\tflag |= REF_KNOWS_PEELED;\n+\t\tentry = create_ref_entry(refname, &oid, flag);\n+\t\tadd_ref_entry(dir, entry);\n \n-\t\trefname = parse_ref_line(&line, &oid);\n-\t\tif (refname) {\n-\t\t\tint flag = REF_ISPACKED;\n+\t\tpos = eol + 1;\n+\n+\t\tif (pos < eof && *pos == '^') {\n+\t\t\tp = pos + 1;\n+\t\t\tif (eof - p < GIT_SHA1_HEXSZ + 1 ||\n+\t\t\t parse_oid_hex(p, &entry->u.value.peeled, &p) ||\n+\t\t\t *p++ != '\\n')\n+\t\t\t\tdie_invalid_line(refs->path, pos, eof - pos);\n \n-\t\t\tif (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL)) {\n-\t\t\t\tif (!refname_is_safe(refname))\n-\t\t\t\t\tdie(\"packed refname is dangerous: %s\", refname);\n-\t\t\t\toidclr(&oid);\n-\t\t\t\tflag |= REF_BAD_NAME | REF_ISBROKEN;\n-\t\t\t}\n-\t\t\tlast = create_ref_entry(refname, &oid, flag);\n-\t\t\tif (peeled == PEELED_FULLY ||\n-\t\t\t (peeled == PEELED_TAGS && starts_with(refname, \"refs/tags/\")))\n-\t\t\t\tlast->flag |= REF_KNOWS_PEELED;\n-\t\t\tadd_ref_entry(dir, last);\n-\t\t} else if (last &&\n-\t\t line.buf[0] == '^' &&\n-\t\t line.len == PEELED_LINE_LENGTH &&\n-\t\t line.buf[PEELED_LINE_LENGTH - 1] == '\\n' &&\n-\t\t !get_oid_hex(line.buf + 1, &oid)) {\n-\t\t\toidcpy(&last->u.value.peeled, &oid);\n \t\t\t/*\n \t\t\t * Regardless of what the file header said,\n \t\t\t * we definitely know the value of *this*\n \t\t\t * reference:\n \t\t\t */\n-\t\t\tlast->flag |= REF_KNOWS_PEELED;\n-\t\t} else {\n-\t\t\tdie_invalid_line(refs->path, line.buf, line.len);\n+\t\t\tentry->flag |= REF_KNOWS_PEELED;\n+\n+\t\t\tpos = p;\n \t\t}\n \n-\t\t/* The \"+ 1\" is for the LF character. */\n-\t\tpos = eol + 1;\n-\t\tstrbuf_reset(&line);\n+\t\tstrbuf_reset(&tmp);\n \t}\n \n \tif (munmap(buf, size))\n \t\tdie_errno(\"error ummapping packed-refs file\");\n \tclose(fd);\n \n-\tstrbuf_release(&line);\n+\tstrbuf_release(&tmp);\n \treturn packed_refs;\n }\n \n-- \n2.14.1\n\n"},{"id":"328000","messageId":"xmqqbmme1dv2.fsf@gitster.mtv.corp.google.com","threadId":"46744","inReplyTo":"cover.1505319366.git.mhagger@alum.mit.edu","subject":"Re: [PATCH 00/20] Read `packed-refs` using mmap()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-09-13T21:35:13Z","receivedAt":"2017-09-13T21:37:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n> Following lots of work to extract the `packed_ref_store` into a\n> separate module and decouple it from the `files_ref_store`, it is now\n> possible to fundamentally change how the `packed-refs` file is read.\n>\n> * `mmap()` the whole file rather than `read()`ing it.\n>\n> * Instead of parsing the complete file at once into a `ref_cache`,\n> parse the references out of the file contents on demand.\n>\n> * Use a binary search to find, very quickly, the reference or group of\n> references that needs to be read. Parse *only* those references out\n> of the file contents, without creating in-memory data structures at\n> all.\n\nOooh, juicy and very exciting.\n\n> ...\n> This branch applies on top of mh/packed-ref-transactions. It can also\n> be obtained from my git fork [1] as branch `mmap-packed-refs`.\n>\n> Michael\n>\n> [1] https://github.com/mhagger/git\n\nThanks for working on this. I expect that it will take more than a\nfew hours for me to pick it up, but I am looking forward to it.\n\n"},{"id":"328020","messageId":"CAGZ79kYHzWaPXGE29RUe1J+T_=AHKADhv2TjkMSUu5NM80zX1w@mail.gmail.com","threadId":"46744","inReplyTo":"8844c66d7b245dda7a2fa2794cb4051b5b2b35e2.1505319366.git.mhagger@alum.mit.edu","subject":"Re: [PATCH 20/20] packed-backend.c: rename a bunch of things and update comments","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-09-13T23:00:39Z","receivedAt":"2017-09-13T23:00:46Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, Sep 13, 2017 at 10:16 AM, Michael Haggerty wrote:\n> We've made huge changes to this file, and some of the old names and\n> comments are no longer very fitting. So rename a bunch of things:\n>\n> * `struct packed_ref_cache` → `struct snapshot`\n> * `acquire_packed_ref_cache()` → `acquire_snapshot()`\n> * `release_packed_ref_buffer()` → `clear_snapshot_buffer()`\n> * `release_packed_ref_cache()` → `release_snapshot()`\n> * `clear_packed_ref_cache()` → `clear_snapshot()`\n> * `struct packed_ref_entry` → `struct snapshot_record`\n> * `cmp_packed_ref_entries()` → `cmp_packed_ref_records()`\n> * `cmp_entry_to_refname()` → `cmp_record_to_refname()`\n> * `sort_packed_refs()` → `sort_snapshot()`\n> * `read_packed_refs()` → `create_snapshot()`\n> * `validate_packed_ref_cache()` → `validate_snapshot()`\n> * `get_packed_ref_cache()` → `get_snapshot()`\n> * Renamed local variables and struct members accordingly.\n>\n> Also update a bunch of comments to reflect the renaming and the\n> accumulated changes that the code has undergone.\n>\n> Signed-off-by: Michael Haggerty \n\nI have skimmed this series and it looks good.\nThanks,\nStefan\n"},{"id":"328065","messageId":"ef919cc7-7e41-afb6-38b2-e2fae4efcbf1@alum.mit.edu","threadId":"46744","inReplyTo":"cover.1505319366.git.mhagger@alum.mit.edu","subject":"Re: [PATCH 00/20] Read `packed-refs` using mmap()","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-09-14T16:12:39Z","receivedAt":"2017-09-14T16:12:56Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 09/13/2017 07:15 PM, Michael Haggerty wrote:\n> [...]\n> * `mmap()` the whole file rather than `read()`ing it.\n> [...]\nApparently this doesn't work on Windows, because the `snapshot` is\nkeeping the `packed-refs` file open too long, so the new file can't be\nrenamed on top of it.\n\nI didn't realize that this is even allowed, but TIL that you can close a\nfile while keeping it mmapped. Does that technique work on Windows? If\nso, I'll change v2 to do it as sketched below.\n\nMichael\n\ndiff --git a/refs/packed-backend.c b/refs/packed-backend.c\nindex 8235ac8506..95c1cd2a27 100644\n--- a/refs/packed-backend.c\n+++ b/refs/packed-backend.c\n@@ -35,11 +35,8 @@ struct snapshot {\n */\n struct packed_ref_store *refs;\n\n- /*\n- * The file descriptor of the `packed-refs` file (open in\n- * read-only mode), or -1 if it is not open.\n- */\n- int fd;\n+ /* Is the `packed-refs` file currently mmapped? */\n+ int mmapped;\n\n /*\n * The contents of the `packed-refs` file. If the file was\n@@ -135,12 +132,11 @@ static void acquire_snapshot(struct snapshot\n*snapshot)\n */\n static void clear_snapshot_buffer(struct snapshot *snapshot)\n {\n- if (snapshot->fd >= 0) {\n+ if (snapshot->mmapped) {\n if (munmap(snapshot->buf, snapshot->eof - snapshot->buf))\n die_errno(\"error ummapping packed-refs file %s\",\n snapshot->refs->path);\n- close(snapshot->fd);\n- snapshot->fd = -1;\n+ snapshot->mmapped = 0;\n } else {\n free(snapshot->buf);\n }\n@@ -525,6 +521,7 @@ static const char *find_reference_location(struct\nsnapshot *snapshot,\n static struct snapshot *create_snapshot(struct packed_ref_store *refs)\n {\n struct snapshot *snapshot = xcalloc(1, sizeof(*snapshot));\n+ int fd;\n struct stat st;\n size_t size;\n int sorted = 0;\n@@ -533,8 +530,8 @@ static struct snapshot *create_snapshot(struct\npacked_ref_store *refs)\n acquire_snapshot(snapshot);\n snapshot->peeled = PEELED_NONE;\n\n- snapshot->fd = open(refs->path, O_RDONLY);\n- if (snapshot->fd < 0) {\n+ fd = open(refs->path, O_RDONLY);\n+ if (fd < 0) {\n if (errno == ENOENT) {\n /*\n * This is OK; it just means that no\n@@ -549,15 +546,16 @@ static struct snapshot *create_snapshot(struct\npacked_ref_store *refs)\n }\n }\n\n- stat_validity_update(&snapshot->validity, snapshot->fd);\n+ stat_validity_update(&snapshot->validity, fd);\n\n- if (fstat(snapshot->fd, &st) < 0)\n+ if (fstat(fd, &st) < 0)\n die_errno(\"couldn't stat %s\", refs->path);\n\n size = xsize_t(st.st_size);\n- snapshot->buf = xmmap(NULL, size, PROT_READ, MAP_PRIVATE,\n- snapshot->fd, 0);\n+ snapshot->buf = xmmap(NULL, size, PROT_READ, MAP_PRIVATE, fd, 0);\n snapshot->eof = snapshot->buf + size;\n+ snapshot->mmapped = 1;\n+ close(fd);\n\n /* If the file has a header line, process it: */\n if (snapshot->buf < snapshot->eof && *snapshot->buf == '#') {\n"},{"id":"328077","messageId":"alpine.DEB.2.21.1.1709142101560.4132@virtualbox","threadId":"46744","inReplyTo":"cover.1505319366.git.mhagger@alum.mit.edu","subject":"Re: [PATCH 00/20] Read `packed-refs` using mmap()","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-09-14T20:23:37Z","receivedAt":"2017-09-14T20:23:57Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Michael,\n\nOn Wed, 13 Sep 2017, Michael Haggerty wrote:\n\n> * `mmap()` the whole file rather than `read()`ing it.\n\nOn Windows, a memory-mapped file cannot be renamed. As a consequence, the\nfollowing tests fail on `pu`:\n\nt1400-update-ref.sh\nt1404-update-ref-errors.sh\nt1405-main-ref-store.sh\nt1408-packed-refs.sh\nt1410-reflog.sh\nt1430-bad-ref-name.sh\nt1450-fsck.sh\nt1507-rev-parse-upstream.sh\nt2018-checkout-branch.sh\nt2020-checkout-detach.sh\nt2024-checkout-dwim.sh\nt3200-branch.sh\nt3204-branch-name-interpretation.sh\nt3210-pack-refs.sh\nt3211-peel-ref.sh\nt3306-notes-prune.sh\nt3400-rebase.sh\nt3404-rebase-interactive.sh\nt3420-rebase-autostash.sh\nt3903-stash.sh\nt3905-stash-include-untracked.sh\nt4151-am-abort.sh\nt5304-prune.sh\nt5312-prune-corruption.sh\nt5400-send-pack.sh\nt5404-tracking-branches.sh\nt5500-fetch-pack.sh\nt5505-remote.sh\nt5510-fetch.sh\nt5514-fetch-multiple.sh\nt5515-fetch-merge-logic.sh\nt5516-fetch-push.sh\nt5520-pull.sh\nt5533-push-cas.sh\nt5572-pull-submodule.sh\nt5600-clone-fail-cleanup.sh\nt5607-clone-bundle.sh\nt6016-rev-list-graph-simplify-history.sh\nt6030-bisect-porcelain.sh\nt6032-merge-large-rename.sh\nt6040-tracking-info.sh\nt6050-replace.sh\nt6500-gc.sh\nt6501-freshen-objects.sh\nt7003-filter-branch.sh\nt7004-tag.sh\nt7102-reset.sh\nt7201-co.sh\nt7406-submodule-update.sh\nt7504-commit-msg-hook.sh\nt7701-repack-unpack-unreachable.sh\nt9300-fast-import.sh\nt9902-completion.sh\nt9903-bash-prompt.sh\n\nThis diff:\n\n-- snip --\ndiff --git a/refs/packed-backend.c b/refs/packed-backend.c\nindex f9c5e771bdd..ee8b3603624 100644\n--- a/refs/packed-backend.c\n+++ b/refs/packed-backend.c\n@@ -1306,13 +1308,13 @@ static int packed_transaction_finish(struct\nref_store *ref_store,\n \tchar *packed_refs_path;\n \n \tpacked_refs_path = get_locked_file_path(&refs->lock);\n+\tclear_snapshot(refs);\n \tif (rename_tempfile(&refs->tempfile, packed_refs_path)) {\n \t\tstrbuf_addf(err, \"error replacing %s: %s\",\n \t\t\t refs->path, strerror(errno));\n \t\tgoto cleanup;\n \t}\n \n-\tclear_snapshot(refs);\n \tret = 0;\n \n cleanup:\n-- snap --\n\nreduces the failed tests to\n\nt1410-reflog.counts.sh\nt3210-pack-refs.counts.sh\nt3211-peel-ref.counts.sh\nt5505-remote.counts.sh\nt5510-fetch.counts.sh\nt5600-clone-fail-cleanup.counts.sh\n\nHowever, much as I tried to wrap my head around it, I could not even start\nto come up with a solution for e.g. the t1410 regression. The failure\nhappens in `git branch -d one/two`.\n\nThe culprit: there is yet another unreleased snapshot while we try to\nfinish the transaction.\n\nIt is the snapshot of the main_worktree_ref_store as acquired by\nadd_head_info() (which is called from get_main_worktree(), which in turn\nwas called from get_worktrees(), in turn having been called from\nfind_shared_symref() that was called from delete_branches()), and it seems\nto me that there was never any plan to have that released, including its\nsnapshot.\n\nAnd the snapshot gets initialized upon add_head_info() calling\nrefs_resolve_ref_unsafe().\n\nFurther down in the delete_branches() function, however, we call\ndelete_ref() which in turn wants to overwrite the packed-refs file via an\natomic rename. That rename fails now because there is still that main\nworktree's ref_store that has the snapshot mmap()ed .\n\nI imagine that the rest of the test failures stems from the same root\ncause.\n\nDo you have any idea how to ensure that all mmap()ed packed refs are\nreleased before attempting to finish a transaction?\n\nCiao,\nDscho\n"},{"id":"328082","messageId":"alpine.DEB.2.21.1.1709150012550.219280@virtualbox","threadId":"46744","inReplyTo":"ef919cc7-7e41-afb6-38b2-e2fae4efcbf1@alum.mit.edu","subject":"Re: [PATCH 00/20] Read `packed-refs` using mmap()","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-09-14T22:16:40Z","receivedAt":"2017-09-14T22:17:17Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Michael,\n\nOn Thu, 14 Sep 2017, Michael Haggerty wrote:\n\n> On 09/13/2017 07:15 PM, Michael Haggerty wrote:\n> > [...]\n> > * `mmap()` the whole file rather than `read()`ing it.\n> > [...]\n>\n> Apparently this doesn't work on Windows, because the `snapshot` is\n> keeping the `packed-refs` file open too long, so the new file can't be\n> renamed on top of it.\n\nIndeed, I sent you a mail about it before I checked for new mails ;-)\n\n> I didn't realize that this is even allowed, but TIL that you can close a\n> file while keeping it mmapped. Does that technique work on Windows? If\n> so, I'll change v2 to do it as sketched below.\n\nI do not know whether that technique works on Windows, but it would not\nsolve the problem *entirely*, as I outlined in my reply: in\ndelete_branches(), for example, the main_worktree_ref_store is implicitly\ngetting its snapshot initialized, and nothing seems to release it.\n\nThis is only one example, as I figure that multiple worktrees could cause\neven more ref_stores to have unreleased snapshots of the packed-refs file.\n\nI imagine that something like close_all_packs() is needed\n(\"clear_all_snapshots()\"?).\n\nCiao,\nDscho\n"},{"id":"328097","messageId":"d0fe1daf-4b44-fa4f-dab6-2590e6c09163@alum.mit.edu","threadId":"46744","inReplyTo":"alpine.DEB.2.21.1.1709142101560.4132@virtualbox","subject":"Re: [PATCH 00/20] Read `packed-refs` using mmap()","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-09-15T04:21:04Z","receivedAt":"2017-09-15T04:21:15Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 09/14/2017 10:23 PM, Johannes Schindelin wrote:\n> On Wed, 13 Sep 2017, Michael Haggerty wrote:\n> \n>> * `mmap()` the whole file rather than `read()`ing it.\n> \n> On Windows, a memory-mapped file cannot be renamed. As a consequence, the\n> following tests fail on `pu`:\n> \n> [...]\n\nThanks for your quick investigation.\n\n> This diff:\n> \n> -- snip --\n> diff --git a/refs/packed-backend.c b/refs/packed-backend.c\n> index f9c5e771bdd..ee8b3603624 100644\n> --- a/refs/packed-backend.c\n> +++ b/refs/packed-backend.c\n> @@ -1306,13 +1308,13 @@ static int packed_transaction_finish(struct\n> ref_store *ref_store,\n> \tchar *packed_refs_path;\n> \n> \tpacked_refs_path = get_locked_file_path(&refs->lock);\n> +\tclear_snapshot(refs);\n> \tif (rename_tempfile(&refs->tempfile, packed_refs_path)) {\n> \t\tstrbuf_addf(err, \"error replacing %s: %s\",\n> \t\t\t refs->path, strerror(errno));\n> \t\tgoto cleanup;\n> \t}\n> \n> -\tclear_snapshot(refs);\n> \tret = 0;\n> \n> cleanup:\n> -- snap --\n> \n> reduces the failed tests to\n> \n> t1410-reflog.counts.sh\n> t3210-pack-refs.counts.sh\n> t3211-peel-ref.counts.sh\n> t5505-remote.counts.sh\n> t5510-fetch.counts.sh\n> t5600-clone-fail-cleanup.counts.sh\n\nThat change is a strict improvement on all systems; I'll integrate it\ninto the series.\n\n> However, much as I tried to wrap my head around it, I could not even start\n> to come up with a solution for e.g. the t1410 regression. The failure\n> happens in `git branch -d one/two`.\n> \n> The culprit: there is yet another unreleased snapshot while we try to\n> finish the transaction.\n> \n> It is the snapshot of the main_worktree_ref_store as acquired by\n> add_head_info() (which is called from get_main_worktree(), which in turn\n> was called from get_worktrees(), in turn having been called from\n> find_shared_symref() that was called from delete_branches()), and it seems\n> to me that there was never any plan to have that released, including its\n> snapshot.\n\nYeah the idea was that the default situation would be that whenever a\n`packed-refs` file needs to be read, it would be kept mmapped for the\nlife of the program (or until the file was detected to have been\nchanged). This was meant to be a replacement for the explicit\n`ref_cache`. So any long-lived git process could be expected to have the\n`packed-refs` file (or even multiple `packed-refs` files in the case of\nsubmodules) mmapped.\n\nThat's obviously not going to work on Windows.\n\n> [...]\n> Do you have any idea how to ensure that all mmap()ed packed refs are\n> released before attempting to finish a transaction?\n\n[And from your other email:]\n> This is only one example, as I figure that multiple worktrees could cause\n> even more ref_stores to have unreleased snapshots of the packed-refs file.\n> \n> I imagine that something like close_all_packs() is needed\n> (\"clear_all_snapshots()\"?).\n\nYes, I wasn't really familiar with `close_all_packs()`, but it sounds\nlike it solves a similar problem.\n\nWithin a single process, we could make this work via an arduous process\nof figuring out what functions might want to overwrite the `packed-refs`\nfile, and making sure that nobody up the call stack is iterating over\nreferences during those function calls. If that condition is met, then\ncalling `clear_snapshot()` earlier, as you propose above, would decrease\nthe reference count to zero, causing the old snapshot to be freed, and\nallowing the rename to succeed. We could even do\n\n if (!clear_snapshot(refs))\n BUG(\"packed-refs snapshot is still in use\");\n\n, analogously to `close_all_packs()`, to help find code that violates\nthe condition.\n\nSimilarly, it wouldn't be allowed to invoke a subprocess or hook\nfunction while iterating over references, and one would have to clear\nany current snapshots before doing so. It might even make sense to do\nthat at the same time as `close_all_packs()`, for example in a new\nfunction `close_all_unnecessary_files()`.\n\nBut what about unrelated processes? Anybody reading `packed-refs` would\nblock anybody who wants to modify it, for example to delete a reference\nor pack the references. I think this is worse than the packfile\nsituation. Packfiles all have unique names, so they never need to be\noverwritten; at worst they are deleted. And the deletion is never\ncritical to correctness. If you can't delete a packfile, the only\nconsequence is that it hangs around until the next GC.\n\nHowever, the `packed-refs` file always has the same name, and\noverwriting it is sometimes critical for correctness. So it sounds to me\nlike even *readers* mustn't keep the file open or mmapped longer than\nnecessary!\n\n(This makes me wonder, doesn't `rename_tempfile()` for the `packed-refs`\nfile *already* fail sometimes on Windows due to a concurrent reader?\nShouldn't it retry if it gets whatever error indicates that the failure\nwas due to the file being open?)\n\nAnyway, if all of my reasoning is correct, it seems that the inescapable\nconclusion is that having the `packed-refs` file open or mmapped is\ntantamount to holding a reader lock on the file, because it causes\nwriters to fail or at least have to retry. Therefore, even if you are\nonly reading the `packed-refs` file, you should read then close/munmap\nit as quickly as possible.\n\nAnd if *that* is true, then ISTM that this mmap idea is unworkable on\nWindows. We would either need to keep the old `ref_cache`-based code\naround for use there, or we would need to make a copy the file contents\nin memory immediately and use *that* as our cache. The latter would be\npretty easy to implement, actually, because something similar is done in\n`sort_snapshot()` to handle the case that the `packed-refs` file on disk\nis not correctly ordered by refname. I think that approach would be\ncompetitive with the `ref_cache`-based code, which also reads (and\nparses!) the whole file whenever any packed reference needs to be read.\nIt is possible that memory usage might go up a bit due to the fact that\nthe SHA-1s would be stored in memory in hex rather than binary form; on\nthe other hand, this would spare some memory overhead associated with\nallocating lots of little objects and the pointers that string the\nobjects together, so it's probably a wash.\n\nPlease let me know if you agree with my reasoning. If so, I'll implement\n\"always copy `packed-refs` file contents to RAM\" for Windows.\n\nA possible future optimization might be that when iterating over a\nsubset of the references (e.g., `refs/replace/`), only that part of the\nfile is copied to RAM. But that would be a bigger change, and it's not\nobvious whether it might make some read access patterns slower.\n\nMichael\n"}]}