Merge branch 'stable-7.3' into stable-7.4

* stable-7.3:
  Do not always refresh packed-refs during ref updates

Change-Id: I9b5afc13f287da2248f2fcfd4be44b691b023843

Author: msohn

Documentation/config-options.md

@@ -57,7 +57,7 @@ For details on native git options see also the official [git config documentatio
 | `core.symlinks` | Auto detect if filesystem supports symlinks| ✅ | If false, symbolic links are checked out as small plain files that contain the link text. |
 | ~~`core.trustFolderStat`~~ | `true` | ⃞ | __Deprecated__, use `core.trustStat` instead. If set to `true` translated to `core.trustStat=always`, if `false` translated to `core.trustStat=never`, see below. If both `core.trustFolderStat` and `core.trustStat` are configured then `trustStat` takes precedence and `trustFolderStat` is ignored. |
 | `core.trustLooseRefStat` | `inherit` | ⃞ | Whether to trust the file attributes of loose refs and its fan-out parent directory. See `core.trustStat` for possible values. If `inherit`, JGit will use the behavior configured in `trustStat`. |
-| `core.trustPackedRefsStat` | `inherit` | ⃞ | Whether to trust the file attributes of the packed-refs file. See `core.trustStat` for possible values. If `inherit`, JGit will use the behavior configured in `core.trustStat`. |
+| `core.trustPackedRefsStat` | `inherit` | ⃞ | Whether to trust the file attributes of the packed-refs file. See `core.trustStat` for possible values. If `inherit`, JGit will use the behavior configured in `core.trustStat`. __Note:__ since 6.10.2, this setting applies during both ref reads and ref updates, but previously only applied during reads.|
 | `core.trustTablesListStat` | `inherit` | ⃞ | Whether to trust the file attributes of the `tables.list` file used by the reftable ref storage backend to store the list of reftable filenames. See `core.trustStat` for possible values. If `inherit`, JGit will use the behavior configured in `core.trustStat`.  The reftable backend is used if `extensions.refStorage = reftable`. |
 | `core.trustLooseObjectStat` | `inherit` | ⃞ | Whether to trust the file attributes of the loose object file and its fan-out parent directory. See `core.trustStat` for possible values. If `inherit`, JGit will use the behavior configured in `core.trustStat`. |
 | `core.trustPackStat` | `inherit` | ⃞ | Whether to trust the file attributes of the `objects/pack` directory. See `core.trustStat` for possible values. If `inherit`, JGit will use the behavior configured in `core.trustStat`. |

org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/PackedBatchRefUpdate.java

@@ -172,7 +172,7 @@ public void execute(RevWalk walk, ProgressMonitor monitor,
 			}
 
 			packedRefsLock = refdb.lockPackedRefsOrThrow();
-			PackedRefList oldPackedList = refdb.refreshPackedRefs();
+			PackedRefList oldPackedList = refdb.getLockedPackedRefs(packedRefsLock);
 			RefList<Ref> newRefs = applyUpdates(walk, oldPackedList, pending);
 			if (newRefs == null) {
 				return;

org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/RefDirectory.java

@@ -720,7 +720,7 @@ void delete(RefDirectoryUpdate update) throws IOException {
 				PackedRefList packed = getPackedRefs();
 				if (packed.contains(name)) {
 					// Force update our packed-refs snapshot before writing
-					packed = refreshPackedRefs();
+					packed = getLockedPackedRefs(lck);
 					int idx = packed.find(name);
 					if (0 <= idx) {
 						commitPackedRefs(lck, packed.remove(idx), packed, true);
@@ -788,7 +788,7 @@ private void pack(Collection<String> refs,
 		try {
 			LockFile lck = lockPackedRefsOrThrow();
 			try {
-				PackedRefList oldPacked = refreshPackedRefs();
+				PackedRefList oldPacked = getLockedPackedRefs(lck);
 				RefList<Ref> newPacked = oldPacked;
 
 				// Iterate over all refs to be packed
@@ -971,6 +971,15 @@ else if (0 <= (idx = packed.find(dst.getName())))
 	}
 
 	PackedRefList getPackedRefs() throws IOException {
+		return refreshPackedRefsIfNeeded();
+	}
+
+	PackedRefList getLockedPackedRefs(LockFile packedRefsFileLock) throws IOException {
+		packedRefsFileLock.requireLock();
+		return refreshPackedRefsIfNeeded();
+	}
+
+	PackedRefList refreshPackedRefsIfNeeded() throws IOException {
 		PackedRefList curList = packedRefs.get();
 		if (!curList.shouldRefresh()) {
 			return curList;
@@ -985,23 +994,29 @@ private PackedRefsRefresher getPackedRefsRefresher(PackedRefList curList)
 			return refresher;
 		}
 		// This synchronized is NOT needed for correctness. Instead it is used
-		// as a throttling mechanism to ensure that only one "read" thread does
-		// the work to refresh the file. In order to avoid stalling writes which
-		// must already be serialized and tend to be a bottleneck,
-		// the refreshPackedRefs() need not be synchronized.
+		// as a mechanism to try to avoid parallel reads of the same file content
+		// since repeating work, even in parallel, hurts performance.
+		// Unfortunately, this approach can still lead to some unnecessary re-reads
+		// during the "racy" window of the snapshot timestamp.
 		synchronized (this) {
 			if (packedRefsRefresher.get() != refresher) {
 				refresher = packedRefsRefresher.get();
 				if (refresher != null) {
-					// Refresher now guaranteed to have been created after the
-					// current thread entered getPackedRefsRefresher(), even if
-					// it's currently out of date.
+					// Refresher now guaranteed to have not started refreshing until
+					// after the current thread entered getPackedRefsRefresher(),
+					// even if it's currently out of date. And if the packed-refs
+					// lock is held before calling this method, then it is also
+					// guaranteed to not be out-of date even during the "racy"
+					// window of the snapshot timestamp.
 					return refresher;
 				}
 			}
-			refresher = createPackedRefsRefresherAsLatest(curList);
+			refresher = new PackedRefsRefresher(curList);
+			packedRefsRefresher.set(refresher);
 		}
-		return runAndClear(refresher);
+		refresher.run();
+		packedRefsRefresher.compareAndSet(refresher, null);
+		return refresher;
 	}
 
 	private boolean shouldRefreshPackedRefs(FileSnapshot snapshot) throws IOException {
@@ -1027,23 +1042,6 @@ private boolean shouldRefreshPackedRefs(FileSnapshot snapshot) throws IOExceptio
 		return true;
 	}
 
-	PackedRefList refreshPackedRefs() throws IOException {
-		return runAndClear(createPackedRefsRefresherAsLatest(packedRefs.get()))
-				.getOrThrowIOException();
-	}
-
-	private PackedRefsRefresher createPackedRefsRefresherAsLatest(PackedRefList curList) {
-		PackedRefsRefresher refresher = new PackedRefsRefresher(curList);
-		packedRefsRefresher.set(refresher);
-		return refresher;
-	}
-
-	private PackedRefsRefresher runAndClear(PackedRefsRefresher refresher) {
-		refresher.run();
-		packedRefsRefresher.compareAndSet(refresher, null);
-		return refresher;
-	}
-
 	private PackedRefList refreshPackedRefs(PackedRefList curList)
 			throws IOException {
 		final PackedRefList newList = readPackedRefs();