{"thread":{"id":"33713","subject":"another packed-refs race","startedAt":"2013-05-03T08:38:48Z","lastAt":"2013-06-14T07:17:49Z","messageCount":45,"participants":["Jeff King","Johan Herland","Michael Haggerty","Junio C Hamano","Eric Sunshine","Thomas Rast"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"216346","messageId":"20130503083847.GA16542@sigill.intra.peff.net","threadId":"33713","inReplyTo":null,"subject":"another packed-refs race","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-05-03T08:38:48Z","receivedAt":"2013-05-03T08:38:48Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"I found another race related to the packed-refs code. Consider for a\nmoment what happens when we are looking at refs and another process does\na simultaneous \"git pack-refs --all --prune\", updating packed-refs and\ndeleting the loose refs.\n\nIf we are resolving a single ref, then we will either find its loose\nform or not. If we do, we're done. If not, we can fall back on what was\nin the packed-refs file. If we read the packed-refs file at that point,\nwe're OK. If the loose ref existed before but was pruned before we could\nread it, then we know the packed-refs file is already in place, because\npack-refs would not have deleted the loose ref until it had finished\nwriting the new file. But imagine that we already loaded the packed-refs\nfile into memory earlier. We may be looking at an _old_ version of it\nthat has an irrelevant sha1 from the older packed-refs file. Or it might\nnot even exist in the packed-refs file at all, and we think the ref does\nnot resolve.\n\nWe could fix this by making sure our packed-refs file is up to date\nbefore using it. E.g., resolving a ref with the following sequence:\n\n 1. Look for loose ref. If found, OK.\n\n 2. Compare inode/size/mtime/etc of on-disk packed-refs to their values\n from the last time we loaded it. If they're not the same, reload\n packed-refs from disk. Otherwise, continue.\n\n 3. Look for ref in in-memory packed-refs.\n\nNot too bad. We introduce one extra stat() for a ref that has been\npacked, and the scheme isn't very complicated.\n\nBut what about enumerating refs via for_each_ref? It's possible to have\nthe same problem there, and the packed-refs file may be moved into place\nmidway through the process of enumerating the loose refs. So we may see\nrefs/heads/master, but when we get to refs/remotes/origin/master, it has\nnow been packed and pruned. I _think_ we can get by with:\n\n 1. Generate the complete sorted list of loose refs.\n\n 2. Check that packed-refs is stat-clean, and reload if necessary, as\n above.\n\n 3. Merge the sorted loose and packed lists, letting loose override\n packed (so even if we get repacked halfway through our loose\n traversal and get half of the refs there, it's OK to see duplicates\n in packed-refs, which we will ignore).\n\nThis is not very far off of what we do now. Obviously we don't do the\nstat-clean check in step 2. But we also don't generate the complete list\nof loose refs before hitting the packed-refs file. Instead we lazily\nload the loose directories as needed. And of course we cache that\ninformation in memory, even though it may go out of date. I think the\nbest we could do is keep a stat() for each individual directory we see,\nand check it before using the in-memory contents. That may be a lot of\nstats, but it's still better than actually opening each loose ref\nseparately.\n\nSo I think it's possible to fix, but I thought you might have some\ninsight on the simplest way to fit it into the current ref code.\n\nDid I explain the problem well enough to understand? Can you think of\nany simpler or better solutions (or is there a case where my proposed\nsolutions don't work?).\n\nFor reference, here's a script that demonstrates the problem during\nenumeration (sometimes for-each-ref fails to realize that\nrefs/heads/master exists at all):\n\n # run this in one terminal\n git init repo &&\n cd repo &&\n git commit --allow-empty -m foo &&\n base=`git rev-parse HEAD` &&\n while true; do\n # this re-creates the loose ref in .git/refs/heads/master\n git update-ref refs/heads/master $base &&\n\n # we can remove packed-refs safely, as we know that\n # its only value is now stale. Real git would not do\n # this, but we are simulating the case that \"master\"\n # simply wasn't included in the last packed-refs file.\n rm -f .git/packed-refs &&\n\n # and now we repack, which will create an up-to-date\n # packed-refs file, and then delete the loose ref\n git pack-refs --all --prune\n done\n\n # then simultaneously run this in another\n cd repo &&\n while true; do\n refs=`git for-each-ref`\n echo \"==> $refs\"\n test -z \"$refs\" && break\n done\n\nObviously the \"rm -f packed-refs\" above is contrived, but it does\nsimulate a real possible state. You could also do it with a packed-refs\nfile that has an outdated value for refs/heads/master, and demonstrate\nthat for-each-ref sees the outdated value.\n\n-Peff\n"},{"id":"216348","messageId":"CALKQrgdHudF1fDLSXzaKfb2kne0B3rC5mM95CJGsLqL_2xemnA@mail.gmail.com","threadId":"33713","inReplyTo":"20130503083847.GA16542@sigill.intra.peff.net","subject":"Re: another packed-refs race","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2013-05-03T09:26:11Z","receivedAt":"2013-05-03T09:26:11Z","isPatch":false,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"On Fri, May 3, 2013 at 10:38 AM, Jeff King wrote:\n> I found another race related to the packed-refs code. Consider for a\n> moment what happens when we are looking at refs and another process does\n> a simultaneous \"git pack-refs --all --prune\", updating packed-refs and\n> deleting the loose refs.\n>\n> [...]\n>\n> We could fix this by making sure our packed-refs file is up to date\n> before using it. E.g., resolving a ref with the following sequence:\n>\n> 1. Look for loose ref. If found, OK.\n>\n> 2. Compare inode/size/mtime/etc of on-disk packed-refs to their values\n> from the last time we loaded it. If they're not the same, reload\n> packed-refs from disk. Otherwise, continue.\n>\n> 3. Look for ref in in-memory packed-refs.\n>\n> Not too bad. We introduce one extra stat() for a ref that has been\n> packed, and the scheme isn't very complicated.\n>\n> But what about enumerating refs via for_each_ref? It's possible to have\n> the same problem there, and the packed-refs file may be moved into place\n> midway through the process of enumerating the loose refs. So we may see\n> refs/heads/master, but when we get to refs/remotes/origin/master, it has\n> now been packed and pruned. I _think_ we can get by with:\n>\n> 1. Generate the complete sorted list of loose refs.\n>\n> 2. Check that packed-refs is stat-clean, and reload if necessary, as\n> above.\n>\n> 3. Merge the sorted loose and packed lists, letting loose override\n> packed (so even if we get repacked halfway through our loose\n> traversal and get half of the refs there, it's OK to see duplicates\n> in packed-refs, which we will ignore).\n>\n> This is not very far off of what we do now. Obviously we don't do the\n> stat-clean check in step 2. But we also don't generate the complete list\n> of loose refs before hitting the packed-refs file. Instead we lazily\n> load the loose directories as needed. And of course we cache that\n> information in memory, even though it may go out of date. I think the\n> best we could do is keep a stat() for each individual directory we see,\n> and check it before using the in-memory contents. That may be a lot of\n> stats, but it's still better than actually opening each loose ref\n> separately.\n>\n> So I think it's possible to fix, but I thought you might have some\n> insight on the simplest way to fit it into the current ref code.\n>\n> Did I explain the problem well enough to understand? Can you think of\n> any simpler or better solutions (or is there a case where my proposed\n> solutions don't work?).\n\nYou don't really need to be sure that packed-refs is up-to-date. You\nonly need to make sure that don't rely on lazily loading loose refs\n_after_ you have loaded packed-refs.\n\nThe following solution might work in both the resolve-a-single-ref and\nenumerating-refs case:\n\n0. Look for ref already cached in memory. If found, OK.\n\n1. Look for loose ref. If found, OK.\n\n2. If not found, load all loose refs and packed-refs from disk (in\nthat order), and store in memory for remainder of this process. Never\nreload packed-refs from disk (unless you also reload all loose refs\nfirst).\n\nMy rationale for this approach is that if you have a packed-refs file,\nyou will likely have fewer loose refs, so loading all of them in\naddition to the pack-refs file won't be that expensive. (Conversely,\nif you do have a lot of loose refs, you're more likely to hit #1, and\nnot have to load all refs.)\n\nThat said, my intuition on the number of loose vs. packed refs, or the\nrelative cost of reading all loose refs might be off here...\n\n\n...Johan\n\n-- \nJohan Herland, \nwww.herland.net\n"},{"id":"216362","messageId":"20130503172853.GB21715@sigill.intra.peff.net","threadId":"33713","inReplyTo":"CALKQrgdHudF1fDLSXzaKfb2kne0B3rC5mM95CJGsLqL_2xemnA@mail.gmail.com","subject":"Re: another packed-refs race","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-05-03T17:28:53Z","receivedAt":"2013-05-03T17:28:53Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, May 03, 2013 at 11:26:11AM +0200, Johan Herland wrote:\n\n> You don't really need to be sure that packed-refs is up-to-date. You\n> only need to make sure that don't rely on lazily loading loose refs\n> _after_ you have loaded packed-refs.\n\nTrue. As long as you load them both together, and always make sure you\ndo loose first, you'd be fine. But I think there will be corner cases\nwhere you have loaded _part_ of the loose ref namespace. I think part of\nthe point of Michael's ref work is that if you call \"for_each_tag_ref\",\nwe would not waste time loading refs/remotes/ at all. If you then follow\nthat with a call to \"for_each_ref\", you would want to re-use the cached\nwork from traversing refs/tags/, and then traverse refs/remotes/. You\nknow that your cached packed-refs is good with respect to refs/tags/,\nbut you don't with respect to refs/remotes.\n\n> The following solution might work in both the resolve-a-single-ref and\n> enumerating-refs case:\n> \n> 0. Look for ref already cached in memory. If found, OK.\n> \n> 1. Look for loose ref. If found, OK.\n> \n> 2. If not found, load all loose refs and packed-refs from disk (in\n> that order), and store in memory for remainder of this process. Never\n> reload packed-refs from disk (unless you also reload all loose refs\n> first).\n\nI think that would be correct (modulo that step 1 cannot happen for\nenumeration). But we would like to avoid loading all loose refs if we\ncan. Especially on a cold cache, it can be quite slow, and you may not\neven care about those refs for the current operation (I do not recall\nthe exact original motivation for the lazy loading, but it was something\nalong those lines).\n\n> My rationale for this approach is that if you have a packed-refs file,\n> you will likely have fewer loose refs, so loading all of them in\n> addition to the pack-refs file won't be that expensive. (Conversely,\n> if you do have a lot of loose refs, you're more likely to hit #1, and\n> not have to load all refs.)\n> \n> That said, my intuition on the number of loose vs. packed refs, or the\n> relative cost of reading all loose refs might be off here...\n\nI don't think that is necessarily true about the number of loose refs.\nIn a busy repository, you may have many loose refs that have been\nupdated.\n\n-Peff\n"},{"id":"216365","messageId":"20130503182649.GA25379@sigill.intra.peff.net","threadId":"33713","inReplyTo":"20130503172853.GB21715@sigill.intra.peff.net","subject":"Re: another packed-refs race","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-05-03T18:26:49Z","receivedAt":"2013-05-03T18:26:49Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, May 03, 2013 at 01:28:53PM -0400, Jeff King wrote:\n\n> > The following solution might work in both the resolve-a-single-ref and\n> > enumerating-refs case:\n> > \n> > 0. Look for ref already cached in memory. If found, OK.\n> > \n> > 1. Look for loose ref. If found, OK.\n> > \n> > 2. If not found, load all loose refs and packed-refs from disk (in\n> > that order), and store in memory for remainder of this process. Never\n> > reload packed-refs from disk (unless you also reload all loose refs\n> > first).\n> \n> I think that would be correct (modulo that step 1 cannot happen for\n> enumeration). But we would like to avoid loading all loose refs if we\n> can. Especially on a cold cache, it can be quite slow, and you may not\n> even care about those refs for the current operation (I do not recall\n> the exact original motivation for the lazy loading, but it was something\n> along those lines).\n\nActually, forgetting about enumeration for a minute, that would make\nsingle-ref lookup quite painful. Running \"git rev-parse foo\" shouldn't\nhave to even look at most loose refs in the first place. It should be a\ncouple of open() calls looking for the right spot, and then fall back to\nloading packed-refs.\n\n-Peff\n"},{"id":"216368","messageId":"CALKQrgdG2QOs4Z0vAg7cZt8F5LKd4X8pMMAS0C2HcztbzaBBeQ@mail.gmail.com","threadId":"33713","inReplyTo":"20130503182649.GA25379@sigill.intra.peff.net","subject":"Re: another packed-refs race","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2013-05-03T21:02:48Z","receivedAt":"2013-05-03T21:02:48Z","isPatch":false,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"On Fri, May 3, 2013 at 8:26 PM, Jeff King wrote:\n> On Fri, May 03, 2013 at 01:28:53PM -0400, Jeff King wrote:\n>> > The following solution might work in both the resolve-a-single-ref and\n>> > enumerating-refs case:\n>> >\n>> > 0. Look for ref already cached in memory. If found, OK.\n>> >\n>> > 1. Look for loose ref. If found, OK.\n>> >\n>> > 2. If not found, load all loose refs and packed-refs from disk (in\n>> > that order), and store in memory for remainder of this process. Never\n>> > reload packed-refs from disk (unless you also reload all loose refs\n>> > first).\n>>\n>> I think that would be correct (modulo that step 1 cannot happen for\n>> enumeration). But we would like to avoid loading all loose refs if we\n>> can. Especially on a cold cache, it can be quite slow, and you may not\n>> even care about those refs for the current operation (I do not recall\n>> the exact original motivation for the lazy loading, but it was something\n>> along those lines).\n>\n> Actually, forgetting about enumeration for a minute, that would make\n> single-ref lookup quite painful. Running \"git rev-parse foo\" shouldn't\n> have to even look at most loose refs in the first place. It should be a\n> couple of open() calls looking for the right spot, and then fall back to\n> loading packed-refs.\n\nTrue. I was overemphasizing the case where we start looking up one\nref, and later look up more refs from the same process (in which case\nthe load-everything step would be amortized across the other lookups),\nbut this is probably not the ref access pattern for most Git commands,\nand definitely not for \"git rev-parse foo\". I think your approach is\nbetter.\n\n\n...Johan\n\n-- \nJohan Herland, \nwww.herland.net\n"},{"id":"216369","messageId":"20130503212144.GA17698@sigill.intra.peff.net","threadId":"33713","inReplyTo":"20130503083847.GA16542@sigill.intra.peff.net","subject":"Re: another packed-refs race","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-05-03T21:21:44Z","receivedAt":"2013-05-03T21:21:44Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, May 03, 2013 at 04:38:47AM -0400, Jeff King wrote:\n\n> For reference, here's a script that demonstrates the problem during\n> enumeration (sometimes for-each-ref fails to realize that\n> refs/heads/master exists at all):\n> \n> # run this in one terminal\n> git init repo &&\n> cd repo &&\n> git commit --allow-empty -m foo &&\n> base=`git rev-parse HEAD` &&\n> while true; do\n> # this re-creates the loose ref in .git/refs/heads/master\n> git update-ref refs/heads/master $base &&\n\nIt turns out this is wrong. Git is smart enough not to bother writing\nout the loose ref if it isn't changing. So the script as I showed it\nactually ends up in a state with _neither_ the packed-refs file nor the\nloose ref for an instant.\n\nThe correct script looks like this (it just flips between two objects):\n\n git init -q repo &&\n cd repo &&\n git commit -q --allow-empty -m one &&\n one=`git rev-parse HEAD` &&\n git commit -q --allow-empty -m two &&\n two=`git rev-parse HEAD` &&\n sha1=$one &&\n while true; do\n # this re-creates the loose ref in .git/refs/heads/master\n if test \"$sha1\" = \"$one\"; then\n sha1=$two\n else\n sha1=$one\n fi &&\n git update-ref refs/heads/master $sha1 &&\n\n # we can remove packed-refs safely, as we know that\n # its only value is now stale. Real git would not do\n # this, but we are simulating the case that \"master\"\n # simply wasn't included in the last packed-refs file.\n rm -f .git/packed-refs &&\n\n # and now we repack, which will create an up-to-date\n # packed-refs file, and then delete the loose ref\n git pack-refs --all --prune\n done\n\nAnd a racy lookup check could look like this:\n\n cd repo &&\n while true; do\n ref=`git rev-parse --verify master`\n echo \"==> $ref\"\n test -z \"$ref\" && break\n done\n\nit doesn't know which of the two flipping refs it will get on any given\ninvocation, but it should never see nothing. It should get one or the\nother. With stock git, running these two looks for me simultaneously\ntypically causes a failure in the second one within about 15 seconds.\nThe (messy, not ready for application) patch below fixes it (at least I\nlet it run for 30 minutes without a problem).\n\nThe fix is actually two-fold:\n\n 1. Re-load the packed-refs file after each loose object lookup\n failure. This is made more palatable by using stat() to avoid\n re-reading the file in the common case that it wasn't updated.\n\n 2. The loose ref reading itself is actually not atomic. We call\n lstat() on the ref to find out whether it exists (and whether it is\n a symlink). If we get ENOENT, we fall back to finding the loose\n ref. If it does exist and is a regular file, we proceed to open()\n it. But if the ref gets packed and pruned in the interim, our open\n will fail and we just return NULL to say \"oops, I guess it doesn't\n exist\". We want the same fallback-to-packed behavior we would get\n if the lstat failed.\n\n We could potentially do the same when we readlink() a symbolic\n link, but I don't think it is necessary. We do not pack symbolic\n refs, so if readlink gets ENOENT, it's OK to say \"nope, the ref\n does not exist\".\n\nThis doesn't cover the for_each_ref enumeration case at all, which\nshould still fail. I'll try to look at that next.\n\n---\ndiff --git a/refs.c b/refs.c\nindex de2d8eb..45a7ee6 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -708,6 +708,7 @@ static struct ref_cache {\n \tstruct ref_cache *next;\n \tstruct ref_entry *loose;\n \tstruct ref_entry *packed;\n+\tstruct stat packed_validity;\n \t/* The submodule name, or \"\" for the main repo. */\n \tchar name[FLEX_ARRAY];\n } *ref_cache;\n@@ -717,6 +718,7 @@ static void clear_packed_ref_cache(struct ref_cache *refs)\n \tif (refs->packed) {\n \t\tfree_ref_entry(refs->packed);\n \t\trefs->packed = NULL;\n+\t\tmemset(&refs->packed_validity, 0, sizeof(refs->packed_validity));\n \t}\n }\n \n@@ -876,19 +878,57 @@ static struct ref_dir *get_packed_refs(struct ref_cache *refs)\n \t}\n }\n \n+/*\n+ * Returns 1 if the cached stat information matches the\n+ * current state of the file, and 0 otherwise. This should\n+ * probably be refactored to share code with ce_match_stat_basic,\n+ * which has platform-specific knobs for which fields to respect.\n+ */\n+static int check_stat_validity(const struct stat *old, const char *fn)\n+{\n+\tstatic struct stat null;\n+\tstruct stat cur;\n+\n+\tif (stat(fn, &cur))\n+\t\treturn errno == ENOENT && !memcmp(old, &null, sizeof(null));\n+\treturn cur.st_ino == old->st_ino &&\n+\t cur.st_size == old->st_size &&\n+\t cur.st_mtime == old->st_mtime;\n+}\n+\n+/*\n+ * Call fstat, but zero out the stat structure if for whatever\n+ * reason we can't get an answer.\n+ */\n+static int safe_fstat(int fd, struct stat *out)\n+{\n+\tint r = fstat(fd, out);\n+\tif (r)\n+\t\tmemset(out, 0, sizeof(*out));\n+\treturn r;\n+}\n+\n static struct ref_dir *get_packed_refs(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 !check_stat_validity(&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 = 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+\n \t\tf = fopen(packed_refs_file, \"r\");\n \t\tif (f) {\n+\t\t\tsafe_fstat(fileno(f), &refs->packed_validity);\n \t\t\tread_packed_refs(f, get_ref_dir(refs->packed));\n \t\t\tfclose(f);\n \t\t}\n@@ -1108,6 +1148,13 @@ const char *resolve_ref_unsafe(const char *refname, unsigned char *sha1, int rea\n \t\tgit_snpath(path, sizeof(path), \"%s\", refname);\n \n \t\tif (lstat(path, &st) < 0) {\n+\t\t\t/*\n+\t\t\t * this lets us reuse this code path\n+\t\t\t * for later syscall failures; it should\n+\t\t\t * almost certainly just get factored out into a\n+\t\t\t * function though\n+\t\t\t */\n+fallback_to_packed:\n \t\t\tif (errno != ENOENT)\n \t\t\t\treturn NULL;\n \t\t\t/*\n@@ -1156,7 +1203,7 @@ const char *resolve_ref_unsafe(const char *refname, unsigned char *sha1, int rea\n \t\t */\n \t\tfd = open(path, O_RDONLY);\n \t\tif (fd < 0)\n-\t\t\treturn NULL;\n+\t\t\tgoto fallback_to_packed;\n \t\tlen = read_in_full(fd, buffer, sizeof(buffer)-1);\n \t\tclose(fd);\n \t\tif (len < 0)\n"},{"id":"216488","messageId":"51879C1C.5000407@alum.mit.edu","threadId":"33713","inReplyTo":"20130503083847.GA16542@sigill.intra.peff.net","subject":"Re: another packed-refs race","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2013-05-06T12:03:40Z","receivedAt":"2013-05-06T12:03:40Z","isPatch":false,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 05/03/2013 10:38 AM, Jeff King wrote:\n> I found another race related to the packed-refs code. Consider for a\n> moment what happens when we are looking at refs and another process does\n> a simultaneous \"git pack-refs --all --prune\", updating packed-refs and\n> deleting the loose refs.\n> \n> If we are resolving a single ref, then we will either find its loose\n> form or not. If we do, we're done. If not, we can fall back on what was\n> in the packed-refs file. If we read the packed-refs file at that point,\n> we're OK. If the loose ref existed before but was pruned before we could\n> read it, then we know the packed-refs file is already in place, because\n> pack-refs would not have deleted the loose ref until it had finished\n> writing the new file. But imagine that we already loaded the packed-refs\n> file into memory earlier. We may be looking at an _old_ version of it\n> that has an irrelevant sha1 from the older packed-refs file. Or it might\n> not even exist in the packed-refs file at all, and we think the ref does\n> not resolve.\n>\n> We could fix this by making sure our packed-refs file is up to date\n\ns/file/cache/\n\n> before using it. E.g., resolving a ref with the following sequence:\n> \n> 1. Look for loose ref. If found, OK.\n> \n> 2. Compare inode/size/mtime/etc of on-disk packed-refs to their values\n> from the last time we loaded it. If they're not the same, reload\n> packed-refs from disk. Otherwise, continue.\n> \n> 3. Look for ref in in-memory packed-refs.\n> \n> Not too bad. We introduce one extra stat() for a ref that has been\n> packed, and the scheme isn't very complicated.\n\nLet me think out loud alongside your analysis...\n\nBy this mechanism the reader can ensure that it never uses a version of\nthe packed-refs file that is older than its information that the\ncorresponding loose ref is absent from the filesystem.\n\nThis is all assuming that the filesystem accesses have a defined order;\nhow is that guaranteed? pack_refs() and commit_ref() both rely on\ncommit_lock_file(), which calls\n\n close(fd) on the lockfile\n rename(lk->filename, result_file)\n\nprune_ref() locks the ref, verifies that its SHA-1 is unchanged, then\ncalls unlink(), then rollback_lock_file().\n\nThe atomicity of rename() guarantees that a reader sees either the old\nor the new version of the file in question. But what guarantees are\nthere about accesses across two files? Suppose we start with ref \"foo\"\nthat exists only as a loose ref, and we have a pack-refs process doing\n\n write packed-refs with \"foo\"\n commit_lock_file() for packed-refs\n read loose ref \"foo\" and verify that its SHA-1 is unchanged\n unlink() loose ref \"foo\"\n\nwhile another process is trying to read the reference:\n\n look for loose ref \"foo\"\n read packed-refs\n\nIs there any guarantee that the second process can't see the loose ref\n\"foo\" as being missing but nevertheless read the old version of\npacked-refs? I'm not strong enough on filesystem semantics to answer\nthat question.\n\n> But what about enumerating refs via for_each_ref? It's possible to have\n> the same problem there, and the packed-refs file may be moved into place\n> midway through the process of enumerating the loose refs. So we may see\n> refs/heads/master, but when we get to refs/remotes/origin/master, it has\n> now been packed and pruned.\n\nYes.\n\n> I _think_ we can get by with:\n> \n> 1. Generate the complete sorted list of loose refs.\n> \n> 2. Check that packed-refs is stat-clean, and reload if necessary, as\n> above.\n> \n> 3. Merge the sorted loose and packed lists, letting loose override\n> packed (so even if we get repacked halfway through our loose\n> traversal and get half of the refs there, it's OK to see duplicates\n> in packed-refs, which we will ignore).\n> \n> This is not very far off of what we do now. Obviously we don't do the\n> stat-clean check in step 2. But we also don't generate the complete list\n> of loose refs before hitting the packed-refs file. Instead we lazily\n> load the loose directories as needed. And of course we cache that\n> information in memory, even though it may go out of date. I think the\n> best we could do is keep a stat() for each individual directory we see,\n> and check it before using the in-memory contents. That may be a lot of\n> stats, but it's still better than actually opening each loose ref\n> separately.\n\nThe loose refs cache is only used by the for_each_ref() functions, not\nfor looking up individual references. Another approach would be to\nchange the top-level for_each_ref() functions to re-stat() all of the\nloose references within the namespace that interests it, *then* verify\nthat the packed-ref cache is not stale, *then* start the iteration.\nThen there would be no need to re-stat() files during the iteration.\n(This would mean that we have to prohibit a second reference iteration\nfrom being started while one is already in progress.)\n\nOf course, clearing (part of) the loose reference cache invalidates any\npointers that other callers might have retained to refnames in the old\nversion of the cache. I've never really investigated what callers might\nhold onto such pointers under the assumption that they will live to the\nend of the process.\n\nGiven all of this trouble, there is an obvious question: why do we have\na loose reference cache in the first place? I think there are a few\nreasons:\n\n1. In case one git process has to iterate through the same part of the\nreference namespace more than once. (Does this frequently happen?)\n\n2. Reading a bunch of loose references at the same time is more\nefficient than reading them one by one interleaved with other file\nreads. (I think this is a significant win.)\n\n3. Keeping references in a cache means that their refnames have a longer\nlife, which callers can take advantage of to avoid making their own\ncopies. I haven't checked which callers might make this assumption, and\nnowhere is the lifetime of such a refname documented so it is not even\nclear what callers are *allowed* to assume. (In my changes I've tried\nto stay on the safe side by not reducing any lifetimes.)\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"216489","messageId":"51879E2D.8060506@alum.mit.edu","threadId":"33713","inReplyTo":"20130503172853.GB21715@sigill.intra.peff.net","subject":"Re: another packed-refs race","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2013-05-06T12:12:29Z","receivedAt":"2013-05-06T12:12:29Z","isPatch":false,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 05/03/2013 07:28 PM, Jeff King wrote:\n> On Fri, May 03, 2013 at 11:26:11AM +0200, Johan Herland wrote:\n> \n>> You don't really need to be sure that packed-refs is up-to-date. You\n>> only need to make sure that don't rely on lazily loading loose refs\n>> _after_ you have loaded packed-refs.\n> \n> True. As long as you load them both together, and always make sure you\n> do loose first, you'd be fine. But I think there will be corner cases\n> where you have loaded _part_ of the loose ref namespace. I think part of\n> the point of Michael's ref work is that if you call \"for_each_tag_ref\",\n> we would not waste time loading refs/remotes/ at all. If you then follow\n> that with a call to \"for_each_ref\", you would want to re-use the cached\n> work from traversing refs/tags/, and then traverse refs/remotes/. You\n> know that your cached packed-refs is good with respect to refs/tags/,\n> but you don't with respect to refs/remotes.\n\nCorrect.\n\n[I wonder if there really are a lot of iterations over overlapping parts\nof the reference namespace within a single git process...]\n\n>> The following solution might work in both the resolve-a-single-ref and\n>> enumerating-refs case:\n>>\n>> 0. Look for ref already cached in memory. If found, OK.\n>>\n>> 1. Look for loose ref. If found, OK.\n>>\n>> 2. If not found, load all loose refs and packed-refs from disk (in\n>> that order), and store in memory for remainder of this process. Never\n>> reload packed-refs from disk (unless you also reload all loose refs\n>> first).\n> \n> I think that would be correct (modulo that step 1 cannot happen for\n> enumeration). But we would like to avoid loading all loose refs if we\n> can. Especially on a cold cache, it can be quite slow, and you may not\n> even care about those refs for the current operation (I do not recall\n> the exact original motivation for the lazy loading, but it was something\n> along those lines).\n\nLazy loading was first inspired by the observation that effectively\nevery git invocation was loading *all* loose references to do an\niteration over refs/replace/ (which I've never even used!) This was\nabsolutely killing the performance of filter-branch, which creates a lot\nof loose references and invokes git many times--even though the cache\nwas warm.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"216527","messageId":"20130506184122.GA23568@sigill.intra.peff.net","threadId":"33713","inReplyTo":"51879C1C.5000407@alum.mit.edu","subject":"Re: another packed-refs race","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-05-06T18:41:23Z","receivedAt":"2013-05-06T18:41:23Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, May 06, 2013 at 02:03:40PM +0200, Michael Haggerty wrote:\n\n> > We could fix this by making sure our packed-refs file is up to date\n> \n> s/file/cache/\n\nYeah, I meant \"our view of the packed-refs file\", but I think \"cache\"\nsays that more succinctly. I'll be sure to make it clear when I start\nwriting real commit messages.\n\n> Let me think out loud alongside your analysis...\n> \n> By this mechanism the reader can ensure that it never uses a version of\n> the packed-refs file that is older than its information that the\n> corresponding loose ref is absent from the filesystem.\n\nYes, that's a good way of saying it.\n\n> This is all assuming that the filesystem accesses have a defined order;\n> how is that guaranteed? pack_refs() and commit_ref() both rely on\n> commit_lock_file(), which calls\n> \n> close(fd) on the lockfile\n> rename(lk->filename, result_file)\n> \n> prune_ref() locks the ref, verifies that its SHA-1 is unchanged, then\n> calls unlink(), then rollback_lock_file().\n> \n> The atomicity of rename() guarantees that a reader sees either the old\n> or the new version of the file in question. But what guarantees are\n> there about accesses across two files?\n\nI don't know offhand if any standard makes such guarantees. But there\nare other parts of git that do depend on that ordering. For example, I\ncreate a loose object representing commit X. Then I update a ref to\npoint to X. If the ordering of those operations is not preserved, then a\nsimultaneous reader would think that the repository is corrupted (the\nref points to a missing object).\n\nI would not be surprised if there are distributed or networked\nfilesystems for which this property does not hold. But I suspect it does\nhold for operations on a single machine with a decent kernel (I would\nimagine the VFS layer takes care of this). I am just basing my suspicion\non experimental results (the patch I sent does seem to work on my ext4\nsystem).\n\n> The loose refs cache is only used by the for_each_ref() functions, not\n> for looking up individual references. Another approach would be to\n> change the top-level for_each_ref() functions to re-stat() all of the\n> loose references within the namespace that interests it, *then* verify\n> that the packed-ref cache is not stale, *then* start the iteration.\n> Then there would be no need to re-stat() files during the iteration.\n> (This would mean that we have to prohibit a second reference iteration\n> from being started while one is already in progress.)\n\nHmm. Thinking on this more, I'm not sure that we need to stat the loose\nreferences at all. We do not need to care if the loose refs are up to\ndate or not. Well, we might care, but the point here is not to pretend\nthat we have an up-to-date atomic view of the loose refs; it is only to\nmake sure that the fallback-to-packed behavior does not lie to us about\nthe existence or value of a ref.\n\nIOW, it is OK to come up with a value for ref X that was true at the\nbeginning of the program, even if it has been simultaneously updated.\nOur program can operate as if it happened in the instant it started,\neven though in real life it takes longer. But it is _not_ OK to miss the\nexistence of a ref, or to come up with a value that it did not hold at\nsome point during the program (e.g., it is not OK to return some cruft\nwe wrote into the packed-refs file when we packed it three weeks ago).\n\nThat is a weaker guarantee, and I think we can provide it with:\n\n 1. Load all loose refs into cache for a particular enumeration.\n\n 2. Make sure the packed-refs cache is up-to-date (by checking its\n stat() information and reloading if necessary).\n\n 3. Run the usual iteration over the loose/packed ref caches.\n\nIf a loose ref is updated after or during (1), that's OK. The ref\nhierarchy is not atomic, so it is possible for us to see a state that\nnever existed (e.g., we read X, somebody updates X to X' and Y to Y',\nthen we read Y'; we see X and Y', a state that never existed on disk).\nBut either the ref was already loose, in which case we always see its\nloose value as it was when we read it, or it was previously only packed\n(or non-existent), in which case we see get its value from the\npacked-refs (or assume it does not exist), which is its state at the\nstart of our program. If the loose ref is deleted instead of updated,\nit's the same thing; we may see it as existing, as it was at the start\nof our program.\n\nIf the packed-refs file is updated after we have read all of the loose\nrefs, we may see it \"ahead\" of the loose refs state we have cached. And\nthat may mean the packed-refs file even has more up-to-date values. But\nwe don't have to care, as long as we return some value that was valid\nduring the course of our program. And we do, either because we have the\nloose value cached (in which case it trumps the packed-refs version and\nwe use it), or it was missing (in which case we will use the updated\npack-refs value, which may not even be the most recent value, but is at\nleast a value that the ref had to hold at some point during the run of\nour program).\n\nI realize my reasoning is a bit hand-wavy there, but I'm not sure how to\nexpress it more formally. But the gist of it is that we would have the\nsame guarantees under my proposed rule as we would if the loose refs\ncache and the packed-refs file did not exist at all. That is, we cannot\nread the whole refs hierarchy from the filesystem atomically _anyway_,\nso we have to accept that the value we get is only going to be one of\nmany possible values during the run of our program.\n\nThe exception, of course, is when locking a ref for write, but we\nperform that read directly from the filesystem once we have the lock.\n\n> Of course, clearing (part of) the loose reference cache invalidates any\n> pointers that other callers might have retained to refnames in the old\n> version of the cache. I've never really investigated what callers might\n> hold onto such pointers under the assumption that they will live to the\n> end of the process.\n\nMy proposal above gets rid of the need to invalidate the loose refs\ncache, so we can ignore that complexity.\n\n> Given all of this trouble, there is an obvious question: why do we have\n> a loose reference cache in the first place? I think there are a few\n> reasons:\n> \n> 1. In case one git process has to iterate through the same part of the\n> reference namespace more than once. (Does this frequently happen?)\n\nI'm not sure how often it happens. There are a few obvious candidates if\nyou \"git grep 'for_each[a-z_]*ref'\", but I'm not sure if the actual\nperformance impact is measurable (the cache should be warm after the\nfirst run-through).\n\n> 2. Reading a bunch of loose references at the same time is more\n> efficient than reading them one by one interleaved with other file\n> reads. (I think this is a significant win.)\n\nMakes some sense, though I don't recall whether it was ever measured.\n\n> 3. Keeping references in a cache means that their refnames have a longer\n> life, which callers can take advantage of to avoid making their own\n> copies. I haven't checked which callers might make this assumption, and\n> nowhere is the lifetime of such a refname documented so it is not even\n> clear what callers are *allowed* to assume. (In my changes I've tried\n> to stay on the safe side by not reducing any lifetimes.)\n\nYeah, I'm not sure if this is used or not.\n\n-Peff\n"},{"id":"216528","messageId":"20130506184451.GB23568@sigill.intra.peff.net","threadId":"33713","inReplyTo":"51879E2D.8060506@alum.mit.edu","subject":"Re: another packed-refs race","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-05-06T18:44:51Z","receivedAt":"2013-05-06T18:44:51Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, May 06, 2013 at 02:12:29PM +0200, Michael Haggerty wrote:\n\n> > I think that would be correct (modulo that step 1 cannot happen for\n> > enumeration). But we would like to avoid loading all loose refs if we\n> > can. Especially on a cold cache, it can be quite slow, and you may not\n> > even care about those refs for the current operation (I do not recall\n> > the exact original motivation for the lazy loading, but it was something\n> > along those lines).\n> \n> Lazy loading was first inspired by the observation that effectively\n> every git invocation was loading *all* loose references to do an\n> iteration over refs/replace/ (which I've never even used!) This was\n> absolutely killing the performance of filter-branch, which creates a lot\n> of loose references and invokes git many times--even though the cache\n> was warm.\n\nYeah, obviously we want to avoid that. I _think_ we can even keep the\nlazy loading, as long as keep the ordering as:\n\n 1. Load a chunk of loose refs (whatever we need for the current\n iteration request).\n\n 2. Double-check that our packed-refs cache is up to date, and reload\n if necessary.\n\n 3. Return the results to the caller.\n\nIt would perhaps increase latency to getting results to the caller\n(since otherwise we can start feeding answers to the caller as we walk\nthe tree), but should not increase the total amount of work (just the\nextra stat() of packed-refs, once per for_each_ref, which is not much).\n\nI'll see if I can cook up a patch.\n\n-Peff\n"},{"id":"216558","messageId":"20130506221828.GA19851@sigill.intra.peff.net","threadId":"33713","inReplyTo":"20130506184122.GA23568@sigill.intra.peff.net","subject":"Re: another packed-refs race","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-05-06T22:18:28Z","receivedAt":"2013-05-06T22:18:28Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, May 06, 2013 at 02:41:22PM -0400, Jeff King wrote:\n\n> That is a weaker guarantee, and I think we can provide it with:\n> \n> 1. Load all loose refs into cache for a particular enumeration.\n> \n> 2. Make sure the packed-refs cache is up-to-date (by checking its\n> stat() information and reloading if necessary).\n> \n> 3. Run the usual iteration over the loose/packed ref caches.\n\nThis does seem to work in my experiments. With stock git, I can trigger\nthe race reliably with:\n\n # base load, in one terminal, as before\n git init -q repo &&\n cd repo &&\n git commit -q --allow-empty -m one &&\n one=`git rev-parse HEAD` &&\n git commit -q --allow-empty -m two &&\n two=`git rev-parse HEAD` &&\n sha1=$one &&\n while true; do\n # this re-creates the loose ref in .git/refs/heads/master\n if test \"$sha1\" = \"$one\"; then\n sha1=$two\n else\n sha1=$one\n fi &&\n git update-ref refs/heads/master $sha1 &&\n\n # we can remove packed-refs safely, as we know that\n # its only value is now stale. Real git would not do\n # this, but we are simulating the case that \"master\"\n # simply wasn't included in the last packed-refs file.\n rm -f .git/packed-refs &&\n\n # and now we repack, which will create an up-to-date\n # packed-refs file, and then delete the loose ref\n git pack-refs --all --prune\n done\n\n # in another terminal, enumerate and make sure we never miss the ref\n cd repo &&\n while true; do\n refs=`git.compile for-each-ref --format='%(refname)'`\n echo \"==> $refs\"\n test -z \"$refs\" && break\n done\n\nIt usually takes about 30 seconds to hit a problem, though I measured\nfailures at up to 90 seconds. With the patch below (on top of the one I\nposted the other day, which refreshes the packed-refs cache in\nget_packed_refs), it has been running fine for 15 minutes.\n\nThe \"noop_each_fn\" is a little gross. I could also just reimplement the\nrecursion from do_for_each_ref_in_dir (except we don't care about flags,\ntrim, base, etc, and would just be calling get_ref_dir recursively).\nIt's a slight repetition of code, but it would be less subtle than what\nI have written below (which uses a no-op callback for the side effect\nthat it primes the loose ref cache). Which poison do you prefer?\n\ndiff --git a/refs.c b/refs.c\nindex 45a7ee6..59ae7e4 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1363,19 +1363,38 @@ static int do_for_each_ref(const char *submodule, const char *base, each_ref_fn\n \tfor_each_rawref(warn_if_dangling_symref, &data);\n }\n \n+static int noop_each_fn(const char *ref, const unsigned char *sha1, int flags,\n+\t\t\tvoid *data)\n+{\n+\treturn 0;\n+}\n+\n static int do_for_each_ref(const char *submodule, const char *base, each_ref_fn fn,\n \t\t\t int trim, int flags, void *cb_data)\n {\n \tstruct ref_cache *refs = get_ref_cache(submodule);\n-\tstruct ref_dir *packed_dir = get_packed_refs(refs);\n-\tstruct ref_dir *loose_dir = get_loose_refs(refs);\n+\tstruct ref_dir *packed_dir;\n+\tstruct ref_dir *loose_dir;\n \tint retval = 0;\n \n-\tif (base && *base) {\n-\t\tpacked_dir = find_containing_dir(packed_dir, base, 0);\n+\t/*\n+\t * Prime the loose ref cache; we must make sure the packed ref cache is\n+\t * uptodate after we read the loose refs in order to avoid race\n+\t * conditions with a simultaneous \"pack-refs --prune\".\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+\tif (loose_dir) {\n+\t\tsort_ref_dir(loose_dir);\n+\t\tdo_for_each_ref_in_dir(loose_dir, 0, base, noop_each_fn, 0, 0,\n+\t\t\t\t NULL);\n \t}\n \n+\tpacked_dir = get_packed_refs(refs);\n+\tif (base && *base)\n+\t\tpacked_dir = find_containing_dir(packed_dir, base, 0);\n+\n \tif (packed_dir && loose_dir) {\n \t\tsort_ref_dir(packed_dir);\n \t\tsort_ref_dir(loose_dir);\n"},{"id":"216570","messageId":"20130507023610.GA22053@sigill.intra.peff.net","threadId":"33713","inReplyTo":"20130503083847.GA16542@sigill.intra.peff.net","subject":"[PATCH 0/4] fix packed-refs races","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-05-07T02:36:11Z","receivedAt":"2013-05-07T02:36:11Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This fixes the races I brought up in the surrounding thread:\n\n http://thread.gmane.org/gmane.comp.version-control.git/223299\n\nThe individual races are describe in more detail in each commit, but for\nreference, here is the complete reproduction recipe (which I posted\nalready in several parts throughout the thread, but is collected here):\n\n # base.sh\n # run this script forever in one terminal\n git init -q repo &&\n cd repo &&\n git commit -q --allow-empty -m one &&\n one=`git rev-parse HEAD` &&\n git commit -q --allow-empty -m two &&\n two=`git rev-parse HEAD` &&\n sha1=$one &&\n while true; do\n # this re-creates the loose ref in .git/refs/heads/master\n if test \"$sha1\" = \"$one\"; then\n sha1=$two\n else\n sha1=$one\n fi &&\n git update-ref refs/heads/master $sha1 &&\n\n # we can remove packed-refs safely, as we know that\n # its only value is now stale. Real git would not do\n # this, but we are simulating the case that \"master\"\n # simply wasn't included in the last packed-refs file.\n rm -f .git/packed-refs &&\n\n # and now we repack, which will create an up-to-date\n # packed-refs file, and then delete the loose ref\n git pack-refs --all --prune\n done\n\n # lookup.sh\n # run this script simultaneously in another terminal; when it exits,\n # git is broken (it erroneously told us that master did not exist).\n # It is fixed by applying up to patch 3/4 below.\n cd repo &&\n while true; do\n ref=`git rev-parse --verify master`\n echo \"==> $ref\"\n test -z \"$ref\" && break\n done\n\n # enumerate.sh\n # run this script simultaneously in another terminal (either with\n # lookup.sh running, too, or just base.sh); when it exits, we\n # for-each-ref has erroneously missed master. It is fixed by applying\n # up to patch 4/4 below.\n cd repo &&\n while true; do\n refs=`git.compile for-each-ref --format='%(refname)'`\n echo \"==> $refs\"\n test -z \"$refs\" && break\n done\n\nI don't think is a purely hypothetical race. About once a month for the\npast six months or so, I'll run across a corrupted repository on\ngithub.com that is inexplicably missing a few objects near the tip of a\nref. This last time, I happened to catch something very odd in our ref\naudit-log. The log for the repo that became corrupted showed a small\npush to the tip of \"master\", but the objects for that push were missing\n(the previous ref tip was fine). At almost the exact same time, our\nalternates repo was running a \"fetch --prune\" from the corrupted repo.\nBut it didn't see the corrupted master ref; it saw that\nrefs/heads/master didn't exist at all, and it deleted it.\n\nI believe that git-upload-pack hit the enumeration race above, and\nshowed a ref advertisement in which refs/heads/master was missing[1],\nleading fetch to delete the ref. At the same time, a gc was running in\nthe corrupted repo which hit the same issue, and ended up pruning[2] the\nnewly pushed objects, which were not referenced from anywhere else.\n\nIt may seem unlikely for two programs to hit the race at the same time\n(especially given the number of iterations you need to trigger the race\nwith the scripts above), but I think it's exacerbated when you have a\nvery large number of refs (because the loose enumeration takes much\nlonger). So I think there's a good chance this was my problem. And if\nnot, it is good to fix anyway. :)\n\n [1/4]: resolve_ref: close race condition for packed refs\n [2/4]: add a stat_validity struct\n [3/4]: get_packed_refs: reload packed-refs file when it changes\n [4/4]: for_each_ref: load all loose refs before packed refs\n\n-Peff\n\n[1] Usually the enumeration race would cause you to see the old value\n from the packed-refs file, not a completely missing ref (unless the\n ref was not packed at all previously, but that is almost certainly\n not the case here). But it does call into resolve_ref to actually\n read the ref, which has its own race that can cause refs to\n \"disappear\" (and is fixed by patch 1).\n\n As an aside, I think that calling resolve_ref is probably wrong. We\n know we have a fully qualified refname, because we keep track of\n which directory we are calling readdir() on. But if for whatever\n reason somebody deletes it at the exact moment between when we see\n it in readdir() and when we try to open it (not packs it, but\n actually deletes it), then we'll see it is missing and fall back to\n trying other ref-lookups. So in theory a race with a delete of\n refs/heads/foo could accidentally retrieve the value for\n refs/heads/refs/heads/foo.\n\n It's quite unlikely, though, and I'm not too concerned about it.\n\n[2] Recently pushed objects should still be safe, even if they are\n unreachable. However, in our case the pruning was exacerbated by a\n gc codepath that could call \"git repack -ad\" (not in upstream git,\n but we do our own custom gc due to the alternates structure of our\n forks). I've also fixed that, as we should always be using \"-A\" to\n keep recent unreachable objects.\n"},{"id":"216571","messageId":"20130507023802.GA22940@sigill.intra.peff.net","threadId":"33713","inReplyTo":"20130507023610.GA22053@sigill.intra.peff.net","subject":"[PATCH 1/4] resolve_ref: close race condition for packed refs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-05-07T02:38:03Z","receivedAt":"2013-05-07T02:38:03Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When we attempt to resolve a ref, the first thing we do is\ncall lstat() to see if it is a symlink or a real ref. If we\nfind that the ref is missing, we fall back to looking it up\nin the packed-refs file. If we find the loose ref does exist\n(and is a regular file), we continue with trying to open it.\n\nHowever, we do not do the same fallback if our open() call\nfails; we just report the ref as missing. A \"git pack-refs\n--prune\" process which is simultaneously running may remove\nthe loose ref between our lstat() and open(). In this case,\nwe would erroneously report the ref as missing, even though\nwe could find it if we checked the packed-refs file.\n\nThis patch solves it by factoring out the fallback code from\nthe lstat() case and calling it from both places. We do not\nneed to do the same thing for the symlink/readlink code\npath, even though it might receive ENOENT, too, because\nsymlinks cannot be packed (so if it disappears after the\nlstat, it is truly being deleted).\n\nNote that this solves only the part of the race within\nresolve_ref_unsafe. In the situation described above, we may\nstill be depending on a cached view of the packed-refs file;\nthat race will be dealt with in a future patch.\n\nSigned-off-by: Jeff King \n---\n\n refs.c | 63 ++++++++++++++++++++++++++++++++++++++++++---------------------\n 1 file changed, 42 insertions(+), 21 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex de2d8eb..5a14703 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1083,6 +1083,43 @@ static int get_packed_ref(const char *refname, unsigned char *sha1)\n \treturn -1;\n }\n \n+/*\n+ * This should be called from resolve_ref_unsafe when a loose ref cannot be\n+ * accessed; err must represent the errno from the last attempt to access the\n+ * loose ref, and the other options are forwarded from resolve_safe_unsaefe.\n+ */\n+static const char *handle_loose_ref_failure(int err,\n+\t\t\t\t\t const char *refname,\n+\t\t\t\t\t unsigned char *sha1,\n+\t\t\t\t\t int reading,\n+\t\t\t\t\t int *flag)\n+{\n+\t/*\n+\t * If we didn't get ENOENT, something is broken\n+\t * with the loose ref, and we should not fallback,\n+\t * but instead pretend it doesn't exist.\n+\t */\n+\tif (err != ENOENT)\n+\t\treturn NULL;\n+\n+\t/*\n+\t * The loose reference file does not exist;\n+\t * check for a packed reference.\n+\t */\n+\tif (!get_packed_ref(refname, sha1)) {\n+\t\tif (flag)\n+\t\t\t*flag |= REF_ISPACKED;\n+\t\treturn refname;\n+\t}\n+\n+\t/* The reference is not a packed reference, either. */\n+\tif (reading)\n+\t\treturn NULL;\n+\n+\thashclr(sha1);\n+\treturn refname;\n+}\n+\n const char *resolve_ref_unsafe(const char *refname, unsigned char *sha1, int reading, int *flag)\n {\n \tint depth = MAXDEPTH;\n@@ -1107,26 +1144,9 @@ const char *resolve_ref_unsafe(const char *refname, unsigned char *sha1, int rea\n \n \t\tgit_snpath(path, sizeof(path), \"%s\", refname);\n \n-\t\tif (lstat(path, &st) < 0) {\n-\t\t\tif (errno != ENOENT)\n-\t\t\t\treturn NULL;\n-\t\t\t/*\n-\t\t\t * The loose reference file does not exist;\n-\t\t\t * check for a packed reference.\n-\t\t\t */\n-\t\t\tif (!get_packed_ref(refname, sha1)) {\n-\t\t\t\tif (flag)\n-\t\t\t\t\t*flag |= REF_ISPACKED;\n-\t\t\t\treturn refname;\n-\t\t\t}\n-\t\t\t/* The reference is not a packed reference, either. */\n-\t\t\tif (reading) {\n-\t\t\t\treturn NULL;\n-\t\t\t} else {\n-\t\t\t\thashclr(sha1);\n-\t\t\t\treturn refname;\n-\t\t\t}\n-\t\t}\n+\t\tif (lstat(path, &st) < 0)\n+\t\t\treturn handle_loose_ref_failure(errno, refname, sha1,\n+\t\t\t\t\t\t\treading, flag);\n \n \t\t/* Follow \"normalized\" - ie \"refs/..\" symlinks by hand */\n \t\tif (S_ISLNK(st.st_mode)) {\n@@ -1156,7 +1176,8 @@ const char *resolve_ref_unsafe(const char *refname, unsigned char *sha1, int rea\n \t\t */\n \t\tfd = open(path, O_RDONLY);\n \t\tif (fd < 0)\n-\t\t\treturn NULL;\n+\t\t\treturn handle_loose_ref_failure(errno, refname, sha1,\n+\t\t\t\t\t\t\treading, flag);\n \t\tlen = read_in_full(fd, buffer, sizeof(buffer)-1);\n \t\tclose(fd);\n \t\tif (len < 0)\n-- \n1.8.3.rc1.2.g12db477\n"},{"id":"216572","messageId":"20130507023942.GB22940@sigill.intra.peff.net","threadId":"33713","inReplyTo":"20130507023610.GA22053@sigill.intra.peff.net","subject":"[PATCH 2/4] add a stat_validity struct","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-05-07T02:39:42Z","receivedAt":"2013-05-07T02:39:42Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?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 cache_entry\" 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: Jeff King \n---\nThis one is prep for the next patch. I'm not super-happy with the way it\nbuilds around cache_entry, just because cache entries may end up\nfollowing different rules in the long run. But I at least tried to\nencapsulate the grossness, so if it turns out to be a problem, we can\nfactor out the relevant bits from ce_match_stat_basic into a shared\nfunction.\n\n cache.h | 27 +++++++++++++++++++++++++++\n read-cache.c | 31 +++++++++++++++++++++++++++++++\n 2 files changed, 58 insertions(+)\n\ndiff --git a/cache.h b/cache.h\nindex 94ca1ac..adf2874 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1326,4 +1326,31 @@ int sane_execvp(const char *file, char *const argv[]);\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 is a simplified version of the up-to-date\n+ * checks we use for the index. The implementation is built on an index entry,\n+ * but we shield the callers from that ugliness with our struct.\n+ */\n+struct stat_validity {\n+\tstruct cache_entry *ce;\n+};\n+\n+void stat_validity_clear(struct stat_validity *sv);\n+\n+/*\n+ * Returns 1 if the path matches the saved stat_validity, 0 otherwise.\n+ * A missing 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 the file\n+ * is missing or inaccessible, the validity will reflect that, and future\n+ * calls to stat_validity_check will match only if it continues to be\n+ * inaccessible).\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 04ed561..a0bd06c 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -1933,3 +1933,34 @@ 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->ce);\n+\tsv->ce = 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->ce == NULL;\n+\tif (!sv->ce)\n+\t\treturn 0;\n+\treturn !ce_match_stat_basic(sv->ce, &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)\n+\t\tstat_validity_clear(sv);\n+\telse {\n+\t\tif (!sv->ce)\n+\t\t\tsv->ce = xcalloc(1, cache_entry_size(0));\n+\t\tfill_stat_cache_info(sv->ce, &st);\n+\t\tsv->ce->ce_mode = create_ce_mode(st.st_mode);\n+\t}\n+}\n-- \n1.8.3.rc1.2.g12db477\n"},{"id":"216573","messageId":"20130507024313.GC22940@sigill.intra.peff.net","threadId":"33713","inReplyTo":"20130507023610.GA22053@sigill.intra.peff.net","subject":"[PATCH 3/4] get_packed_refs: reload packed-refs file when it changes","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-05-07T02:43:13Z","receivedAt":"2013-05-07T02:43:13Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Once 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 it's 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: Jeff King \n---\nI hooked the refreshing into get_packed_refs, since then all callers get\nit for free. It makes me a little nervous, though, just in case some\ncaller really cares about calling get_packed_refs but not having the\nlist of packed-refs change during the call. peel_ref looks like such a\nfunction, but isn't, for reasons I'll explain in a followup patch.\n\nClone also looks like such a caller, as it calls get_packed_refs once\nfor each upstream ref it adds (it puts them in the packed-refs list, and\nthen writes them all out at the end). But it's OK because there is no\npacked-refs file for it to refresh from.\n\nAn alternative would be to move the refreshing to an explicit\nrefresh_packed_refs() function, and call it from a few places\n(resolve_ref, and later from do_for_each_ref).\n\n refs.c | 20 +++++++++++++++-----\n 1 file changed, 15 insertions(+), 5 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 5a14703..6afe8cc 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -708,6 +708,7 @@ static struct ref_cache {\n \tstruct ref_cache *next;\n \tstruct ref_entry *loose;\n \tstruct ref_entry *packed;\n+\tstruct stat_validity packed_validity;\n \t/* The submodule name, or \"\" for the main repo. */\n \tchar name[FLEX_ARRAY];\n } *ref_cache;\n@@ -717,6 +718,7 @@ static void clear_packed_ref_cache(struct ref_cache *refs)\n \tif (refs->packed) {\n \t\tfree_ref_entry(refs->packed);\n \t\trefs->packed = NULL;\n+\t\tstat_validity_clear(&refs->packed_validity);\n \t}\n }\n \n@@ -878,17 +880,25 @@ static struct ref_dir *get_packed_refs(struct ref_cache *refs)\n \n static struct ref_dir *get_packed_refs(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 = 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+\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));\n \t\t\tfclose(f);\n \t\t}\n-- \n1.8.3.rc1.2.g12db477\n"},{"id":"216574","messageId":"20130507025103.GD22940@sigill.intra.peff.net","threadId":"33713","inReplyTo":"20130507023610.GA22053@sigill.intra.peff.net","subject":"[PATCH 4/4] for_each_ref: load all loose refs before packed refs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-05-07T02:51:04Z","receivedAt":"2013-05-07T02:51:04Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"If 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: Jeff King \n---\nSo this cache-priming is ugly, but I don't think it will have a\nperformance impact, as it his hitting only directories and\nfiles that we were about to see in a second anyway during\nthe real traversal. The only difference is that there is a\nslight latency before we would start producing any output,\nas we read the whole loose refs tree (well, the part we are\ninterested for this call) before processing any refs.\n\nIn theory we could do our lazy walk, and then at the end\ndouble-check that packed-refs had not changed. But if it\ndoes change, we couldn't output any newly found refs, then;\nthey would be out of sorted order. The only thing we could\ndo is die(). Which might be OK, and is certainly preferable\nto silently dropping a ref and indicating that we processed\nthe whole list. But I think the latency is probably not that\nbig a deal.\n\n refs.c | 38 +++++++++++++++++++++++++++++++++-----\n 1 file changed, 33 insertions(+), 5 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 6afe8cc..c127baf 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -641,6 +641,21 @@ static int do_for_each_ref_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@@ -1351,14 +1366,27 @@ static int do_for_each_ref(const char *submodule, const char *base, each_ref_fn\n \t\t\t int trim, int flags, void *cb_data)\n {\n \tstruct ref_cache *refs = get_ref_cache(submodule);\n-\tstruct ref_dir *packed_dir = get_packed_refs(refs);\n-\tstruct ref_dir *loose_dir = get_loose_refs(refs);\n+\tstruct ref_dir *packed_dir;\n+\tstruct ref_dir *loose_dir;\n \tint retval = 0;\n \n-\tif (base && *base) {\n-\t\tpacked_dir = find_containing_dir(packed_dir, base, 0);\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_refs()\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_dir = get_packed_refs(refs);\n+\tif (base && *base)\n+\t\tpacked_dir = find_containing_dir(packed_dir, base, 0);\n \n \tif (packed_dir && loose_dir) {\n \t\tsort_ref_dir(packed_dir);\n-- \n1.8.3.rc1.2.g12db477\n"},{"id":"216575","messageId":"20130507025458.GA22912@sigill.intra.peff.net","threadId":"33713","inReplyTo":"20130507024313.GC22940@sigill.intra.peff.net","subject":"[PATCH 0/2] peel_ref cleanups changes","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-05-07T02:54:58Z","receivedAt":"2013-05-07T02:54:58Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, May 06, 2013 at 10:43:13PM -0400, Jeff King wrote:\n\n> I hooked the refreshing into get_packed_refs, since then all callers get\n> it for free. It makes me a little nervous, though, just in case some\n> caller really cares about calling get_packed_refs but not having the\n> list of packed-refs change during the call. peel_ref looks like such a\n> function, but isn't, for reasons I'll explain in a followup patch.\n\nI thought I would need the cleanups below for peel_ref, but it turns out\nthat the middle chunk of the function is never exercised. They conflict\ntextually with Michael's mh/packed-refs-various topic, so I think it may\nmake sense to just rebase them on top of that.\n\n [1/2]: peel_ref: rename \"sha1\" argument to \"peeled\"\n [2/2]: peel_ref: refactor for safety with simultaneous update\n\n-Peff\n"},{"id":"216576","messageId":"20130507025621.GA23219@sigill.intra.peff.net","threadId":"33713","inReplyTo":"20130507025458.GA22912@sigill.intra.peff.net","subject":"[PATCH 1/2] peel_ref: rename \"sha1\" argument to \"peeled\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-05-07T02:56:22Z","receivedAt":"2013-05-07T02:56:22Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The \"sha1\" argument of peel_ref is meant to hold the peeled\nobject name for output. Let's call it \"peeled\" which makes\nit more clear that it is not an input sha1 (especially as we\nwill be adding such an input sha1 in the next patch).\n\nSigned-off-by: Jeff King \n---\nSimple cleanup for the next step.\n\n refs.c | 8 ++++----\n refs.h | 2 +-\n 2 files changed, 5 insertions(+), 5 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex de2d8eb..89f8141 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1231,7 +1231,7 @@ static int filter_refs(const char *refname, const unsigned char *sha1, int flags\n \treturn filter->fn(refname, sha1, flags, filter->cb_data);\n }\n \n-int peel_ref(const char *refname, unsigned char *sha1)\n+int peel_ref(const char *refname, unsigned char *peeled)\n {\n \tint flag;\n \tunsigned char base[20];\n@@ -1242,7 +1242,7 @@ int peel_ref(const char *refname, unsigned char *sha1)\n \t\tif (current_ref->flag & REF_KNOWS_PEELED) {\n \t\t\tif (is_null_sha1(current_ref->u.value.peeled))\n \t\t\t return -1;\n-\t\t\thashcpy(sha1, current_ref->u.value.peeled);\n+\t\t\thashcpy(peeled, current_ref->u.value.peeled);\n \t\t\treturn 0;\n \t\t}\n \t\thashcpy(base, current_ref->u.value.sha1);\n@@ -1257,7 +1257,7 @@ int peel_ref(const char *refname, unsigned char *sha1)\n \t\tstruct ref_entry *r = find_ref(dir, refname);\n \n \t\tif (r != NULL && r->flag & REF_KNOWS_PEELED) {\n-\t\t\thashcpy(sha1, r->u.value.peeled);\n+\t\t\thashcpy(peeled, r->u.value.peeled);\n \t\t\treturn 0;\n \t\t}\n \t}\n@@ -1274,7 +1274,7 @@ fallback:\n \tif (o->type == OBJ_TAG) {\n \t\to = deref_tag_noverify(o);\n \t\tif (o) {\n-\t\t\thashcpy(sha1, o->sha1);\n+\t\t\thashcpy(peeled, o->sha1);\n \t\t\treturn 0;\n \t\t}\n \t}\ndiff --git a/refs.h b/refs.h\nindex a35eafc..1e8b4e1 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -61,7 +61,7 @@ extern int ref_exists(const char *);\n \n extern int ref_exists(const char *);\n \n-extern int peel_ref(const char *refname, unsigned char *sha1);\n+extern int peel_ref(const char *refname, unsigned char *peeled);\n \n /** Locks a \"refs/\" ref returning the lock on success and NULL on failure. **/\n extern struct ref_lock *lock_ref_sha1(const char *refname, const unsigned char *old_sha1);\n-- \n1.8.3.rc1.2.g12db477\n"},{"id":"216578","messageId":"20130507030641.GB23219@sigill.intra.peff.net","threadId":"33713","inReplyTo":"20130507025458.GA22912@sigill.intra.peff.net","subject":"[PATCH 2/2] peel_ref: refactor for safety with simultaneous update","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-05-07T03:06:41Z","receivedAt":"2013-05-07T03:06:41Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The peel_ref function lets a caller peel an object, taking\nadvantage of the fact that we may have the peeled value\ncached for a packed ref. However, this function is not\nnecessarily safe in the face of simultaneous ref updates.\n\nThe caller has typically already loaded the ref from disk\n(e.g., as part of a for_each_ref enumeration), and therefore\nknows what the ref's base sha1 is. But if the asked-for ref\nis not the current ref, we will load the ref from disk\nourselves. If another process is simultaneously updating the\nref, we may not get the same sha1 that the caller thinks is\nin the ref, and as a result may return a peeled value that\ndoes not match the sha1 that the caller has.\n\nTo make this safer, we can have the caller pass in its idea\nof the sha1 of the ref, and use that as the base for peeling\n(and we can also double-check that it matches the\ncurrent_ref pointer when we use that).\n\nThis makes the function harder to call (it is not really\n\"peel this ref\", but \"peel this object, and by the way, it\nis called by this refname\"). However, none of the current\ncallers care, because they always call it from a\nfor_each_ref enumeration, meaning it is no extra work to\npass in the sha1.\n\nFurthermore, that means that none of the current callers hit\nthe \"read_ref_full\" codepath at all (so this bug is not\ncurrently triggerable). They either follow the current_ref\noptimization, or if the ref is not packed, they go straight\nto the deref_tag fallback.\n\nLet's just rip out the unused code path entirely. Not only\nis it not used now, but it would not even make sense to use:\nyou would get the peeled value of the ref, but you would\nhave no clue which base object led to that peel.\n\nSigned-off-by: Jeff King \n---\nSo I think peel_ref is buggy as-is (in both master and in\nmh/packed-refs-various), it's just not triggerable because of the\ndead code path. And it would get buggier again with my other patch\nseries, as then get_packed_ref could actually re-read the packed-refs\nfile. But I really think peel_ref is a badly designed function.\n\nYou do not peel a ref; you peel an object. You might think it is\nconvenient to have a function do the ref lookup and the peeling\ntogether, but in practice nobody wants that, because you would not know\nwhat you had just peeled. The real point is to hit the current_ref\noptimization. Michael's series already has factored out a peel_object\nfunction with the relevant bits. I think we can simply do away with\npeel_ref entirely, and move the current_ref optimization there. I think\nit should be perfectly cromulent to do:\n\n int peel_object(const unsigned char *base, const unsigned char *peeled)\n {\n if (current_ref && current_ref->flag & REF_KNOWS_PEELED &&\n !hashcmp(base, current_ref->u.value.sha1)) {\n hashcpy(peeled, current_ref->u.value.peeled);\n return 0;\n }\n\n /* peel as usual */\n }\n\nThat is, we do not need to care that it is _our_ ref that peels to that.\nWe happen to have a ref whose peeled value we know, and its object\nmatches the one we want to peel. Whether it is the same ref or not, the\nsame object sha1 should peel to the same peeled sha1. Of course, in\npractice it will be our ref, because that is how the current callers\nwork. But it is also a safe function for callers who do not know or care\nabout the optimization.\n\nMy original patch on \"master\" is below for illustration, but do not look\ntoo closely. I think the path I outlined above makes more sense, but I\nam not going to work on it tonight.\n\n builtin/describe.c | 2 +-\n builtin/pack-objects.c | 4 ++--\n builtin/show-ref.c | 2 +-\n refs.c | 24 ++++--------------------\n refs.h | 3 ++-\n upload-pack.c | 2 +-\n 6 files changed, 11 insertions(+), 26 deletions(-)\n\ndiff --git a/builtin/describe.c b/builtin/describe.c\nindex 6636a68..23d7f1a 100644\n--- a/builtin/describe.c\n+++ b/builtin/describe.c\n@@ -150,7 +150,7 @@ static int get_name(const char *path, const unsigned char *sha1, int flag, void\n \t\treturn 0;\n \n \t/* Is it annotated? */\n-\tif (!peel_ref(path, peeled)) {\n+\tif (!peel_ref(path, sha1, peeled)) {\n \t\tis_annotated = !!hashcmp(sha1, peeled);\n \t} else {\n \t\thashcpy(peeled, sha1);\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex f069462..76352df 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -556,7 +556,7 @@ static int mark_tagged(const char *path, const unsigned char *sha1, int flag,\n \n \tif (entry)\n \t\tentry->tagged = 1;\n-\tif (!peel_ref(path, peeled)) {\n+\tif (!peel_ref(path, sha1, peeled)) {\n \t\tentry = locate_object_entry(peeled);\n \t\tif (entry)\n \t\t\tentry->tagged = 1;\n@@ -2032,7 +2032,7 @@ static int add_ref_tag(const char *path, const unsigned char *sha1, int flag, vo\n \tunsigned char peeled[20];\n \n \tif (!prefixcmp(path, \"refs/tags/\") && /* is a tag? */\n-\t !peel_ref(path, peeled) && /* peelable? */\n+\t !peel_ref(path, sha1, peeled) && /* peelable? */\n \t locate_object_entry(peeled)) /* object packed? */\n \t\tadd_object_entry(sha1, OBJ_TAG, NULL, 0);\n \treturn 0;\ndiff --git a/builtin/show-ref.c b/builtin/show-ref.c\nindex 8d9b76a..64f339d 100644\n--- a/builtin/show-ref.c\n+++ b/builtin/show-ref.c\n@@ -78,7 +78,7 @@ match:\n \tif (!deref_tags)\n \t\treturn 0;\n \n-\tif (!peel_ref(refname, peeled)) {\n+\tif (!peel_ref(refname, sha1, peeled)) {\n \t\thex = find_unique_abbrev(peeled, abbrev);\n \t\tprintf(\"%s %s^{}\\n\", hex, refname);\n \t}\ndiff --git a/refs.c b/refs.c\nindex 89f8141..c16bd75 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1231,38 +1231,22 @@ int peel_ref(const char *refname, unsigned char *peeled)\n \treturn filter->fn(refname, sha1, flags, filter->cb_data);\n }\n \n-int peel_ref(const char *refname, unsigned char *peeled)\n+int peel_ref(const char *refname, const unsigned char *base,\n+\t unsigned char *peeled)\n {\n-\tint flag;\n-\tunsigned char base[20];\n \tstruct object *o;\n \n \tif (current_ref && (current_ref->name == refname\n-\t\t|| !strcmp(current_ref->name, refname))) {\n+\t\t|| !strcmp(current_ref->name, refname))\n+\t && !hashcmp(current_ref->u.value.sha1, base)) {\n \t\tif (current_ref->flag & REF_KNOWS_PEELED) {\n \t\t\tif (is_null_sha1(current_ref->u.value.peeled))\n \t\t\t return -1;\n \t\t\thashcpy(peeled, current_ref->u.value.peeled);\n \t\t\treturn 0;\n \t\t}\n-\t\thashcpy(base, current_ref->u.value.sha1);\n-\t\tgoto fallback;\n-\t}\n-\n-\tif (read_ref_full(refname, base, 1, &flag))\n-\t\treturn -1;\n-\n-\tif ((flag & REF_ISPACKED)) {\n-\t\tstruct ref_dir *dir = get_packed_refs(get_ref_cache(NULL));\n-\t\tstruct ref_entry *r = find_ref(dir, refname);\n-\n-\t\tif (r != NULL && r->flag & REF_KNOWS_PEELED) {\n-\t\t\thashcpy(peeled, r->u.value.peeled);\n-\t\t\treturn 0;\n-\t\t}\n \t}\n \n-fallback:\n \to = lookup_unknown_object(base);\n \tif (o->type == OBJ_NONE) {\n \t\tint type = sha1_object_info(base, NULL);\ndiff --git a/refs.h b/refs.h\nindex 1e8b4e1..39817a4 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -61,7 +61,8 @@ extern int ref_exists(const char *);\n \n extern int ref_exists(const char *);\n \n-extern int peel_ref(const char *refname, unsigned char *peeled);\n+extern int peel_ref(const char *refname, const unsigned char *base,\n+\t\t unsigned char *peeled);\n \n /** Locks a \"refs/\" ref returning the lock on success and NULL on failure. **/\n extern struct ref_lock *lock_ref_sha1(const char *refname, const unsigned char *old_sha1);\ndiff --git a/upload-pack.c b/upload-pack.c\nindex bfa6279..6acff92 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -755,7 +755,7 @@ static int send_ref(const char *refname, const unsigned char *sha1, int flag, vo\n \telse\n \t\tpacket_write(1, \"%s %s\\n\", sha1_to_hex(sha1), refname_nons);\n \tcapabilities = NULL;\n-\tif (!peel_ref(refname, peeled))\n+\tif (!peel_ref(refname, sha1, peeled))\n \t\tpacket_write(1, \"%s %s^{}\\n\", sha1_to_hex(peeled), refname_nons);\n \treturn 0;\n }\n-- \n1.8.3.rc1.2.g12db477\n"},{"id":"216584","messageId":"518883CC.7050609@alum.mit.edu","threadId":"33713","inReplyTo":"20130506184122.GA23568@sigill.intra.peff.net","subject":"Re: another packed-refs race","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2013-05-07T04:32:12Z","receivedAt":"2013-05-07T04:32:12Z","isPatch":false,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 05/06/2013 08:41 PM, Jeff King wrote:\n> On Mon, May 06, 2013 at 02:03:40PM +0200, Michael Haggerty wrote:\n> [...]\n>> The loose refs cache is only used by the for_each_ref() functions, not\n>> for looking up individual references. Another approach would be to\n>> change the top-level for_each_ref() functions to re-stat() all of the\n>> loose references within the namespace that interests it, *then* verify\n>> that the packed-ref cache is not stale, *then* start the iteration.\n>> Then there would be no need to re-stat() files during the iteration.\n>> (This would mean that we have to prohibit a second reference iteration\n>> from being started while one is already in progress.)\n> \n> Hmm. Thinking on this more, I'm not sure that we need to stat the loose\n> references at all. We do not need to care if the loose refs are up to\n> date or not. Well, we might care, but the point here is not to pretend\n> that we have an up-to-date atomic view of the loose refs; it is only to\n> make sure that the fallback-to-packed behavior does not lie to us about\n> the existence or value of a ref.\n> \n> IOW, it is OK to come up with a value for ref X that was true at the\n> beginning of the program, even if it has been simultaneously updated.\n> Our program can operate as if it happened in the instant it started,\n> even though in real life it takes longer. But it is _not_ OK to miss the\n> existence of a ref, or to come up with a value that it did not hold at\n> some point during the program (e.g., it is not OK to return some cruft\n> we wrote into the packed-refs file when we packed it three weeks ago).\n\nThis all sounds correct to me.\n\nAnother potential problem caused by the non-atomicity of loose reference\nreading could be to confuse reachability tests if process 1 is reading\nloose references while process 2 is renaming a reference:\n\n1. Process 1 looks for refs/heads/aaaaa and finds it missing.\n\n2. Process 2 renames zzzzz -> aaaaa\n\n3. Process 1 looks for refs/heads/zzzzz and finds it missing.\n\nProcess 2 would think that any objects pointed to by aaaaa (formerly\nzzzzz) are unreachable. This would be unfortunate if it is doing an\nupload-pack and very bad if it is doing a gc. I wonder if this could be\na problem in practice? (Gee, wouldn't it be nice to keep reflogs for\ndeleted refs? :-) )\n\n> That is a weaker guarantee, and I think we can provide it with:\n> \n> 1. Load all loose refs into cache for a particular enumeration.\n> \n> 2. Make sure the packed-refs cache is up-to-date (by checking its\n> stat() information and reloading if necessary).\n> \n> 3. Run the usual iteration over the loose/packed ref caches.\n\nThis sounds reasonable, too, though I'll need some more time to digest\nyour suggestion in detail.\n\n> [...]\n>> Of course, clearing (part of) the loose reference cache invalidates any\n>> pointers that other callers might have retained to refnames in the old\n>> version of the cache. I've never really investigated what callers might\n>> hold onto such pointers under the assumption that they will live to the\n>> end of the process.\n> \n> My proposal above gets rid of the need to invalidate the loose refs\n> cache, so we can ignore that complexity.\n\nThe same concern applies to invalidating the packed-ref cache, which you\nstill want to do.\n\n>> Given all of this trouble, there is an obvious question: why do we have\n>> a loose reference cache in the first place? I think there are a few\n>> reasons:\n>>\n>> 1. In case one git process has to iterate through the same part of the\n>> reference namespace more than once. (Does this frequently happen?)\n> \n> I'm not sure how often it happens. There are a few obvious candidates if\n> you \"git grep 'for_each[a-z_]*ref'\", but I'm not sure if the actual\n> performance impact is measurable (the cache should be warm after the\n> first run-through).\n> \n>> 2. Reading a bunch of loose references at the same time is more\n>> efficient than reading them one by one interleaved with other file\n>> reads. (I think this is a significant win.)\n> \n> Makes some sense, though I don't recall whether it was ever measured.\n\nI think I measured it once and found it a significant benefit, though I\ncan't remember whether this was with a warm cache or only with a cold cache.\n\nIn fact, I experimented with some other strategies for loose reference\nloading for performance reasons. Currently loose references are read\ninto the cache lazily, one directory at a time. I experimented with\nchanges in both directions:\n\n* Preloading the whole tree of loose references before starting an\niteration. As I recall, this was a performance *win*. It was on my\nto-do list of things to pursue when I have some free time (ha, ha). I\nmostly wanted to check first that there are not callers who abort the\niteration soon after starting it. For example, imagine a caller who\ntries to determine \"are there any tags at all\" by iterating over\n\"refs/tags\" with a callback that just returns 1; such a caller would\nsuffer the cost of reading all of the loose references in \"refs/tags\".\n\n* Lazy loading loose references one reference at a time. The ideas was\nthat this would allow the reference cache to be used for\nsingle-reference lookups. This change alone caused a significant\nperformance loss, so it would have had to be combined with code for\npreloading directories or subtrees before a for_each_ref() iteration.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"216589","messageId":"20130507044456.GA29757@sigill.intra.peff.net","threadId":"33713","inReplyTo":"518883CC.7050609@alum.mit.edu","subject":"Re: another packed-refs race","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-05-07T04:44:56Z","receivedAt":"2013-05-07T04:44:56Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, May 07, 2013 at 06:32:12AM +0200, Michael Haggerty wrote:\n\n> Another potential problem caused by the non-atomicity of loose reference\n> reading could be to confuse reachability tests if process 1 is reading\n> loose references while process 2 is renaming a reference:\n> \n> 1. Process 1 looks for refs/heads/aaaaa and finds it missing.\n> \n> 2. Process 2 renames zzzzz -> aaaaa\n> \n> 3. Process 1 looks for refs/heads/zzzzz and finds it missing.\n> \n> Process 2 would think that any objects pointed to by aaaaa (formerly\n> zzzzz) are unreachable. This would be unfortunate if it is doing an\n> upload-pack and very bad if it is doing a gc. I wonder if this could be\n> a problem in practice? (Gee, wouldn't it be nice to keep reflogs for\n> deleted refs? :-) )\n\nUgh. Yeah, that is definitely a possible race, and it could be quite\ndisastrous for prune.\n\nI am really starting to think that we need a pluggable refs\narchitecture, and that busy servers can use something like sqlite to\nkeep the ref storage. That would require bumping repositoryformatversion,\nof course, but it would be OK for a server accessible only by git\nprotocols.\n\nI also wonder if we can spend extra time to get more reliable results\nfor prune, like checking refs, coming up with a prune list, and then\nchecking again. I have a feeling it's a 2-generals sort of problem where\nwe can always miss a ref that keeps bouncing around, but we could bound\nthe probability. I haven't thought that hard about it. Perhaps this will\ngive us something to talk about on Thursday. :)\n\n> > My proposal above gets rid of the need to invalidate the loose refs\n> > cache, so we can ignore that complexity.\n> \n> The same concern applies to invalidating the packed-ref cache, which you\n> still want to do.\n\nTrue. In theory a call to resolve_ref can invalidate it, so any call\nfrom inside a for_each_ref callback would be suspect.\n\n> * Preloading the whole tree of loose references before starting an\n> iteration. As I recall, this was a performance *win*. It was on my\n> to-do list of things to pursue when I have some free time (ha, ha). I\n> mostly wanted to check first that there are not callers who abort the\n> iteration soon after starting it. For example, imagine a caller who\n> tries to determine \"are there any tags at all\" by iterating over\n> \"refs/tags\" with a callback that just returns 1; such a caller would\n> suffer the cost of reading all of the loose references in \"refs/tags\".\n\nWell, you can measure my patches, because that's what they do. :) I\ndidn't really consider an early termination from the iteration.\nCertainly something like:\n\n git for-each-ref refs/tags | head -1\n\nwould take longer. Though if you have that many refs that the latency is\na big problem, you should probably consider packing them (it can't\npossibly bite you with a race condition, right?).\n\n-Peff\n"},{"id":"216600","messageId":"7vobcnmgwg.fsf@alter.siamese.dyndns.org","threadId":"33713","inReplyTo":"20130507023610.GA22053@sigill.intra.peff.net","subject":"Re: [PATCH 0/4] fix packed-refs races","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-05-07T06:40:47Z","receivedAt":"2013-05-07T06:40:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thanks.\n\nI queued this, and will push the result out on the side of 'pu'\ncloser to 'next'. Could you double check the conflict resolution?\n\nI haven't got around to the peel-ref-cleanup yet.\n"},{"id":"216609","messageId":"5188B53A.3090106@alum.mit.edu","threadId":"33713","inReplyTo":"20130507044456.GA29757@sigill.intra.peff.net","subject":"Re: another packed-refs race","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2013-05-07T08:03:06Z","receivedAt":"2013-05-07T08:03:06Z","isPatch":false,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 05/07/2013 06:44 AM, Jeff King wrote:\n> On Tue, May 07, 2013 at 06:32:12AM +0200, Michael Haggerty wrote:\n> \n>> Another potential problem caused by the non-atomicity of loose reference\n>> reading could be to confuse reachability tests if process 1 is reading\n>> loose references while process 2 is renaming a reference:\n>>\n>> 1. Process 1 looks for refs/heads/aaaaa and finds it missing.\n>>\n>> 2. Process 2 renames zzzzz -> aaaaa\n>>\n>> 3. Process 1 looks for refs/heads/zzzzz and finds it missing.\n>>\n>> Process 2 would think that any objects pointed to by aaaaa (formerly\n>> zzzzz) are unreachable. This would be unfortunate if it is doing an\n>> upload-pack and very bad if it is doing a gc. I wonder if this could be\n>> a problem in practice? (Gee, wouldn't it be nice to keep reflogs for\n>> deleted refs? :-) )\n> \n> Ugh. Yeah, that is definitely a possible race, and it could be quite\n> disastrous for prune.\n> \n> I am really starting to think that we need a pluggable refs\n> architecture, and that busy servers can use something like sqlite to\n> keep the ref storage. That would require bumping repositoryformatversion,\n> of course, but it would be OK for a server accessible only by git\n> protocols.\n\nThat would be a fun project. I like the idea of not burdening people's\nlocal mostly-one-user-at-a-time repositories with code that is hardened\nagainst server-level pounding.\n\n> I also wonder if we can spend extra time to get more reliable results\n> for prune, like checking refs, coming up with a prune list, and then\n> checking again. I have a feeling it's a 2-generals sort of problem where\n> we can always miss a ref that keeps bouncing around, but we could bound\n> the probability. I haven't thought that hard about it. Perhaps this will\n> give us something to talk about on Thursday. :)\n\nIt's not 100% solvable without big changes; there could always be a\nmalign Dijkstra running around your system, renaming references right\nbefore you read them. But I guess it would be pretty safe if pack would\nkeep the union of objects reachable from the references read at the\nbeginning of the run and objects reachable from the references read at\n(aka near) the end of the run.\n\n>> * Preloading the whole tree of loose references before starting an\n>> iteration. As I recall, this was a performance *win*. It was on my\n>> to-do list of things to pursue when I have some free time (ha, ha). I\n>> mostly wanted to check first that there are not callers who abort the\n>> iteration soon after starting it. For example, imagine a caller who\n>> tries to determine \"are there any tags at all\" by iterating over\n>> \"refs/tags\" with a callback that just returns 1; such a caller would\n>> suffer the cost of reading all of the loose references in \"refs/tags\".\n> \n> Well, you can measure my patches, because that's what they do. :) I\n> didn't really consider an early termination from the iteration.\n> Certainly something like:\n> \n> git for-each-ref refs/tags | head -1\n> \n> would take longer. Though if you have that many refs that the latency is\n> a big problem, you should probably consider packing them (it can't\n> possibly bite you with a race condition, right?).\n\nNo, I don't see a correctness issue.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"216611","messageId":"20130507141942.GA3658@sigill.intra.peff.net","threadId":"33713","inReplyTo":"7vobcnmgwg.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 0/4] fix packed-refs races","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-05-07T14:19:42Z","receivedAt":"2013-05-07T14:19:42Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, May 06, 2013 at 11:40:47PM -0700, Junio C Hamano wrote:\n\n> I queued this, and will push the result out on the side of 'pu'\n> closer to 'next'. Could you double check the conflict resolution?\n\nI eyeballed the \"--cc\" output, as well as the resulting file, and it\nlooks fine to me.\n\n> I haven't got around to the peel-ref-cleanup yet.\n\nI'd leave that for now; the current code does have a bug, but it's not\ntriggerable through any of the current callers, so we can afford to take\nour time. I'll try to work up a patch that just goes on top of Michael's\nseries.\n\n-Peff\n"},{"id":"216808","messageId":"CAPig+cSOryztWJBXJErQv-i7L863vOmtUBtm0_u-OcNZxQ5Q7w@mail.gmail.com","threadId":"33713","inReplyTo":"20130507024313.GC22940@sigill.intra.peff.net","subject":"Re: [PATCH 3/4] get_packed_refs: reload packed-refs file when it changes","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2013-05-09T19:18:51Z","receivedAt":"2013-05-09T19:18:51Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, May 6, 2013 at 10:43 PM, Jeff King wrote:\n> Once we read the packed-refs file into memory, we cache it\n> to save work on future ref lookups. However, our cache may\n> be out of date with respect to what is on disk if another\n> process is simultaneously packing the refs. Normally it\n> is acceptable for us to be a little out of date, since there\n> is no guarantee whether we read the file before or after the\n> simultaneous update. However, there is an important special\n> case: our packed-refs file must be up to date with respect\n> to any loose refs we read. Otherwise, we risk the following\n> race 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>\n> There are many variants (e.g., step 1 may involve process A\n> looking up another ref entirely, so even a fully qualified\n> refname can fail). One of the most interesting ones is if\n> \"refs/heads/master\" is already packed. In that case process\n> A will not see it as missing, but rather will report\n> whatever value happened to be in the packed-refs file before\n> process B repacked (which might be an arbitrarily old\n> value).\n>\n> We can fix this by making sure we reload the packed-refs\n> file from disk after looking at any loose refs. That's\n> unacceptably slow, so we can check it's stat()-validity as a\n\ns/it's/its/\n\n> proxy, and read it only when it appears to have changed.\n>\n> Reading the packed-refs file after performing any loose-ref\n> system calls is sufficient because we know the ordering of\n> the pack-refs process: it always makes sure the newly\n> written packed-refs file is installed into place before\n> pruning any loose refs. As long as those operations by B\n> appear in their executed order to process A, by the time A\n> sees the missing loose ref, the new packed-refs file must be\n> in place.\n>\n> Signed-off-by: Jeff King \n"},{"id":"217100","messageId":"51901E05.1080905@alum.mit.edu","threadId":"33713","inReplyTo":"20130507023802.GA22940@sigill.intra.peff.net","subject":"Re: [PATCH 1/4] resolve_ref: close race condition for packed refs","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2013-05-12T22:56:05Z","receivedAt":"2013-05-12T22:56:05Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 05/07/2013 04:38 AM, Jeff King wrote:\n> When we attempt to resolve a ref, the first thing we do is\n> call lstat() to see if it is a symlink or a real ref. If we\n> find that the ref is missing, we fall back to looking it up\n> in the packed-refs file. If we find the loose ref does exist\n> (and is a regular file), we continue with trying to open it.\n> \n> However, we do not do the same fallback if our open() call\n> fails; we just report the ref as missing. A \"git pack-refs\n> --prune\" process which is simultaneously running may remove\n> the loose ref between our lstat() and open(). In this case,\n> we would erroneously report the ref as missing, even though\n> we could find it if we checked the packed-refs file.\n> \n> This patch solves it by factoring out the fallback code from\n> the lstat() case and calling it from both places. We do not\n> need to do the same thing for the symlink/readlink code\n> path, even though it might receive ENOENT, too, because\n> symlinks cannot be packed (so if it disappears after the\n> lstat, it is truly being deleted).\n> \n> Note that this solves only the part of the race within\n> resolve_ref_unsafe. In the situation described above, we may\n> still be depending on a cached view of the packed-refs file;\n> that race will be dealt with in a future patch.\n> \n> Signed-off-by: Jeff King \n\n+1, with trivial suggestions below.\n\n> ---\n> \n> refs.c | 63 ++++++++++++++++++++++++++++++++++++++++++---------------------\n> 1 file changed, 42 insertions(+), 21 deletions(-)\n> \n> diff --git a/refs.c b/refs.c\n> index de2d8eb..5a14703 100644\n> --- a/refs.c\n> +++ b/refs.c\n> @@ -1083,6 +1083,43 @@ static int get_packed_ref(const char *refname, unsigned char *sha1)\n> \treturn -1;\n> }\n> \n> +/*\n> + * This should be called from resolve_ref_unsafe when a loose ref cannot be\n> + * accessed; err must represent the errno from the last attempt to access the\n> + * loose ref, and the other options are forwarded from resolve_safe_unsaefe.\n\ns/resolve_ref_unsaefe/resolve_ref_unsafe/\n\n> + */\n> +static const char *handle_loose_ref_failure(int err,\n> +\t\t\t\t\t const char *refname,\n> +\t\t\t\t\t unsigned char *sha1,\n> +\t\t\t\t\t int reading,\n> +\t\t\t\t\t int *flag)\n> +{\n> +\t/*\n> +\t * If we didn't get ENOENT, something is broken\n> +\t * with the loose ref, and we should not fallback,\n> +\t * but instead pretend it doesn't exist.\n> +\t */\n> +\tif (err != ENOENT)\n> +\t\treturn NULL;\n> +\n> +\t/*\n> +\t * The loose reference file does not exist;\n> +\t * check for a packed reference.\n> +\t */\n> +\tif (!get_packed_ref(refname, sha1)) {\n> +\t\tif (flag)\n> +\t\t\t*flag |= REF_ISPACKED;\n> +\t\treturn refname;\n> +\t}\n> +\n> +\t/* The reference is not a packed reference, either. */\n> +\tif (reading)\n> +\t\treturn NULL;\n> +\n> +\thashclr(sha1);\n> +\treturn refname;\n> +}\n> +\n> const char *resolve_ref_unsafe(const char *refname, unsigned char *sha1, int reading, int *flag)\n> {\n> \tint depth = MAXDEPTH;\n> @@ -1107,26 +1144,9 @@ const char *resolve_ref_unsafe(const char *refname, unsigned char *sha1, int rea\n> \n> \t\tgit_snpath(path, sizeof(path), \"%s\", refname);\n> \n> -\t\tif (lstat(path, &st) < 0) {\n> -\t\t\tif (errno != ENOENT)\n> -\t\t\t\treturn NULL;\n> -\t\t\t/*\n> -\t\t\t * The loose reference file does not exist;\n> -\t\t\t * check for a packed reference.\n> -\t\t\t */\n> -\t\t\tif (!get_packed_ref(refname, sha1)) {\n> -\t\t\t\tif (flag)\n> -\t\t\t\t\t*flag |= REF_ISPACKED;\n> -\t\t\t\treturn refname;\n> -\t\t\t}\n> -\t\t\t/* The reference is not a packed reference, either. */\n> -\t\t\tif (reading) {\n> -\t\t\t\treturn NULL;\n> -\t\t\t} else {\n> -\t\t\t\thashclr(sha1);\n> -\t\t\t\treturn refname;\n> -\t\t\t}\n> -\t\t}\n> +\t\tif (lstat(path, &st) < 0)\n> +\t\t\treturn handle_loose_ref_failure(errno, refname, sha1,\n> +\t\t\t\t\t\t\treading, flag);\n> \n> \t\t/* Follow \"normalized\" - ie \"refs/..\" symlinks by hand */\n> \t\tif (S_ISLNK(st.st_mode)) {\n> @@ -1156,7 +1176,8 @@ const char *resolve_ref_unsafe(const char *refname, unsigned char *sha1, int rea\n> \t\t */\n> \t\tfd = open(path, O_RDONLY);\n> \t\tif (fd < 0)\n> -\t\t\treturn NULL;\n> +\t\t\treturn handle_loose_ref_failure(errno, refname, sha1,\n> +\t\t\t\t\t\t\treading, flag);\n\nI probably would have separated the rest of the patch, which is a pure\nrefactoring, from this last chunk, which is a functional change. But\nthat's just me.\n\nI suggest adding a comment here mentioning briefly the race condition\nthat the call to handle_loose_ref_failure() is meant to address;\notherwise somebody not thinking of race conditions might have the clever\nidea of \"inlining\" the call to \"return NULL\" because it seems redundant\nwith the check of ENOENT following the lstat() call above.\n\n> \t\tlen = read_in_full(fd, buffer, sizeof(buffer)-1);\n> \t\tclose(fd);\n> \t\tif (len < 0)\n> \n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"217101","messageId":"51902515.7010500@alum.mit.edu","threadId":"33713","inReplyTo":"20130507023802.GA22940@sigill.intra.peff.net","subject":"Re: [PATCH 1/4] resolve_ref: close race condition for packed refs","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2013-05-12T23:26:13Z","receivedAt":"2013-05-12T23:26:13Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 05/07/2013 04:38 AM, Jeff King wrote:\n> [...]\n> This patch solves it by factoring out the fallback code from\n> the lstat() case and calling it from both places. We do not\n> need to do the same thing for the symlink/readlink code\n> path, even though it might receive ENOENT, too, because\n> symlinks cannot be packed (so if it disappears after the\n> lstat, it is truly being deleted).\n\nTo be really pedantic, we should handle all kinds of changes that\nanother process might make simultaneously:\n\n* symlink -> deleted\n\n Was already OK, as you explained above.\n\n* regular file -> deleted\n\n Handled by your patch\n\n* symlink -> regular file\n\n Could come from\n\n update-ref --no-deref\n\n if the old version of the reference were a symlink-based symbolic\n reference. Oops, wait, that's broken anyway:\n\n $ ln -sf master .git/refs/heads/foo\n $ git for-each-ref\n 4204906e2ee75f9b7224860c40df712d112c004b commit\trefs/heads/foo\n 4204906e2ee75f9b7224860c40df712d112c004b commit\trefs/heads/master\n $ git update-ref --no-deref refs/heads/foo master\n $ dir .git/refs/heads/foo\n lrwxrwxrwx 1 mhagger mhagger 6 May 13 01:07 .git/refs/heads/foo ->\nmaster\n\n But supposing that were fixed and it occurred between the call to\n lstat() and the call to readlink(), then readlink() would fail with\n errno == EINVAL and we should respond by repeating the code starting\n with the lstat().\n\n* regular file -> symlink\n\n Could come from overwriting a reference with a symlink-based symbolic\n reference. But this could only happen if the parallel process were\n an old version of Git\n\nI think it's been quite a while since Git has used symlinks for symbolic\nreferences, so I doubt that it is worth fixing the second two cases.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"217104","messageId":"5190500E.8060907@alum.mit.edu","threadId":"33713","inReplyTo":"20130507023942.GB22940@sigill.intra.peff.net","subject":"Re: [PATCH 2/4] add a stat_validity struct","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2013-05-13T02:29:34Z","receivedAt":"2013-05-13T02:29:34Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 05/07/2013 04:39 AM, Jeff King wrote:\n> It can sometimes be useful to know whether a path in the\n> filesystem has been updated without going to the work of\n> opening and re-reading its content. We trust the stat()\n> information on disk already to handle index updates, and we\n> can use the same trick here.\n> \n> This patch introduces a \"stat_validity\" struct which\n> encapsulates the concept of checking the stat-freshness of a\n> file. It is implemented on top of \"struct cache_entry\" to\n> reuse the logic about which stat entries to trust for a\n> particular platform, but hides the complexity behind two\n> simple functions: check and update.\n\nGiven that \"struct cache_entry\" holds a lot of information that is\nirrelevant to stat-freshness and that ce_match_stat_basic() has a lot of\nlogic that is geared towards cache_entries, it seems like there should\nbe some kind of \"struct stat_data\" that holds the portion of the stat\ninformation that we use for checking whether a file might have been\nchanged between two accesses. cache_entry would contain a stat_data as\na member, and the functionality for checking that part of a cache_entry\nvalidity would be moved to a separate function.\n\nThen your \"struct validity_info\" could hold a pointer to a stat_data\nrather than to a cache_entry, and it wouldn't have to contain the\nunrelated cache_entry stuff.\n\nI'll send patches shortly to show what I mean.\n\nMichael\n\n> Signed-off-by: Jeff King \n> ---\n> This one is prep for the next patch. I'm not super-happy with the way it\n> builds around cache_entry, just because cache entries may end up\n> following different rules in the long run. But I at least tried to\n> encapsulate the grossness, so if it turns out to be a problem, we can\n> factor out the relevant bits from ce_match_stat_basic into a shared\n> function.\n> \n> cache.h | 27 +++++++++++++++++++++++++++\n> read-cache.c | 31 +++++++++++++++++++++++++++++++\n> 2 files changed, 58 insertions(+)\n> \n> diff --git a/cache.h b/cache.h\n> index 94ca1ac..adf2874 100644\n> --- a/cache.h\n> +++ b/cache.h\n> @@ -1326,4 +1326,31 @@ int sane_execvp(const char *file, char *const argv[]);\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 is a simplified version of the up-to-date\n> + * checks we use for the index. The implementation is built on an index entry,\n> + * but we shield the callers from that ugliness with our struct.\n> + */\n> +struct stat_validity {\n> +\tstruct cache_entry *ce;\n> +};\n> +\n> +void stat_validity_clear(struct stat_validity *sv);\n> +\n> +/*\n> + * Returns 1 if the path matches the saved stat_validity, 0 otherwise.\n> + * A missing 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 the file\n> + * is missing or inaccessible, the validity will reflect that, and future\n> + * calls to stat_validity_check will match only if it continues to be\n> + * inaccessible).\n> + */\n> +void stat_validity_update(struct stat_validity *sv, int fd);\n> +\n> #endif /* CACHE_H */\n> diff --git a/read-cache.c b/read-cache.c\n> index 04ed561..a0bd06c 100644\n> --- a/read-cache.c\n> +++ b/read-cache.c\n> @@ -1933,3 +1933,34 @@ 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->ce);\n> +\tsv->ce = 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->ce == NULL;\n> +\tif (!sv->ce)\n> +\t\treturn 0;\n> +\treturn !ce_match_stat_basic(sv->ce, &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)\n> +\t\tstat_validity_clear(sv);\n> +\telse {\n> +\t\tif (!sv->ce)\n> +\t\t\tsv->ce = xcalloc(1, cache_entry_size(0));\n> +\t\tfill_stat_cache_info(sv->ce, &st);\n> +\t\tsv->ce->ce_mode = create_ce_mode(st.st_mode);\n> +\t}\n> +}\n> \n\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"217106","messageId":"5190536A.5090606@alum.mit.edu","threadId":"33713","inReplyTo":"20130507024313.GC22940@sigill.intra.peff.net","subject":"Re: [PATCH 3/4] get_packed_refs: reload packed-refs file when it changes","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2013-05-13T02:43:54Z","receivedAt":"2013-05-13T02:43:54Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 05/07/2013 04:43 AM, Jeff King wrote:\n> Once we read the packed-refs file into memory, we cache it\n> to save work on future ref lookups. However, our cache may\n> be out of date with respect to what is on disk if another\n> process is simultaneously packing the refs. Normally it\n> is acceptable for us to be a little out of date, since there\n> is no guarantee whether we read the file before or after the\n> simultaneous update. However, there is an important special\n> case: our packed-refs file must be up to date with respect\n> to any loose refs we read. Otherwise, we risk the following\n> race 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> \n> There are many variants (e.g., step 1 may involve process A\n> looking up another ref entirely, so even a fully qualified\n> refname can fail). One of the most interesting ones is if\n> \"refs/heads/master\" is already packed. In that case process\n> A will not see it as missing, but rather will report\n> whatever value happened to be in the packed-refs file before\n> process B repacked (which might be an arbitrarily old\n> value).\n> \n> We can fix this by making sure we reload the packed-refs\n> file from disk after looking at any loose refs. That's\n> unacceptably slow, so we can check it's stat()-validity as a\n> proxy, and read it only when it appears to have changed.\n> \n> Reading the packed-refs file after performing any loose-ref\n> system calls is sufficient because we know the ordering of\n> the pack-refs process: it always makes sure the newly\n> written packed-refs file is installed into place before\n> pruning any loose refs. As long as those operations by B\n> appear in their executed order to process A, by the time A\n> sees the missing loose ref, the new packed-refs file must be\n> in place.\n> \n> Signed-off-by: Jeff King \n> ---\n> I hooked the refreshing into get_packed_refs, since then all callers get\n> it for free. It makes me a little nervous, though, just in case some\n> caller really cares about calling get_packed_refs but not having the\n> list of packed-refs change during the call. peel_ref looks like such a\n> function, but isn't, for reasons I'll explain in a followup patch.\n> \n> Clone also looks like such a caller, as it calls get_packed_refs once\n> for each upstream ref it adds (it puts them in the packed-refs list, and\n> then writes them all out at the end). But it's OK because there is no\n> packed-refs file for it to refresh from.\n> \n> An alternative would be to move the refreshing to an explicit\n> refresh_packed_refs() function, and call it from a few places\n> (resolve_ref, and later from do_for_each_ref).\n\nI think this will be necessary, because otherwise there are too many\nplaces where the packed-refs cache can be invalidated and re-read.\n\nAs a test, I changed your stat_validity_check() to return 0 *all* of the\ntime when a file exists. I think this simulates a hyperactive\nrepository in which the packed-refs file changes every time it is\nchecked. With this change, hundreds of tests in the test suite fail.\n\nI haven't had time to dig into the failures. One example is\n\ngit-upload-pack: refs.c:542: do_for_each_ref_in_dir: Assertion\n`dir->sorted == dir->nr' failed.\n\nThis suggests that the packed ref cache was invalidated while somebody\nwas iterating over it, resulting perhaps in illegal memory references.\n\nMaybe my test is misguided. But I definitely would not proceed on this\npatch until the situation has been understood better.\n\nMichael\n\n> refs.c | 20 +++++++++++++++-----\n> 1 file changed, 15 insertions(+), 5 deletions(-)\n> \n> diff --git a/refs.c b/refs.c\n> index 5a14703..6afe8cc 100644\n> --- a/refs.c\n> +++ b/refs.c\n> @@ -708,6 +708,7 @@ static struct ref_cache {\n> \tstruct ref_cache *next;\n> \tstruct ref_entry *loose;\n> \tstruct ref_entry *packed;\n> +\tstruct stat_validity packed_validity;\n> \t/* The submodule name, or \"\" for the main repo. */\n> \tchar name[FLEX_ARRAY];\n> } *ref_cache;\n> @@ -717,6 +718,7 @@ static void clear_packed_ref_cache(struct ref_cache *refs)\n> \tif (refs->packed) {\n> \t\tfree_ref_entry(refs->packed);\n> \t\trefs->packed = NULL;\n> +\t\tstat_validity_clear(&refs->packed_validity);\n> \t}\n> }\n> \n> @@ -878,17 +880,25 @@ static struct ref_dir *get_packed_refs(struct ref_cache *refs)\n> \n> static struct ref_dir *get_packed_refs(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 = 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> +\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));\n> \t\t\tfclose(f);\n> \t\t}\n> \n\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"217108","messageId":"1368414007-3819-1-git-send-email-mhagger@alum.mit.edu","threadId":"33713","inReplyTo":"5190500E.8060907@alum.mit.edu","subject":"[RFC 0/2] Separate stat_data from cache_entry","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2013-05-13T03:00:05Z","receivedAt":"2013-05-13T03:00:05Z","isPatch":false,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"This series is a possible replacement for Peff's\n\n \"[PATCH 2/4] add a stat_validity struct\"\n\nthat doesn't misuse the cache_entry too much in the implementation of\nthe new functions. It is only interesting in the context of Peff's\nseries (or something like it), or if there are other places where\nsomebody wants to be able to tell whether a file has changed since\nit was last checked.\n\nMichael\n\nMichael Haggerty (2):\n Extract a struct stat_data from cache_entry\n add a stat_validity struct\n\n builtin/ls-files.c | 12 ++--\n cache.h | 60 +++++++++++++++--\n read-cache.c | 191 ++++++++++++++++++++++++++++++++++-------------------\n 3 files changed, 183 insertions(+), 80 deletions(-)\n\n-- \n1.8.2.2\n"},{"id":"217109","messageId":"1368414007-3819-2-git-send-email-mhagger@alum.mit.edu","threadId":"33713","inReplyTo":"1368414007-3819-1-git-send-email-mhagger@alum.mit.edu","subject":"[RFC 1/2] Extract a struct stat_data from cache_entry","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2013-05-13T03:00:06Z","receivedAt":"2013-05-13T03:00:06Z","isPatch":false,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"This structure will later be used to check the validity of other types\nof file.\n\nSigned-off-by: Michael Haggerty \n---\nFeel free to suggest better names. I tried to follow the convention\nof names used nearby.\n\n builtin/ls-files.c | 12 ++--\n cache.h | 33 ++++++++---\n read-cache.c | 161 +++++++++++++++++++++++++++++++----------------------\n 3 files changed, 126 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 94ca1ac..55b4b14 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..9c1e089 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -68,21 +68,78 @@ void rename_index_entry_at(struct index_state *istate, int nr, const char *new_n\n }\n \n /*\n+ * Record to sd the data from st that we use to check whether a file\n+ * might have changed.\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+/*\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+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 * to validate the cache.\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 +252,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 +270,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 +365,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 +1347,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 +1633,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 +1672,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 +1682,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.2.2\n"},{"id":"217110","messageId":"1368414007-3819-3-git-send-email-mhagger@alum.mit.edu","threadId":"33713","inReplyTo":"1368414007-3819-1-git-send-email-mhagger@alum.mit.edu","subject":"[RFC 2/2] add a stat_validity struct","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2013-05-13T03:00:07Z","receivedAt":"2013-05-13T03:00:07Z","isPatch":false,"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 essentially the same as Peff's patch\n\n \"[PATCH 2/4] add a stat_validity struct\"\n\nexcept that it uses the \"struct stat_data\" from the previous commit\nrather than grabbing the fields directly out of cache_entry.\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 55b4b14..71385dc 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1345,4 +1345,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 9c1e089..9dd157f 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -1958,3 +1958,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.2.2\n"},{"id":"217119","messageId":"7vli7jqxca.fsf@alter.siamese.dyndns.org","threadId":"33713","inReplyTo":"1368414007-3819-1-git-send-email-mhagger@alum.mit.edu","subject":"Re: [RFC 0/2] Separate stat_data from cache_entry","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-05-13T05:10:13Z","receivedAt":"2013-05-13T05:10:13Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n> Michael Haggerty (2):\n> Extract a struct stat_data from cache_entry\n> add a stat_validity struct\n\nSome indentation glitches carried over from the original in that\n\"from-ondisk\" function were distracting while reading it through,\nbut I think these show us a reasonable direction to go, if we want\nto have \"We have read that file and already know what it says. Do\nwe have to read it again?\" check for files that are not tracked\nworking tree files.\n"},{"id":"217504","messageId":"20130516034721.GD13296@sigill.intra.peff.net","threadId":"33713","inReplyTo":"51901E05.1080905@alum.mit.edu","subject":"Re: [PATCH 1/4] resolve_ref: close race condition for packed refs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-05-16T03:47:21Z","receivedAt":"2013-05-16T03:47:21Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, May 13, 2013 at 12:56:05AM +0200, Michael Haggerty wrote:\n\n> > + * This should be called from resolve_ref_unsafe when a loose ref cannot be\n> > + * accessed; err must represent the errno from the last attempt to access the\n> > + * loose ref, and the other options are forwarded from resolve_safe_unsaefe.\n> \n> s/resolve_ref_unsaefe/resolve_ref_unsafe/\n\nOops, thanks.\n\n> > -\t\t\treturn NULL;\n> > +\t\t\treturn handle_loose_ref_failure(errno, refname, sha1,\n> > +\t\t\t\t\t\t\treading, flag);\n> \n> I probably would have separated the rest of the patch, which is a pure\n> refactoring, from this last chunk, which is a functional change. But\n> that's just me.\n\nYeah, I go back and forth on whether it is better to have strict\nrefactors followed by changes or not. Sometimes it is hard to understand\nthe motivation for the refactor without seeing the change, and you end\nup explaining it twice.\n\nMy usual rule of thumb is:\n\n - If you are factoring out some code, and then are going to change\n that code, make it two separate changes. That keeps the diffs\n readable (the first one is pure movement and you do not need to look\n closely, and the second shows a sane diff of the change).\n\n - If you are factoring out some code, and then adding more callers,\n it's OK to do it together. The new caller provides the motivation\n for the refactor.\n\nThis is the latter case. But I'm open to arguments that the rule is not\na good one. :)\n\n> I suggest adding a comment here mentioning briefly the race condition\n> that the call to handle_loose_ref_failure() is meant to address;\n> otherwise somebody not thinking of race conditions might have the clever\n> idea of \"inlining\" the call to \"return NULL\" because it seems redundant\n> with the check of ENOENT following the lstat() call above.\n\nYeah, I thought I had mentioned that at the top of\nhandle_loose_ref_failure, but I see that I didn't. Probably something\nlike this squashed on top makes sense:\n\ndiff --git a/refs.c b/refs.c\nindex c127baf..1a7e4ef 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1111,7 +1111,7 @@ static int get_packed_ref(const char *refname, unsigned char *sha1)\n /*\n * This should be called from resolve_ref_unsafe when a loose ref cannot be\n * accessed; err must represent the errno from the last attempt to access the\n- * loose ref, and the other options are forwarded from resolve_safe_unsaefe.\n+ * loose ref, and the other options are forwarded from resolve_safe_unsafe.\n */\n static const char *handle_loose_ref_failure(int err,\n \t\t\t\t\t const char *refname,\n@@ -1200,9 +1200,16 @@ const char *resolve_ref_unsafe(const char *refname, unsigned char *sha1, int rea\n \t\t * a ref\n \t\t */\n \t\tfd = open(path, O_RDONLY);\n-\t\tif (fd < 0)\n+\t\tif (fd < 0) {\n+\t\t\t/*\n+\t\t\t * We need to check again here for ENOENT and fall back\n+\t\t\t * to the packed-refs file to avoid a race condition in\n+\t\t\t * which the ref is packed and pruned between our\n+\t\t\t * lstat() and open() calls.\n+\t\t\t */\n \t\t\treturn handle_loose_ref_failure(errno, refname, sha1,\n \t\t\t\t\t\t\treading, flag);\n+\t\t}\n \t\tlen = read_in_full(fd, buffer, sizeof(buffer)-1);\n \t\tclose(fd);\n \t\tif (len < 0)\n"},{"id":"217505","messageId":"20130516035158.GE13296@sigill.intra.peff.net","threadId":"33713","inReplyTo":"5190500E.8060907@alum.mit.edu","subject":"Re: [PATCH 2/4] add a stat_validity struct","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-05-16T03:51:58Z","receivedAt":"2013-05-16T03:51:58Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, May 13, 2013 at 04:29:34AM +0200, Michael Haggerty wrote:\n\n> > This patch introduces a \"stat_validity\" struct which\n> > encapsulates the concept of checking the stat-freshness of a\n> > file. It is implemented on top of \"struct cache_entry\" to\n> > reuse the logic about which stat entries to trust for a\n> > particular platform, but hides the complexity behind two\n> > simple functions: check and update.\n> \n> Given that \"struct cache_entry\" holds a lot of information that is\n> irrelevant to stat-freshness and that ce_match_stat_basic() has a lot of\n> logic that is geared towards cache_entries, it seems like there should\n> be some kind of \"struct stat_data\" that holds the portion of the stat\n> information that we use for checking whether a file might have been\n> changed between two accesses. cache_entry would contain a stat_data as\n> a member, and the functionality for checking that part of a cache_entry\n> validity would be moved to a separate function.\n> \n> Then your \"struct validity_info\" could hold a pointer to a stat_data\n> rather than to a cache_entry, and it wouldn't have to contain the\n> unrelated cache_entry stuff.\n\nYes, I had the exact same thought, and came up with the exact same\nsolution. I was just hesitant to do it because it meant touching all of\nthe cache_entry users to adjust their struct accesses.\n\nBut from your patches, it doesn't look too bad (and we can be sure we\nupdated every spot, because it's easy for the compiler to enforce).\n\nI'm sorry to be so slow lately; I'm on vacation (and will be for a bit\nyet). I'm happy to rebuild my series on top of yours, and it looks like\nit is still only in pu (I haven't yet read \"What's Cooking\" to see the\nplans for it). I don't think there's a rush, as it is post-1.8.3 anyway\n(I _do_ think it's a serious bug, in that it can cause data loss, but in\npractice I doubt most people would hit it).\n\n-Peff\n"},{"id":"217508","messageId":"519473BA.9030106@alum.mit.edu","threadId":"33713","inReplyTo":"20130516034721.GD13296@sigill.intra.peff.net","subject":"Re: [PATCH 1/4] resolve_ref: close race condition for packed refs","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2013-05-16T05:50:50Z","receivedAt":"2013-05-16T05:50:50Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 05/16/2013 05:47 AM, Jeff King wrote:\n>> I probably would have separated the rest of the patch, which is a pure\n>> refactoring, from this last chunk, which is a functional change. But\n>> that's just me.\n> \n> Yeah, I go back and forth on whether it is better to have strict\n> refactors followed by changes or not. Sometimes it is hard to understand\n> the motivation for the refactor without seeing the change, and you end\n> up explaining it twice.\n\nA pure refactoring doesn't need much justification. Something like\n\"extract function foo()\" plus maybe \"this function will soon have\nmultiple callers\" is IMO usually adequate, especially if the function is\nwell-named and documented in the patch itself.\n\n> My usual rule of thumb is:\n> \n> - If you are factoring out some code, and then are going to change\n> that code, make it two separate changes. That keeps the diffs\n> readable (the first one is pure movement and you do not need to look\n> closely, and the second shows a sane diff of the change).\n> \n> - If you are factoring out some code, and then adding more callers,\n> it's OK to do it together. The new caller provides the motivation\n> for the refactor.\n> \n> This is the latter case. But I'm open to arguments that the rule is not\n> a good one. :)\n\nYes, I see how keeping the changes together makes the justification of\nthe refactoring more obvious. On the other hand, splitting has the\nfollowing benefits:\n\n1. Reviewers have a single thing to check in each patch: \"Did he\n transcribe the code correctly into a function and choose a good\n API?\" vs. \"Does it make sense to call the function from this new\n place?\" The threads of feedback emails about each patch are\n similarly separated.\n\n On the other hand, of course these two changes are not completely\n independent, because having an idea what new callers want to do\n with the function affects what its API should be.\n\n2. If the patch series needs to be revised, it is quite possible that\n the revisions affect only one patch or the other. Therefore, the\n unaffected patch can be carried along through interactive rebases\n etc. intact, or might serve as a building block for an alternative\n solution.\n\n3. If there's a problem, bisect can figure out which half of the change\n was to blame.\n\nThat being said, this case is very much in the gray area where it is a\nmatter of personal preference and I don't mind at all if you leave it as\na single patch.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"220439","messageId":"1370960780-1055-1-git-send-email-mhagger@alum.mit.edu","threadId":"33713","inReplyTo":"20130507023802.GA22940@sigill.intra.peff.net","subject":"[PATCH 0/4] Fix a race condition when reading loose refs","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2013-06-11T14:26:16Z","receivedAt":"2013-06-11T14:26:16Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"This patch series is inspired by the first patch [1] in a series by\nPeff and solves the race condition that he discovered plus a second,\nsimilar race. The general problem is that loose ref files are read in\ntwo steps: lstat() followed by readlink() or open(). If another\nprocess changes the file between our two steps, then the old code gets\nconfused and can give incorrect results. The new code detects if an\nerror reported by readlink()/open() is inconsistent with the results\nof the previous lstat(), and if so, tries anew.\n\nThe first three commits are just preparatory. All four commits could\neasily be squashed together if it is preferred (especially commits 2\nthrough 4), but I did it this way to separate the\ncode-churning-but-trivial changes from the functional changes to make\nreview easier.\n\n[1] http://article.gmane.org/gmane.comp.version-control.git/223525\n\nMichael Haggerty (4):\n resolve_ref_unsafe(): extract function handle_missing_loose_ref()\n resolve_ref_unsafe(): handle the case of an SHA-1 within loop\n resolve_ref_unsafe(): nest reference-reading code in an infinite loop\n resolve_ref_unsafe(): close race condition reading loose refs\n\n refs.c | 187 ++++++++++++++++++++++++++++++++++++++++-------------------------\n 1 file changed, 114 insertions(+), 73 deletions(-)\n\n-- \n1.8.3\n"},{"id":"220441","messageId":"1370960780-1055-2-git-send-email-mhagger@alum.mit.edu","threadId":"33713","inReplyTo":"1370960780-1055-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 1/4] resolve_ref_unsafe(): extract function handle_missing_loose_ref()","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2013-06-11T14:26:17Z","receivedAt":"2013-06-11T14:26:17Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"The nesting was getting a bit out of hand, and it's about to get\nworse.\n\nSigned-off-by: Michael Haggerty \n---\n refs.c | 55 ++++++++++++++++++++++++++++++++++---------------------\n 1 file changed, 34 insertions(+), 21 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex d17931a..1acce17 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1193,6 +1193,37 @@ static struct ref_entry *get_packed_ref(const char *refname)\n \treturn find_ref(get_packed_refs(&ref_cache), refname);\n }\n \n+/*\n+ * A loose ref file doesn't exist; check for a packed ref. The\n+ * options are forwarded from resolve_safe_unsafe().\n+ */\n+static const char *handle_missing_loose_ref(const char *refname,\n+\t\t\t\t\t unsigned char *sha1,\n+\t\t\t\t\t int reading,\n+\t\t\t\t\t int *flag)\n+{\n+\tstruct ref_entry *entry;\n+\n+\t/*\n+\t * The loose reference file does not exist; check for a packed\n+\t * reference.\n+\t */\n+\tentry = get_packed_ref(refname);\n+\tif (entry) {\n+\t\thashcpy(sha1, entry->u.value.sha1);\n+\t\tif (flag)\n+\t\t\t*flag |= REF_ISPACKED;\n+\t\treturn refname;\n+\t}\n+\t/* The reference is not a packed reference, either. */\n+\tif (reading) {\n+\t\treturn NULL;\n+\t} else {\n+\t\thashclr(sha1);\n+\t\treturn refname;\n+\t}\n+}\n+\n const char *resolve_ref_unsafe(const char *refname, unsigned char *sha1, int reading, int *flag)\n {\n \tint depth = MAXDEPTH;\n@@ -1218,28 +1249,10 @@ const char *resolve_ref_unsafe(const char *refname, unsigned char *sha1, int rea\n \t\tgit_snpath(path, sizeof(path), \"%s\", refname);\n \n \t\tif (lstat(path, &st) < 0) {\n-\t\t\tstruct ref_entry *entry;\n-\n-\t\t\tif (errno != ENOENT)\n+\t\t\tif (errno == ENOENT)\n+\t\t\t\treturn handle_missing_loose_ref(refname, sha1, reading, flag);\n+\t\t\telse\n \t\t\t\treturn NULL;\n-\t\t\t/*\n-\t\t\t * The loose reference file does not exist;\n-\t\t\t * check for a packed reference.\n-\t\t\t */\n-\t\t\tentry = get_packed_ref(refname);\n-\t\t\tif (entry) {\n-\t\t\t\thashcpy(sha1, entry->u.value.sha1);\n-\t\t\t\tif (flag)\n-\t\t\t\t\t*flag |= REF_ISPACKED;\n-\t\t\t\treturn refname;\n-\t\t\t}\n-\t\t\t/* The reference is not a packed reference, either. */\n-\t\t\tif (reading) {\n-\t\t\t\treturn NULL;\n-\t\t\t} else {\n-\t\t\t\thashclr(sha1);\n-\t\t\t\treturn refname;\n-\t\t\t}\n \t\t}\n \n \t\t/* Follow \"normalized\" - ie \"refs/..\" symlinks by hand */\n-- \n1.8.3\n"},{"id":"220440","messageId":"1370960780-1055-3-git-send-email-mhagger@alum.mit.edu","threadId":"33713","inReplyTo":"1370960780-1055-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 2/4] resolve_ref_unsafe(): handle the case of an SHA-1 within loop","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2013-06-11T14:26:18Z","receivedAt":"2013-06-11T14:26:18Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"There is only one \"break\" statement within the loop, which jumps to\nthe code after the loop that handles the case of a file that holds a\nSHA-1. So move that code from below the loop into the if statement\nwhere the break was previously located. This makes the logic flow\nmore local and also opens the way for nesting the checking code in\nan inner loop.\n\nSigned-off-by: Michael Haggerty \n---\n refs.c | 22 +++++++++++++---------\n 1 file changed, 13 insertions(+), 9 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 1acce17..2722f75 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1295,8 +1295,19 @@ const char *resolve_ref_unsafe(const char *refname, unsigned char *sha1, int rea\n \t\t/*\n \t\t * Is it a symbolic ref?\n \t\t */\n-\t\tif (prefixcmp(buffer, \"ref:\"))\n-\t\t\tbreak;\n+\t\tif (prefixcmp(buffer, \"ref:\")) {\n+\t\t\t/*\n+\t\t\t * Please note that FETCH_HEAD has a second\n+\t\t\t * line containing other data.\n+\t\t\t */\n+\t\t\tif (get_sha1_hex(buffer, sha1) ||\n+\t\t\t (buffer[40] != '\\0' && !isspace(buffer[40]))) {\n+\t\t\t\tif (flag)\n+\t\t\t\t\t*flag |= REF_ISBROKEN;\n+\t\t\t\treturn NULL;\n+\t\t\t}\n+\t\t\treturn refname;\n+\t\t}\n \t\tif (flag)\n \t\t\t*flag |= REF_ISSYMREF;\n \t\tbuf = buffer + 4;\n@@ -1309,13 +1320,6 @@ const char *resolve_ref_unsafe(const char *refname, unsigned char *sha1, int rea\n \t\t}\n \t\trefname = strcpy(refname_buffer, buf);\n \t}\n-\t/* Please note that FETCH_HEAD has a second line containing other data. */\n-\tif (get_sha1_hex(buffer, sha1) || (buffer[40] != '\\0' && !isspace(buffer[40]))) {\n-\t\tif (flag)\n-\t\t\t*flag |= REF_ISBROKEN;\n-\t\treturn NULL;\n-\t}\n-\treturn refname;\n }\n \n char *resolve_refdup(const char *ref, unsigned char *sha1, int reading, int *flag)\n-- \n1.8.3\n"},{"id":"220442","messageId":"1370960780-1055-4-git-send-email-mhagger@alum.mit.edu","threadId":"33713","inReplyTo":"1370960780-1055-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 3/4] resolve_ref_unsafe(): nest reference-reading code in an infinite loop","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2013-06-11T14:26:19Z","receivedAt":"2013-06-11T14:26:19Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"The last statement of the loop body is \"break\", so this doesn't change\nthe logical flow...yet.\n\nSigned-off-by: Michael Haggerty \n---\nThis commit is 98% whitespace changes, so do it in a separate commit\nto make the changes in the next commit more obvious.\n\n refs.c | 123 ++++++++++++++++++++++++++++++++++-------------------------------\n 1 file changed, 64 insertions(+), 59 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 2722f75..7a77d76 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1248,77 +1248,82 @@ const char *resolve_ref_unsafe(const char *refname, unsigned char *sha1, int rea\n \n \t\tgit_snpath(path, sizeof(path), \"%s\", refname);\n \n-\t\tif (lstat(path, &st) < 0) {\n-\t\t\tif (errno == ENOENT)\n-\t\t\t\treturn handle_missing_loose_ref(refname, sha1, reading, flag);\n-\t\t\telse\n-\t\t\t\treturn NULL;\n-\t\t}\n+\t\tfor (;;) {\n+\t\t\tif (lstat(path, &st) < 0) {\n+\t\t\t\tif (errno == ENOENT)\n+\t\t\t\t\treturn handle_missing_loose_ref(\n+\t\t\t\t\t\t\trefname, sha1,\n+\t\t\t\t\t\t\treading, flag);\n+\t\t\t\telse\n+\t\t\t\t\treturn NULL;\n+\t\t\t}\n \n-\t\t/* Follow \"normalized\" - ie \"refs/..\" symlinks by hand */\n-\t\tif (S_ISLNK(st.st_mode)) {\n-\t\t\tlen = readlink(path, buffer, sizeof(buffer)-1);\n-\t\t\tif (len < 0)\n-\t\t\t\treturn NULL;\n-\t\t\tbuffer[len] = 0;\n-\t\t\tif (!prefixcmp(buffer, \"refs/\") &&\n-\t\t\t\t\t!check_refname_format(buffer, 0)) {\n-\t\t\t\tstrcpy(refname_buffer, buffer);\n-\t\t\t\trefname = refname_buffer;\n-\t\t\t\tif (flag)\n-\t\t\t\t\t*flag |= REF_ISSYMREF;\n-\t\t\t\tcontinue;\n+\t\t\t/* Follow \"normalized\" - ie \"refs/..\" symlinks by hand */\n+\t\t\tif (S_ISLNK(st.st_mode)) {\n+\t\t\t\tlen = readlink(path, buffer, sizeof(buffer)-1);\n+\t\t\t\tif (len < 0)\n+\t\t\t\t\treturn NULL;\n+\t\t\t\tbuffer[len] = 0;\n+\t\t\t\tif (!prefixcmp(buffer, \"refs/\") &&\n+\t\t\t\t !check_refname_format(buffer, 0)) {\n+\t\t\t\t\tstrcpy(refname_buffer, buffer);\n+\t\t\t\t\trefname = refname_buffer;\n+\t\t\t\t\tif (flag)\n+\t\t\t\t\t\t*flag |= REF_ISSYMREF;\n+\t\t\t\t\tbreak;\n+\t\t\t\t}\n \t\t\t}\n-\t\t}\n \n-\t\t/* Is it a directory? */\n-\t\tif (S_ISDIR(st.st_mode)) {\n-\t\t\terrno = EISDIR;\n-\t\t\treturn NULL;\n-\t\t}\n+\t\t\t/* Is it a directory? */\n+\t\t\tif (S_ISDIR(st.st_mode)) {\n+\t\t\t\terrno = EISDIR;\n+\t\t\t\treturn NULL;\n+\t\t\t}\n \n-\t\t/*\n-\t\t * Anything else, just open it and try to use it as\n-\t\t * a ref\n-\t\t */\n-\t\tfd = open(path, O_RDONLY);\n-\t\tif (fd < 0)\n-\t\t\treturn NULL;\n-\t\tlen = read_in_full(fd, buffer, sizeof(buffer)-1);\n-\t\tclose(fd);\n-\t\tif (len < 0)\n-\t\t\treturn NULL;\n-\t\twhile (len && isspace(buffer[len-1]))\n-\t\t\tlen--;\n-\t\tbuffer[len] = '\\0';\n+\t\t\t/*\n+\t\t\t * Anything else, just open it and try to use it as\n+\t\t\t * a ref\n+\t\t\t */\n+\t\t\tfd = open(path, O_RDONLY);\n+\t\t\tif (fd < 0)\n+\t\t\t\treturn NULL;\n+\t\t\tlen = read_in_full(fd, buffer, sizeof(buffer)-1);\n+\t\t\tclose(fd);\n+\t\t\tif (len < 0)\n+\t\t\t\treturn NULL;\n+\t\t\twhile (len && isspace(buffer[len-1]))\n+\t\t\t\tlen--;\n+\t\t\tbuffer[len] = '\\0';\n \n-\t\t/*\n-\t\t * Is it a symbolic ref?\n-\t\t */\n-\t\tif (prefixcmp(buffer, \"ref:\")) {\n \t\t\t/*\n-\t\t\t * Please note that FETCH_HEAD has a second\n-\t\t\t * line containing other data.\n+\t\t\t * Is it a symbolic ref?\n \t\t\t */\n-\t\t\tif (get_sha1_hex(buffer, sha1) ||\n-\t\t\t (buffer[40] != '\\0' && !isspace(buffer[40]))) {\n+\t\t\tif (prefixcmp(buffer, \"ref:\")) {\n+\t\t\t\t/*\n+\t\t\t\t * Please note that FETCH_HEAD has a second\n+\t\t\t\t * line containing other data.\n+\t\t\t\t */\n+\t\t\t\tif (get_sha1_hex(buffer, sha1) ||\n+\t\t\t\t (buffer[40] != '\\0' && !isspace(buffer[40]))) {\n+\t\t\t\t\tif (flag)\n+\t\t\t\t\t\t*flag |= REF_ISBROKEN;\n+\t\t\t\t\treturn NULL;\n+\t\t\t\t}\n+\t\t\t\treturn refname;\n+\t\t\t}\n+\t\t\tif (flag)\n+\t\t\t\t*flag |= REF_ISSYMREF;\n+\t\t\tbuf = buffer + 4;\n+\t\t\twhile (isspace(*buf))\n+\t\t\t\tbuf++;\n+\t\t\tif (check_refname_format(buf, REFNAME_ALLOW_ONELEVEL)) {\n \t\t\t\tif (flag)\n \t\t\t\t\t*flag |= REF_ISBROKEN;\n \t\t\t\treturn NULL;\n \t\t\t}\n-\t\t\treturn refname;\n-\t\t}\n-\t\tif (flag)\n-\t\t\t*flag |= REF_ISSYMREF;\n-\t\tbuf = buffer + 4;\n-\t\twhile (isspace(*buf))\n-\t\t\tbuf++;\n-\t\tif (check_refname_format(buf, REFNAME_ALLOW_ONELEVEL)) {\n-\t\t\tif (flag)\n-\t\t\t\t*flag |= REF_ISBROKEN;\n-\t\t\treturn NULL;\n+\t\t\trefname = strcpy(refname_buffer, buf);\n+\t\t\tbreak;\n \t\t}\n-\t\trefname = strcpy(refname_buffer, buf);\n \t}\n }\n \n-- \n1.8.3\n"},{"id":"220443","messageId":"1370960780-1055-5-git-send-email-mhagger@alum.mit.edu","threadId":"33713","inReplyTo":"1370960780-1055-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 4/4] resolve_ref_unsafe(): close race condition reading loose refs","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2013-06-11T14:26:20Z","receivedAt":"2013-06-11T14:26:20Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"We read loose references in two steps. The code is roughly:\n\n lstat()\n if error ENOENT:\n loose ref is missing; look for corresponding packed ref\n else if S_ISLNK:\n readlink()\n if error:\n report failure\n else if S_ISDIR:\n report failure\n else\n open()\n if error:\n report failure\n read()\n\nThe problem is that the first filesystem call, to lstat(), is not\natomic with the second filesystem call, to readlink() or open().\nTherefore it is possible for another process to change the file\nbetween our two calls, for example:\n\n* If the other process deletes the file, our second call will fail\n with ENOENT, which we *should* interpret as \"loose ref is missing;\n look for corresponding packed ref\". This can arise if the other\n process is pack-refs; it might have just written a new packed-refs\n file containing the old contents of the reference then deleted the\n loose ref.\n\n* If the other process changes a symlink into a plain file, our call\n to readlink() will fail with EINVAL, which we *should* respond to by\n trying to open() and read() the file.\n\nThe old code treats the reference as missing in both of these cases,\nwhich is incorrect.\n\nSo instead, wrap the above code in a loop. If the result of\nreadline()/open() is a failure that is inconsistent with the result of\nthe previous lstat(), then just loop again starting with a fresh call\nto lstat().\n\nOne race is still possible and undetected: another process could\nchange the file from a regular file into a symlink between the call to\nlstat and the call to open(). The open() call would silently follow\nthe symlink and not know that something is wrong. I don't see a way\nto detect this situation without the use of the O_NOFOLLOW option,\nwhich is not portable and is not used elsewhere in our code base.\n\nHowever, we don't use symlinks anymore, so this situation is unlikely.\nAnd it doesn't appear that treating a symlink as a regular file would\nhave grave consequences; after all, this is exactly how the code\nhandles non-relative symlinks.\n\nNote that this solves only the part of the race within\nresolve_ref_unsafe. In the situation described above, we may still be\ndepending on a cached view of the packed-refs file; that race will be\ndealt with in a future patch.\n\nThis problem was reported and diagnosed by Jeff King ,\nand this solution is derived from his patch.\n\nSigned-off-by: Michael Haggerty \n---\nPlease note that if there is some bizarre filesystem somewhere for\nwhich, for a single, static file\n\n lstat() reports S_ISLNK and readlink() fails with ENOENT or EINVAL\n\nor\n\n lstat() reports neither S_ISLNK nor S_ISDIR and open() fails with ENOENT\n\nthen the inner loop would never terminate. I can't imagine this\nhappening, but if somebody is worried about it the solution is simple:\nlimit the inner loop to 3 iterations or so.\n\nAnother obvious way to solve the problem would be to skip the lstat()\ncall altogether, and to rely on errno to distinguish the cases:\n\n readlink()\n if error ENOENT:\n loose ref is missing; look for corresponding packed ref\n else if error EINVAL:\n must not be a symlink; fall through to open()\n else if other error:\n report failure\n else:\n handle as symlink\n\n open()\n if error ENOENT:\n file must have been deleted since readlink(); look for packed ref\n else if other error:\n report failure\n else:\n read() and handle file contents\n\nThere is still a gap between readlink() and open(), during which the\nfile could have been changed from a regular file into a symlink, so\nthe remaining race mentioned above would still be undetected. I don't\nsee a strong reason to prefer the looping code in this patch vs. the\nno-lstat() alternative so I took the variant that was closer to the\nold code.\n\n refs.c | 27 +++++++++++++++++++++++----\n 1 file changed, 23 insertions(+), 4 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 7a77d76..6e7c1fd 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1248,6 +1248,15 @@ const char *resolve_ref_unsafe(const char *refname, unsigned char *sha1, int rea\n \n \t\tgit_snpath(path, sizeof(path), \"%s\", refname);\n \n+\t\t/*\n+\t\t * This loop is to avoid race conditions. First we\n+\t\t * lstat() the file, then we try to read it as a link\n+\t\t * or as a file. But if somebody changes the type of\n+\t\t * the file (file <-> directory <-> symlink) between\n+\t\t * the lstat() and reading, then we don't want to\n+\t\t * report that as an error but rather try again\n+\t\t * starting with the lstat().\n+\t\t */\n \t\tfor (;;) {\n \t\t\tif (lstat(path, &st) < 0) {\n \t\t\t\tif (errno == ENOENT)\n@@ -1261,8 +1270,13 @@ const char *resolve_ref_unsafe(const char *refname, unsigned char *sha1, int rea\n \t\t\t/* Follow \"normalized\" - ie \"refs/..\" symlinks by hand */\n \t\t\tif (S_ISLNK(st.st_mode)) {\n \t\t\t\tlen = readlink(path, buffer, sizeof(buffer)-1);\n-\t\t\t\tif (len < 0)\n-\t\t\t\t\treturn NULL;\n+\t\t\t\tif (len < 0) {\n+\t\t\t\t\tif (errno == ENOENT || errno == EINVAL)\n+\t\t\t\t\t\t/* inconsistent with lstat; retry */\n+\t\t\t\t\t\tcontinue;\n+\t\t\t\t\telse\n+\t\t\t\t\t\treturn NULL;\n+\t\t\t\t}\n \t\t\t\tbuffer[len] = 0;\n \t\t\t\tif (!prefixcmp(buffer, \"refs/\") &&\n \t\t\t\t !check_refname_format(buffer, 0)) {\n@@ -1285,8 +1299,13 @@ const char *resolve_ref_unsafe(const char *refname, unsigned char *sha1, int rea\n \t\t\t * a ref\n \t\t\t */\n \t\t\tfd = open(path, O_RDONLY);\n-\t\t\tif (fd < 0)\n-\t\t\t\treturn NULL;\n+\t\t\tif (fd < 0) {\n+\t\t\t\tif (errno == ENOENT)\n+\t\t\t\t\t/* inconsistent with lstat; retry */\n+\t\t\t\t\tcontinue;\n+\t\t\t\telse\n+\t\t\t\t\treturn NULL;\n+\t\t\t}\n \t\t\tlen = read_in_full(fd, buffer, sizeof(buffer)-1);\n \t\t\tclose(fd);\n \t\t\tif (len < 0)\n-- \n1.8.3\n"},{"id":"220511","messageId":"7v61xk9xgo.fsf@alter.siamese.dyndns.org","threadId":"33713","inReplyTo":"1370960780-1055-1-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH 0/4] Fix a race condition when reading loose refs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-06-11T20:57:59Z","receivedAt":"2013-06-11T20:57:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"I've read the series while on a bus and found all of them sensible.\nI do share the worry of retry storm you expressed in the last one,\nand I agree that giving up after N times is a reasonable way out,\nwhen it becomes necessary.\n\nThanks.\n"},{"id":"220586","messageId":"20130612080449.GA537@sigill.intra.peff.net","threadId":"33713","inReplyTo":"1370960780-1055-5-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH 4/4] resolve_ref_unsafe(): close race condition reading loose refs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-06-12T08:04:49Z","receivedAt":"2013-06-12T08:04:49Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jun 11, 2013 at 04:26:20PM +0200, Michael Haggerty wrote:\n\n> Please note that if there is some bizarre filesystem somewhere for\n> which, for a single, static file\n> \n> lstat() reports S_ISLNK and readlink() fails with ENOENT or EINVAL\n> [...]\n> then the inner loop would never terminate.\n\nYeah, I had the exact same thought while reading your description above.\nI think we can call that \"too crazy to worry about\", and deal with it if\nit ever actually comes up.\n\nOverall your series looks correct to me. The use of the extra \"for(;;)\"\nand its control flow feels a little tortured to me. Would it be\nsimpler to just do:\n\n stat_ref:\n if (lstat(...) < 0)\n ...\n if (readlink(...) < 0)\n if (errno == ENOENT || errno == EINVAL)\n /* inconsistent with lstat; retry */\n goto stat_ref;\n\n if (open(...) < 0)\n if (errno == ENOENT)\n /* inconsistent with lstat; retry */\n goto stat_ref;\n\nIt is really the same thing, but somehow the goto makes it more obvious\nto me that it is the exceptional case to loop at all (and I think your\npatch 3/4 could just go away entirely). But I don't mind that much if\nit stays with the for loop.\n\nThanks for a pleasant read; the split made the refactoring easy to\nfollow.\n\n-Peff\n"},{"id":"220699","messageId":"87d2rqs9ma.fsf@linux-k42r.v.cablecom.net","threadId":"33713","inReplyTo":"1370960780-1055-5-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH 4/4] resolve_ref_unsafe(): close race condition reading loose refs","fromName":"Thomas Rast","fromEmail":"trast@inf.ethz.ch","sentAt":"2013-06-13T08:22:37Z","receivedAt":"2013-06-13T08:22:37Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"Michael Haggerty writes:\n\n> One race is still possible and undetected: another process could\n> change the file from a regular file into a symlink between the call to\n> lstat and the call to open(). The open() call would silently follow\n> the symlink and not know that something is wrong. I don't see a way\n> to detect this situation without the use of the O_NOFOLLOW option,\n> which is not portable and is not used elsewhere in our code base.\n>\n> However, we don't use symlinks anymore, so this situation is unlikely.\n> And it doesn't appear that treating a symlink as a regular file would\n> have grave consequences; after all, this is exactly how the code\n> handles non-relative symlinks.\n\nYou could fstat() the fd you got from open(), and verify that it is\nstill the same inode/device. That's wasting one syscall per ref for\npretty much everyone, but perhaps if we really cared about this (and I\ngather from the above that we don't), we could conditionally use\nO_NOFOLLOW if available, otherwise do that fstat().\n\n-- \nThomas Rast\ntrast@{inf,student}.ethz.ch\n"},{"id":"220783","messageId":"51BAC39D.5030201@alum.mit.edu","threadId":"33713","inReplyTo":"87d2rqs9ma.fsf@linux-k42r.v.cablecom.net","subject":"Re: [PATCH 4/4] resolve_ref_unsafe(): close race condition reading loose refs","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2013-06-14T07:17:49Z","receivedAt":"2013-06-14T07:17:49Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 06/13/2013 10:22 AM, Thomas Rast wrote:\n> Michael Haggerty writes:\n> \n>> One race is still possible and undetected: another process could\n>> change the file from a regular file into a symlink between the call to\n>> lstat and the call to open(). The open() call would silently follow\n>> the symlink and not know that something is wrong. I don't see a way\n>> to detect this situation without the use of the O_NOFOLLOW option,\n>> which is not portable and is not used elsewhere in our code base.\n>>\n>> However, we don't use symlinks anymore, so this situation is unlikely.\n>> And it doesn't appear that treating a symlink as a regular file would\n>> have grave consequences; after all, this is exactly how the code\n>> handles non-relative symlinks.\n> \n> You could fstat() the fd you got from open(), and verify that it is\n> still the same inode/device. That's wasting one syscall per ref for\n> pretty much everyone, but perhaps if we really cared about this (and I\n> gather from the above that we don't), we could conditionally use\n> O_NOFOLLOW if available, otherwise do that fstat().\n\nYes, that would work. For now I think I will not worry about it, but\nI'll keep your trick in mind.\n\nThanks,\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"}]}