From ffd1ac5dde053e861ad249d7d8d27df7cef56eb9 Mon Sep 17 00:00:00 2001 From: Thomas Wolf Date: Sun, 19 Aug 2018 20:43:50 +0200 Subject: [PATCH] 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 --- .../eclipse/jgit/api/FetchCommandTest.java | 29 +++++++++++++++++++ .../eclipse/jgit/transport/FetchProcess.java | 28 ++++++++++++++---- 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 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 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 { -- 2.39.5