Merge branch 'stable-7.3'HEADmaster

* stable-7.3:
  Shortcut PackWriter reuse selection when possible
  Don't use Yoda style conditions to improve readability

Change-Id: Ic8ca92cfc294ca01540218151bafca889256847b

4 files changed, 128 insertions, 22 deletions

diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/ObjectDirectory.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/ObjectDirectory.java
index dc25bfde40..af1671d442 100644
--- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/ObjectDirectory.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/ObjectDirectory.java
@@ -446,12 +446,12 @@ public class ObjectDirectory extends FileObjectDatabase {
 			AnyObjectId id) throws IOException {
 		if (loose.hasCached(id)) {
 			long len = loose.getSize(curs, id);
-			if (0 <= len) {
+			if (len >= 0) {
 				return len;
 			}
 		}
 		long len = getPackedSizeFromSelfOrAlternate(curs, id, null);
-		if (0 <= len) {
+		if (len >= 0) {
 			return len;
 		}
 		return getLooseSizeFromSelfOrAlternate(curs, id, null);
@@ -461,14 +461,14 @@ public class ObjectDirectory extends FileObjectDatabase {
 			AnyObjectId id, Set<AlternateHandle.Id> skips)
 			throws PackMismatchException {
 		long len = packed.getSize(curs, id);
-		if (0 <= len) {
+		if (len >= 0) {
 			return len;
 		}
 		skips = addMe(skips);
 		for (AlternateHandle alt : myAlternates()) {
 			if (!skips.contains(alt.getId())) {
 				len = alt.db.getPackedSizeFromSelfOrAlternate(curs, id, skips);
-				if (0 <= len) {
+				if (len >= 0) {
 					return len;
 				}
 			}
@@ -479,14 +479,14 @@ public class ObjectDirectory extends FileObjectDatabase {
 	private long getLooseSizeFromSelfOrAlternate(WindowCursor curs,
 			AnyObjectId id, Set<AlternateHandle.Id> skips) throws IOException {
 		long len = loose.getSize(curs, id);
-		if (0 <= len) {
+		if (len >= 0) {
 			return len;
 		}
 		skips = addMe(skips);
 		for (AlternateHandle alt : myAlternates()) {
 			if (!skips.contains(alt.getId())) {
 				len = alt.db.getLooseSizeFromSelfOrAlternate(curs, id, skips);
-				if (0 <= len) {
+				if (len >= 0) {
 					return len;
 				}
 			}
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 f50c17eafa..544961b936 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
@@ -27,6 +27,7 @@ import java.util.Collections;
 import java.util.EnumMap;
 import java.util.HashMap;
 import java.util.HashSet;
+import java.util.Iterator;
 import java.util.List;
 import java.util.Map;
 import java.util.Set;
@@ -49,6 +50,7 @@ import org.eclipse.jgit.lib.CoreConfig.TrustStat;
 import org.eclipse.jgit.lib.ObjectId;
 import org.eclipse.jgit.lib.ObjectLoader;
 import org.eclipse.jgit.util.FileUtils;
+import org.eclipse.jgit.util.Iterators;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
@@ -275,12 +277,14 @@ class PackDirectory {
 		PackList pList = packList.get();
 		int retries = 0;
 		SEARCH: for (;;) {
-			for (Pack p : pList.packs) {
+			for (Pack p : pList.inReverse()) {
 				try {
 					LocalObjectRepresentation rep = p.representation(curs, otp);
 					p.resetTransientErrorCount();
 					if (rep != null) {
-						packer.select(otp, rep);
+						if (!packer.select(otp, rep)) {
+							return;
+						}
 						packer.checkSearchForReuseTimeout();
 					}
 				} catch (SearchForReuseTimeout e) {
@@ -584,5 +588,13 @@ class PackDirectory {
 			this.snapshot = monitor;
 			this.packs = packs;
 		}
+
+		Iterable<Pack> inReverse() {
+			return Iterators.iterable(reverseIterator());
+		}
+
+		Iterator<Pack> reverseIterator() {
+			return Iterators.reverseIterator(packs);
+		}
 	}
 }
diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/pack/PackWriter.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/pack/PackWriter.java
index f025d4e3d5..27fb814f64 100644
--- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/pack/PackWriter.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/pack/PackWriter.java
@@ -2394,17 +2394,46 @@ public class PackWriter implements AutoCloseable {
 	 * this method once for each representation available for an object, to
 	 * allow the writer to find the most suitable one for the output.
 	 *
+	 * This method tries to take a very simple approach to avoiding delta chains
+	 * during object reuse by selecting from the oldest pack that contains them.
+	 * This helps select many objects from the same pack which helps make good
+	 * use of any WindowCache caching, and it helps prevent cycles because a
+	 * pack must not have a cycle in the delta chain. If both objects A and B
+	 * are chosen out of the same source pack then there cannot be an A->B->A
+	 * cycle.
+	 *
+	 * The oldest pack is also the most likely to have the smallest deltas. It
+	 * generally is the biggest pack in the system and probably came from the
+	 * clone (or last GC) of this repository, where all objects were previously
+	 * considered and packed tightly together. If an object appears again (for
+	 * example due to a revert and a push into this repository) the newer copy
+	 * won't be nearly as small as the older delta version of it, even if the
+	 * newer one is also itself a delta.
+	 *
+	 * Thus this method is optimized for being called in an order that presumes
+	 * that earlier representations are better than later ones, and it expects
+	 * representations from older pack files to be tested first, and it will
+	 * shortcut any searching once it is satisfied with the selected
+	 * representation. Perhaps ideally representation testing ordering should be
+	 * based on packfile object count instead of age since file age can be
+	 * altered, and be deceiving for other reasons. Perhaps the presence of a
+	 * bitmap file for a pack file should prioritize it to be tested even
+	 * earlier than object count?
+	 *
 	 * @param otp
 	 *            the object being packed.
 	 * @param next
 	 *            the next available representation from the repository.
+	 * @return whether the search should continue in the hopes of finding a
+	 *         better representation
 	 */
-	public void select(ObjectToPack otp, StoredObjectRepresentation next) {
+	public boolean select(ObjectToPack otp, StoredObjectRepresentation next) {
 		int nFmt = next.getFormat();
 
 		if (!cachedPacks.isEmpty()) {
-			if (otp.isEdge())
-				return;
+			if (otp.isEdge()) {
+				return false;
+			}
 			if (nFmt == PACK_WHOLE || nFmt == PACK_DELTA) {
 				for (CachedPack pack : cachedPacks) {
 					if (pack.hasObject(otp, next)) {
@@ -2412,7 +2441,7 @@ public class PackWriter implements AutoCloseable {
 						otp.clearDeltaBase();
 						otp.clearReuseAsIs();
 						pruneCurrentObjectList = true;
-						return;
+						return false;
 					}
 				}
 			}
@@ -2422,23 +2451,22 @@ public class PackWriter implements AutoCloseable {
 			ObjectId baseId = next.getDeltaBase();
 			ObjectToPack ptr = objectsMap.get(baseId);
 			if (ptr != null && !ptr.isEdge()) {
-				otp.setDeltaBase(ptr);
-				otp.setReuseAsIs();
-			} else if (thin && have(ptr, baseId)) {
-				otp.setDeltaBase(baseId);
-				otp.setReuseAsIs();
-			} else {
-				otp.clearDeltaBase();
-				otp.clearReuseAsIs();
+				return useDelta(otp, next, ptr);
+			}
+			if (thin && have(ptr, baseId)) {
+				return useDelta(otp, next, baseId);
 			}
+			otp.clearDeltaBase();
+			otp.clearReuseAsIs();
 		} else if (nFmt == PACK_WHOLE && config.isReuseObjects()) {
 			int nWeight = next.getWeight();
 			if (otp.isReuseAsIs() && !otp.isDeltaRepresentation()) {
 				// We've chosen another PACK_WHOLE format for this object,
 				// choose the one that has the smaller compressed size.
 				//
-				if (otp.getWeight() <= nWeight)
-					return;
+				if (otp.getWeight() < nWeight) {
+					return true;
+				}
 			}
 			otp.clearDeltaBase();
 			otp.setReuseAsIs();
@@ -2450,6 +2478,15 @@ public class PackWriter implements AutoCloseable {
 
 		otp.setDeltaAttempted(reuseDeltas && next.wasDeltaAttempted());
 		otp.select(next);
+		return true;
+	}
+
+	private boolean useDelta(ObjectToPack otp, StoredObjectRepresentation rep, ObjectId base) {
+		otp.setDeltaBase(base);
+		otp.setReuseAsIs();
+		otp.setDeltaAttempted(reuseDeltas && rep.wasDeltaAttempted());
+		otp.select(rep);
+		return false;
 	}
 
 	private final boolean have(ObjectToPack ptr, AnyObjectId objectId) {
diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/util/Iterators.java b/org.eclipse.jgit/src/org/eclipse/jgit/util/Iterators.java
new file mode 100644
index 0000000000..74b728bdf7
--- /dev/null
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/util/Iterators.java
@@ -0,0 +1,57 @@
+/*
+ * Copyright (C) 2025, NVIDIA Corporation.
+ *
+ * This program and the accompanying materials are made available under the
+ * terms of the Eclipse Distribution License v. 1.0 which is available at
+ * https://www.eclipse.org/org/documents/edl-v10.php.
+ *
+ * SPDX-License-Identifier: BSD-3-Clause
+ */
+
+package org.eclipse.jgit.util;
+
+import java.util.Iterator;
+
+/**
+ * Utility class for Iterators
+ *
+ * @since 6.10.2
+ */
+public class Iterators {
+	/**
+	 * Create an iterator which traverses an array in reverse.
+	 *
+	 * @param array T[]
+	 * @return Iterator<T>
+	 */
+	public static <T> Iterator<T> reverseIterator(T[] array) {
+		return new Iterator<>() {
+			int index = array.length;
+
+			@Override
+			public boolean hasNext() {
+				return index > 0;
+			}
+
+			@Override
+			public T next() {
+				return array[--index];
+			}
+		};
+	}
+
+	/**
+	 * Make an iterable for easy use in modern for loops.
+	 *
+	 * @param iterator Iterator<T>
+	 * @return Iterable<T>
+	 */
+	public static <T> Iterable<T> iterable(Iterator<T> iterator) {
+		return new Iterable<>() {
+			@Override
+			public Iterator<T> iterator() {
+				return iterator;
+			}
+		};
+	}
+}