{"thread":{"id":"34110","subject":"[PATCH 00/12] Fix some reference-related races","startedAt":"2013-06-11T21:48:20Z","lastAt":"2013-06-19T05:51:22Z","messageCount":22,"participants":["Michael Haggerty","Jeff King","Ramsay Jones"],"isPatch":true,"patchVersion":1,"patchTotal":12},"messages":[{"id":"220515","messageId":"1370987312-6761-1-git-send-email-mhagger@alum.mit.edu","threadId":"34110","inReplyTo":null,"subject":"[PATCH 00/12] Fix some reference-related races","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2013-06-11T21:48:20Z","receivedAt":"2013-06-11T21:48:20Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"*This patch series must be built on top of mh/reflife.*\n\nThis patch series fixes some races reading loose and packed refs.\nMost of the problems, and some of the solutions, were pointed out by\nJeff King [1] but some other work was necessary to prevent his fixes\nfrom causing problems elsewhere.\n\nThe basic race being addressed is that at any time \"pack-refs --prune\"\nmight be run at any time. It rewrites the packed-refs file and then\ndeletes the just-packed loose refs.\n\nReaders, then, to get a self-consistent snapshot of references [2],\nmust be sure to read all of the loose references it will need *before*\nreading the packed references (or at least verifying that the packed\nreferences that it read earlier are still valid). But given the\nlazy-loading of the loose references cache, this was not always the\ncase.\n\nSo the core of this patch series is to force loose references for an\niteration to be read all at once into the cache, and *then* to verify\nthat the packed-refs cache is up-to-date and if not to reload it.\nSimilarly, when looking up single references, a loose reference is\nsought first, and then the validity of the packed-refs cache is\nverified, and then the loose reference is sought in the cache.\n\nThe problem is that there was a lot of code that assumed that the\nlifetime of the reference cache was essentially infinite. The\nmh/reflife patch series (which has made it to next) fixed callers who\nretained pointers to refnames in the cache.\n\nThe other problem was that the for_each_ref() functions will die if\nthe ref cache that they are iterating over is freed out from under\nthem. This problem is solved by using reference counts to avoid\nfreeing the old packed ref cache (even if it is no longer valid) until\nall users are done with it.\n\nOnce those are done, it is possible to invalidate the packed refs\ncache when needed. So (1) we always read all loose references that\nwill be needed in an iteration before the iteration starts, and (2) we\nadd a check (based on file metadata) whenever the packed-refs cache is\naccessed that it is still up-to-date WRT the packed-refs file, and if\nnot reread it (but leave the old copy in memory as long as its\nrefcount is nonzero).\n\nAlong the way, this patch series adds simple transactions around the\npacked-refs file/cache. The transaction interface is public. I think\nthis is a step in a good direction, because other race conditions not\naddressed by this patch series are likely to require transactions\nacross the whole reference namespace to be made 100% reliable.\n\nAs a stress test, the test suite can be run with a simulated\n\"hyperactive repository\" in which the packed-refs file is made to look\nlike it changes every time it is checked (except when its lock is\nheld):\n\n ------------------------------------ refs.c ------------------------------------\n @@ -1075,8 +1075,8 @@ static struct packed_ref_cache *get_packed_ref_cache(struct ref_cache *refs)\n \telse\n \t\tpacked_refs_file = git_path(\"packed-refs\");\n\n -\tif (refs->packed &&\n -\t !stat_validity_check(&refs->packed->validity, packed_refs_file))\n +\tif (refs->packed && !refs->packed->lock\n +\t /*!stat_validity_check(&refs->packed->validity, packed_refs_file)*/)\n \t\tclear_packed_ref_cache(refs);\n\n \tif (!refs->packed) {\n\n\nIt passes the stress test.\n\n[1] http://thread.gmane.org/gmane.comp.version-control.git/223299/focus=223526\n\nJeff King (2):\n get_packed_ref_cache: reload packed-refs file when it changes\n for_each_ref: load all loose refs before packed refs\n\nMichael Haggerty (10):\n repack_without_ref(): split list curation and entry writing\n pack_refs(): split creation of packed refs and entry writing\n refs: wrap the packed refs cache in a level of indirection\n refs: implement simple transactions for the packed-refs file\n refs: manage lifetime of packed refs cache via reference counting\n do_for_each_entry(): increment the packed refs cache refcount\n packed_ref_cache: increment refcount when locked\n Extract a struct stat_data from cache_entry\n add a stat_validity struct\n refs: do not invalidate the packed-refs cache unnecessarily\n\n builtin/clone.c | 7 +-\n builtin/ls-files.c | 12 ++-\n cache.h | 60 +++++++++--\n read-cache.c | 181 +++++++++++++++++++------------\n refs.c | 308 ++++++++++++++++++++++++++++++++++++++++++++---------\n refs.h | 27 ++++-\n 6 files changed, 464 insertions(+), 131 deletions(-)\n\n-- \n1.8.3\n"},{"id":"220516","messageId":"1370987312-6761-2-git-send-email-mhagger@alum.mit.edu","threadId":"34110","inReplyTo":"1370987312-6761-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 01/12] repack_without_ref(): split list curation and entry writing","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2013-06-11T21:48:21Z","receivedAt":"2013-06-11T21:48:21Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Split repack_without_ref() into multiple passes:\n\n* collect the list of refnames that should be deleted from packed_refs\n\n* delete those refnames from the cache\n\n* write the remainder to the packed-refs file\n\nThe purpose of this change is to make the \"write the remainder\" part\nreusable.\n\nSigned-off-by: Michael Haggerty \n---\n refs.c | 48 +++++++++++++++++++++++++++++++++++++-----------\n 1 file changed, 37 insertions(+), 11 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex d17931a..dccfe14 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1994,6 +1994,23 @@ static void write_packed_entry(int fd, char *refname, unsigned char *sha1,\n \t}\n }\n \n+/*\n+ * An each_ref_entry_fn that writes the entry to a packed-refs file.\n+ */\n+static int write_packed_entry_fn(struct ref_entry *entry, void *cb_data)\n+{\n+\tint *fd = cb_data;\n+\tenum peel_status peel_status = peel_entry(entry, 0);\n+\n+\tif (peel_status != PEEL_PEELED && peel_status != PEEL_NON_TAG)\n+\t\terror(\"internal error: %s is not a valid packed reference!\",\n+\t\t entry->name);\n+\twrite_packed_entry(*fd, entry->name, entry->u.value.sha1,\n+\t\t\t peel_status == PEEL_PEELED ?\n+\t\t\t entry->u.value.peeled : NULL);\n+\treturn 0;\n+}\n+\n struct ref_to_prune {\n \tstruct ref_to_prune *next;\n \tunsigned char sha1[20];\n@@ -2113,14 +2130,18 @@ int pack_refs(unsigned int flags)\n \treturn 0;\n }\n \n-static int repack_ref_fn(struct ref_entry *entry, void *cb_data)\n+/*\n+ * If entry should be deleted from packed-refs, add it to the string\n+ * list pointed to by cb_data.\n+ */\n+static int curate_packed_ref_fn(struct ref_entry *entry, void *cb_data)\n {\n-\tint *fd = cb_data;\n-\tenum peel_status peel_status;\n+\tstruct string_list *refs_to_delete = cb_data;\n \n \tif (entry->flag & REF_ISBROKEN) {\n \t\t/* This shouldn't happen to packed refs. */\n \t\terror(\"%s is broken!\", entry->name);\n+\t\tstring_list_append(refs_to_delete, entry->name);\n \t\treturn 0;\n \t}\n \tif (!has_sha1_file(entry->u.value.sha1)) {\n@@ -2130,7 +2151,7 @@ static int repack_ref_fn(struct ref_entry *entry, void *cb_data)\n \t\tif (read_ref_full(entry->name, sha1, 0, &flags))\n \t\t\t/* We should at least have found the packed ref. */\n \t\t\tdie(\"Internal error\");\n-\t\tif ((flags & REF_ISSYMREF) || !(flags & REF_ISPACKED))\n+\t\tif ((flags & REF_ISSYMREF) || !(flags & REF_ISPACKED)) {\n \t\t\t/*\n \t\t\t * This packed reference is overridden by a\n \t\t\t * loose reference, so it is OK that its value\n@@ -2139,9 +2160,11 @@ static int repack_ref_fn(struct ref_entry *entry, void *cb_data)\n \t\t\t * collected. For this purpose we don't even\n \t\t\t * care whether the loose reference itself is\n \t\t\t * invalid, broken, symbolic, etc. Silently\n-\t\t\t * omit the packed reference from the output.\n+\t\t\t * remove the packed reference.\n \t\t\t */\n+\t\t\tstring_list_append(refs_to_delete, entry->name);\n \t\t\treturn 0;\n+\t\t}\n \t\t/*\n \t\t * There is no overriding loose reference, so the fact\n \t\t * that this reference doesn't refer to a valid object\n@@ -2150,14 +2173,10 @@ static int repack_ref_fn(struct ref_entry *entry, void *cb_data)\n \t\t * the output.\n \t\t */\n \t\terror(\"%s does not point to a valid object!\", entry->name);\n+\t\tstring_list_append(refs_to_delete, entry->name);\n \t\treturn 0;\n \t}\n \n-\tpeel_status = peel_entry(entry, 0);\n-\twrite_packed_entry(*fd, entry->name, entry->u.value.sha1,\n-\t\t\t peel_status == PEEL_PEELED ?\n-\t\t\t entry->u.value.peeled : NULL);\n-\n \treturn 0;\n }\n \n@@ -2165,6 +2184,8 @@ static int repack_without_ref(const char *refname)\n {\n \tint fd;\n \tstruct ref_dir *packed;\n+\tstruct string_list refs_to_delete = STRING_LIST_INIT_DUP;\n+\tstruct string_list_item *ref_to_delete;\n \n \tif (!get_packed_ref(refname))\n \t\treturn 0; /* refname does not exist in packed refs */\n@@ -2185,8 +2206,13 @@ static int repack_without_ref(const char *refname)\n \t\trollback_lock_file(&packlock);\n \t\treturn 0;\n \t}\n+\tdo_for_each_entry_in_dir(packed, 0, curate_packed_ref_fn, &refs_to_delete);\n+\tfor_each_string_list_item(ref_to_delete, &refs_to_delete) {\n+\t\tif (remove_entry(packed, ref_to_delete->string) == -1)\n+\t\t\tdie(\"internal error\");\n+\t}\n \twrite_or_die(fd, PACKED_REFS_HEADER, strlen(PACKED_REFS_HEADER));\n-\tdo_for_each_entry_in_dir(packed, 0, repack_ref_fn, &fd);\n+\tdo_for_each_entry_in_dir(packed, 0, write_packed_entry_fn, &fd);\n \treturn commit_lock_file(&packlock);\n }\n \n-- \n1.8.3\n"},{"id":"220518","messageId":"1370987312-6761-3-git-send-email-mhagger@alum.mit.edu","threadId":"34110","inReplyTo":"1370987312-6761-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 02/12] pack_refs(): split creation of packed refs and entry writing","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2013-06-11T21:48:22Z","receivedAt":"2013-06-11T21:48:22Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Split pack_refs() into multiple passes:\n\n* Iterate over loose refs. For each one that can be turned into a\n packed ref, create a corresponding entry in the packed refs cache.\n\n* Write the packed refs to the packed-refs file.\n\nThis change isolates the mutation of the packed-refs file to a single\nplace.\n\nSigned-off-by: Michael Haggerty \n---\n refs.c | 48 ++++++++++++++++++++++++++++++++++--------------\n 1 file changed, 34 insertions(+), 14 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex dccfe14..bb3640f 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2019,35 +2019,50 @@ struct ref_to_prune {\n \n struct pack_refs_cb_data {\n \tunsigned int flags;\n+\tstruct ref_dir *packed_refs;\n \tstruct ref_to_prune *ref_to_prune;\n-\tint fd;\n };\n \n-static int pack_one_ref(struct ref_entry *entry, void *cb_data)\n+/*\n+ * An each_ref_entry_fn that is run over loose references only. If\n+ * the loose reference can be packed, add an entry in the packed ref\n+ * cache. If the reference should be pruned, also add it to\n+ * ref_to_prune in the pack_refs_cb_data.\n+ */\n+static int pack_if_possible_fn(struct ref_entry *entry, void *cb_data)\n {\n \tstruct pack_refs_cb_data *cb = cb_data;\n \tenum peel_status peel_status;\n+\tstruct ref_entry *packed_entry;\n \tint is_tag_ref = !prefixcmp(entry->name, \"refs/tags/\");\n \n-\t/* ALWAYS pack refs that were already packed or are tags */\n-\tif (!(cb->flags & PACK_REFS_ALL) && !is_tag_ref &&\n-\t !(entry->flag & REF_ISPACKED))\n+\t/* ALWAYS pack tags */\n+\tif (!(cb->flags & PACK_REFS_ALL) && !is_tag_ref)\n \t\treturn 0;\n \n \t/* Do not pack symbolic or broken refs: */\n \tif ((entry->flag & REF_ISSYMREF) || !ref_resolves_to_object(entry))\n \t\treturn 0;\n \n+\t/* Add a packed ref cache entry equivalent to the loose entry. */\n \tpeel_status = peel_entry(entry, 1);\n \tif (peel_status != PEEL_PEELED && peel_status != PEEL_NON_TAG)\n \t\tdie(\"internal error peeling reference %s (%s)\",\n \t\t entry->name, sha1_to_hex(entry->u.value.sha1));\n-\twrite_packed_entry(cb->fd, entry->name, entry->u.value.sha1,\n-\t\t\t peel_status == PEEL_PEELED ?\n-\t\t\t entry->u.value.peeled : NULL);\n+\tpacked_entry = find_ref(cb->packed_refs, entry->name);\n+\tif (packed_entry) {\n+\t\t/* Overwrite existing packed entry with info from loose entry */\n+\t\tpacked_entry->flag = REF_ISPACKED | REF_KNOWS_PEELED;\n+\t\thashcpy(packed_entry->u.value.sha1, entry->u.value.sha1);\n+\t} else {\n+\t\tpacked_entry = create_ref_entry(entry->name, entry->u.value.sha1,\n+\t\t\t\t\t\tREF_ISPACKED | REF_KNOWS_PEELED, 0);\n+\t\tadd_ref(cb->packed_refs, packed_entry);\n+\t}\n+\thashcpy(packed_entry->u.value.peeled, entry->u.value.peeled);\n \n-\t/* If the ref was already packed, there is no need to prune it. */\n-\tif ((cb->flags & PACK_REFS_PRUNE) && !(entry->flag & REF_ISPACKED)) {\n+\t/* Schedule the loose reference for pruning if requested. */\n+\tif ((cb->flags & PACK_REFS_PRUNE)) {\n \t\tint namelen = strlen(entry->name) + 1;\n \t\tstruct ref_to_prune *n = xcalloc(1, sizeof(*n) + namelen);\n \t\thashcpy(n->sha1, entry->u.value.sha1);\n@@ -2114,16 +2129,21 @@ static struct lock_file packlock;\n int pack_refs(unsigned int flags)\n {\n \tstruct pack_refs_cb_data cbdata;\n+\tint fd;\n \n \tmemset(&cbdata, 0, sizeof(cbdata));\n \tcbdata.flags = flags;\n \n-\tcbdata.fd = hold_lock_file_for_update(&packlock, git_path(\"packed-refs\"),\n-\t\t\t\t\t LOCK_DIE_ON_ERROR);\n+\tfd = hold_lock_file_for_update(&packlock, git_path(\"packed-refs\"),\n+\t\t\t\t LOCK_DIE_ON_ERROR);\n+\tcbdata.packed_refs = get_packed_refs(&ref_cache);\n \n-\twrite_or_die(cbdata.fd, PACKED_REFS_HEADER, strlen(PACKED_REFS_HEADER));\n+\tdo_for_each_entry_in_dir(get_loose_refs(&ref_cache), 0,\n+\t\t\t\t pack_if_possible_fn, &cbdata);\n+\n+\twrite_or_die(fd, PACKED_REFS_HEADER, strlen(PACKED_REFS_HEADER));\n+\tdo_for_each_entry_in_dir(cbdata.packed_refs, 0, write_packed_entry_fn, &fd);\n \n-\tdo_for_each_entry(&ref_cache, \"\", pack_one_ref, &cbdata);\n \tif (commit_lock_file(&packlock) < 0)\n \t\tdie_errno(\"unable to overwrite old ref-pack file\");\n \tprune_refs(cbdata.ref_to_prune);\n-- \n1.8.3\n"},{"id":"220517","messageId":"1370987312-6761-4-git-send-email-mhagger@alum.mit.edu","threadId":"34110","inReplyTo":"1370987312-6761-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 03/12] refs: wrap the packed refs cache in a level of indirection","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2013-06-11T21:48:23Z","receivedAt":"2013-06-11T21:48:23Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"As we know, we can solve any problem in this manner. In this case,\nthe problem is to avoid freeing a packed refs cache while somebody is\nusing it. So add a level of indirection as a prelude to\nreference-counting the packed refs cache.\n\nSigned-off-by: Michael Haggerty \n---\n refs.c | 32 ++++++++++++++++++++++++++------\n 1 file changed, 26 insertions(+), 6 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex bb3640f..4cee36b 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -802,6 +802,10 @@ static int is_refname_available(const char *refname, const char *oldrefname,\n \treturn 1;\n }\n \n+struct packed_ref_cache {\n+\tstruct ref_entry *root;\n+};\n+\n /*\n * Future: need to be in \"struct repository\"\n * when doing a full libification.\n@@ -809,7 +813,7 @@ static int is_refname_available(const char *refname, const char *oldrefname,\n static struct ref_cache {\n \tstruct ref_cache *next;\n \tstruct ref_entry *loose;\n-\tstruct ref_entry *packed;\n+\tstruct packed_ref_cache *packed;\n \t/*\n \t * The submodule name, or \"\" for the main repo. We allocate\n \t * length 1 rather than FLEX_ARRAY so that the main ref_cache\n@@ -821,7 +825,8 @@ static struct ref_cache {\n static void clear_packed_ref_cache(struct ref_cache *refs)\n {\n \tif (refs->packed) {\n-\t\tfree_ref_entry(refs->packed);\n+\t\tfree_ref_entry(refs->packed->root);\n+\t\tfree(refs->packed);\n \t\trefs->packed = NULL;\n \t}\n }\n@@ -992,24 +997,39 @@ static void read_packed_refs(FILE *f, struct ref_dir *dir)\n \t}\n }\n \n-static struct ref_dir *get_packed_refs(struct ref_cache *refs)\n+/*\n+ * Get the packed_ref_cache for the specified ref_cache, creating it\n+ * if necessary.\n+ */\n+static struct packed_ref_cache *get_packed_ref_cache(struct ref_cache *refs)\n {\n \tif (!refs->packed) {\n \t\tconst char *packed_refs_file;\n \t\tFILE *f;\n \n-\t\trefs->packed = create_dir_entry(refs, \"\", 0, 0);\n+\t\trefs->packed = xcalloc(1, sizeof(*refs->packed));\n+\t\trefs->packed->root = create_dir_entry(refs, \"\", 0, 0);\n \t\tif (*refs->name)\n \t\t\tpacked_refs_file = git_path_submodule(refs->name, \"packed-refs\");\n \t\telse\n \t\t\tpacked_refs_file = git_path(\"packed-refs\");\n \t\tf = fopen(packed_refs_file, \"r\");\n \t\tif (f) {\n-\t\t\tread_packed_refs(f, get_ref_dir(refs->packed));\n+\t\t\tread_packed_refs(f, get_ref_dir(refs->packed->root));\n \t\t\tfclose(f);\n \t\t}\n \t}\n-\treturn get_ref_dir(refs->packed);\n+\treturn refs->packed;\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->root);\n+}\n+\n+static struct ref_dir *get_packed_refs(struct ref_cache *refs)\n+{\n+\treturn get_packed_ref_dir(get_packed_ref_cache(refs));\n }\n \n void add_packed_ref(const char *refname, const unsigned char *sha1)\n-- \n1.8.3\n"},{"id":"220519","messageId":"1370987312-6761-5-git-send-email-mhagger@alum.mit.edu","threadId":"34110","inReplyTo":"1370987312-6761-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 04/12] refs: implement simple transactions for the packed-refs file","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2013-06-11T21:48:24Z","receivedAt":"2013-06-11T21:48:24Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Handle simple transactions for the packed-refs file at the\npacked_ref_cache level via new functions lock_packed_refs(),\ncommit_packed_refs(), and rollback_packed_refs().\n\nOnly allow the packed ref cache to be modified (via add_packed_ref())\nwhile the packed refs file is locked.\n\nChange clone to add the new references within a transaction.\n\nSigned-off-by: Michael Haggerty \n---\nThe API docs are not clear about whether it is kosher to read\nlock_file::fd directly. It is only done in one file outside of\nlockfile.c. So this patch stores the fd of the lockfile separately in\nstruct packed_ref_cache, even though the same struct also has a\npointer to the struct lock_file.\n\nSo please let me know if it is OK to read lock_file::fd directly. If\nso, then I will drop the fd member of struct packed_ref_cache, as well\nas the local variable \"fd\" in lock_packed_refs().\n\n builtin/clone.c | 7 ++++-\n refs.c | 91 +++++++++++++++++++++++++++++++++++++++++++++++----------\n refs.h | 27 +++++++++++++++--\n 3 files changed, 106 insertions(+), 19 deletions(-)\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex 66bff57..b0c000a 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -489,17 +489,22 @@ static struct ref *wanted_peer_refs(const struct ref *refs,\n \treturn local_refs;\n }\n \n+static struct lock_file packed_refs_lock;\n+\n static void write_remote_refs(const struct ref *local_refs)\n {\n \tconst struct ref *r;\n \n+\tlock_packed_refs(&packed_refs_lock, LOCK_DIE_ON_ERROR);\n+\n \tfor (r = local_refs; r; r = r->next) {\n \t\tif (!r->peer_ref)\n \t\t\tcontinue;\n \t\tadd_packed_ref(r->peer_ref->name, r->old_sha1);\n \t}\n \n-\tpack_refs(PACK_REFS_ALL);\n+\tif (commit_packed_refs())\n+\t\tdie_errno(\"unable to overwrite old ref-pack file\");\n }\n \n static void write_followtags(const struct ref *refs, const char *msg)\ndiff --git a/refs.c b/refs.c\nindex 4cee36b..114bd5b 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -804,6 +804,16 @@ static int is_refname_available(const char *refname, const char *oldrefname,\n \n struct packed_ref_cache {\n \tstruct ref_entry *root;\n+\n+\t/*\n+\t * Iff the packed-refs file associated with this instance is\n+\t * currently locked for writing, this points at the associated\n+\t * lock (which is owned by somebody else).\n+\t */\n+\tstruct lock_file *lock;\n+\n+\t/* If locked, the file descriptor of the lock file. */\n+\tint fd;\n };\n \n /*\n@@ -825,6 +835,8 @@ static struct ref_cache {\n static void clear_packed_ref_cache(struct ref_cache *refs)\n {\n \tif (refs->packed) {\n+\t\tif (refs->packed->lock)\n+\t\t\tdie(\"internal error: packed-ref cache cleared while locked\");\n \t\tfree_ref_entry(refs->packed->root);\n \t\tfree(refs->packed);\n \t\trefs->packed = NULL;\n@@ -1009,6 +1021,7 @@ static struct packed_ref_cache *get_packed_ref_cache(struct ref_cache *refs)\n \n \t\trefs->packed = xcalloc(1, sizeof(*refs->packed));\n \t\trefs->packed->root = create_dir_entry(refs, \"\", 0, 0);\n+\t\trefs->packed->fd = -1;\n \t\tif (*refs->name)\n \t\t\tpacked_refs_file = git_path_submodule(refs->name, \"packed-refs\");\n \t\telse\n@@ -1034,7 +1047,12 @@ static struct ref_dir *get_packed_refs(struct ref_cache *refs)\n \n void add_packed_ref(const char *refname, const unsigned char *sha1)\n {\n-\tadd_ref(get_packed_refs(&ref_cache),\n+\tstruct packed_ref_cache *packed_ref_cache =\n+\t\tget_packed_ref_cache(&ref_cache);\n+\n+\tif (!packed_ref_cache->lock)\n+\t\tdie(\"internal error: packed refs not locked\");\n+\tadd_ref(get_packed_ref_dir(packed_ref_cache),\n \t\tcreate_ref_entry(refname, sha1, REF_ISPACKED, 1));\n }\n \n@@ -2031,6 +2049,56 @@ static int write_packed_entry_fn(struct ref_entry *entry, void *cb_data)\n \treturn 0;\n }\n \n+int lock_packed_refs(struct lock_file *lock, int flags)\n+{\n+\tint fd;\n+\tstruct packed_ref_cache *packed_ref_cache;\n+\n+\t/* Discard the old cache because it might be invalid: */\n+\tclear_packed_ref_cache(&ref_cache);\n+\tfd = hold_lock_file_for_update(lock, git_path(\"packed-refs\"), flags);\n+\tif (fd < 0)\n+\t\treturn -1;\n+\t/* Read the current packed-refs while holding the lock: */\n+\tpacked_ref_cache = get_packed_ref_cache(&ref_cache);\n+\tpacked_ref_cache->lock = lock;\n+\tpacked_ref_cache->fd = fd;\n+\treturn 0;\n+}\n+\n+int commit_packed_refs(void)\n+{\n+\tstruct packed_ref_cache *packed_ref_cache =\n+\t\tget_packed_ref_cache(&ref_cache);\n+\tint fd = packed_ref_cache->fd;\n+\tint error = 0;\n+\n+\tif (!packed_ref_cache->lock)\n+\t\tdie(\"internal error: packed-refs not locked\");\n+\twrite_or_die(fd, PACKED_REFS_HEADER, strlen(PACKED_REFS_HEADER));\n+\n+\tdo_for_each_entry_in_dir(get_packed_ref_dir(packed_ref_cache),\n+\t\t\t\t 0, write_packed_entry_fn, &fd);\n+\tif (commit_lock_file(packed_ref_cache->lock))\n+\t\terror = -1;\n+\tpacked_ref_cache->lock = NULL;\n+\tpacked_ref_cache->fd = -1;\n+\treturn error;\n+}\n+\n+void rollback_packed_refs(void)\n+{\n+\tstruct packed_ref_cache *packed_ref_cache =\n+\t\tget_packed_ref_cache(&ref_cache);\n+\n+\tif (!packed_ref_cache->lock)\n+\t\tdie(\"internal error: packed-refs not locked\");\n+\trollback_lock_file(packed_ref_cache->lock);\n+\tpacked_ref_cache->lock = NULL;\n+\tpacked_ref_cache->fd = -1;\n+\tclear_packed_ref_cache(&ref_cache);\n+}\n+\n struct ref_to_prune {\n \tstruct ref_to_prune *next;\n \tunsigned char sha1[20];\n@@ -2149,23 +2217,19 @@ static struct lock_file packlock;\n int pack_refs(unsigned int flags)\n {\n \tstruct pack_refs_cb_data cbdata;\n-\tint fd;\n \n \tmemset(&cbdata, 0, sizeof(cbdata));\n \tcbdata.flags = flags;\n \n-\tfd = hold_lock_file_for_update(&packlock, git_path(\"packed-refs\"),\n-\t\t\t\t LOCK_DIE_ON_ERROR);\n+\tlock_packed_refs(&packlock, LOCK_DIE_ON_ERROR);\n \tcbdata.packed_refs = get_packed_refs(&ref_cache);\n \n \tdo_for_each_entry_in_dir(get_loose_refs(&ref_cache), 0,\n \t\t\t\t pack_if_possible_fn, &cbdata);\n \n-\twrite_or_die(fd, PACKED_REFS_HEADER, strlen(PACKED_REFS_HEADER));\n-\tdo_for_each_entry_in_dir(cbdata.packed_refs, 0, write_packed_entry_fn, &fd);\n-\n-\tif (commit_lock_file(&packlock) < 0)\n+\tif (commit_packed_refs())\n \t\tdie_errno(\"unable to overwrite old ref-pack file\");\n+\n \tprune_refs(cbdata.ref_to_prune);\n \treturn 0;\n }\n@@ -2222,7 +2286,6 @@ static int curate_packed_ref_fn(struct ref_entry *entry, void *cb_data)\n \n static int repack_without_ref(const char *refname)\n {\n-\tint fd;\n \tstruct ref_dir *packed;\n \tstruct string_list refs_to_delete = STRING_LIST_INIT_DUP;\n \tstruct string_list_item *ref_to_delete;\n@@ -2230,12 +2293,10 @@ static int repack_without_ref(const char *refname)\n \tif (!get_packed_ref(refname))\n \t\treturn 0; /* refname does not exist in packed refs */\n \n-\tfd = hold_lock_file_for_update(&packlock, git_path(\"packed-refs\"), 0);\n-\tif (fd < 0) {\n+\tif (lock_packed_refs(&packlock, 0)) {\n \t\tunable_to_lock_error(git_path(\"packed-refs\"), errno);\n \t\treturn error(\"cannot delete '%s' from packed refs\", refname);\n \t}\n-\tclear_packed_ref_cache(&ref_cache);\n \tpacked = get_packed_refs(&ref_cache);\n \t/* Remove refname from the cache. */\n \tif (remove_entry(packed, refname) == -1) {\n@@ -2243,7 +2304,7 @@ static int repack_without_ref(const char *refname)\n \t\t * The packed entry disappeared while we were\n \t\t * acquiring the lock.\n \t\t */\n-\t\trollback_lock_file(&packlock);\n+\t\trollback_packed_refs();\n \t\treturn 0;\n \t}\n \tdo_for_each_entry_in_dir(packed, 0, curate_packed_ref_fn, &refs_to_delete);\n@@ -2251,9 +2312,7 @@ static int repack_without_ref(const char *refname)\n \t\tif (remove_entry(packed, ref_to_delete->string) == -1)\n \t\t\tdie(\"internal error\");\n \t}\n-\twrite_or_die(fd, PACKED_REFS_HEADER, strlen(PACKED_REFS_HEADER));\n-\tdo_for_each_entry_in_dir(packed, 0, write_packed_entry_fn, &fd);\n-\treturn commit_lock_file(&packlock);\n+\treturn commit_packed_refs();\n }\n \n int delete_ref(const char *refname, const unsigned char *sha1, int delopt)\ndiff --git a/refs.h b/refs.h\nindex 246bf60..b275124 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -77,12 +77,35 @@ extern int for_each_rawref(each_ref_fn, void *);\n extern void warn_dangling_symref(FILE *fp, const char *msg_fmt, const char *refname);\n \n /*\n- * Add a reference to the in-memory packed reference cache. To actually\n- * write the reference to the packed-refs file, call pack_refs().\n+ * Lock the packed-refs file for writing using the specified lock_file\n+ * instance. Flags is passed to hold_lock_file_for_update(). Return\n+ * 0 on success.\n+ */\n+extern int lock_packed_refs(struct lock_file *lock, int flags);\n+\n+/*\n+ * Add a reference to the in-memory packed reference cache. This may\n+ * only be called while the packed-refs file is locked (see\n+ * lock_packed_refs()). To actually write the packed-refs file, call\n+ * commit_packed_refs().\n */\n extern void add_packed_ref(const char *refname, const unsigned char *sha1);\n \n /*\n+ * Write the current version of the packed refs cache from memory to\n+ * disk. The packed-refs file must already be locked for writing (see\n+ * lock_packed_refs()). Return zero on success.\n+ */\n+extern int commit_packed_refs(void);\n+\n+/*\n+ * Rollback the lockfile for the packed-refs file, and discard the\n+ * in-memory packed reference cache. (The packed-refs file will be\n+ * read anew if it is needed again after this function is called.)\n+ */\n+extern void rollback_packed_refs(void);\n+\n+/*\n * Flags for controlling behaviour of pack_refs()\n * PACK_REFS_PRUNE: Prune loose refs after packing\n * PACK_REFS_ALL: Pack _all_ refs, not just tags and already packed refs\n-- \n1.8.3\n"},{"id":"220525","messageId":"1370987312-6761-6-git-send-email-mhagger@alum.mit.edu","threadId":"34110","inReplyTo":"1370987312-6761-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 05/12] refs: manage lifetime of packed refs cache via reference counting","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2013-06-11T21:48:25Z","receivedAt":"2013-06-11T21:48:25Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"In struct packed_ref_cache, keep a count of the number of users of the\ndata structure. Only free the packed ref cache when the reference\ncount goes to zero rather than when the packed ref cache is cleared.\nThis mechanism will be used to prevent the cache data structure from\nbeing freed while it is being iterated over.\n\nSo far, only the reference in struct ref_cache::packed is counted;\nother users will be adjusted in separate commits.\n\nSigned-off-by: Michael Haggerty \n---\n refs.c | 39 ++++++++++++++++++++++++++++++++++++---\n 1 file changed, 36 insertions(+), 3 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 114bd5b..b0d77a7 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -806,6 +806,14 @@ struct packed_ref_cache {\n \tstruct ref_entry *root;\n \n \t/*\n+\t * Count of references to the data structure in this instance,\n+\t * including the pointer from ref_cache::packed if any. The\n+\t * data will not be freed as long as the reference count is\n+\t * nonzero.\n+\t */\n+\tunsigned int referrers;\n+\n+\t/*\n \t * Iff the packed-refs file associated with this instance is\n \t * currently locked for writing, this points at the associated\n \t * lock (which is owned by somebody else).\n@@ -832,14 +840,38 @@ static struct ref_cache {\n \tchar name[1];\n } ref_cache, *submodule_ref_caches;\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_entry(packed_refs->root);\n+\t\tfree(packed_refs);\n+\t\treturn 1;\n+\t} else {\n+\t\treturn 0;\n+\t}\n+}\n+\n static void clear_packed_ref_cache(struct ref_cache *refs)\n {\n \tif (refs->packed) {\n-\t\tif (refs->packed->lock)\n+\t\tstruct packed_ref_cache *packed_refs = refs->packed;\n+\n+\t\tif (packed_refs->lock)\n \t\t\tdie(\"internal error: packed-ref cache cleared while locked\");\n-\t\tfree_ref_entry(refs->packed->root);\n-\t\tfree(refs->packed);\n \t\trefs->packed = NULL;\n+\t\trelease_packed_ref_cache(packed_refs);\n \t}\n }\n \n@@ -1020,6 +1052,7 @@ static struct packed_ref_cache *get_packed_ref_cache(struct ref_cache *refs)\n \t\tFILE *f;\n \n \t\trefs->packed = xcalloc(1, sizeof(*refs->packed));\n+\t\tacquire_packed_ref_cache(refs->packed);\n \t\trefs->packed->root = create_dir_entry(refs, \"\", 0, 0);\n \t\trefs->packed->fd = -1;\n \t\tif (*refs->name)\n-- \n1.8.3\n"},{"id":"220527","messageId":"1370987312-6761-7-git-send-email-mhagger@alum.mit.edu","threadId":"34110","inReplyTo":"1370987312-6761-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 06/12] do_for_each_entry(): increment the packed refs cache refcount","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2013-06-11T21:48:26Z","receivedAt":"2013-06-11T21:48:26Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"This function calls a user-supplied callback function which could do\nsomething that causes the packed refs cache to be invalidated. So\nacquire a reference count on the data structure to prevent our copy\nfrom being freed while we are iterating over it.\n\nSigned-off-by: Michael Haggerty \n---\n refs.c | 5 ++++-\n 1 file changed, 4 insertions(+), 1 deletion(-)\n\ndiff --git a/refs.c b/refs.c\nindex b0d77a7..d8e8ce2 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1587,10 +1587,12 @@ void warn_dangling_symref(FILE *fp, const char *msg_fmt, const char *refname)\n static int do_for_each_entry(struct ref_cache *refs, const char *base,\n \t\t\t each_ref_entry_fn fn, void *cb_data)\n {\n-\tstruct ref_dir *packed_dir = get_packed_refs(refs);\n+\tstruct packed_ref_cache *packed_ref_cache = get_packed_ref_cache(refs);\n+\tstruct ref_dir *packed_dir = get_packed_ref_dir(packed_ref_cache);\n \tstruct ref_dir *loose_dir = get_loose_refs(refs);\n \tint retval = 0;\n \n+\tacquire_packed_ref_cache(packed_ref_cache);\n \tif (base && *base) {\n \t\tpacked_dir = find_containing_dir(packed_dir, base, 0);\n \t\tloose_dir = find_containing_dir(loose_dir, base, 0);\n@@ -1611,6 +1613,7 @@ static int do_for_each_entry(struct ref_cache *refs, const char *base,\n \t\t\t\tloose_dir, 0, fn, cb_data);\n \t}\n \n+\trelease_packed_ref_cache(packed_ref_cache);\n \treturn retval;\n }\n \n-- \n1.8.3\n"},{"id":"220520","messageId":"1370987312-6761-8-git-send-email-mhagger@alum.mit.edu","threadId":"34110","inReplyTo":"1370987312-6761-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 07/12] packed_ref_cache: increment refcount when locked","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2013-06-11T21:48:27Z","receivedAt":"2013-06-11T21:48:27Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Increment the packed_ref_cache reference count while it is locked to\nprevent its being freed.\n\nSigned-off-by: Michael Haggerty \n---\n refs.c | 8 +++++++-\n 1 file changed, 7 insertions(+), 1 deletion(-)\n\ndiff --git a/refs.c b/refs.c\nindex d8e8ce2..92c8e97 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -816,7 +816,9 @@ struct packed_ref_cache {\n \t/*\n \t * Iff the packed-refs file associated with this instance is\n \t * currently locked for writing, this points at the associated\n-\t * lock (which is owned by somebody else).\n+\t * lock (which is owned by somebody else). The referrer count\n+\t * is also incremented when the file is locked and decremented\n+\t * when it is unlocked.\n \t */\n \tstruct lock_file *lock;\n \n@@ -2099,6 +2101,8 @@ int lock_packed_refs(struct lock_file *lock, int flags)\n \tpacked_ref_cache = get_packed_ref_cache(&ref_cache);\n \tpacked_ref_cache->lock = lock;\n \tpacked_ref_cache->fd = fd;\n+\t/* Increment the reference count to prevent it from being freed: */\n+\tacquire_packed_ref_cache(packed_ref_cache);\n \treturn 0;\n }\n \n@@ -2119,6 +2123,7 @@ int commit_packed_refs(void)\n \t\terror = -1;\n \tpacked_ref_cache->lock = NULL;\n \tpacked_ref_cache->fd = -1;\n+\trelease_packed_ref_cache(packed_ref_cache);\n \treturn error;\n }\n \n@@ -2132,6 +2137,7 @@ void rollback_packed_refs(void)\n \trollback_lock_file(packed_ref_cache->lock);\n \tpacked_ref_cache->lock = NULL;\n \tpacked_ref_cache->fd = -1;\n+\trelease_packed_ref_cache(packed_ref_cache);\n \tclear_packed_ref_cache(&ref_cache);\n }\n \n-- \n1.8.3\n"},{"id":"220523","messageId":"1370987312-6761-9-git-send-email-mhagger@alum.mit.edu","threadId":"34110","inReplyTo":"1370987312-6761-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 08/12] Extract a struct stat_data from cache_entry","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2013-06-11T21:48:28Z","receivedAt":"2013-06-11T21:48:28Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Add public functions fill_stat_data() and match_stat_data() to work\nwith it. This infrastructure will later be used to check the validity\nof other types of file.\n\nSigned-off-by: Michael Haggerty \n---\nI'm not too familiar with this part of the code, so please make sure\nthat I've put the dividing line at the right place.\n\n builtin/ls-files.c | 12 +++--\n cache.h | 33 +++++++++---\n read-cache.c | 151 +++++++++++++++++++++++++++++------------------------\n 3 files changed, 116 insertions(+), 80 deletions(-)\n\ndiff --git a/builtin/ls-files.c b/builtin/ls-files.c\nindex 2202072..6a0730f 100644\n--- a/builtin/ls-files.c\n+++ b/builtin/ls-files.c\n@@ -165,11 +165,13 @@ static void show_ce_entry(const char *tag, struct cache_entry *ce)\n \t}\n \twrite_name(ce->name, ce_namelen(ce));\n \tif (debug_mode) {\n-\t\tprintf(\" ctime: %d:%d\\n\", ce->ce_ctime.sec, ce->ce_ctime.nsec);\n-\t\tprintf(\" mtime: %d:%d\\n\", ce->ce_mtime.sec, ce->ce_mtime.nsec);\n-\t\tprintf(\" dev: %d\\tino: %d\\n\", ce->ce_dev, ce->ce_ino);\n-\t\tprintf(\" uid: %d\\tgid: %d\\n\", ce->ce_uid, ce->ce_gid);\n-\t\tprintf(\" size: %d\\tflags: %x\\n\", ce->ce_size, ce->ce_flags);\n+\t\tstruct stat_data *sd = &ce->ce_stat_data;\n+\n+\t\tprintf(\" ctime: %d:%d\\n\", sd->sd_ctime.sec, sd->sd_ctime.nsec);\n+\t\tprintf(\" mtime: %d:%d\\n\", sd->sd_mtime.sec, sd->sd_mtime.nsec);\n+\t\tprintf(\" dev: %d\\tino: %d\\n\", sd->sd_dev, sd->sd_ino);\n+\t\tprintf(\" uid: %d\\tgid: %d\\n\", sd->sd_uid, sd->sd_gid);\n+\t\tprintf(\" size: %d\\tflags: %x\\n\", sd->sd_size, ce->ce_flags);\n \t}\n }\n \ndiff --git a/cache.h b/cache.h\nindex df532f8..207f849 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -119,15 +119,19 @@ struct cache_time {\n \tunsigned int nsec;\n };\n \n+struct stat_data {\n+\tstruct cache_time sd_ctime;\n+\tstruct cache_time sd_mtime;\n+\tunsigned int sd_dev;\n+\tunsigned int sd_ino;\n+\tunsigned int sd_uid;\n+\tunsigned int sd_gid;\n+\tunsigned int sd_size;\n+};\n+\n struct cache_entry {\n-\tstruct cache_time ce_ctime;\n-\tstruct cache_time ce_mtime;\n-\tunsigned int ce_dev;\n-\tunsigned int ce_ino;\n+\tstruct stat_data ce_stat_data;\n \tunsigned int ce_mode;\n-\tunsigned int ce_uid;\n-\tunsigned int ce_gid;\n-\tunsigned int ce_size;\n \tunsigned int ce_flags;\n \tunsigned int ce_namelen;\n \tunsigned char sha1[20];\n@@ -509,6 +513,21 @@ extern int limit_pathspec_to_literal(void);\n #define HASH_FORMAT_CHECK 2\n extern int index_fd(unsigned char *sha1, int fd, struct stat *st, enum object_type type, const char *path, unsigned flags);\n extern int index_path(unsigned char *sha1, const char *path, struct stat *st, unsigned flags);\n+\n+/*\n+ * Record to sd the data from st that we use to check whether a file\n+ * might have changed.\n+ */\n+extern void fill_stat_data(struct stat_data *sd, struct stat *st);\n+\n+/*\n+ * Return 0 if st is consistent with a file not having been changed\n+ * since sd was filled. If there are differences, return a\n+ * combination of MTIME_CHANGED, CTIME_CHANGED, OWNER_CHANGED,\n+ * INODE_CHANGED, and DATA_CHANGED.\n+ */\n+extern int match_stat_data(struct stat_data *sd, struct stat *st);\n+\n extern void fill_stat_cache_info(struct cache_entry *ce, struct stat *st);\n \n #define REFRESH_REALLY\t\t0x0001\t/* ignore_valid */\ndiff --git a/read-cache.c b/read-cache.c\nindex 04ed561..4c4328e 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -67,6 +67,61 @@ void rename_index_entry_at(struct index_state *istate, int nr, const char *new_n\n \tadd_index_entry(istate, new, ADD_CACHE_OK_TO_ADD|ADD_CACHE_OK_TO_REPLACE);\n }\n \n+void fill_stat_data(struct stat_data *sd, struct stat *st)\n+{\n+\tsd->sd_ctime.sec = (unsigned int)st->st_ctime;\n+\tsd->sd_mtime.sec = (unsigned int)st->st_mtime;\n+\tsd->sd_ctime.nsec = ST_CTIME_NSEC(*st);\n+\tsd->sd_mtime.nsec = ST_MTIME_NSEC(*st);\n+\tsd->sd_dev = st->st_dev;\n+\tsd->sd_ino = st->st_ino;\n+\tsd->sd_uid = st->st_uid;\n+\tsd->sd_gid = st->st_gid;\n+\tsd->sd_size = st->st_size;\n+}\n+\n+int match_stat_data(struct stat_data *sd, struct stat *st)\n+{\n+\tint changed = 0;\n+\n+\tif (sd->sd_mtime.sec != (unsigned int)st->st_mtime)\n+\t\tchanged |= MTIME_CHANGED;\n+\tif (trust_ctime && check_stat &&\n+\t sd->sd_ctime.sec != (unsigned int)st->st_ctime)\n+\t\tchanged |= CTIME_CHANGED;\n+\n+#ifdef USE_NSEC\n+\tif (check_stat && sd->sd_mtime.nsec != ST_MTIME_NSEC(*st))\n+\t\tchanged |= MTIME_CHANGED;\n+\tif (trust_ctime && check_stat &&\n+\t sd->sd_ctime.nsec != ST_CTIME_NSEC(*st))\n+\t\tchanged |= CTIME_CHANGED;\n+#endif\n+\n+\tif (check_stat) {\n+\t\tif (sd->sd_uid != (unsigned int) st->st_uid ||\n+\t\t\tsd->sd_gid != (unsigned int) st->st_gid)\n+\t\t\tchanged |= OWNER_CHANGED;\n+\t\tif (sd->sd_ino != (unsigned int) st->st_ino)\n+\t\t\tchanged |= INODE_CHANGED;\n+\t}\n+\n+#ifdef USE_STDEV\n+\t/*\n+\t * st_dev breaks on network filesystems where different\n+\t * clients will have different views of what \"device\"\n+\t * the filesystem is on\n+\t */\n+\tif (check_stat && sd->sd_dev != (unsigned int) st->st_dev)\n+\t\t\tchanged |= INODE_CHANGED;\n+#endif\n+\n+\tif (sd->sd_size != (unsigned int) st->st_size)\n+\t\tchanged |= DATA_CHANGED;\n+\n+\treturn changed;\n+}\n+\n /*\n * This only updates the \"non-critical\" parts of the directory\n * cache, ie the parts that aren't tracked by GIT, and only used\n@@ -74,15 +129,7 @@ void rename_index_entry_at(struct index_state *istate, int nr, const char *new_n\n */\n void fill_stat_cache_info(struct cache_entry *ce, struct stat *st)\n {\n-\tce->ce_ctime.sec = (unsigned int)st->st_ctime;\n-\tce->ce_mtime.sec = (unsigned int)st->st_mtime;\n-\tce->ce_ctime.nsec = ST_CTIME_NSEC(*st);\n-\tce->ce_mtime.nsec = ST_MTIME_NSEC(*st);\n-\tce->ce_dev = st->st_dev;\n-\tce->ce_ino = st->st_ino;\n-\tce->ce_uid = st->st_uid;\n-\tce->ce_gid = st->st_gid;\n-\tce->ce_size = st->st_size;\n+\tfill_stat_data(&ce->ce_stat_data, st);\n \n \tif (assume_unchanged)\n \t\tce->ce_flags |= CE_VALID;\n@@ -195,43 +242,11 @@ static int ce_match_stat_basic(struct cache_entry *ce, struct stat *st)\n \tdefault:\n \t\tdie(\"internal error: ce_mode is %o\", ce->ce_mode);\n \t}\n-\tif (ce->ce_mtime.sec != (unsigned int)st->st_mtime)\n-\t\tchanged |= MTIME_CHANGED;\n-\tif (trust_ctime && check_stat &&\n-\t ce->ce_ctime.sec != (unsigned int)st->st_ctime)\n-\t\tchanged |= CTIME_CHANGED;\n \n-#ifdef USE_NSEC\n-\tif (check_stat && ce->ce_mtime.nsec != ST_MTIME_NSEC(*st))\n-\t\tchanged |= MTIME_CHANGED;\n-\tif (trust_ctime && check_stat &&\n-\t ce->ce_ctime.nsec != ST_CTIME_NSEC(*st))\n-\t\tchanged |= CTIME_CHANGED;\n-#endif\n-\n-\tif (check_stat) {\n-\t\tif (ce->ce_uid != (unsigned int) st->st_uid ||\n-\t\t\tce->ce_gid != (unsigned int) st->st_gid)\n-\t\t\tchanged |= OWNER_CHANGED;\n-\t\tif (ce->ce_ino != (unsigned int) st->st_ino)\n-\t\t\tchanged |= INODE_CHANGED;\n-\t}\n-\n-#ifdef USE_STDEV\n-\t/*\n-\t * st_dev breaks on network filesystems where different\n-\t * clients will have different views of what \"device\"\n-\t * the filesystem is on\n-\t */\n-\tif (check_stat && ce->ce_dev != (unsigned int) st->st_dev)\n-\t\t\tchanged |= INODE_CHANGED;\n-#endif\n-\n-\tif (ce->ce_size != (unsigned int) st->st_size)\n-\t\tchanged |= DATA_CHANGED;\n+\tchanged |= match_stat_data(&ce->ce_stat_data, st);\n \n \t/* Racily smudged entry? */\n-\tif (!ce->ce_size) {\n+\tif (!ce->ce_stat_data.sd_size) {\n \t\tif (!is_empty_blob_sha1(ce->sha1))\n \t\t\tchanged |= DATA_CHANGED;\n \t}\n@@ -245,11 +260,11 @@ static int is_racy_timestamp(const struct index_state *istate, struct cache_entr\n \t\tistate->timestamp.sec &&\n #ifdef USE_NSEC\n \t\t /* nanosecond timestamped files can also be racy! */\n-\t\t(istate->timestamp.sec < ce->ce_mtime.sec ||\n-\t\t (istate->timestamp.sec == ce->ce_mtime.sec &&\n-\t\t istate->timestamp.nsec <= ce->ce_mtime.nsec))\n+\t\t(istate->timestamp.sec < ce->ce_stat_data.sd_mtime.sec ||\n+\t\t (istate->timestamp.sec == ce->ce_stat_data.sd_mtime.sec &&\n+\t\t istate->timestamp.nsec <= ce->ce_stat_data.sd_mtime.nsec))\n #else\n-\t\tistate->timestamp.sec <= ce->ce_mtime.sec\n+\t\tistate->timestamp.sec <= ce->ce_stat_data.sd_mtime.sec\n #endif\n \t\t );\n }\n@@ -340,7 +355,7 @@ int ie_modified(const struct index_state *istate,\n \t * then we know it is.\n \t */\n \tif ((changed & DATA_CHANGED) &&\n-\t (S_ISGITLINK(ce->ce_mode) || ce->ce_size != 0))\n+\t (S_ISGITLINK(ce->ce_mode) || ce->ce_stat_data.sd_size != 0))\n \t\treturn changed;\n \n \tchanged_fs = ce_modified_check_fs(ce, st);\n@@ -1322,16 +1337,16 @@ static struct cache_entry *cache_entry_from_ondisk(struct ondisk_cache_entry *on\n {\n \tstruct cache_entry *ce = xmalloc(cache_entry_size(len));\n \n-\tce->ce_ctime.sec = ntoh_l(ondisk->ctime.sec);\n-\tce->ce_mtime.sec = ntoh_l(ondisk->mtime.sec);\n-\tce->ce_ctime.nsec = ntoh_l(ondisk->ctime.nsec);\n-\tce->ce_mtime.nsec = ntoh_l(ondisk->mtime.nsec);\n-\tce->ce_dev = ntoh_l(ondisk->dev);\n-\tce->ce_ino = ntoh_l(ondisk->ino);\n+\tce->ce_stat_data.sd_ctime.sec = ntoh_l(ondisk->ctime.sec);\n+\tce->ce_stat_data.sd_mtime.sec = ntoh_l(ondisk->mtime.sec);\n+\tce->ce_stat_data.sd_ctime.nsec = ntoh_l(ondisk->ctime.nsec);\n+\tce->ce_stat_data.sd_mtime.nsec = ntoh_l(ondisk->mtime.nsec);\n+\tce->ce_stat_data.sd_dev = ntoh_l(ondisk->dev);\n+\tce->ce_stat_data.sd_ino = ntoh_l(ondisk->ino);\n \tce->ce_mode = ntoh_l(ondisk->mode);\n-\tce->ce_uid = ntoh_l(ondisk->uid);\n-\tce->ce_gid = ntoh_l(ondisk->gid);\n-\tce->ce_size = ntoh_l(ondisk->size);\n+\tce->ce_stat_data.sd_uid = ntoh_l(ondisk->uid);\n+\tce->ce_stat_data.sd_gid = ntoh_l(ondisk->gid);\n+\tce->ce_stat_data.sd_size = ntoh_l(ondisk->size);\n \tce->ce_flags = flags & ~CE_NAMEMASK;\n \tce->ce_namelen = len;\n \thashcpy(ce->sha1, ondisk->sha1);\n@@ -1608,7 +1623,7 @@ static void ce_smudge_racily_clean_entry(struct cache_entry *ce)\n \t * The only thing we care about in this function is to smudge the\n \t * falsely clean entry due to touch-update-touch race, so we leave\n \t * everything else as they are. We are called for entries whose\n-\t * ce_mtime match the index file mtime.\n+\t * ce_stat_data.sd_mtime match the index file mtime.\n \t *\n \t * Note that this actually does not do much for gitlinks, for\n \t * which ce_match_stat_basic() always goes to the actual\n@@ -1647,7 +1662,7 @@ static void ce_smudge_racily_clean_entry(struct cache_entry *ce)\n \t\t * file, and never calls us, so the cached size information\n \t\t * for \"frotz\" stays 6 which does not match the filesystem.\n \t\t */\n-\t\tce->ce_size = 0;\n+\t\tce->ce_stat_data.sd_size = 0;\n \t}\n }\n \n@@ -1657,16 +1672,16 @@ static char *copy_cache_entry_to_ondisk(struct ondisk_cache_entry *ondisk,\n {\n \tshort flags;\n \n-\tondisk->ctime.sec = htonl(ce->ce_ctime.sec);\n-\tondisk->mtime.sec = htonl(ce->ce_mtime.sec);\n-\tondisk->ctime.nsec = htonl(ce->ce_ctime.nsec);\n-\tondisk->mtime.nsec = htonl(ce->ce_mtime.nsec);\n-\tondisk->dev = htonl(ce->ce_dev);\n-\tondisk->ino = htonl(ce->ce_ino);\n+\tondisk->ctime.sec = htonl(ce->ce_stat_data.sd_ctime.sec);\n+\tondisk->mtime.sec = htonl(ce->ce_stat_data.sd_mtime.sec);\n+\tondisk->ctime.nsec = htonl(ce->ce_stat_data.sd_ctime.nsec);\n+\tondisk->mtime.nsec = htonl(ce->ce_stat_data.sd_mtime.nsec);\n+\tondisk->dev = htonl(ce->ce_stat_data.sd_dev);\n+\tondisk->ino = htonl(ce->ce_stat_data.sd_ino);\n \tondisk->mode = htonl(ce->ce_mode);\n-\tondisk->uid = htonl(ce->ce_uid);\n-\tondisk->gid = htonl(ce->ce_gid);\n-\tondisk->size = htonl(ce->ce_size);\n+\tondisk->uid = htonl(ce->ce_stat_data.sd_uid);\n+\tondisk->gid = htonl(ce->ce_stat_data.sd_gid);\n+\tondisk->size = htonl(ce->ce_stat_data.sd_size);\n \thashcpy(ondisk->sha1, ce->sha1);\n \n \tflags = ce->ce_flags;\n-- \n1.8.3\n"},{"id":"220521","messageId":"1370987312-6761-10-git-send-email-mhagger@alum.mit.edu","threadId":"34110","inReplyTo":"1370987312-6761-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 09/12] add a stat_validity struct","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2013-06-11T21:48:29Z","receivedAt":"2013-06-11T21:48:29Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"It can sometimes be useful to know whether a path in the\nfilesystem has been updated without going to the work of\nopening and re-reading its content. We trust the stat()\ninformation on disk already to handle index updates, and we\ncan use the same trick here.\n\nThis patch introduces a \"stat_validity\" struct which\nencapsulates the concept of checking the stat-freshness of a\nfile. It is implemented on top of \"struct stat_data\" to\nreuse the logic about which stat entries to trust for a\nparticular platform, but hides the complexity behind two\nsimple functions: check and update.\n\nSigned-off-by: Michael Haggerty \n---\nThis is *very* similar to a patch by Jeff King [1],\nexcept that it is based on the struct stat_data that I extracted from\ncache_entry rather than using cache_entries directly. I would have\nleft Peff the author except that I don't want to risk putting him on\nthe hook for any mistakes that I might have made. But if it is\nappropriate, don't hesitate to make him author again.\n\n[1] http://article.gmane.org/gmane.comp.version-control.git/223526\n\n cache.h | 27 +++++++++++++++++++++++++++\n read-cache.c | 30 ++++++++++++++++++++++++++++++\n 2 files changed, 57 insertions(+)\n\ndiff --git a/cache.h b/cache.h\nindex 207f849..15f5110 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1358,4 +1358,31 @@ int checkout_fast_forward(const unsigned char *from,\n \n int sane_execvp(const char *file, char *const argv[]);\n \n+/*\n+ * A struct to encapsulate the concept of whether a file has changed\n+ * since we last checked it. This uses criteria similar to those used\n+ * for the index.\n+ */\n+struct stat_validity {\n+\tstruct stat_data *sd;\n+};\n+\n+void stat_validity_clear(struct stat_validity *sv);\n+\n+/*\n+ * Returns 1 if the path is a regular file (or a symlink to a regular\n+ * file) and matches the saved stat_validity, 0 otherwise. A missing\n+ * or inaccessible file is considered a match if the struct was just\n+ * initialized, or if the previous update found an inaccessible file.\n+ */\n+int stat_validity_check(struct stat_validity *sv, const char *path);\n+\n+/*\n+ * Update the stat_validity from a file opened at descriptor fd. If\n+ * the file is missing, inaccessible, or not a regular file, then\n+ * future calls to stat_validity_check will match iff one of those\n+ * conditions continues to be true.\n+ */\n+void stat_validity_update(struct stat_validity *sv, int fd);\n+\n #endif /* CACHE_H */\ndiff --git a/read-cache.c b/read-cache.c\nindex 4c4328e..73e85a4 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -1948,3 +1948,33 @@ void *read_blob_data_from_index(struct index_state *istate, const char *path, un\n \t\t*size = sz;\n \treturn data;\n }\n+\n+void stat_validity_clear(struct stat_validity *sv)\n+{\n+\tfree(sv->sd);\n+\tsv->sd = NULL;\n+}\n+\n+int stat_validity_check(struct stat_validity *sv, const char *path)\n+{\n+\tstruct stat st;\n+\n+\tif (stat(path, &st) < 0)\n+\t\treturn sv->sd == NULL;\n+\tif (!sv->sd)\n+\t\treturn 0;\n+\treturn S_ISREG(st.st_mode) && !match_stat_data(sv->sd, &st);\n+}\n+\n+void stat_validity_update(struct stat_validity *sv, int fd)\n+{\n+\tstruct stat st;\n+\n+\tif (fstat(fd, &st) < 0 || !S_ISREG(st.st_mode))\n+\t\tstat_validity_clear(sv);\n+\telse {\n+\t\tif (!sv->sd)\n+\t\t\tsv->sd = xcalloc(1, sizeof(struct stat_data));\n+\t\tfill_stat_data(sv->sd, &st);\n+\t}\n+}\n-- \n1.8.3\n"},{"id":"220526","messageId":"1370987312-6761-11-git-send-email-mhagger@alum.mit.edu","threadId":"34110","inReplyTo":"1370987312-6761-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 10/12] get_packed_ref_cache: reload packed-refs file when it changes","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2013-06-11T21:48:30Z","receivedAt":"2013-06-11T21:48:30Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"From: Jeff King \n\nOnce we read the packed-refs file into memory, we cache it\nto save work on future ref lookups. However, our cache may\nbe out of date with respect to what is on disk if another\nprocess is simultaneously packing the refs. Normally it\nis acceptable for us to be a little out of date, since there\nis no guarantee whether we read the file before or after the\nsimultaneous update. However, there is an important special\ncase: our packed-refs file must be up to date with respect\nto any loose refs we read. Otherwise, we risk the following\nrace condition:\n\n 0. There exists a loose ref refs/heads/master.\n\n 1. Process A starts and looks up the ref \"master\". It\n first checks $GIT_DIR/master, which does not exist. It\n then loads (and caches) the packed-refs file to see if\n \"master\" exists in it, which it does not.\n\n 2. Meanwhile, process B runs \"pack-refs --all --prune\". It\n creates a new packed-refs file which contains\n refs/heads/master, and removes the loose copy at\n $GIT_DIR/refs/heads/master.\n\n 3. Process A continues its lookup, and eventually tries\n $GIT_DIR/refs/heads/master. It sees that the loose ref\n is missing, and falls back to the packed-refs file. But\n it examines its cached version, which does not have\n refs/heads/master. After trying a few other prefixes,\n it reports master as a non-existent ref.\n\nThere are many variants (e.g., step 1 may involve process A\nlooking up another ref entirely, so even a fully qualified\nrefname can fail). One of the most interesting ones is if\n\"refs/heads/master\" is already packed. In that case process\nA will not see it as missing, but rather will report\nwhatever value happened to be in the packed-refs file before\nprocess B repacked (which might be an arbitrarily old\nvalue).\n\nWe can fix this by making sure we reload the packed-refs\nfile from disk after looking at any loose refs. That's\nunacceptably slow, so we can check its stat()-validity as a\nproxy, and read it only when it appears to have changed.\n\nReading the packed-refs file after performing any loose-ref\nsystem calls is sufficient because we know the ordering of\nthe pack-refs process: it always makes sure the newly\nwritten packed-refs file is installed into place before\npruning any loose refs. As long as those operations by B\nappear in their executed order to process A, by the time A\nsees the missing loose ref, the new packed-refs file must be\nin place.\n\nSigned-off-by: Michael Haggerty \n---\nThis is Peff's work, rebased and with some smallish changes to fit it\nin with the packed_ref_cache data structure.\n\n refs.c | 21 ++++++++++++++++-----\n 1 file changed, 16 insertions(+), 5 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 92c8e97..64f72ab 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -824,6 +824,9 @@ struct packed_ref_cache {\n \n \t/* If locked, the file descriptor of the lock file. */\n \tint fd;\n+\n+\t/* The metadata from when this packed-refs cache was read */\n+\tstruct stat_validity validity;\n };\n \n /*\n@@ -858,6 +861,7 @@ static int release_packed_ref_cache(struct packed_ref_cache *packed_refs)\n {\n \tif (!--packed_refs->referrers) {\n \t\tfree_ref_entry(packed_refs->root);\n+\t\tstat_validity_clear(&packed_refs->validity);\n \t\tfree(packed_refs);\n \t\treturn 1;\n \t} else {\n@@ -1049,20 +1053,27 @@ static void read_packed_refs(FILE *f, struct ref_dir *dir)\n */\n static struct packed_ref_cache *get_packed_ref_cache(struct ref_cache *refs)\n {\n+\tconst char *packed_refs_file;\n+\n+\tif (*refs->name)\n+\t\tpacked_refs_file = git_path_submodule(refs->name, \"packed-refs\");\n+\telse\n+\t\tpacked_refs_file = git_path(\"packed-refs\");\n+\n+\tif (refs->packed &&\n+\t !stat_validity_check(&refs->packed->validity, packed_refs_file))\n+\t\tclear_packed_ref_cache(refs);\n+\n \tif (!refs->packed) {\n-\t\tconst char *packed_refs_file;\n \t\tFILE *f;\n \n \t\trefs->packed = xcalloc(1, sizeof(*refs->packed));\n \t\tacquire_packed_ref_cache(refs->packed);\n \t\trefs->packed->root = create_dir_entry(refs, \"\", 0, 0);\n \t\trefs->packed->fd = -1;\n-\t\tif (*refs->name)\n-\t\t\tpacked_refs_file = git_path_submodule(refs->name, \"packed-refs\");\n-\t\telse\n-\t\t\tpacked_refs_file = git_path(\"packed-refs\");\n \t\tf = fopen(packed_refs_file, \"r\");\n \t\tif (f) {\n+\t\t\tstat_validity_update(&refs->packed->validity, fileno(f));\n \t\t\tread_packed_refs(f, get_ref_dir(refs->packed->root));\n \t\t\tfclose(f);\n \t\t}\n-- \n1.8.3\n"},{"id":"220524","messageId":"1370987312-6761-12-git-send-email-mhagger@alum.mit.edu","threadId":"34110","inReplyTo":"1370987312-6761-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 11/12] for_each_ref: load all loose refs before packed refs","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2013-06-11T21:48:31Z","receivedAt":"2013-06-11T21:48:31Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"From: Jeff King \n\nIf we are iterating through the refs using for_each_ref (or\nany of its sister functions), we can get into a race\ncondition with a simultaneous \"pack-refs --prune\" that looks\nlike this:\n\n 0. We have a large number of loose refs, and a few packed\n refs. refs/heads/z/foo is loose, with no matching entry\n in the packed-refs file.\n\n 1. Process A starts iterating through the refs. It loads\n the packed-refs file from disk, then starts lazily\n traversing through the loose ref directories.\n\n 2. Process B, running \"pack-refs --prune\", writes out the\n new packed-refs file. It then deletes the newly packed\n refs, including refs/heads/z/foo.\n\n 3. Meanwhile, process A has finally gotten to\n refs/heads/z (it traverses alphabetically). It\n descends, but finds nothing there. It checks its\n cached view of the packed-refs file, but it does not\n mention anything in \"refs/heads/z/\" at all (it predates\n the new file written by B in step 2).\n\nThe traversal completes successfully without mentioning\nrefs/heads/z/foo at all (the name, of course, isn't\nimportant; but the more refs you have and the farther down\nthe alphabetical list a ref is, the more likely it is to hit\nthe race). If refs/heads/z/foo did exist in the packed refs\nfile at state 0, we would see an entry for it, but it would\nshow whatever sha1 the ref had the last time it was packed\n(which could be an arbitrarily long time ago).\n\nThis can be especially dangerous when process A is \"git\nprune\", as it means our set of reachable tips will be\nincomplete, and we may erroneously prune objects reachable\nfrom that tip (the same thing can happen if \"repack -ad\" is\nused, as it simply drops unreachable objects that are\npacked).\n\nThis patch solves it by loading all of the loose refs for\nour traversal into our in-memory cache, and then refreshing\nthe packed-refs cache. Because a pack-refs writer will\nalways put the new packed-refs file into place before\nstarting the prune, we know that any loose refs we fail to\nsee will either truly be missing, or will have already been\nput in the packed-refs file by the time we refresh.\n\nSigned-off-by: Michael Haggerty \n---\nDitto.\n\n refs.c | 39 +++++++++++++++++++++++++++++++++++----\n 1 file changed, 35 insertions(+), 4 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 64f72ab..aa4641b 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -746,6 +746,21 @@ static int do_for_each_entry_in_dirs(struct ref_dir *dir1,\n }\n \n /*\n+ * Load all of the refs from the dir into our in-memory cache. The hard work\n+ * of loading loose refs is done by get_ref_dir(), so we just need to recurse\n+ * through all of the sub-directories. We do not even need to care about\n+ * sorting, as traversal order does not matter to us.\n+ */\n+static void prime_ref_dir(struct ref_dir *dir)\n+{\n+\tint i;\n+\tfor (i = 0; i < dir->nr; i++) {\n+\t\tstruct ref_entry *entry = dir->entries[i];\n+\t\tif (entry->flag & REF_DIR)\n+\t\t\tprime_ref_dir(get_ref_dir(entry));\n+\t}\n+}\n+/*\n * Return true iff refname1 and refname2 conflict with each other.\n * Two reference names conflict if one of them exactly matches the\n * leading components of the other; e.g., \"foo/bar\" conflicts with\n@@ -1600,15 +1615,31 @@ void warn_dangling_symref(FILE *fp, const char *msg_fmt, const char *refname)\n static int do_for_each_entry(struct ref_cache *refs, const char *base,\n \t\t\t each_ref_entry_fn fn, void *cb_data)\n {\n-\tstruct packed_ref_cache *packed_ref_cache = get_packed_ref_cache(refs);\n-\tstruct ref_dir *packed_dir = get_packed_ref_dir(packed_ref_cache);\n-\tstruct ref_dir *loose_dir = get_loose_refs(refs);\n+\tstruct packed_ref_cache *packed_ref_cache;\n+\tstruct ref_dir *loose_dir;\n+\tstruct ref_dir *packed_dir;\n \tint retval = 0;\n \n+\t/*\n+\t * We must make sure that all loose refs are read before accessing the\n+\t * packed-refs file; this avoids a race condition in which loose refs\n+\t * are migrated to the packed-refs file by a simultaneous process, but\n+\t * our in-memory view is from before the migration. get_packed_ref_cache()\n+\t * takes care of making sure our view is up to date with what is on\n+\t * disk.\n+\t */\n+\tloose_dir = get_loose_refs(refs);\n+\tif (base && *base) {\n+\t\tloose_dir = find_containing_dir(loose_dir, base, 0);\n+\t}\n+\tif (loose_dir)\n+\t\tprime_ref_dir(loose_dir);\n+\n+\tpacked_ref_cache = get_packed_ref_cache(refs);\n \tacquire_packed_ref_cache(packed_ref_cache);\n+\tpacked_dir = get_packed_ref_dir(packed_ref_cache);\n \tif (base && *base) {\n \t\tpacked_dir = find_containing_dir(packed_dir, base, 0);\n-\t\tloose_dir = find_containing_dir(loose_dir, base, 0);\n \t}\n \n \tif (packed_dir && loose_dir) {\n-- \n1.8.3\n"},{"id":"220522","messageId":"1370987312-6761-13-git-send-email-mhagger@alum.mit.edu","threadId":"34110","inReplyTo":"1370987312-6761-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 12/12] refs: do not invalidate the packed-refs cache unnecessarily","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2013-06-11T21:48:32Z","receivedAt":"2013-06-11T21:48:32Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Now that we keep track of the packed-refs file metadata, we can detect\nwhen the packed-refs file has been modified since we last read it, and\nwe do so automatically every time that get_packed_ref_cache() is\ncalled. So there is no need to invalidate the cache automatically\nwhen lock_packed_refs() is called; usually the old copy will still be\nvalid.\n\nSigned-off-by: Michael Haggerty \n---\nThis patch is optional. It makes the assumption that the metadata\nstored in stat_validity are adequate to reliably detect when the\npacked-refs file has changed. Given that we are about to rewrite the\nfile, it is perhaps even more crucial not to make a mistake in this\ncodepath than in others. So if the stat_validity check is not\nconsidered safe enough, it might be prudent to omit this patch and\ncontinue to reload the packed-refs data here unconditionally.\n\n refs.c | 9 ++++++---\n 1 file changed, 6 insertions(+), 3 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex aa4641b..d9cbfc4 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2134,12 +2134,15 @@ int lock_packed_refs(struct lock_file *lock, int flags)\n \tint fd;\n \tstruct packed_ref_cache *packed_ref_cache;\n \n-\t/* Discard the old cache because it might be invalid: */\n-\tclear_packed_ref_cache(&ref_cache);\n \tfd = hold_lock_file_for_update(lock, git_path(\"packed-refs\"), flags);\n \tif (fd < 0)\n \t\treturn -1;\n-\t/* Read the current packed-refs while holding the lock: */\n+\t/*\n+\t * Get the current packed-refs while holding the lock. If the\n+\t * packed-refs file has been modified since we last read it,\n+\t * this will automatically invalidate the cache and re-read\n+\t * the packed-refs file.\n+\t */\n \tpacked_ref_cache = get_packed_ref_cache(&ref_cache);\n \tpacked_ref_cache->lock = lock;\n \tpacked_ref_cache->fd = fd;\n-- \n1.8.3\n"},{"id":"220595","messageId":"20130612113823.GA20461@sigill.intra.peff.net","threadId":"34110","inReplyTo":"1370987312-6761-2-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH 01/12] repack_without_ref(): split list curation and entry writing","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-06-12T11:38:24Z","receivedAt":"2013-06-12T11:38:24Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jun 11, 2013 at 11:48:21PM +0200, Michael Haggerty wrote:\n\n> Split repack_without_ref() into multiple passes:\n> \n> * collect the list of refnames that should be deleted from packed_refs\n> \n> * delete those refnames from the cache\n> \n> * write the remainder to the packed-refs file\n> \n> The purpose of this change is to make the \"write the remainder\" part\n> reusable.\n\nIt took me several reads to figure out what was going on here, because I\ndid not see the deleted ref passed to the list of items to delete from\npacked_refs. The part I was missing is something like:\n\n The repack_without_ref() function first removes the deleted ref from\n the internal packed-refs list, then writes the packed-refs list to\n disk, omitting any broken or stale entries. This patch splits that\n second step into multiple passes:\n\n ...\n\nIs that accurate?\n\n-Peff\n"},{"id":"220596","messageId":"51B861E8.4090200@alum.mit.edu","threadId":"34110","inReplyTo":"20130612113823.GA20461@sigill.intra.peff.net","subject":"Re: [PATCH 01/12] repack_without_ref(): split list curation and entry writing","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2013-06-12T11:56:24Z","receivedAt":"2013-06-12T11:56:24Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 06/12/2013 01:38 PM, Jeff King wrote:\n> On Tue, Jun 11, 2013 at 11:48:21PM +0200, Michael Haggerty wrote:\n> \n>> Split repack_without_ref() into multiple passes:\n>>\n>> * collect the list of refnames that should be deleted from packed_refs\n>>\n>> * delete those refnames from the cache\n>>\n>> * write the remainder to the packed-refs file\n>>\n>> The purpose of this change is to make the \"write the remainder\" part\n>> reusable.\n> \n> It took me several reads to figure out what was going on here, because I\n> did not see the deleted ref passed to the list of items to delete from\n> packed_refs. The part I was missing is something like:\n> \n> The repack_without_ref() function first removes the deleted ref from\n> the internal packed-refs list, then writes the packed-refs list to\n> disk, omitting any broken or stale entries. This patch splits that\n> second step into multiple passes:\n> \n> ...\n> \n> Is that accurate?\n\nYes. You are right that I should make that clearer.\n\nThanks,\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"220598","messageId":"20130612120156.GB20461@sigill.intra.peff.net","threadId":"34110","inReplyTo":"1370987312-6761-5-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH 04/12] refs: implement simple transactions for the packed-refs file","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-06-12T12:01:56Z","receivedAt":"2013-06-12T12:01:56Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jun 11, 2013 at 11:48:24PM +0200, Michael Haggerty wrote:\n\n> The API docs are not clear about whether it is kosher to read\n> lock_file::fd directly. It is only done in one file outside of\n> lockfile.c. So this patch stores the fd of the lockfile separately in\n> struct packed_ref_cache, even though the same struct also has a\n> pointer to the struct lock_file.\n> \n> So please let me know if it is OK to read lock_file::fd directly. If\n> so, then I will drop the fd member of struct packed_ref_cache, as well\n> as the local variable \"fd\" in lock_packed_refs().\n\nI think it's fine; the fact that you have such an fd is a public part of\nthe interface, so you are only relying on the struct member being there.\nAnd since the lock_file must hold the fd itself somewhere, I don't think\nthat's unreasonable.\n\nI'm not sure how you got your \"in one file\" list, but it appears to\nhappen in credential-store.c, bundle.c, fast-import.c, and read-cache.c.\n\n-Peff\n"},{"id":"220603","messageId":"20130612123931.GD20461@sigill.intra.peff.net","threadId":"34110","inReplyTo":"1370987312-6761-13-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH 12/12] refs: do not invalidate the packed-refs cache unnecessarily","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-06-12T12:39:31Z","receivedAt":"2013-06-12T12:39:31Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jun 11, 2013 at 11:48:32PM +0200, Michael Haggerty wrote:\n\n> Now that we keep track of the packed-refs file metadata, we can detect\n> when the packed-refs file has been modified since we last read it, and\n> we do so automatically every time that get_packed_ref_cache() is\n> called. So there is no need to invalidate the cache automatically\n> when lock_packed_refs() is called; usually the old copy will still be\n> valid.\n> \n> Signed-off-by: Michael Haggerty \n> ---\n> This patch is optional. It makes the assumption that the metadata\n> stored in stat_validity are adequate to reliably detect when the\n> packed-refs file has changed. Given that we are about to rewrite the\n> file, it is perhaps even more crucial not to make a mistake in this\n> codepath than in others. So if the stat_validity check is not\n> considered safe enough, it might be prudent to omit this patch and\n> continue to reload the packed-refs data here unconditionally.\n\nI doubt that the risk is very high, but I'd also doubt that this\nprovides any useful performance improvement, because it is mostly\nhappening during a `git pack-refs` call. The exception is packing for a\nref deletion, and it could potentially speed up a receive-pack that was\ndeleting many refs.\n\nI don't have a strong opinion either way.\n\n-Peff\n"},{"id":"220606","messageId":"20130612125235.GA28876@sigill.intra.peff.net","threadId":"34110","inReplyTo":"1370987312-6761-1-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH 00/12] Fix some reference-related races","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-06-12T12:52:35Z","receivedAt":"2013-06-12T12:52:35Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jun 11, 2013 at 11:48:20PM +0200, Michael Haggerty wrote:\n\n> *This patch series must be built on top of mh/reflife.*\n\nApplying on top of what Junio has in mh/reflife seems to create\nconflicts at the first patch. I didn't look into it, though, but just\nread the patches and looked at the packed-refs-transactions branch at\ngit://github.com/mhagger/git.git.\n\n> The other problem was that the for_each_ref() functions will die if\n> the ref cache that they are iterating over is freed out from under\n> them. This problem is solved by using reference counts to avoid\n> freeing the old packed ref cache (even if it is no longer valid) until\n> all users are done with it.\n\nI was worried that the reference-counting would end up invasive and\nugly, but it turned out quite nice.\n\nOverall, I think the series goes in the right direction, and I didn't\nsee any implementation flaws. The core of the race-handling is the same\nas in mine, but your \"hyperactive repository\" patch shows how badly mine\ncan break under load.\n\nI sent a few comments to specific patches, but I'd be fine with applying\nit as-is. Thanks for working on it.\n\n-Peff\n"},{"id":"220960","messageId":"51BCCAD2.9070106@ramsay1.demon.co.uk","threadId":"34110","inReplyTo":"1370987312-6761-1-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH 00/12] Fix some reference-related races","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsay1.demon.co.uk","sentAt":"2013-06-15T20:13:06Z","receivedAt":"2013-06-15T20:13:06Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"Michael Haggerty wrote:\n> *This patch series must be built on top of mh/reflife.*\n> \n\n[...]\n\n> The other problem was that the for_each_ref() functions will die if\n> the ref cache that they are iterating over is freed out from under\n> them. This problem is solved by using reference counts to avoid\n> freeing the old packed ref cache (even if it is no longer valid) until\n> all users are done with it.\n\nYes, I found exactly this happened to me on cygwin, earlier this week,\nwith the previous version of this code. After seeing this mail, I had\ndecided not to describe the failure on the old version, but wait and\ntest this version instead.\n\nThis version is a great improvement, but it still has some failures on\ncygwin. So, it may be worth (briefly) describing the old failure anyway!\nNote that several tests failed, but I will only mention t3211-peel-ref.sh\ntests #7-8.\n\n $ pwd\n /home/ramsay/git/t/trash directory.t3211-peel-ref\n $\n\n $ ../../bin-wrappers/git show-ref -d\n d1ff1c9224ae5e58a7656fb9ecc95865d42ed71e refs/heads/master\n eb0e854c2cd2511c7571b1a5e3c8b8146193fb30 refs/outside/foo\n d1ff1c9224ae5e58a7656fb9ecc95865d42ed71e refs/outside/foo^{}\n 5 [main] git 3540 _cygtls::handle_exceptions: Error while dumping state (p\n robably corrupted stack)\n Segmentation fault (core dumped)\n $\n\nThe stack-trace for the faulting code looks something like:\n\n cmd_show_ref()\n for_each_ref(show_ref, NULL)\n do_for_each_ref(&ref_cache, \"\", show_ref, 0, 0, NULL)\n do_for_each_entry(&ref_cache, \"\", do_one_ref, &data)\n do_for_each_entry_in_dirs(packed_dir, loose_dir, do_one_ref, &data)\n *dfeeid() recursive calls*\n do_one_ref(entry, &data)\n show_ref(\"refs/outside/foo\", sha1, NULL) [2nd match]\n peel_ref(\"refs/outside/foo\", sha1)\n peel_entry(entry, 0)\n peel_object(name, sha1)\n deref_tag_noverify(o)\n parse_object(sha1 )\n lookup_replace_object(sha1)\n do_lookup_replace_object(sha1)\n prepare_replace_object()\n\n [un-indent here!]\n for_each_replace_ref(register_replace_ref, NULL)\n do_for_each_ref(&ref_cache, \"refs/replace\", fn, 13, 0, NULL)\n do_for_each_entry(&ref_cache, \"refs/replace\", fn, &data)\n get_packed_refs(&ref_cache)\n clear_packed_ref_cache(&ref_cache) *free_ref_entries etc*\n ** return to show_ref() [2nd match] above **\n ** return to recursive dfeeid() call in original iteration\n ** dir1->entries has been free()-ed and reused => segmentation fault\n [dir1->entries == 0x64633263 => dc2c => part of sha1 for refs/outside/foo]\n\nSo, the nested \"replace-reference-iteration\" causes the ref_cache to be\nfreed out from under the initial show-ref iteration, so this works:\n\n $ GIT_NO_REPLACE_OBJECTS=1 ../../bin-wrappers/git show-ref -d\n d1ff1c9224ae5e58a7656fb9ecc95865d42ed71e refs/heads/master\n eb0e854c2cd2511c7571b1a5e3c8b8146193fb30 refs/outside/foo\n d1ff1c9224ae5e58a7656fb9ecc95865d42ed71e refs/outside/foo^{}\n d1ff1c9224ae5e58a7656fb9ecc95865d42ed71e refs/tags/base\n eb0e854c2cd2511c7571b1a5e3c8b8146193fb30 refs/tags/foo\n d1ff1c9224ae5e58a7656fb9ecc95865d42ed71e refs/tags/foo^{}\n $\n\nYou may be wondering why clear_packed_ref_cache() is called? Well, that\nis because stat_validity_check() *incorrectly* indicates that the\npacked-refs file has changed. Why does it do that? Well, this is one\nmore example of the problems caused by the cygwin schizophrenic stat()\nfunctions. :( [ARGHHHHHHHHH]\n\nAt this point, I tried running 'git show-ref' with core.checkstat set\non the command line; but that didn't work! I had to fix show-ref and\nre-build git, and then, this works:\n\n $ ../../bin-wrappers/git -c core.checkstat=minimal -c core.trustctime=f\n alse show-ref -d\n d1ff1c9224ae5e58a7656fb9ecc95865d42ed71e refs/heads/master\n eb0e854c2cd2511c7571b1a5e3c8b8146193fb30 refs/outside/foo\n d1ff1c9224ae5e58a7656fb9ecc95865d42ed71e refs/outside/foo^{}\n d1ff1c9224ae5e58a7656fb9ecc95865d42ed71e refs/tags/base\n eb0e854c2cd2511c7571b1a5e3c8b8146193fb30 refs/tags/foo\n d1ff1c9224ae5e58a7656fb9ecc95865d42ed71e refs/tags/foo^{}\n $\n\nNow, turning to the new code, t3211-peel-ref.sh test #7 now works, but\ntest #8 still fails...\n\n $ ./t3211-peel-ref.sh -i -v\n\n ...\n\n ok 7 - refs are peeled outside of refs/tags (old packed)\n\n expecting success:\n git pack-refs --all &&\n cp .git/packed-refs fully-peeled &&\n git branch yadda &&\n git pack-refs --all &&\n git branch -d yadda &&\n test_cmp fully-peeled .git/packed-refs\n\n fatal: internal error: packed-ref cache cleared while locked\n not ok 8 - peeled refs survive deletion of packed ref\n #\n # git pack-refs --all &&\n # cp .git/packed-refs fully-peeled &&\n # git branch yadda &&\n # git pack-refs --all &&\n # git branch -d yadda &&\n # test_cmp fully-peeled .git/packed-refs\n #\n $ cd trash\\ directory.t3211-peel-ref/\n $ ../../bin-wrappers/git pack-refs --all\n fatal: internal error: packed-ref cache cleared while locked\n $ ls\n actual base.t expect\n $ ls .git\n COMMIT_EDITMSG branches/ description index logs/ packed-refs\n HEAD config hooks-disabled/ info/ objects/ refs/\n $ ls -l .git/packed-refs\n -rw-r--r-- 1 ramsay None 296 Jun 14 20:34 .git/packed-refs\n $ cat .git/packed-refs\n # pack-refs with: peeled\n d1ff1c9224ae5e58a7656fb9ecc95865d42ed71e refs/heads/master\n eb0e854c2cd2511c7571b1a5e3c8b8146193fb30 refs/outside/foo\n d1ff1c9224ae5e58a7656fb9ecc95865d42ed71e refs/tags/base\n eb0e854c2cd2511c7571b1a5e3c8b8146193fb30 refs/tags/foo\n ^d1ff1c9224ae5e58a7656fb9ecc95865d42ed71e\n $\n\nNow, I have a test-stat program which prints the difference between\nthe two stat implementations used in cygwin git, thus:\n\n $ ../../test-stat .git/packed-refs\n stat for '.git/packed-refs':\n *dev: -1395862925, 0\n *ino: 166044, 0\n *mode: 100644 -rw-, 100600 -rw-\n nlink: 1, 1\n *uid: 1005, 0\n *gid: 513, 0\n *rdev: -1395862925, 0\n size: 296, 296\n atime: 1371238550, 1371238550 Fri Jun 14 20:35:50 2013\n mtime: 1371238469, 1371238469 Fri Jun 14 20:34:29 2013\n ctime: 1371238469, 1371238469 Fri Jun 14 20:34:29 2013\n $ ../../bin-wrappers/git -c core.checkstat=minimal pack-refs --all\n fatal: internal error: packed-ref cache cleared while locked\n $\n\nHmmm, that should have worked! Wait, fix 'git pack-refs' to support\nsetting config variables on the command line, rebuild and:\n\n $ ../../bin-wrappers/git -c core.checkstat=minimal pack-refs --all\n $ cat .git/packed-refs\n # pack-refs with: peeled fully-peeled\n d1ff1c9224ae5e58a7656fb9ecc95865d42ed71e refs/heads/master\n eb0e854c2cd2511c7571b1a5e3c8b8146193fb30 refs/outside/foo\n ^d1ff1c9224ae5e58a7656fb9ecc95865d42ed71e\n d1ff1c9224ae5e58a7656fb9ecc95865d42ed71e refs/tags/base\n eb0e854c2cd2511c7571b1a5e3c8b8146193fb30 refs/tags/foo\n ^d1ff1c9224ae5e58a7656fb9ecc95865d42ed71e\n $\n\nI haven't checked the remaining test failures to see if they are\ncaused by this code (I don't think so, but ...), but this failure\nis clearly a cygwin specific issue.\n\nHTH\n\nATB,\nRamsay Jones\n"},{"id":"220979","messageId":"51BD5232.20205@alum.mit.edu","threadId":"34110","inReplyTo":"51BCCAD2.9070106@ramsay1.demon.co.uk","subject":"Re: [PATCH 00/12] Fix some reference-related races","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2013-06-16T05:50:42Z","receivedAt":"2013-06-16T05:50:42Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Thanks for all of the information.\n\nOn 06/15/2013 10:13 PM, Ramsay Jones wrote:\n> Michael Haggerty wrote:\n>> *This patch series must be built on top of mh/reflife.*\n>>\n> \n> [...]\n> \n>> The other problem was that the for_each_ref() functions will die if\n>> the ref cache that they are iterating over is freed out from under\n>> them. This problem is solved by using reference counts to avoid\n>> freeing the old packed ref cache (even if it is no longer valid) until\n>> all users are done with it.\n> \n> Yes, I found exactly this happened to me on cygwin, earlier this week,\n> with the previous version of this code. After seeing this mail, I had\n> decided not to describe the failure on the old version, but wait and\n> test this version instead.\n> \n> This version is a great improvement, but it still has some failures on\n> cygwin. So, it may be worth (briefly) describing the old failure anyway!\n> Note that several tests failed, but I will only mention t3211-peel-ref.sh\n> tests #7-8.\n> \n> $ pwd\n> /home/ramsay/git/t/trash directory.t3211-peel-ref\n> $\n> \n> $ ../../bin-wrappers/git show-ref -d\n> d1ff1c9224ae5e58a7656fb9ecc95865d42ed71e refs/heads/master\n> eb0e854c2cd2511c7571b1a5e3c8b8146193fb30 refs/outside/foo\n> d1ff1c9224ae5e58a7656fb9ecc95865d42ed71e refs/outside/foo^{}\n> 5 [main] git 3540 _cygtls::handle_exceptions: Error while dumping state (p\n> robably corrupted stack)\n> Segmentation fault (core dumped)\n> $\n> \n> The stack-trace for the faulting code looks something like:\n> \n> cmd_show_ref()\n> for_each_ref(show_ref, NULL)\n> do_for_each_ref(&ref_cache, \"\", show_ref, 0, 0, NULL)\n> do_for_each_entry(&ref_cache, \"\", do_one_ref, &data)\n> do_for_each_entry_in_dirs(packed_dir, loose_dir, do_one_ref, &data)\n> *dfeeid() recursive calls*\n> do_one_ref(entry, &data)\n> show_ref(\"refs/outside/foo\", sha1, NULL) [2nd match]\n> peel_ref(\"refs/outside/foo\", sha1)\n> peel_entry(entry, 0)\n> peel_object(name, sha1)\n> deref_tag_noverify(o)\n> parse_object(sha1 )\n> lookup_replace_object(sha1)\n> do_lookup_replace_object(sha1)\n> prepare_replace_object()\n> \n> [un-indent here!]\n> for_each_replace_ref(register_replace_ref, NULL)\n> do_for_each_ref(&ref_cache, \"refs/replace\", fn, 13, 0, NULL)\n> do_for_each_entry(&ref_cache, \"refs/replace\", fn, &data)\n> get_packed_refs(&ref_cache)\n> clear_packed_ref_cache(&ref_cache) *free_ref_entries etc*\n> ** return to show_ref() [2nd match] above **\n> ** return to recursive dfeeid() call in original iteration\n> ** dir1->entries has been free()-ed and reused => segmentation fault\n> [dir1->entries == 0x64633263 => dc2c => part of sha1 for refs/outside/foo]\n> \n> So, the nested \"replace-reference-iteration\" causes the ref_cache to be\n> freed out from under the initial show-ref iteration, so this works:\n> \n> $ GIT_NO_REPLACE_OBJECTS=1 ../../bin-wrappers/git show-ref -d\n> d1ff1c9224ae5e58a7656fb9ecc95865d42ed71e refs/heads/master\n> eb0e854c2cd2511c7571b1a5e3c8b8146193fb30 refs/outside/foo\n> d1ff1c9224ae5e58a7656fb9ecc95865d42ed71e refs/outside/foo^{}\n> d1ff1c9224ae5e58a7656fb9ecc95865d42ed71e refs/tags/base\n> eb0e854c2cd2511c7571b1a5e3c8b8146193fb30 refs/tags/foo\n> d1ff1c9224ae5e58a7656fb9ecc95865d42ed71e refs/tags/foo^{}\n> $\n> \n> You may be wondering why clear_packed_ref_cache() is called? Well, that\n> is because stat_validity_check() *incorrectly* indicates that the\n> packed-refs file has changed. Why does it do that? Well, this is one\n> more example of the problems caused by the cygwin schizophrenic stat()\n> functions. :( [ARGHHHHHHHHH]\n> \n> At this point, I tried running 'git show-ref' with core.checkstat set\n> on the command line; but that didn't work! I had to fix show-ref and\n> re-build git, and then, this works:\n> \n> $ ../../bin-wrappers/git -c core.checkstat=minimal -c core.trustctime=f\n> alse show-ref -d\n> d1ff1c9224ae5e58a7656fb9ecc95865d42ed71e refs/heads/master\n> eb0e854c2cd2511c7571b1a5e3c8b8146193fb30 refs/outside/foo\n> d1ff1c9224ae5e58a7656fb9ecc95865d42ed71e refs/outside/foo^{}\n> d1ff1c9224ae5e58a7656fb9ecc95865d42ed71e refs/tags/base\n> eb0e854c2cd2511c7571b1a5e3c8b8146193fb30 refs/tags/foo\n> d1ff1c9224ae5e58a7656fb9ecc95865d42ed71e refs/tags/foo^{}\n> $\n\nSo if I understand correctly, all of the above is *without* the\nrefcounting changes introduced in mh/ref-races? Is so, then it is not\nsurprising, as this is exactly the sort of problem that the reference\ncounting is meant to solve.\n\n> Now, turning to the new code, t3211-peel-ref.sh test #7 now works, but\n> test #8 still fails...\n> \n> $ ./t3211-peel-ref.sh -i -v\n> \n> ...\n> \n> ok 7 - refs are peeled outside of refs/tags (old packed)\n> \n> expecting success:\n> git pack-refs --all &&\n> cp .git/packed-refs fully-peeled &&\n> git branch yadda &&\n> git pack-refs --all &&\n> git branch -d yadda &&\n> test_cmp fully-peeled .git/packed-refs\n> \n> fatal: internal error: packed-ref cache cleared while locked\n> not ok 8 - peeled refs survive deletion of packed ref\n> #\n> # git pack-refs --all &&\n> # cp .git/packed-refs fully-peeled &&\n> # git branch yadda &&\n> # git pack-refs --all &&\n> # git branch -d yadda &&\n> # test_cmp fully-peeled .git/packed-refs\n> #\n> $ cd trash\\ directory.t3211-peel-ref/\n> $ ../../bin-wrappers/git pack-refs --all\n> fatal: internal error: packed-ref cache cleared while locked\n\nThese \"internal error: packed-ref cache cleared while locked\" failures\nresult from an internal consistency check that clear_packed_ref_cache()\nis not called while the write lock is held on the packed-refs file. A\ncall to c_p_r_c() could result from\n\n* a programming error\n\n* a determination based on the packed-refs file stat that the file needs\nto be re-read\n\nJudging from what you said about cygwin, I assume that the latter is\nhappening. It should be impossible, because the current process is\nholding packed-refs.lock, and therefore other git processes should\nrefuse to change the packed-refs file.\n\nBut if the stat information is not reliable, then the current process\nwould *think* that the packed-refs file has been changed even though it\nhasn't, then it would call c_p_r_c() even though it holds the lock on\npacked-refs, and the internal consistency check would fail.\n\nSo apparently in these cases cygwin is reporting that the packed-refs\nstat information has changed (in the sense defined by the new\nstat_validity_check() function, which does essentially the same checks\nas the old ce_match_stat_basic() function).\n\n> $ ls\n> actual base.t expect\n> $ ls .git\n> COMMIT_EDITMSG branches/ description index logs/ packed-refs\n> HEAD config hooks-disabled/ info/ objects/ refs/\n> $ ls -l .git/packed-refs\n> -rw-r--r-- 1 ramsay None 296 Jun 14 20:34 .git/packed-refs\n> $ cat .git/packed-refs\n> # pack-refs with: peeled\n> d1ff1c9224ae5e58a7656fb9ecc95865d42ed71e refs/heads/master\n> eb0e854c2cd2511c7571b1a5e3c8b8146193fb30 refs/outside/foo\n> d1ff1c9224ae5e58a7656fb9ecc95865d42ed71e refs/tags/base\n> eb0e854c2cd2511c7571b1a5e3c8b8146193fb30 refs/tags/foo\n> ^d1ff1c9224ae5e58a7656fb9ecc95865d42ed71e\n> $\n> \n> Now, I have a test-stat program which prints the difference between\n> the two stat implementations used in cygwin git, thus:\n> \n> $ ../../test-stat .git/packed-refs\n> stat for '.git/packed-refs':\n> *dev: -1395862925, 0\n> *ino: 166044, 0\n> *mode: 100644 -rw-, 100600 -rw-\n> nlink: 1, 1\n> *uid: 1005, 0\n> *gid: 513, 0\n> *rdev: -1395862925, 0\n> size: 296, 296\n> atime: 1371238550, 1371238550 Fri Jun 14 20:35:50 2013\n> mtime: 1371238469, 1371238469 Fri Jun 14 20:34:29 2013\n> ctime: 1371238469, 1371238469 Fri Jun 14 20:34:29 2013\n\nYikes! ECYGWINFAIL. I have no doubt they have reason why they cannot\nimplement this correctly, but this is rather limiting. (I has assumed\nthat the #ifdef magic that was already in ce_match_stat_basic() would\nhave papered over the problems in stat, but I guess that is not the case.)\n\nYou say that there are two stat references in cygwin. Is there a way to\nensure that the same one is used in both cases? Or is it so hopelessly\nbroken that there is no point?\n\nOr let me step back and pose a slightly more abstract question:\n\nHow, under cygwin, can we implement a quick check of whether a file\nmight have changed? The check should not have any false negatives\n(claiming that a file is unchanged when actually it was rewritten via\nthe usual lock_file mechanism) nor should it have any \"strong\" false\npositives (claiming that a file has changed even though it has never\nbeen touched), though a \"weak\" false positive would be OK (claiming that\na file has changed even though it was replaced by a version with\nidentical contents).\n\nIf such a check is possible, then we should build it into the\nimplementation of match_stat_data(). If not, we have to think of\nanother way to implement the checks of packed-refs cache up-to-dateness.\n\n(One horrible hack would be: when in doubt, read the packed-refs file\ninto a temporary ref_dir, then compare *the contents* to the version in\nthe cache. If they are the same, then discard the newly-read version,\nupdate the stat_validity, and continue to use the old version. If they\nare different, *then* verify that the lock file was not held, call\nclear_packed_ref_cache(), and start using the new version.)\n\n> $ ../../bin-wrappers/git -c core.checkstat=minimal pack-refs --all\n> fatal: internal error: packed-ref cache cleared while locked\n> $\n> \n> Hmmm, that should have worked! Wait, fix 'git pack-refs' to support\n> setting config variables on the command line, rebuild and:\n> \n> $ ../../bin-wrappers/git -c core.checkstat=minimal pack-refs --all\n> $ cat .git/packed-refs\n> # pack-refs with: peeled fully-peeled\n> d1ff1c9224ae5e58a7656fb9ecc95865d42ed71e refs/heads/master\n> eb0e854c2cd2511c7571b1a5e3c8b8146193fb30 refs/outside/foo\n> ^d1ff1c9224ae5e58a7656fb9ecc95865d42ed71e\n> d1ff1c9224ae5e58a7656fb9ecc95865d42ed71e refs/tags/base\n> eb0e854c2cd2511c7571b1a5e3c8b8146193fb30 refs/tags/foo\n> ^d1ff1c9224ae5e58a7656fb9ecc95865d42ed71e\n> $\n> \n> I haven't checked the remaining test failures to see if they are\n> caused by this code (I don't think so, but ...), but this failure\n> is clearly a cygwin specific issue.\n\nThanks again for the testing and analysis,\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"221281","messageId":"51C0A33A.7060802@ramsay1.demon.co.uk","threadId":"34110","inReplyTo":"51BD5232.20205@alum.mit.edu","subject":"Re: [PATCH 00/12] Fix some reference-related races","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsay1.demon.co.uk","sentAt":"2013-06-18T18:13:14Z","receivedAt":"2013-06-18T18:13:14Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"Michael Haggerty wrote:\n> Thanks for all of the information.\n> \n> On 06/15/2013 10:13 PM, Ramsay Jones wrote:\n>> Michael Haggerty wrote:\n>>> *This patch series must be built on top of mh/reflife.*\n\n[ ... ]\n\n>> You may be wondering why clear_packed_ref_cache() is called? Well, that\n>> is because stat_validity_check() *incorrectly* indicates that the\n>> packed-refs file has changed. Why does it do that? Well, this is one\n>> more example of the problems caused by the cygwin schizophrenic stat()\n>> functions. :( [ARGHHHHHHHHH]\n>>\n\n[ ... ]\n\n> So if I understand correctly, all of the above is *without* the\n> refcounting changes introduced in mh/ref-races? Is so, then it is not\n> surprising, as this is exactly the sort of problem that the reference\n> counting is meant to solve.\n\nYes, as I said, this describes the old (non refcounted) series.\nThis particular problem (the segmentation fault) is fixed by the new\nseries (as noted below). [Note, however, that the packed-refs file\nwill still be re-read more often than needed.]\n\n>> Now, turning to the new code, t3211-peel-ref.sh test #7 now works, but\n>> test #8 still fails...\n\n[ ... ]\n\n> These \"internal error: packed-ref cache cleared while locked\" failures\n> result from an internal consistency check that clear_packed_ref_cache()\n> is not called while the write lock is held on the packed-refs file. A\n> call to c_p_r_c() could result from\n> \n> * a programming error\n> \n> * a determination based on the packed-refs file stat that the file needs\n> to be re-read\n> \n> Judging from what you said about cygwin, I assume that the latter is\n> happening.\n\nIndeed.\n\n> It should be impossible, because the current process is\n> holding packed-refs.lock, and therefore other git processes should\n> refuse to change the packed-refs file.\n\n:-P You are assuming that a single process can't lie to itself ...\n\n[ ... ]\n\n> Yikes! ECYGWINFAIL.\n\nAh, NO, this should read ECYGWINGITFAIL.\nThis is a self-inflicted wound; it has nothing much to do with cygwin.\n\nI should not have assumed that you knew what I meant by \"schizophrenic\nstat() functions\" above; sorry about that! If you are interested, then\nthe following commits may be useful reading: adbc0b6, 7faee6b, 7974843,\n05bab3ea, 924aaf3e and b8a97333.\n\n[ ... ]\n\n>> I haven't checked the remaining test failures to see if they are\n>> caused by this code (I don't think so, but ...), but this failure\n>> is clearly a cygwin specific issue.\n> \n> Thanks again for the testing and analysis,\n\nSo, unless you feel the need to fix this yourself, you can probably\nignore this issue for now. I will hopefully find time to fix it up\nbefore this topic progresses to next. (Although I don't have any\nfeeling for the time-frame of this topic).\n\nHTH\n\nATB,\nRamsay Jones\n"},{"id":"221333","messageId":"51C146DA.5000500@alum.mit.edu","threadId":"34110","inReplyTo":"51C0A33A.7060802@ramsay1.demon.co.uk","subject":"Re: [PATCH 00/12] Fix some reference-related races","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2013-06-19T05:51:22Z","receivedAt":"2013-06-19T05:51:22Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 06/18/2013 08:13 PM, Ramsay Jones wrote:\n> Michael Haggerty wrote:\n>> On 06/15/2013 10:13 PM, Ramsay Jones wrote:\n>>> Now, turning to the new code, t3211-peel-ref.sh test #7 now works, but\n>>> test #8 still fails...\n> \n> [ ... ]\n> \n>> It should be impossible, because the current process is\n>> holding packed-refs.lock, and therefore other git processes should\n>> refuse to change the packed-refs file.\n> \n> :-P You are assuming that a single process can't lie to itself ...\n> \n> [ ... ]\n> \n> I should not have assumed that you knew what I meant by \"schizophrenic\n> stat() functions\" above; sorry about that! If you are interested, then\n> the following commits may be useful reading: adbc0b6, 7faee6b, 7974843,\n> 05bab3ea, 924aaf3e and b8a97333.\n\nThanks, that helps.\n\n>>> I haven't checked the remaining test failures to see if they are\n>>> caused by this code (I don't think so, but ...), but this failure\n>>> is clearly a cygwin specific issue.\n>>\n>> Thanks again for the testing and analysis,\n> \n> So, unless you feel the need to fix this yourself, you can probably\n> ignore this issue for now. I will hopefully find time to fix it up\n> before this topic progresses to next. (Although I don't have any\n> feeling for the time-frame of this topic).\n\nDespite reading the commits that you referenced, I still don't feel\ncompetent to fix this myself so I gratefully accept your offer.\nIdeally, whatever complexity is needed would be hidden in the functions\nstat_validity_check() and stat_validity_update() added by patch 09/12 of\nmy series, and possibly match_stat_data() from 08/12.\n\nLet me know if I can help.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"}]}