Ensure ObjectReader used by PackWriter is released

The ObjectReader API demands that we release the reader when we are
done with it.  PackWriter contains a reader, which it uses for the
entire packing session.  Expose the release of the reader through
a release method on the writer.

This still doesn't address the RevWalk and TreeWalk users, who
don't correctly release their reader.  But its a small step in the
right direction.

Change-Id: I5cb0b5c1b432434a799fceb21b86479e09b84a0a
Signed-off-by: Shawn O. Pearce <spearce@spearce.org>

org.eclipse.jgit.junit/src/org/eclipse/jgit/junit/TestRepository.java

@@ -600,33 +600,38 @@ public class TestRepository<R extends Repository> {
 	public void packAndPrune() throws Exception {
 		if (db.getObjectDatabase() instanceof ObjectDirectory) {
 			ObjectDirectory odb = (ObjectDirectory) db.getObjectDatabase();
-			PackWriter pw = new PackWriter(db, NullProgressMonitor.INSTANCE);
 
-			Set<ObjectId> all = new HashSet<ObjectId>();
-			for (Ref r : db.getAllRefs().values())
-				all.add(r.getObjectId());
-			pw.preparePack(all, Collections.<ObjectId> emptySet());
+			final File pack, idx;
+			PackWriter pw = new PackWriter(db, NullProgressMonitor.INSTANCE);
+			try {
+				Set<ObjectId> all = new HashSet<ObjectId>();
+				for (Ref r : db.getAllRefs().values())
+					all.add(r.getObjectId());
+				pw.preparePack(all, Collections.<ObjectId> emptySet());
 
-			final ObjectId name = pw.computeName();
-			OutputStream out;
+				final ObjectId name = pw.computeName();
+				OutputStream out;
 
-			final File pack = nameFor(odb, name, ".pack");
-			out = new BufferedOutputStream(new FileOutputStream(pack));
-			try {
-				pw.writePack(out);
-			} finally {
-				out.close();
-			}
-			pack.setReadOnly();
+				pack = nameFor(odb, name, ".pack");
+				out = new BufferedOutputStream(new FileOutputStream(pack));
+				try {
+					pw.writePack(out);
+				} finally {
+					out.close();
+				}
+				pack.setReadOnly();
 
-			final File idx = nameFor(odb, name, ".idx");
-			out = new BufferedOutputStream(new FileOutputStream(idx));
-			try {
-				pw.writeIndex(out);
+				idx = nameFor(odb, name, ".idx");
+				out = new BufferedOutputStream(new FileOutputStream(idx));
+				try {
+					pw.writeIndex(out);
+				} finally {
+					out.close();
+				}
+				idx.setReadOnly();
 			} finally {
-				out.close();
+				pw.release();
 			}
-			idx.setReadOnly();
 
 			odb.openPack(pack, idx);
 			updateServerInfo();

org.eclipse.jgit.test/tst/org/eclipse/jgit/storage/file/ConcurrentRepackTest.java

@@ -138,6 +138,7 @@ public class ConcurrentRepackTest extends RepositoryTestCase {
 		pw.addObject(o2);
 		pw.addObject(o1);
 		write(out1, pw);
+		pw.release();
 
 		// Try the old name, then the new name. The old name should cause the
 		// pack to reload when it opens and the index and pack mismatch.
@@ -208,6 +209,7 @@ public class ConcurrentRepackTest extends RepositoryTestCase {
 		final File idxFile = fullPackFileName(name, ".idx");
 		final File[] files = new File[] { packFile, idxFile };
 		write(files, pw);
+		pw.release();
 		return files;
 	}
 

org.eclipse.jgit.test/tst/org/eclipse/jgit/storage/file/PackWriterTest.java

@@ -484,6 +484,7 @@ public class PackWriterTest extends SampleDataRepositoryTestCase {
 		writer.setIgnoreMissingUninteresting(ignoreMissingUninteresting);
 		writer.preparePack(interestings, uninterestings);
 		writer.writePack(os);
+		writer.release();
 		verifyOpenPack(thin);
 	}
 
@@ -491,6 +492,7 @@ public class PackWriterTest extends SampleDataRepositoryTestCase {
 			throws MissingObjectException, IOException {
 		writer.preparePack(objectSource);
 		writer.writePack(os);
+		writer.release();
 		verifyOpenPack(false);
 	}
 

org.eclipse.jgit/src/org/eclipse/jgit/storage/pack/PackWriter.java

@@ -610,22 +610,24 @@ public class PackWriter {
 	 *             stream.
 	 */
 	public void writePack(OutputStream packStream) throws IOException {
-		try {
-			if ((reuseDeltas || reuseObjects) && reuseSupport != null)
-				searchForReuse();
-
-			out = new PackOutputStream(packStream, isDeltaBaseAsOffset());
-
-			int cnt = getObjectsNumber();
-			writeMonitor.beginTask(WRITING_OBJECTS_PROGRESS, cnt);
-			out.writeFileHeader(PACK_VERSION_GENERATED, cnt);
-			writeObjects();
-			writeChecksum();
-			writeMonitor.endTask();
-		} finally {
-			out = null;
-			reader.release();
-		}
+		if ((reuseDeltas || reuseObjects) && reuseSupport != null)
+			searchForReuse();
+
+		out = new PackOutputStream(packStream, isDeltaBaseAsOffset());
+
+		writeMonitor.beginTask(WRITING_OBJECTS_PROGRESS, getObjectsNumber());
+		out.writeFileHeader(PACK_VERSION_GENERATED, getObjectsNumber());
+		writeObjects();
+		writeChecksum();
+
+		out = null;
+		reader.release();
+		writeMonitor.endTask();
+	}
+
+	/** Release all resources used by this writer. */
+	public void release() {
+		reader.release();
 	}
 
 	private void searchForReuse() throws IOException {

org.eclipse.jgit/src/org/eclipse/jgit/transport/BasePackPushConnection.java

@@ -48,6 +48,7 @@ import java.io.IOException;
 import java.text.MessageFormat;
 import java.util.ArrayList;
 import java.util.Collection;
+import java.util.List;
 import java.util.Map;
 
 import org.eclipse.jgit.JGitText;
@@ -226,25 +227,29 @@ class BasePackPushConnection extends BasePackConnection implements
 
 	private void writePack(final Map<String, RemoteRefUpdate> refUpdates,
 			final ProgressMonitor monitor) throws IOException {
+		List<ObjectId> remoteObjects = new ArrayList<ObjectId>(getRefs().size());
+		List<ObjectId> newObjects = new ArrayList<ObjectId>(refUpdates.size());
+
+		final long start;
 		final PackWriter writer = new PackWriter(local, monitor);
-		final ArrayList<ObjectId> remoteObjects = new ArrayList<ObjectId>(
-				getRefs().size());
-		final ArrayList<ObjectId> newObjects = new ArrayList<ObjectId>(
-				refUpdates.size());
-
-		for (final Ref r : getRefs())
-			remoteObjects.add(r.getObjectId());
-		remoteObjects.addAll(additionalHaves);
-		for (final RemoteRefUpdate r : refUpdates.values()) {
-			if (!ObjectId.zeroId().equals(r.getNewObjectId()))
-				newObjects.add(r.getNewObjectId());
-		}
+		try {
 
-		writer.setThin(thinPack);
-		writer.setDeltaBaseAsOffset(capableOfsDelta);
-		writer.preparePack(newObjects, remoteObjects);
-		final long start = System.currentTimeMillis();
-		writer.writePack(out);
+			for (final Ref r : getRefs())
+				remoteObjects.add(r.getObjectId());
+			remoteObjects.addAll(additionalHaves);
+			for (final RemoteRefUpdate r : refUpdates.values()) {
+				if (!ObjectId.zeroId().equals(r.getNewObjectId()))
+					newObjects.add(r.getNewObjectId());
+			}
+
+			writer.setThin(thinPack);
+			writer.setDeltaBaseAsOffset(capableOfsDelta);
+			writer.preparePack(newObjects, remoteObjects);
+			start = System.currentTimeMillis();
+			writer.writePack(out);
+		} finally {
+			writer.release();
+		}
 		out.flush();
 		packTransferTime = System.currentTimeMillis() - start;
 	}

org.eclipse.jgit/src/org/eclipse/jgit/transport/BundleWriter.java

@@ -165,37 +165,41 @@ public class BundleWriter {
 	 *             stream.
 	 */
 	public void writeBundle(OutputStream os) throws IOException {
-		final HashSet<ObjectId> inc = new HashSet<ObjectId>();
-		final HashSet<ObjectId> exc = new HashSet<ObjectId>();
-		inc.addAll(include.values());
-		for (final RevCommit r : assume)
-			exc.add(r.getId());
-		packWriter.setThin(exc.size() > 0);
-		packWriter.preparePack(inc, exc);
+		try {
+			final HashSet<ObjectId> inc = new HashSet<ObjectId>();
+			final HashSet<ObjectId> exc = new HashSet<ObjectId>();
+			inc.addAll(include.values());
+			for (final RevCommit r : assume)
+				exc.add(r.getId());
+			packWriter.setThin(exc.size() > 0);
+			packWriter.preparePack(inc, exc);
 
-		final Writer w = new OutputStreamWriter(os, Constants.CHARSET);
-		w.write(TransportBundle.V2_BUNDLE_SIGNATURE);
-		w.write('\n');
+			final Writer w = new OutputStreamWriter(os, Constants.CHARSET);
+			w.write(TransportBundle.V2_BUNDLE_SIGNATURE);
+			w.write('\n');
 
-		final char[] tmp = new char[Constants.OBJECT_ID_STRING_LENGTH];
-		for (final RevCommit a : assume) {
-			w.write('-');
-			a.copyTo(tmp, w);
-			if (a.getRawBuffer() != null) {
+			final char[] tmp = new char[Constants.OBJECT_ID_STRING_LENGTH];
+			for (final RevCommit a : assume) {
+				w.write('-');
+				a.copyTo(tmp, w);
+				if (a.getRawBuffer() != null) {
+					w.write(' ');
+					w.write(a.getShortMessage());
+				}
+				w.write('\n');
+			}
+			for (final Map.Entry<String, ObjectId> e : include.entrySet()) {
+				e.getValue().copyTo(tmp, w);
 				w.write(' ');
-				w.write(a.getShortMessage());
+				w.write(e.getKey());
+				w.write('\n');
 			}
+
 			w.write('\n');
+			w.flush();
+			packWriter.writePack(os);
+		} finally {
+			packWriter.release();
 		}
-		for (final Map.Entry<String, ObjectId> e : include.entrySet()) {
-			e.getValue().copyTo(tmp, w);
-			w.write(' ');
-			w.write(e.getKey());
-			w.write('\n');
-		}
-
-		w.write('\n');
-		w.flush();
-		packWriter.writePack(os);
 	}
 }

org.eclipse.jgit/src/org/eclipse/jgit/transport/UploadPack.java

@@ -569,25 +569,29 @@ public class UploadPack {
 
 		final PackWriter pw;
 		pw = new PackWriter(db, pm, NullProgressMonitor.INSTANCE);
-		pw.setDeltaBaseAsOffset(options.contains(OPTION_OFS_DELTA));
-		pw.setThin(thin);
-		pw.preparePack(wantAll, commonBase);
-		if (options.contains(OPTION_INCLUDE_TAG)) {
-			for (final Ref r : refs.values()) {
-				final RevObject o;
-				try {
-					o = walk.parseAny(r.getObjectId());
-				} catch (IOException e) {
-					continue;
+		try {
+			pw.setDeltaBaseAsOffset(options.contains(OPTION_OFS_DELTA));
+			pw.setThin(thin);
+			pw.preparePack(wantAll, commonBase);
+			if (options.contains(OPTION_INCLUDE_TAG)) {
+				for (final Ref r : refs.values()) {
+					final RevObject o;
+					try {
+						o = walk.parseAny(r.getObjectId());
+					} catch (IOException e) {
+						continue;
+					}
+					if (o.has(WANT) || !(o instanceof RevTag))
+						continue;
+					final RevTag t = (RevTag) o;
+					if (!pw.willInclude(t) && pw.willInclude(t.getObject()))
+						pw.addObject(t);
 				}
-				if (o.has(WANT) || !(o instanceof RevTag))
-					continue;
-				final RevTag t = (RevTag) o;
-				if (!pw.willInclude(t) && pw.willInclude(t.getObject()))
-					pw.addObject(t);
 			}
+			pw.writePack(packOut);
+		} finally {
+			pw.release();
 		}
-		pw.writePack(packOut);
 		packOut.flush();
 
 		if (sideband)

org.eclipse.jgit/src/org/eclipse/jgit/transport/WalkPushConnection.java

@@ -209,8 +209,8 @@ class WalkPushConnection extends BaseConnection implements PushConnection {
 		String pathPack = null;
 		String pathIdx = null;
 
+		final PackWriter pw = new PackWriter(local, monitor);
 		try {
-			final PackWriter pw = new PackWriter(local, monitor);
 			final List<ObjectId> need = new ArrayList<ObjectId>();
 			final List<ObjectId> have = new ArrayList<ObjectId>();
 			for (final RemoteRefUpdate r : updates)
@@ -281,6 +281,8 @@ class WalkPushConnection extends BaseConnection implements PushConnection {
 			safeDelete(pathPack);
 
 			throw new TransportException(uri, JGitText.get().cannotStoreObjects, err);
+		} finally {
+			pw.release();
 		}
 	}