{"thread":{"id":"20560","subject":"[JGIT PATCH (RESEND) 0/4] Fix race condition on loading pack files","startedAt":"2009-08-12T19:45:19Z","lastAt":"2009-08-12T19:45:23Z","messageCount":5,"participants":["Shawn O. Pearce"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"120436","messageId":"1250106323-19408-1-git-send-email-spearce@spearce.org","threadId":"20560","inReplyTo":null,"subject":"[JGIT PATCH (RESEND) 0/4] Fix race condition on loading pack files","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2009-08-12T19:45:19Z","receivedAt":"2009-08-12T19:45:19Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"I had sent this series while you were on vacation, so I assume it\njust got lost in the shuffle and am resending.\n\nThis series tries to fix a \"racy git\" problem when reading pack\nfiles in a repository; its possible that the directory is modified,\nread and modified again in the same filesystem timestamp, resulting\nin the reader not noticing the second modification.  This may cause\na reader to lose access to objects on disk.\n\nShawn O. Pearce (4):\n  Avoid unnecessary stat when scanning packs in the objects directory\n  Make ObjectDirectory last modified time atomically updated with list\n  Don't create new pack lists if the directory hasn't changed\n  Fix racy condition when a repository is repacked\n\n .../src/org/spearce/jgit/lib/ObjectDirectory.java  |  235 ++++++++++++-------\n 1 files changed, 148 insertions(+), 87 deletions(-)\n"},{"id":"120440","messageId":"1250106323-19408-2-git-send-email-spearce@spearce.org","threadId":"20560","inReplyTo":"1250106323-19408-1-git-send-email-spearce@spearce.org","subject":"[JGIT PATCH (RESEND) 1/4] Avoid unnecessary stat when scanning packs in the objects directory","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2009-08-12T19:45:20Z","receivedAt":"2009-08-12T19:45:20Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"We only care that the pack name exists in the directory at this stage\nof processing.  Performing a isFile() test against the pack file name\nis likely to be slower than checking an in-memory HashSet, since the\nOS call has to actually check the inode to determine whether or not\nthe name is a file, or some other node type.  Since we already have\nforced the OS to give us a complete listing of the paths, its just\nfaster to consult that existing list.\n\nSigned-off-by: Shawn O. Pearce <spearce@spearce.org>\n---\n .../src/org/spearce/jgit/lib/ObjectDirectory.java  |   49 +++++++++++--------\n 1 files changed, 28 insertions(+), 21 deletions(-)\n\ndiff --git a/org.spearce.jgit/src/org/spearce/jgit/lib/ObjectDirectory.java b/org.spearce.jgit/src/org/spearce/jgit/lib/ObjectDirectory.java\nindex 7cc459c..4419f9c 100644\n--- a/org.spearce.jgit/src/org/spearce/jgit/lib/ObjectDirectory.java\n+++ b/org.spearce.jgit/src/org/spearce/jgit/lib/ObjectDirectory.java\n@@ -41,14 +41,16 @@\n import java.io.File;\n import java.io.FileNotFoundException;\n import java.io.FileReader;\n-import java.io.FilenameFilter;\n import java.io.IOException;\n import java.util.ArrayList;\n import java.util.Arrays;\n import java.util.Collection;\n+import java.util.Collections;\n import java.util.HashMap;\n+import java.util.HashSet;\n import java.util.List;\n import java.util.Map;\n+import java.util.Set;\n import java.util.concurrent.atomic.AtomicReference;\n \n import org.spearce.jgit.errors.PackMismatchException;\n@@ -339,11 +341,23 @@ private static boolean inList(final PackFile[] list, final PackFile pack) {\n \n \tprivate PackFile[] scanPacksImpl(final PackFile[] old) {\n \t\tfinal Map<String, PackFile> forReuse = reuseMap(old);\n-\t\tfinal String[] idxList = listPackIdx();\n-\t\tfinal List<PackFile> list = new ArrayList<PackFile>(idxList.length);\n-\t\tfor (final String indexName : idxList) {\n+\t\tfinal Set<String> names = listPackDirectory();\n+\t\tfinal List<PackFile> list = new ArrayList<PackFile>(names.size() >> 2);\n+\t\tfor (final String indexName : names) {\n+\t\t\t// Must match \"pack-[0-9a-f]{40}.idx\" to be an index.\n+\t\t\t//\n+\t\t\tif (indexName.length() != 49 || !indexName.endsWith(\".idx\"))\n+\t\t\t\tcontinue;\n+\n \t\t\tfinal String base = indexName.substring(0, indexName.length() - 4);\n \t\t\tfinal String packName = base + \".pack\";\n+\t\t\tif (!names.contains(packName)) {\n+\t\t\t\t// Sometimes C Git's HTTP fetch transport leaves a\n+\t\t\t\t// .idx file behind and does not download the .pack.\n+\t\t\t\t// We have to skip over such useless indexes.\n+\t\t\t\t//\n+\t\t\t\tcontinue;\n+\t\t\t}\n \n \t\t\tfinal PackFile oldPack = forReuse.remove(packName);\n \t\t\tif (oldPack != null) {\n@@ -352,14 +366,6 @@ private static boolean inList(final PackFile[] list, final PackFile pack) {\n \t\t\t}\n \n \t\t\tfinal File packFile = new File(packDirectory, packName);\n-\t\t\tif (!packFile.isFile()) {\n-\t\t\t\t// Sometimes C Git's HTTP fetch transport leaves a\n-\t\t\t\t// .idx file behind and does not download the .pack.\n-\t\t\t\t// We have to skip over such useless indexes.\n-\t\t\t\t//\n-\t\t\t\tcontinue;\n-\t\t\t}\n-\n \t\t\tfinal File idxFile = new File(packDirectory, indexName);\n \t\t\tlist.add(new PackFile(idxFile, packFile));\n \t\t}\n@@ -401,16 +407,17 @@ private static boolean inList(final PackFile[] list, final PackFile pack) {\n \t\treturn forReuse;\n \t}\n \n-\tprivate String[] listPackIdx() {\n+\tprivate Set<String> listPackDirectory() {\n \t\tpackDirectoryLastModified = packDirectory.lastModified();\n-\t\tfinal String[] idxList = packDirectory.list(new FilenameFilter() {\n-\t\t\tpublic boolean accept(final File baseDir, final String n) {\n-\t\t\t\t// Must match \"pack-[0-9a-f]{40}.idx\" to be an index.\n-\t\t\t\treturn n.length() == 49 && n.endsWith(\".idx\")\n-\t\t\t\t\t\t&& n.startsWith(\"pack-\");\n-\t\t\t}\n-\t\t});\n-\t\treturn idxList != null ? idxList : new String[0];\n+\t\tfinal String[] nameList = packDirectory.list();\n+\t\tif (nameList == null)\n+\t\t\treturn Collections.emptySet();\n+\t\tfinal Set<String> nameSet = new HashSet<String>(nameList.length << 1);\n+\t\tfor (final String name : nameList) {\n+\t\t\tif (name.startsWith(\"pack-\"))\n+\t\t\t\tnameSet.add(name);\n+\t\t}\n+\t\treturn nameSet;\n \t}\n \n \t@Override\n-- \n1.6.4.225.gb589e\n"},{"id":"120438","messageId":"1250106323-19408-3-git-send-email-spearce@spearce.org","threadId":"20560","inReplyTo":"1250106323-19408-2-git-send-email-spearce@spearce.org","subject":"[JGIT PATCH (RESEND) 2/4] Make ObjectDirectory last modified time atomically updated with list","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2009-08-12T19:45:21Z","receivedAt":"2009-08-12T19:45:21Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"By moving the last modified time and the PackFile[] into the same\nobject and using that object as the atomic access unit we ensure\nthat the two values change atomically for all readers.\n\nThis refactoring step prepares the code so we can later fix a race\ncondition where the objects directory is modified multiple times in\nthe same filesystem clock window, but a reader scanned the directory\nduring that same clock window.\n\nA nice cleanup performed here is to eliminate the null pointer value\nfrom the packList reference field.  Instead of getting a null we\nget NO_PACKS, which uses an \"impossible\" last modified time of -1,\nforcing any caller to scan the directory the first time a pack\nis required.\n\nSigned-off-by: Shawn O. Pearce <spearce@spearce.org>\n---\n .../src/org/spearce/jgit/lib/ObjectDirectory.java  |  126 ++++++++++----------\n 1 files changed, 63 insertions(+), 63 deletions(-)\n\ndiff --git a/org.spearce.jgit/src/org/spearce/jgit/lib/ObjectDirectory.java b/org.spearce.jgit/src/org/spearce/jgit/lib/ObjectDirectory.java\nindex 4419f9c..5b28207 100644\n--- a/org.spearce.jgit/src/org/spearce/jgit/lib/ObjectDirectory.java\n+++ b/org.spearce.jgit/src/org/spearce/jgit/lib/ObjectDirectory.java\n@@ -66,7 +66,7 @@\n  * {@link PackFile}s.\n  */\n public class ObjectDirectory extends ObjectDatabase {\n-\tprivate static final PackFile[] NO_PACKS = {};\n+\tprivate static final PackList NO_PACKS = new PackList(-1, new PackFile[0]);\n \n \tprivate final File objects;\n \n@@ -76,9 +76,7 @@\n \n \tprivate final File alternatesFile;\n \n-\tprivate final AtomicReference<PackFile[]> packList;\n-\n-\tprivate volatile long packDirectoryLastModified;\n+\tprivate final AtomicReference<PackList> packList;\n \n \t/**\n \t * Initialize a reference to an on-disk object directory.\n@@ -91,8 +89,7 @@ public ObjectDirectory(final File dir) {\n \t\tinfoDirectory = new File(objects, \"info\");\n \t\tpackDirectory = new File(objects, \"pack\");\n \t\talternatesFile = new File(infoDirectory, \"alternates\");\n-\n-\t\tpackList = new AtomicReference<PackFile[]>();\n+\t\tpackList = new AtomicReference<PackList>(NO_PACKS);\n \t}\n \n \t/**\n@@ -116,13 +113,10 @@ public void create() throws IOException {\n \n \t@Override\n \tpublic void closeSelf() {\n-\t\tPackFile[] packs = packList.get();\n-\t\tif (packs != null) {\n-\t\t\tpackList.set(null);\n-\t\t\tfor (final PackFile p : packs) {\n-\t\t\t\tp.close();\n-\t\t\t}\n-\t\t}\n+\t\tfinal PackList packs = packList.get();\n+\t\tpackList.set(NO_PACKS);\n+\t\tfor (final PackFile p : packs.packs)\n+\t\t\tp.close();\n \t}\n \n \t/**\n@@ -176,7 +170,7 @@ public String toString() {\n \n \t@Override\n \tprotected boolean hasObject1(final AnyObjectId objectId) {\n-\t\tfor (final PackFile p : packs()) {\n+\t\tfor (final PackFile p : packList.get().packs) {\n \t\t\ttry {\n \t\t\t\tif (p.hasObject(objectId)) {\n \t\t\t\t\treturn true;\n@@ -196,9 +190,9 @@ protected boolean hasObject1(final AnyObjectId objectId) {\n \t@Override\n \tprotected ObjectLoader openObject1(final WindowCursor curs,\n \t\t\tfinal AnyObjectId objectId) throws IOException {\n-\t\tPackFile[] pList = packs();\n+\t\tPackList pList = packList.get();\n \t\tSEARCH: for (;;) {\n-\t\t\tfor (final PackFile p : pList) {\n+\t\t\tfor (final PackFile p : pList.packs) {\n \t\t\t\ttry {\n \t\t\t\t\tfinal PackedObjectLoader ldr = p.get(curs, objectId);\n \t\t\t\t\tif (ldr != null) {\n@@ -224,9 +218,9 @@ protected ObjectLoader openObject1(final WindowCursor curs,\n \tvoid openObjectInAllPacks1(final Collection<PackedObjectLoader> out,\n \t\t\tfinal WindowCursor curs, final AnyObjectId objectId)\n \t\t\tthrows IOException {\n-\t\tPackFile[] pList = packs();\n+\t\tPackList pList = packList.get();\n \t\tSEARCH: for (;;) {\n-\t\t\tfor (final PackFile p : pList) {\n+\t\t\tfor (final PackFile p : pList.packs) {\n \t\t\t\ttry {\n \t\t\t\t\tfinal PackedObjectLoader ldr = p.get(curs, objectId);\n \t\t\t\t\tif (ldr != null) {\n@@ -265,8 +259,8 @@ protected ObjectLoader openObject2(final WindowCursor curs,\n \n \t@Override\n \tprotected boolean tryAgain1() {\n-\t\tfinal PackFile[] old = packList.get();\n-\t\tif (packDirectoryLastModified < packDirectory.lastModified()) {\n+\t\tfinal PackList old = packList.get();\n+\t\tif (old.tryAgain(packDirectory.lastModified())) {\n \t\t\tscanPacks(old);\n \t\t\treturn true;\n \t\t}\n@@ -274,57 +268,46 @@ protected boolean tryAgain1() {\n \t}\n \n \tprivate void insertPack(final PackFile pf) {\n-\t\tPackFile[] o, n;\n+\t\tPackList o, n;\n \t\tdo {\n-\t\t\to = packs();\n-\t\t\tn = new PackFile[1 + o.length];\n-\t\t\tn[0] = pf;\n-\t\t\tSystem.arraycopy(o, 0, n, 1, o.length);\n+\t\t\to = packList.get();\n+\t\t\tfinal PackFile[] oldList = o.packs;\n+\t\t\tfinal PackFile[] newList = new PackFile[1 + oldList.length];\n+\t\t\tnewList[0] = pf;\n+\t\t\tSystem.arraycopy(oldList, 0, newList, 1, oldList.length);\n+\t\t\tn = new PackList(o.lastModified, newList);\n \t\t} while (!packList.compareAndSet(o, n));\n \t}\n \n \tprivate void removePack(final PackFile deadPack) {\n-\t\tPackFile[] o, n;\n+\t\tPackList o, n;\n \t\tdo {\n \t\t\to = packList.get();\n-\t\t\tif (o == null || !inList(o, deadPack)) {\n-\t\t\t\tbreak;\n \n-\t\t\t} else if (o.length == 1) {\n-\t\t\t\tn = NO_PACKS;\n+\t\t\tfinal PackFile[] oldList = o.packs;\n+\t\t\tfinal int j = indexOf(oldList, deadPack);\n+\t\t\tif (j < 0)\n+\t\t\t\tbreak;\n \n-\t\t\t} else {\n-\t\t\t\tn = new PackFile[o.length - 1];\n-\t\t\t\tint j = 0;\n-\t\t\t\tfor (final PackFile p : o) {\n-\t\t\t\t\tif (p != deadPack) {\n-\t\t\t\t\t\tn[j++] = p;\n-\t\t\t\t\t}\n-\t\t\t\t}\n-\t\t\t}\n+\t\t\tfinal PackFile[] newList = new PackFile[oldList.length - 1];\n+\t\t\tSystem.arraycopy(oldList, 0, newList, 0, j);\n+\t\t\tSystem.arraycopy(oldList, j + 1, newList, j, newList.length - j);\n+\t\t\tn = new PackList(o.lastModified, newList);\n \t\t} while (!packList.compareAndSet(o, n));\n \t\tdeadPack.close();\n \t}\n \n-\tprivate static boolean inList(final PackFile[] list, final PackFile pack) {\n-\t\tfor (final PackFile p : list) {\n-\t\t\tif (p == pack) {\n-\t\t\t\treturn true;\n-\t\t\t}\n+\tprivate static int indexOf(final PackFile[] list, final PackFile pack) {\n+\t\tfor (int i = 0; i < list.length; i++) {\n+\t\t\tif (list[i] == pack)\n+\t\t\t\treturn i;\n \t\t}\n-\t\treturn false;\n-\t}\n-\n-\tprivate PackFile[] packs() {\n-\t\tPackFile[] r = packList.get();\n-\t\tif (r == null)\n-\t\t\tr = scanPacks(null);\n-\t\treturn r;\n+\t\treturn -1;\n \t}\n \n-\tprivate PackFile[] scanPacks(final PackFile[] original) {\n+\tprivate PackList scanPacks(final PackList original) {\n \t\tsynchronized (packList) {\n-\t\t\tPackFile[] o, n;\n+\t\t\tPackList o, n;\n \t\t\tdo {\n \t\t\t\to = packList.get();\n \t\t\t\tif (o != original) {\n@@ -333,14 +316,15 @@ private static boolean inList(final PackFile[] list, final PackFile pack) {\n \t\t\t\t\t//\n \t\t\t\t\treturn o;\n \t\t\t\t}\n-\t\t\t\tn = scanPacksImpl(o != null ? o : NO_PACKS);\n+\t\t\t\tn = scanPacksImpl(o);\n \t\t\t} while (!packList.compareAndSet(o, n));\n \t\t\treturn n;\n \t\t}\n \t}\n \n-\tprivate PackFile[] scanPacksImpl(final PackFile[] old) {\n+\tprivate PackList scanPacksImpl(final PackList old) {\n \t\tfinal Map<String, PackFile> forReuse = reuseMap(old);\n+\t\tfinal long lastModified = packDirectory.lastModified();\n \t\tfinal Set<String> names = listPackDirectory();\n \t\tfinal List<PackFile> list = new ArrayList<PackFile>(names.size() >> 2);\n \t\tfor (final String indexName : names) {\n@@ -374,17 +358,17 @@ private static boolean inList(final PackFile[] list, final PackFile pack) {\n \t\t\tp.close();\n \t\t}\n \n-\t\tif (list.isEmpty()) {\n-\t\t\treturn NO_PACKS;\n-\t\t}\n+\t\tif (list.isEmpty())\n+\t\t\treturn new PackList(lastModified, NO_PACKS.packs);\n+\n \t\tfinal PackFile[] r = list.toArray(new PackFile[list.size()]);\n \t\tArrays.sort(r, PackFile.SORT);\n-\t\treturn r;\n+\t\treturn new PackList(lastModified, r);\n \t}\n \n-\tprivate static Map<String, PackFile> reuseMap(final PackFile[] old) {\n+\tprivate static Map<String, PackFile> reuseMap(final PackList old) {\n \t\tfinal Map<String, PackFile> forReuse = new HashMap<String, PackFile>();\n-\t\tfor (final PackFile p : old) {\n+\t\tfor (final PackFile p : old.packs) {\n \t\t\tif (p.invalid()) {\n \t\t\t\t// The pack instance is corrupted, and cannot be safely used\n \t\t\t\t// again. Do not include it in our reuse map.\n@@ -408,7 +392,6 @@ private static boolean inList(final PackFile[] list, final PackFile pack) {\n \t}\n \n \tprivate Set<String> listPackDirectory() {\n-\t\tpackDirectoryLastModified = packDirectory.lastModified();\n \t\tfinal String[] nameList = packDirectory.list();\n \t\tif (nameList == null)\n \t\t\treturn Collections.emptySet();\n@@ -454,4 +437,21 @@ private ObjectDatabase openAlternate(final String location)\n \t\t}\n \t\treturn new ObjectDirectory(objdir);\n \t}\n+\n+\tprivate static final class PackList {\n+\t\t/** Last modification time of {@link ObjectDirectory#packDirectory}. */\n+\t\tfinal long lastModified;\n+\n+\t\t/** All known packs, sorted by {@link PackFile#SORT}. */\n+\t\tfinal PackFile[] packs;\n+\n+\t\tPackList(final long lastModified, final PackFile[] packs) {\n+\t\t\tthis.lastModified = lastModified;\n+\t\t\tthis.packs = packs;\n+\t\t}\n+\n+\t\tboolean tryAgain(final long currLastModified) {\n+\t\t\treturn lastModified < currLastModified;\n+\t\t}\n+\t}\n }\n-- \n1.6.4.225.gb589e\n"},{"id":"120437","messageId":"1250106323-19408-4-git-send-email-spearce@spearce.org","threadId":"20560","inReplyTo":"1250106323-19408-3-git-send-email-spearce@spearce.org","subject":"[JGIT PATCH (RESEND) 3/4] Don't create new pack lists if the directory hasn't changed","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2009-08-12T19:45:22Z","receivedAt":"2009-08-12T19:45:22Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"If we scan the directory and it hasn't changed since the last time we\nscanned it, there is no reason to build a new PackList and update the\nvolatile reference.  This just generates unnecessary garbage and may\nmake it more difficult to detect an unmodified directory.\n\nSigned-off-by: Shawn O. Pearce <spearce@spearce.org>\n---\n .../src/org/spearce/jgit/lib/ObjectDirectory.java  |   18 ++++++++++++++----\n 1 files changed, 14 insertions(+), 4 deletions(-)\n\ndiff --git a/org.spearce.jgit/src/org/spearce/jgit/lib/ObjectDirectory.java b/org.spearce.jgit/src/org/spearce/jgit/lib/ObjectDirectory.java\nindex 5b28207..0bb3c01 100644\n--- a/org.spearce.jgit/src/org/spearce/jgit/lib/ObjectDirectory.java\n+++ b/org.spearce.jgit/src/org/spearce/jgit/lib/ObjectDirectory.java\n@@ -260,10 +260,8 @@ protected ObjectLoader openObject2(final WindowCursor curs,\n \t@Override\n \tprotected boolean tryAgain1() {\n \t\tfinal PackList old = packList.get();\n-\t\tif (old.tryAgain(packDirectory.lastModified())) {\n-\t\t\tscanPacks(old);\n-\t\t\treturn true;\n-\t\t}\n+\t\tif (old.tryAgain(packDirectory.lastModified()))\n+\t\t\treturn old != scanPacks(old);\n \t\treturn false;\n \t}\n \n@@ -317,6 +315,8 @@ private PackList scanPacks(final PackList original) {\n \t\t\t\t\treturn o;\n \t\t\t\t}\n \t\t\t\tn = scanPacksImpl(o);\n+\t\t\t\tif (n == o)\n+\t\t\t\t\treturn n;\n \t\t\t} while (!packList.compareAndSet(o, n));\n \t\t\treturn n;\n \t\t}\n@@ -327,6 +327,7 @@ private PackList scanPacksImpl(final PackList old) {\n \t\tfinal long lastModified = packDirectory.lastModified();\n \t\tfinal Set<String> names = listPackDirectory();\n \t\tfinal List<PackFile> list = new ArrayList<PackFile>(names.size() >> 2);\n+\t\tboolean foundNew = false;\n \t\tfor (final String indexName : names) {\n \t\t\t// Must match \"pack-[0-9a-f]{40}.idx\" to be an index.\n \t\t\t//\n@@ -352,8 +353,17 @@ private PackList scanPacksImpl(final PackList old) {\n \t\t\tfinal File packFile = new File(packDirectory, packName);\n \t\t\tfinal File idxFile = new File(packDirectory, indexName);\n \t\t\tlist.add(new PackFile(idxFile, packFile));\n+\t\t\tfoundNew = true;\n \t\t}\n \n+\t\t// If we did not discover any new files, the modification time was not\n+\t\t// changed, and we did not remove any files, then the set of files is\n+\t\t// the same as the set we were given. Instead of building a new object\n+\t\t// return the same collection.\n+\t\t//\n+\t\tif (!foundNew && lastModified == old.lastModified && forReuse.isEmpty())\n+\t\t\treturn old;\n+\n \t\tfor (final PackFile p : forReuse.values()) {\n \t\t\tp.close();\n \t\t}\n-- \n1.6.4.225.gb589e\n"},{"id":"120439","messageId":"1250106323-19408-5-git-send-email-spearce@spearce.org","threadId":"20560","inReplyTo":"1250106323-19408-4-git-send-email-spearce@spearce.org","subject":"[JGIT PATCH (RESEND) 4/4] Fix racy condition when a repository is repacked","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2009-08-12T19:45:23Z","receivedAt":"2009-08-12T19:45:23Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"If the filesystem clock granularity is sufficiently large enough it\nis possible for a repacking program such as `git repack` to change\nthe same directory more than once within the same modification time.\n\nIf JGit were to scan the directory between changes in the same\nclock step it will never see the later edits, because the directory\nmodification time has not changed.\n\nInstead we now keep track of the last time we read the directory.\nIf an object cannot be found on disk and the pack directory's last\nmodified time is less than 2 minutes since the last time we read\nthe directory's contents, we scan it again looking for changes.\n\nWorst case scenario, JGit will list the pack directory once for\neach requested missing object, until the directory has aged at\nleast 2 minutes.  Most repositories modify this directory only a\nfew times a week, so this is not an undue burden on the host.\n\nSigned-off-by: Shawn O. Pearce <spearce@spearce.org>\n---\n .../src/org/spearce/jgit/lib/ObjectDirectory.java  |   60 +++++++++++++++++---\n 1 files changed, 52 insertions(+), 8 deletions(-)\n\ndiff --git a/org.spearce.jgit/src/org/spearce/jgit/lib/ObjectDirectory.java b/org.spearce.jgit/src/org/spearce/jgit/lib/ObjectDirectory.java\nindex 0bb3c01..859824d 100644\n--- a/org.spearce.jgit/src/org/spearce/jgit/lib/ObjectDirectory.java\n+++ b/org.spearce.jgit/src/org/spearce/jgit/lib/ObjectDirectory.java\n@@ -66,7 +66,7 @@\n  * {@link PackFile}s.\n  */\n public class ObjectDirectory extends ObjectDatabase {\n-\tprivate static final PackList NO_PACKS = new PackList(-1, new PackFile[0]);\n+\tprivate static final PackList NO_PACKS = new PackList(-1, -1, new PackFile[0]);\n \n \tprivate final File objects;\n \n@@ -273,7 +273,7 @@ private void insertPack(final PackFile pf) {\n \t\t\tfinal PackFile[] newList = new PackFile[1 + oldList.length];\n \t\t\tnewList[0] = pf;\n \t\t\tSystem.arraycopy(oldList, 0, newList, 1, oldList.length);\n-\t\t\tn = new PackList(o.lastModified, newList);\n+\t\t\tn = new PackList(o.lastRead, o.lastModified, newList);\n \t\t} while (!packList.compareAndSet(o, n));\n \t}\n \n@@ -290,7 +290,7 @@ private void removePack(final PackFile deadPack) {\n \t\t\tfinal PackFile[] newList = new PackFile[oldList.length - 1];\n \t\t\tSystem.arraycopy(oldList, 0, newList, 0, j);\n \t\t\tSystem.arraycopy(oldList, j + 1, newList, j, newList.length - j);\n-\t\t\tn = new PackList(o.lastModified, newList);\n+\t\t\tn = new PackList(o.lastRead, o.lastModified, newList);\n \t\t} while (!packList.compareAndSet(o, n));\n \t\tdeadPack.close();\n \t}\n@@ -324,6 +324,7 @@ private PackList scanPacks(final PackList original) {\n \n \tprivate PackList scanPacksImpl(final PackList old) {\n \t\tfinal Map<String, PackFile> forReuse = reuseMap(old);\n+\t\tfinal long lastRead = System.currentTimeMillis();\n \t\tfinal long lastModified = packDirectory.lastModified();\n \t\tfinal Set<String> names = listPackDirectory();\n \t\tfinal List<PackFile> list = new ArrayList<PackFile>(names.size() >> 2);\n@@ -362,18 +363,18 @@ private PackList scanPacksImpl(final PackList old) {\n \t\t// return the same collection.\n \t\t//\n \t\tif (!foundNew && lastModified == old.lastModified && forReuse.isEmpty())\n-\t\t\treturn old;\n+\t\t\treturn old.updateLastRead(lastRead);\n \n \t\tfor (final PackFile p : forReuse.values()) {\n \t\t\tp.close();\n \t\t}\n \n \t\tif (list.isEmpty())\n-\t\t\treturn new PackList(lastModified, NO_PACKS.packs);\n+\t\t\treturn new PackList(lastRead, lastModified, NO_PACKS.packs);\n \n \t\tfinal PackFile[] r = list.toArray(new PackFile[list.size()]);\n \t\tArrays.sort(r, PackFile.SORT);\n-\t\treturn new PackList(lastModified, r);\n+\t\treturn new PackList(lastRead, lastModified, r);\n \t}\n \n \tprivate static Map<String, PackFile> reuseMap(final PackList old) {\n@@ -449,19 +450,62 @@ private ObjectDatabase openAlternate(final String location)\n \t}\n \n \tprivate static final class PackList {\n+\t\t/** Last wall-clock time the directory was read. */\n+\t\tvolatile long lastRead;\n+\n \t\t/** Last modification time of {@link ObjectDirectory#packDirectory}. */\n \t\tfinal long lastModified;\n \n \t\t/** All known packs, sorted by {@link PackFile#SORT}. */\n \t\tfinal PackFile[] packs;\n \n-\t\tPackList(final long lastModified, final PackFile[] packs) {\n+\t\tprivate boolean cannotBeRacilyClean;\n+\n+\t\tPackList(final long lastRead, final long lastModified,\n+\t\t\t\tfinal PackFile[] packs) {\n+\t\t\tthis.lastRead = lastRead;\n \t\t\tthis.lastModified = lastModified;\n \t\t\tthis.packs = packs;\n+\t\t\tthis.cannotBeRacilyClean = notRacyClean(lastRead);\n+\t\t}\n+\n+\t\tprivate boolean notRacyClean(final long read) {\n+\t\t\treturn read - lastModified > 2 * 60 * 1000L;\n+\t\t}\n+\n+\t\tPackList updateLastRead(final long now) {\n+\t\t\tif (notRacyClean(now))\n+\t\t\t\tcannotBeRacilyClean = true;\n+\t\t\tlastRead = now;\n+\t\t\treturn this;\n \t\t}\n \n \t\tboolean tryAgain(final long currLastModified) {\n-\t\t\treturn lastModified < currLastModified;\n+\t\t\t// Any difference indicates the directory was modified.\n+\t\t\t//\n+\t\t\tif (lastModified != currLastModified)\n+\t\t\t\treturn true;\n+\n+\t\t\t// We have already determined the last read was far enough\n+\t\t\t// after the last modification that any new modifications\n+\t\t\t// are certain to change the last modified time.\n+\t\t\t//\n+\t\t\tif (cannotBeRacilyClean)\n+\t\t\t\treturn false;\n+\n+\t\t\tif (notRacyClean(lastRead)) {\n+\t\t\t\t// Our last read should have marked cannotBeRacilyClean,\n+\t\t\t\t// but this thread may not have seen the change. The read\n+\t\t\t\t// of the volatile field lastRead should have fixed that.\n+\t\t\t\t//\n+\t\t\t\treturn false;\n+\t\t\t}\n+\n+\t\t\t// We last read this directory too close to its last observed\n+\t\t\t// modification time. We may have missed a modification. Scan\n+\t\t\t// the directory again, to ensure we still see the same state.\n+\t\t\t//\n+\t\t\treturn true;\n \t\t}\n \t}\n }\n-- \n1.6.4.225.gb589e\n"}]}