FetchProcess: Report lock failure when destination ref changes Snapshot local refs before opening the fetch connection so `TrackingRefUpdate`s use the value populated when the fetch starts as their expected old object ID. Before this change, local refs were loaded lazily while processing "wants". If another process changed or created the destination ref after the fetch started but before that lazy read, JGit used the newer local value as the expected old object ID. The final ref update could then succeed and overwrite the concurrent local change. Load the local ref snapshot before the remote fetch connection is opened. Concurrent destination ref changes now make the expected old object ID differ from the current ref value, causing the update to report LOCK_FAILURE instead of clobbering the newer local ref. The tests trigger the race condition by using TestProtocol.registerProto(...) with a custom UploadPackFactory. The factory updates the local destination ref immediately before returning the UploadPack, which places the concurrent local change after FetchProcess starts but before it finishes building the TrackingRefUpdate. Bug: jgit-252 Change-Id: Ib46363251862a7018c60bd4c36f003d267f354d5
diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/transport/TestProtocolTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/transport/TestProtocolTest.java index 657a93b..29d080b 100644 --- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/transport/TestProtocolTest.java +++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/transport/TestProtocolTest.java
@@ -32,6 +32,7 @@ import org.eclipse.jgit.lib.RefUpdate; import org.eclipse.jgit.lib.Repository; import org.eclipse.jgit.revwalk.RevCommit; +import org.eclipse.jgit.revwalk.RevTag; import org.eclipse.jgit.revwalk.RevWalk; import org.eclipse.jgit.storage.pack.PackStatistics; import org.eclipse.jgit.transport.BasePackFetchConnection.FetchConfig; @@ -117,6 +118,106 @@ public void testFetch() throws Exception { } @Test + public void fetchReportsLockFailureWhenAutoFollowTagIsCreated() + throws Exception { + String tagName = "foo"; + String tagRef = "refs/tags/" + tagName; + String remoteBranchName = "remotefoo"; + String forcedRemoteRef = "+refs/heads/" + remoteBranchName; + + RevCommit localCommit = local.commit().add("local.txt", "new").create(); + RevCommit remoteCommit = remote.branch(remoteBranchName).commit() + .add("remote.txt", "remote").create(); + RevTag remoteTag = remote.tag("foo", remoteCommit); + remote.update(tagRef, remoteTag); + + URIish uri = registerTestProtocolUpdatingRef((repo) -> repo.lightweightTag(tagName, localCommit)); + + + try (Git git = new Git(local.getRepository())) { + FetchResult result = git.fetch().setRemote(uri.toString()) + .setRefSpecs(new RefSpec(forcedRemoteRef + + ":refs/remotes/origin/" + remoteBranchName)) + .setTagOpt(TagOpt.AUTO_FOLLOW).call(); + + TrackingRefUpdate update = result.getTrackingRefUpdate(tagRef); + assertEquals(RefUpdate.Result.LOCK_FAILURE, update.getResult()); + // Tag hasn't been clobbered... + assertEquals(localCommit, + local.getRepository().exactRef(tagRef).getObjectId()); + // ...however the object has been downloaded + assertTrue(local.getRepository().getObjectDatabase() + .has(remoteCommit)); + } + } + + @Test + public void fetchReportsLockFailureWhenDestinationRefChanges() + throws Exception { + String localBranchName = "localfoo"; + String localRef = "refs/heads/" + localBranchName; + String remoteBranchName = "remotefoo"; + String forcedRemoteRef = "+refs/heads/" + remoteBranchName; + + RevCommit oldLocalCommit = local.branch(localBranchName).commit() + .add("local.txt", "old").create(); + RevCommit newLocalCommit = local.commit().parent(oldLocalCommit) + .add("local.txt", "new").create(); + RevCommit remoteCommit = remote.branch(remoteBranchName).commit() + .add("remote.txt", "remote").create(); + + URIish uri = registerTestProtocolUpdatingRef((repo) -> repo.update(localBranchName, newLocalCommit)); + + try (Git git = new Git(local.getRepository())) { + FetchResult result = git.fetch().setRemote(uri.toString()) + .setRefSpecs( + new RefSpec(forcedRemoteRef + ":" + localRef)) + .call(); + + TrackingRefUpdate update = result.getTrackingRefUpdate(localRef); + assertEquals(RefUpdate.Result.LOCK_FAILURE, update.getResult()); + // Ref hasn't been clobbered... + assertEquals(newLocalCommit, + local.getRepository().exactRef(localRef).getObjectId()); + // ...however the object has been downloaded + assertTrue(local.getRepository().getObjectDatabase() + .has(remoteCommit)); + } + } + + @Test + public void fetchReportsLockFailureWhenDestinationRefIsCreated() + throws Exception { + String localBranchName = "localfoo"; + String localRef = "refs/heads/" + localBranchName; + String remoteBranchName = "remotefoo"; + String forcedRemoteRef = "+refs/heads/" + remoteBranchName; + + RevCommit newLocalCommit = local.commit().add("local.txt", "new") + .create(); + RevCommit remoteCommit = remote.branch(remoteBranchName).commit() + .add("remote.txt", "remote").create(); + + URIish uri = registerTestProtocolUpdatingRef((repo) -> repo.update(localBranchName, newLocalCommit)); + + try (Git git = new Git(local.getRepository())) { + FetchResult result = git.fetch().setRemote(uri.toString()) + .setRefSpecs( + new RefSpec(forcedRemoteRef + ":" + localRef)) + .call(); + + TrackingRefUpdate update = result.getTrackingRefUpdate(localRef); + assertEquals(RefUpdate.Result.LOCK_FAILURE, update.getResult()); + // Ref hasn't been clobbered... + assertEquals(newLocalCommit, + local.getRepository().exactRef(localRef).getObjectId()); + // ...however the object has been downloaded + assertTrue(local.getRepository().getObjectDatabase() + .has(remoteCommit)); + } + } + + @Test public void testPush() throws Exception { ObjectId master = local.branch("master").commit().create(); @@ -288,8 +389,7 @@ public void nonForcedFetchRejectedWithConcurrentLocalRefUpdate() .add("remote.txt", "remote").create(); remote.update(remoteBranchName, remoteCommit); - URIish uri = registerTestProtocolUpdatingRef(localRef, updatedLocalCommit); - + URIish uri = registerTestProtocolUpdatingRef((repo) -> repo.update(localRef, updatedLocalCommit)); FetchResult result = git.fetch().setRemote(uri.toString()) .setRefSpecs(nonForcedFetchRefSpec).call(); @@ -318,13 +418,17 @@ private TestProtocol<User> registerProto(UploadPackFactory<User> upf, return proto; } - private <T extends AnyObjectId> URIish registerTestProtocolUpdatingRef(String refName, T objectId) { + @FunctionalInterface + interface TestRepositoryUpdateFunc { + void call(TestRepository<InMemoryRepository> repository) throws Exception; + } + + private <T extends AnyObjectId> URIish registerTestProtocolUpdatingRef(TestRepositoryUpdateFunc repositoryUpdateFunc) { TestProtocol<User> proto = registerProto((User req, Repository db) -> { try { - local.update(refName, objectId); + repositoryUpdateFunc.call(local); } catch (Exception e) { - throw new AssertionError("Cannot update local ref " + refName, - e); + throw new AssertionError("Cannot update local ref", e); } return new UploadPack(db); }, new DefaultReceive());
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 c510194..e430442 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/transport/FetchProcess.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/transport/FetchProcess.java
@@ -139,6 +139,18 @@ private void executeImp(final ProgressMonitor monitor, final TagOpt tagopt = transport.getTagOpt(); String getTags = (tagopt == TagOpt.NO_TAGS) ? null : Constants.R_TAGS; String getHead = null; + // Snapshot local refs before opening the fetch connection when the + // fetch has positive refspecs or may run AUTO_FOLLOW. Those are the + // paths that can later create TrackingRefUpdates from this fetch and + // need a stable expected-old-id baseline to turn concurrent local ref + // changes into LOCK_FAILURE. + // + // Skip the snapshot for the remaining cases to avoid an unnecessary + // local ref scan when this fetch cannot reach those update paths. + if (!toFetch.isEmpty() || tagopt == TagOpt.AUTO_FOLLOW) { + localRefs(); + } + try { // If we don't have a HEAD yet, we're cloning and need to get the // upstream HEAD, too. @@ -235,10 +247,23 @@ else if (tagopt == TagOpt.FETCH_TAGS) addUpdateBatchCommands(result, batch); for (ReceiveCommand cmd : batch.getCommands()) { cmd.updateType(walk); - if (cmd.getType() == UPDATE_NONFASTFORWARD - && cmd instanceof TrackingRefUpdate.Command - && !((TrackingRefUpdate.Command) cmd).canForceUpdate()) + if (!(cmd instanceof TrackingRefUpdate.Command) + || ((TrackingRefUpdate.Command) cmd).canForceUpdate()) { + continue; + } + + ObjectId currentId = currentObjectId(cmd.getRefName()); + // The initial type check used the ref value snapshotted when + // the fetch started. If the local ref moved since then, + // re-check the non-fast-forward against the current ref before + // executing the batch so non-forced rejects remain REJECTED + // instead of surfacing as LOCK_FAILURE. + ReceiveCommand refreshedCmd = new ReceiveCommand(currentId, + cmd.getNewId(), cmd.getRefName()); + refreshedCmd.updateType(walk); + if (refreshedCmd.getType() == UPDATE_NONFASTFORWARD) { cmd.setResult(REJECTED_NONFASTFORWARD); + } } if (transport.isDryRun()) { for (ReceiveCommand cmd : batch.getCommands()) { @@ -574,6 +599,14 @@ private Map<String, Ref> localRefs() throws TransportException { return localRefs; } + private ObjectId currentObjectId(String refName) throws IOException { + Ref current = transport.local.exactRef(refName); + if (current == null) { + return ObjectId.zeroId(); + } + return current.getObjectId(); + } + private void deleteStaleTrackingRefs(FetchResult result, BatchRefUpdate batch) throws IOException { Set<Ref> processed = new HashSet<>();