Avoid using #refs in UploadPack#sendPack

When OPTION_INCLUDE_TAG is set, UploadPack#sendPack uses the #refs
instance variable as a source of information of tags. A subsequent patch
will need to supply this information to #sendPack without
modifying #refs, so refactor #sendPack to take in this information
through a parameter instead.

Note that prior to this patch, #refs was used twice in #sendPack: once
to generate the argument to PackWriter#setTagTargets, and once to
determine if any tags need to be included in the packfile. This patch
only updates the latter use, since the former is meant not only for
"true" tag targets but any object that should be hoisted earlier during
packing (see the documentation of PackWriter#setTagTargets).

This patch does not introduce any functionality change.

Change-Id: I70ed65a1041334abeda8d4bac98cce7cae7efcdf
Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
Signed-off-by: Jonathan Nieder <jrn@google.com>

1 files changed, 41 insertions, 9 deletions

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 393b3aa1a7..b1f4d11865 100644
--- a/org.eclipse.jgit/src/org/eclipse/jgit/transport/UploadPack.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/transport/UploadPack.java
@@ -76,6 +76,7 @@ import java.util.List;
 import java.util.Map;
 import java.util.Set;
 
+import org.eclipse.jgit.annotations.Nullable;
 import org.eclipse.jgit.errors.CorruptObjectException;
 import org.eclipse.jgit.errors.IncorrectObjectTypeException;
 import org.eclipse.jgit.errors.MissingObjectException;
@@ -866,7 +867,7 @@ public class UploadPack {
 		}
 
 		if (sendPack)
-			sendPack(accumulator);
+			sendPack(accumulator, refs == null ? null : refs.values());
 	}
 
 	private void lsRefsV2() throws IOException {
@@ -988,7 +989,8 @@ public class UploadPack {
 			if (sectionSent)
 				pckOut.writeDelim();
 			pckOut.writeString("packfile\n"); //$NON-NLS-1$
-			sendPack(new PackStatistics.Accumulator());
+			sendPack(new PackStatistics.Accumulator(),
+					refs == null ? null : refs.values());
 		}
 		pckOut.end();
 	}
@@ -1734,13 +1736,25 @@ public class UploadPack {
 		return false;
 	}
 
-	private void sendPack(PackStatistics.Accumulator accumulator)
-			throws IOException {
+	/**
+	 * Send the requested objects to the client.
+	 *
+	 * @param accumulator
+	 *                where to write statistics about the content of the pack.
+	 * @param allTags
+	 *                refs to search for annotated tags to include in the pack
+	 *                if the {@link #OPTION_INCLUDE_TAG} capability was
+	 *                requested.
+	 * @throws IOException
+	 *                if an error occured while generating or writing the pack.
+	 */
+	private void sendPack(PackStatistics.Accumulator accumulator,
+			@Nullable Collection<Ref> allTags) throws IOException {
 		final boolean sideband = options.contains(OPTION_SIDE_BAND)
 				|| options.contains(OPTION_SIDE_BAND_64K);
 		if (sideband) {
 			try {
-				sendPack(true, accumulator);
+				sendPack(true, accumulator, allTags);
 			} catch (ServiceMayNotContinueException noPack) {
 				// This was already reported on (below).
 				throw noPack;
@@ -1761,7 +1775,7 @@ public class UploadPack {
 					throw err;
 			}
 		} else {
-			sendPack(false, accumulator);
+			sendPack(false, accumulator, allTags);
 		}
 	}
 
@@ -1781,8 +1795,24 @@ public class UploadPack {
 		}
 	}
 
+	/**
+	 * Send the requested objects to the client.
+	 *
+	 * @param sideband
+	 *                whether to wrap the pack in side-band pkt-lines,
+	 *                interleaved with progress messages and errors.
+	 * @param accumulator
+	 *                where to write statistics about the content of the pack.
+	 * @param allTags
+	 *                refs to search for annotated tags to include in the pack
+	 *                if the {@link #OPTION_INCLUDE_TAG} capability was
+	 *                requested.
+	 * @throws IOException
+	 *                if an error occured while generating or writing the pack.
+	 */
 	private void sendPack(final boolean sideband,
-			PackStatistics.Accumulator accumulator) throws IOException {
+			PackStatistics.Accumulator accumulator,
+			@Nullable Collection<Ref> allTags) throws IOException {
 		ProgressMonitor pm = NullProgressMonitor.INSTANCE;
 		OutputStream packOut = rawOut;
 
@@ -1842,6 +1872,8 @@ public class UploadPack {
 			pw.setThin(options.contains(OPTION_THIN_PACK));
 			pw.setReuseValidatingObjects(false);
 
+			// Objects named directly by references go at the beginning
+			// of the pack.
 			if (commonBase.isEmpty() && refs != null) {
 				Set<ObjectId> tagTargets = new HashSet<>();
 				for (Ref ref : refs.values()) {
@@ -1872,8 +1904,8 @@ public class UploadPack {
 				rw = ow;
 			}
 
-			if (options.contains(OPTION_INCLUDE_TAG) && refs != null) {
-				for (Ref ref : refs.values()) {
+			if (options.contains(OPTION_INCLUDE_TAG) && allTags != null) {
+				for (Ref ref : allTags) {
 					ObjectId objectId = ref.getObjectId();
 					if (objectId == null) {
 						// skip unborn branch