Merge branch 'stable-7.1'

* stable-7.1:
  FileSnapshot: silence "Not a Directory" exceptions
  FileSnapshot: refactor to share error handling
  Mark Attribute#getValue as @Nullable
  Fix potential NPE in ResolveMerger#getAttributesContentMergeStrategy
  Fix NPE in DiffFormatter#getDiffDriver
  Pack: ensure packfile is still valid while still recoverable
  WindowCache: add bulk purge(), call from bulk sites
  UploadPack#implies: add missing @since tag
  Disable MergeToolTest#testEmptyToolName

Change-Id: Ifa8df2b9e6e4ee371113b7114fe20b42333e3718

9 files changed, 87 insertions, 48 deletions

diff --git a/org.eclipse.jgit.pgm.test/tst/org/eclipse/jgit/pgm/MergeToolTest.java b/org.eclipse.jgit.pgm.test/tst/org/eclipse/jgit/pgm/MergeToolTest.java
index 65c7e9a88c..6339831a40 100644
--- a/org.eclipse.jgit.pgm.test/tst/org/eclipse/jgit/pgm/MergeToolTest.java
+++ b/org.eclipse.jgit.pgm.test/tst/org/eclipse/jgit/pgm/MergeToolTest.java
@@ -27,6 +27,7 @@ import org.eclipse.jgit.internal.diffmergetool.ExternalMergeTool;
 import org.eclipse.jgit.internal.diffmergetool.MergeTools;
 import org.eclipse.jgit.lib.StoredConfig;
 import org.junit.Before;
+import org.junit.Ignore;
 import org.junit.Test;
 
 /**
@@ -77,6 +78,7 @@ public class MergeToolTest extends ToolTestCase {
 				+ errorReturnCode);
 	}
 
+	@Ignore
 	@Test
 	public void testEmptyToolName() throws Exception {
 		assumeLinuxPlatform();
diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/attributes/Attribute.java b/org.eclipse.jgit/src/org/eclipse/jgit/attributes/Attribute.java
index fe3e22a21f..9c4d8700a2 100644
--- a/org.eclipse.jgit/src/org/eclipse/jgit/attributes/Attribute.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/attributes/Attribute.java
@@ -9,6 +9,8 @@
  */
 package org.eclipse.jgit.attributes;
 
+import org.eclipse.jgit.annotations.Nullable;
+
 /**
  * Represents an attribute.
  * <p>
@@ -139,6 +141,7 @@ public final class Attribute {
 	 *
 	 * @return the attribute value (may be <code>null</code>)
 	 */
+	@Nullable
 	public String getValue() {
 		return value;
 	}
diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/diff/DiffFormatter.java b/org.eclipse.jgit/src/org/eclipse/jgit/diff/DiffFormatter.java
index bc07cedf4f..cbac3f90b7 100644
--- a/org.eclipse.jgit/src/org/eclipse/jgit/diff/DiffFormatter.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/diff/DiffFormatter.java
@@ -1356,13 +1356,17 @@ public class DiffFormatter implements AutoCloseable {
 
 	private DiffDriver getDiffDriver(DiffEntry entry) {
 		Attribute diffAttr = entry.getDiffAttribute();
-		if (diffAttr != null) {
-			try {
-				return DiffDriver.valueOf(diffAttr.getValue());
-			} catch (IllegalArgumentException e) {
-				return null;
-			}
+		if (diffAttr == null) {
+			return null;
+		}
+		String diffAttrValue = diffAttr.getValue();
+		if (diffAttrValue == null) {
+			return null;
+		}
+		try {
+			return DiffDriver.valueOf(diffAttrValue);
+		} catch (IllegalArgumentException e) {
+			return null;
 		}
-		return null;
 	}
 }
diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/FileSnapshot.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/FileSnapshot.java
index c4fa3eeb99..8f83a37efc 100644
--- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/FileSnapshot.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/FileSnapshot.java
@@ -15,6 +15,7 @@ import static org.eclipse.jgit.util.FS.FileStoreAttributes.FALLBACK_TIMESTAMP_RE
 
 import java.io.File;
 import java.io.IOException;
+import java.nio.file.FileSystemException;
 import java.nio.file.NoSuchFileException;
 import java.nio.file.attribute.BasicFileAttributes;
 import java.time.Duration;
@@ -22,6 +23,7 @@ import java.time.Instant;
 import java.time.ZoneId;
 import java.time.format.DateTimeFormatter;
 import java.util.Locale;
+import java.util.NoSuchElementException;
 import java.util.Objects;
 import java.util.concurrent.TimeUnit;
 
@@ -208,14 +210,8 @@ public class FileSnapshot {
 		this.useConfig = useConfig;
 		BasicFileAttributes fileAttributes = null;
 		try {
-			fileAttributes = FS.DETECTED.fileAttributes(file);
-		} catch (NoSuchFileException e) {
-			this.lastModified = Instant.EPOCH;
-			this.size = 0L;
-			this.fileKey = MISSING_FILEKEY;
-			return;
-		} catch (IOException e) {
-			LOG.error(e.getMessage(), e);
+			fileAttributes = getFileAttributes(file);
+		} catch (NoSuchElementException e) {
 			this.lastModified = Instant.EPOCH;
 			this.size = 0L;
 			this.fileKey = MISSING_FILEKEY;
@@ -285,16 +281,11 @@ public class FileSnapshot {
 		long currSize;
 		Object currFileKey;
 		try {
-			BasicFileAttributes fileAttributes = FS.DETECTED.fileAttributes(path);
+			BasicFileAttributes fileAttributes = getFileAttributes(path);
 			currLastModified = fileAttributes.lastModifiedTime().toInstant();
 			currSize = fileAttributes.size();
 			currFileKey = getFileKey(fileAttributes);
-		} catch (NoSuchFileException e) {
-			currLastModified = Instant.EPOCH;
-			currSize = 0L;
-			currFileKey = MISSING_FILEKEY;
-		} catch (IOException e) {
-			LOG.error(e.getMessage(), e);
+		} catch (NoSuchElementException e) {
 			currLastModified = Instant.EPOCH;
 			currSize = 0L;
 			currFileKey = MISSING_FILEKEY;
@@ -552,4 +543,18 @@ public class FileSnapshot {
 		}
 		return fileStoreAttributeCache;
 	}
+
+	private static BasicFileAttributes getFileAttributes(File path) throws NoSuchElementException {
+		try {
+			return FS.DETECTED.fileAttributes(path);
+		} catch (NoSuchFileException e) {
+		} catch (FileSystemException e) {
+			if (!e.getMessage().endsWith("Not a directory")) {
+				LOG.error(e.getMessage(), e);
+			}
+		} catch (IOException e) {
+			LOG.error(e.getMessage(), e);
+		}
+		throw new NoSuchElementException(path.toString());
+  }
 }
diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/Pack.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/Pack.java
index 3518342f97..61577a956c 100644
--- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/Pack.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/Pack.java
@@ -296,15 +296,28 @@ public class Pack implements Iterable<PackIndex.MutableEntry> {
 	}
 
 	/**
-	 * Close the resources utilized by this repository
+	 * Close the resources utilized by these pack files
+	 *
+	 * @param packs
+	 *            packs to close
+	 */
+	public static void close(Set<Pack> packs) {
+		WindowCache.purge(packs);
+		packs.forEach(p -> p.closeIndices());
+	}
+
+	/**
+	 * Close the resources utilized by this pack file
 	 */
 	public void close() {
 		WindowCache.purge(this);
-		synchronized (this) {
-			loadedIdx.clear();
-			reverseIdx.clear();
-			bitmapIdx.clear();
-		}
+		closeIndices();
+	}
+
+	private synchronized void closeIndices() {
+		loadedIdx.clear();
+		reverseIdx.clear();
+		bitmapIdx.clear();
 	}
 
 	/**
@@ -584,13 +597,19 @@ public class Pack implements Iterable<PackIndex.MutableEntry> {
 						assert(crc2 != null);
 						crc2.update(buf, 0, n);
 					}
+					cnt -= n;
 					if (!isHeaderWritten) {
+						if (invalid && cnt > 0) {
+							// Since this is not the last iteration and the packfile is invalid,
+							// better to assume the iterations will not all complete here while
+							// it is still likely recoverable.
+							throw new StoredObjectRepresentationNotAvailableException(invalidatingCause);
+						}
 						out.writeHeader(src, inflatedLength);
 						isHeaderWritten = true;
 					}
 					out.write(buf, 0, n);
 					pos += n;
-					cnt -= n;
 				}
 				if (validate) {
 					assert(crc2 != null);
diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/PackDirectory.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/PackDirectory.java
index 51a8148838..e31126f027 100644
--- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/PackDirectory.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/PackDirectory.java
@@ -24,6 +24,7 @@ import java.util.Collection;
 import java.util.Collections;
 import java.util.EnumMap;
 import java.util.HashMap;
+import java.util.HashSet;
 import java.util.List;
 import java.util.Map;
 import java.util.Set;
@@ -111,9 +112,7 @@ class PackDirectory {
 	void close() {
 		PackList packs = packList.get();
 		if (packs != NO_PACKS && packList.compareAndSet(packs, NO_PACKS)) {
-			for (Pack p : packs.packs) {
-				p.close();
-			}
+			Pack.close(Set.of(packs.packs));
 		}
 	}
 
@@ -484,9 +483,7 @@ class PackDirectory {
 			return old;
 		}
 
-		for (Pack p : forReuse.values()) {
-			p.close();
-		}
+		Pack.close(new HashSet<>(forReuse.values()));
 
 		if (list.isEmpty()) {
 			return new PackList(snapshot, NO_PACKS.packs);
diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/WindowCache.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/WindowCache.java
index 30f8240aa9..3ec7e6e666 100644
--- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/WindowCache.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/WindowCache.java
@@ -17,6 +17,7 @@ import java.lang.ref.SoftReference;
 import java.util.Collections;
 import java.util.Map;
 import java.util.Random;
+import java.util.Set;
 import java.util.concurrent.ConcurrentHashMap;
 import java.util.concurrent.ConcurrentLinkedQueue;
 import java.util.concurrent.atomic.AtomicBoolean;
@@ -397,7 +398,11 @@ public class WindowCache {
 	}
 
 	static final void purge(Pack pack) {
-		cache.removeAll(pack);
+		purge(Collections.singleton(pack));
+	}
+
+	static final void purge(Set<Pack> packs) {
+		cache.removeAll(packs);
 	}
 
 	/** cleanup released and/or garbage collected windows. */
@@ -710,20 +715,21 @@ public class WindowCache {
 	/**
 	 * Clear all entries related to a single file.
 	 * <p>
-	 * Typically this method is invoked during {@link Pack#close()}, when we
-	 * know the pack is never going to be useful to us again (for example, it no
-	 * longer exists on disk). A concurrent reader loading an entry from this
-	 * same pack may cause the pack to become stuck in the cache anyway.
+	 * Typically this method is invoked during {@link Pack#close()}, or
+	 * {@link Pack#close(Set)}, when we know the packs are never going to be
+	 * useful to us again (for example, they no longer exist on disk after gc).
+	 * A concurrent reader loading an entry from these same packs may cause a
+	 * pack to become stuck in the cache anyway.
 	 *
-	 * @param pack
-	 *            the file to purge all entries of.
+	 * @param packs
+	 *            the files to purge all entries of.
 	 */
-	private void removeAll(Pack pack) {
+	private void removeAll(Set<Pack> packs) {
 		for (int s = 0; s < tableSize; s++) {
 			final Entry e1 = table.get(s);
 			boolean hasDead = false;
 			for (Entry e = e1; e != null; e = e.next) {
-				if (e.ref.getPack() == pack) {
+				if (packs.contains(e.ref.getPack())) {
 					e.kill();
 					hasDead = true;
 				} else if (e.dead)
diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/merge/ResolveMerger.java b/org.eclipse.jgit/src/org/eclipse/jgit/merge/ResolveMerger.java
index 033f28031a..dc96f65b87 100644
--- a/org.eclipse.jgit/src/org/eclipse/jgit/merge/ResolveMerger.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/merge/ResolveMerger.java
@@ -1520,9 +1520,12 @@ public class ResolveMerger extends ThreeWayMerger {
 	private ContentMergeStrategy getAttributesContentMergeStrategy(
 			Attributes attributes, ContentMergeStrategy strategy) {
 		Attribute attr = attributes.get(Constants.ATTR_MERGE);
-		if (attr != null && attr.getValue()
-				.equals(Constants.ATTR_BUILTIN_UNION_MERGE_DRIVER)) {
-			return ContentMergeStrategy.UNION;
+		if (attr != null) {
+			String attrValue = attr.getValue();
+			if (attrValue != null && attrValue
+					.equals(Constants.ATTR_BUILTIN_UNION_MERGE_DRIVER)) {
+				return ContentMergeStrategy.UNION;
+			}
 		}
 		return strategy;
 	}
diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/transport/UploadPack.java b/org.eclipse.jgit/src/org/eclipse/jgit/transport/UploadPack.java
index 2270c5e5c5..6013832cac 100644
--- a/org.eclipse.jgit/src/org/eclipse/jgit/transport/UploadPack.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/transport/UploadPack.java
@@ -158,7 +158,7 @@ public class UploadPack implements Closeable {
 		 * @param implied
 		 *            the implied policy based on its bitmask.
 		 * @return true if the policy is implied.
-		 * @since 7.1
+		 * @since 6.10.1
 		 */
 		public boolean implies(RequestPolicy implied) {
 			return (bitmask & implied.bitmask) != 0;