Fix fetching with duplicate ref updates

If packed refs are used, duplicate updates result in an exception
because JGit tries to lock the same lock file twice. With non-atomic
ref updates, this used to work, since the same ref would simply be
locked and updated twice in succession.

Let's be more lenient in this case and remove duplicates before
trying to do the ref updates. Silently skip duplicate updates
for the same ref, if they both would update the ref to the same
object ID. (If they don't, behavior is undefined anyway, and we
still throw an exception.)

Add a test that results in a duplicate ref update for a tag.

Bug: 529400
Change-Id: Ide97f20b219646ac24c22e28de0c194a29cb62a5
Signed-off-by: Thomas Wolf <thomas.wolf@paranor.ch>

2 files changed, 52 insertions, 5 deletions

diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/api/FetchCommandTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/api/FetchCommandTest.java
index 0622851822..530fb1b2fe 100644
--- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/api/FetchCommandTest.java
+++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/api/FetchCommandTest.java
@@ -45,7 +45,9 @@ package org.eclipse.jgit.api;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertNull;
 
+import java.util.ArrayList;
 import java.util.Collection;
+import java.util.List;
 
 import org.eclipse.jgit.junit.RepositoryTestCase;
 import org.eclipse.jgit.lib.Constants;
@@ -315,4 +317,31 @@ public class FetchCommandTest extends RepositoryTestCase {
 				.setRemoveDeletedRefs(true).call();
 		assertNull(db.resolve(remoteBranchName));
 	}
+
+	@Test
+	public void fetchUpdateRefsWithDuplicateRefspec() throws Exception {
+		final String tagName = "foo";
+		remoteGit.commit().setMessage("commit").call();
+		Ref tagRef1 = remoteGit.tag().setName(tagName).call();
+		List<RefSpec> refSpecs = new ArrayList<>();
+		refSpecs.add(new RefSpec("+refs/heads/*:refs/remotes/origin/*"));
+		refSpecs.add(new RefSpec("+refs/tags/*:refs/tags/*"));
+		// Updating tags via the RefSpecs and setting TagOpt.FETCH_TAGS (or
+		// AUTO_FOLLOW) will result internally in *two* updates for the same
+		// ref.
+		git.fetch().setRemote("test").setRefSpecs(refSpecs)
+				.setTagOpt(TagOpt.AUTO_FOLLOW).call();
+		assertEquals(tagRef1.getObjectId(), db.resolve(tagName));
+
+		remoteGit.commit().setMessage("commit 2").call();
+		Ref tagRef2 = remoteGit.tag().setName(tagName).setForceUpdate(true)
+				.call();
+		FetchResult result = git.fetch().setRemote("test").setRefSpecs(refSpecs)
+				.setTagOpt(TagOpt.FETCH_TAGS).call();
+		assertEquals(2, result.getTrackingRefUpdates().size());
+		TrackingRefUpdate update = result
+				.getTrackingRefUpdate(Constants.R_TAGS + tagName);
+		assertEquals(RefUpdate.Result.FORCED, update.getResult());
+		assertEquals(tagRef2.getObjectId(), db.resolve(tagName));
+	}
 }
diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/transport/FetchProcess.java b/org.eclipse.jgit/src/org/eclipse/jgit/transport/FetchProcess.java
index 39f0eea274..ed10f449df 100644
--- a/org.eclipse.jgit/src/org/eclipse/jgit/transport/FetchProcess.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/transport/FetchProcess.java
@@ -203,12 +203,10 @@ class FetchProcess {
 				((BatchingProgressMonitor) monitor).setDelayStart(
 						250, TimeUnit.MILLISECONDS);
 			}
-			if (transport.isRemoveDeletedRefs())
+			if (transport.isRemoveDeletedRefs()) {
 				deleteStaleTrackingRefs(result, batch);
-			for (TrackingRefUpdate u : localUpdates) {
-				result.add(u);
-				batch.addCommand(u.asReceiveCommand());
 			}
+			addUpdateBatchCommands(result, batch);
 			for (ReceiveCommand cmd : batch.getCommands()) {
 				cmd.updateType(walk);
 				if (cmd.getType() == UPDATE_NONFASTFORWARD
@@ -221,8 +219,11 @@ class FetchProcess {
 					if (cmd.getResult() == NOT_ATTEMPTED)
 						cmd.setResult(OK);
 				}
-			} else
+			} else {
 				batch.execute(walk, monitor);
+			}
+		} catch (TransportException e) {
+			throw e;
 		} catch (IOException err) {
 			throw new TransportException(MessageFormat.format(
 					JGitText.get().failureUpdatingTrackingRef,
@@ -239,6 +240,23 @@ class FetchProcess {
 		}
 	}
 
+	private void addUpdateBatchCommands(FetchResult result,
+			BatchRefUpdate batch) throws TransportException {
+		Map<String, ObjectId> refs = new HashMap<>();
+		for (TrackingRefUpdate u : localUpdates) {
+			// Try to skip duplicates if they'd update to the same object ID
+			ObjectId existing = refs.get(u.getLocalName());
+			if (existing == null) {
+				refs.put(u.getLocalName(), u.getNewObjectId());
+				result.add(u);
+				batch.addCommand(u.asReceiveCommand());
+			} else if (!existing.equals(u.getNewObjectId())) {
+				throw new TransportException(MessageFormat
+						.format(JGitText.get().duplicateRef, u.getLocalName()));
+			}
+		}
+	}
+
 	private void fetchObjects(final ProgressMonitor monitor)
 			throws TransportException {
 		try {