WalkFetchConnection: Remove marked packs on all function exits

[1] replaces Iterator.remove() with a list of "toRemove" that gets
processed when returning at the end. There are two others returns in
the function where the list is not processed.

Let the method report the broken packages and wrap it so the caller
can clean them up in any case.

[1] https://review.gerrithub.io/c/eclipse-jgit/jgit/+/1194812

Change-Id: I1ffb7829039f644df03b0b3ea1e5d10408ce19b7

1 files changed, 15 insertions, 6 deletions

diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/transport/WalkFetchConnection.java b/org.eclipse.jgit/src/org/eclipse/jgit/transport/WalkFetchConnection.java
index ea789b1c09..b7bb0cbce3 100644
--- a/org.eclipse.jgit/src/org/eclipse/jgit/transport/WalkFetchConnection.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/transport/WalkFetchConnection.java
@@ -424,8 +424,9 @@ class WalkFetchConnection extends BaseFetchConnection {
 				if (packNameList == null || packNameList.isEmpty())
 					continue;
 				for (String packName : packNameList) {
-					if (packsConsidered.add(packName))
+					if (packsConsidered.add(packName)) {
 						unfetchedPacks.put(packName, new RemotePack(wrr, packName));
+					}
 				}
 				if (downloadPackedObject(pm, id))
 					return;
@@ -468,13 +469,22 @@ class WalkFetchConnection extends BaseFetchConnection {
 		}
 	}
 
+	private boolean downloadPackedObject(ProgressMonitor monitor,
+			AnyObjectId id) throws TransportException {
+		Set<String> brokenPacks = new HashSet<>();
+		try {
+			return downloadPackedObject(monitor, id, brokenPacks);
+		} finally {
+			brokenPacks.forEach(unfetchedPacks::remove);
+		}
+	}
+
 	@SuppressWarnings("Finally")
 	private boolean downloadPackedObject(final ProgressMonitor monitor,
-			final AnyObjectId id) throws TransportException {
+			final AnyObjectId id, Set<String> brokenPacks) throws TransportException {
 		// Search for the object in a remote pack whose index we have,
 		// but whose pack we do not yet have.
 		//
-		Set<String> toRemove = new HashSet<>();
 		for (Entry<String, RemotePack> entry : unfetchedPacks.entrySet()) {
 			if (monitor.isCancelled()) {
 				break;
@@ -489,7 +499,7 @@ class WalkFetchConnection extends BaseFetchConnection {
 				// another source, so don't consider it a failure.
 				//
 				recordError(id, err);
-				toRemove.add(entry.getKey());
+				brokenPacks.add(entry.getKey());
 				continue;
 			}
 
@@ -540,7 +550,7 @@ class WalkFetchConnection extends BaseFetchConnection {
 					}
 					throw new TransportException(e.getMessage(), e);
 				}
-				toRemove.add(entry.getKey());
+				brokenPacks.add(entry.getKey());
 			}
 
 			if (!alreadyHave(id)) {
@@ -566,7 +576,6 @@ class WalkFetchConnection extends BaseFetchConnection {
 			return true;
 
 		}
-		toRemove.forEach(unfetchedPacks::remove);
 		return false;
 	}