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

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 ea789b1c09b1ebd84cedda21a911951b9a251302..b7bb0cbce31dc6d17714c3c94e9f7a4abe1fb0b2 100644 (file)
--- 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;
        }