From 46d35a85025ff2e5184422a79451ce724a69a9ec Mon Sep 17 00:00:00 2001 From: David Turner Date: Tue, 3 Jan 2017 23:56:08 -0500 Subject: [PATCH] push: support per-ref force-with-lease When rebasing, force-pushing has a race condition: someone else might have pushed a commit since the one you just rewrote. The force-with-lease option prevents this by ensuring that the ref's old value is the one that you expected. Change-Id: I97ca9f8395396c76332bdd07c486e60549ca4401 Signed-off-by: David Turner --- .../org/eclipse/jgit/api/PushCommandTest.java | 54 ++++++++++ .../eclipse/jgit/transport/TransportTest.java | 41 ++++++++ .../src/org/eclipse/jgit/api/PushCommand.java | 46 ++++++++- .../eclipse/jgit/transport/RefLeaseSpec.java | 99 +++++++++++++++++++ .../org/eclipse/jgit/transport/Transport.java | 74 +++++++++++++- 5 files changed, 308 insertions(+), 6 deletions(-) create mode 100644 org.eclipse.jgit/src/org/eclipse/jgit/transport/RefLeaseSpec.java diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/api/PushCommandTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/api/PushCommandTest.java index 2a325405e..eaf64b649 100644 --- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/api/PushCommandTest.java +++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/api/PushCommandTest.java @@ -66,8 +66,10 @@ import org.eclipse.jgit.lib.Repository; import org.eclipse.jgit.lib.StoredConfig; import org.eclipse.jgit.revwalk.RevCommit; import org.eclipse.jgit.transport.PushResult; +import org.eclipse.jgit.transport.RefLeaseSpec; import org.eclipse.jgit.transport.RefSpec; import org.eclipse.jgit.transport.RemoteConfig; +import org.eclipse.jgit.transport.RemoteRefUpdate; import org.eclipse.jgit.transport.TrackingRefUpdate; import org.eclipse.jgit.transport.URIish; import org.eclipse.jgit.util.FS; @@ -379,4 +381,56 @@ public class PushCommandTest extends RepositoryTestCase { db2.resolve(commit3.getId().getName() + "^{commit}")); } } + + @Test + public void testPushWithLease() throws JGitInternalException, IOException, + GitAPIException, URISyntaxException { + + // create other repository + Repository db2 = createWorkRepository(); + + // setup the first repository + final StoredConfig config = db.getConfig(); + RemoteConfig remoteConfig = new RemoteConfig(config, "test"); + URIish uri = new URIish(db2.getDirectory().toURI().toURL()); + remoteConfig.addURI(uri); + remoteConfig.update(config); + config.save(); + + try (Git git1 = new Git(db)) { + // create one commit and push it + RevCommit commit = git1.commit().setMessage("initial commit").call(); + Ref branchRef = git1.branchCreate().setName("initial").call(); + + RefSpec spec = new RefSpec("refs/heads/master:refs/heads/x"); + git1.push().setRemote("test").setRefSpecs(spec) + .call(); + + assertEquals(commit.getId(), + db2.resolve(commit.getId().getName() + "^{commit}")); + //now try to force-push a new commit, with a good lease + + RevCommit commit2 = git1.commit().setMessage("second commit").call(); + Iterable results = + git1.push().setRemote("test").setRefSpecs(spec) + .setRefLeaseSpecs(new RefLeaseSpec("refs/heads/x", "initial")) + .call(); + for (PushResult result : results) { + RemoteRefUpdate update = result.getRemoteUpdate("refs/heads/x"); + assertEquals(update.getStatus(), RemoteRefUpdate.Status.OK); + } + + RevCommit commit3 = git1.commit().setMessage("third commit").call(); + //now try to force-push a new commit, with a bad lease + + results = + git1.push().setRemote("test").setRefSpecs(spec) + .setRefLeaseSpecs(new RefLeaseSpec("refs/heads/x", "initial")) + .call(); + for (PushResult result : results) { + RemoteRefUpdate update = result.getRemoteUpdate("refs/heads/x"); + assertEquals(update.getStatus(), RemoteRefUpdate.Status.REJECTED_REMOTE_CHANGED); + } + } + } } diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/transport/TransportTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/transport/TransportTest.java index 5519f61ac..d4c47d37e 100644 --- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/transport/TransportTest.java +++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/transport/TransportTest.java @@ -53,7 +53,9 @@ import java.io.IOException; import java.util.Arrays; import java.util.Collection; import java.util.Collections; +import java.util.HashMap; import java.util.List; +import java.util.Map; import org.eclipse.jgit.lib.Config; import org.eclipse.jgit.lib.Constants; @@ -209,6 +211,45 @@ public class TransportTest extends SampleDataRepositoryTestCase { assertEquals(ObjectId.zeroId(), tru.getOldObjectId()); } + /** + * Test RefSpec to RemoteRefUpdate conversion with leases. + * + * @throws IOException + */ + @Test + public void testFindRemoteRefUpdatesWithLeases() throws IOException { + final RefSpec specA = new RefSpec("+refs/heads/a:refs/heads/b"); + final RefSpec specC = new RefSpec("+refs/heads/c:refs/heads/d"); + final Collection specs = Arrays.asList(specA, specC); + final Map leases = new HashMap<>(); + leases.put("refs/heads/b", + new RefLeaseSpec("refs/heads/b", "refs/heads/c")); + + Collection result; + try (Transport transport = Transport.open(db, remoteConfig)) { + result = transport.findRemoteRefUpdatesFor(specs, leases); + } + + assertEquals(2, result.size()); + boolean foundA = false; + boolean foundC = false; + for (final RemoteRefUpdate rru : result) { + if ("refs/heads/a".equals(rru.getSrcRef()) + && "refs/heads/b".equals(rru.getRemoteName())) { + foundA = true; + assertEquals(db.exactRef("refs/heads/c").getObjectId(), + rru.getExpectedOldObjectId()); + } + if ("refs/heads/c".equals(rru.getSrcRef()) + && "refs/heads/d".equals(rru.getRemoteName())) { + foundC = true; + assertNull(rru.getExpectedOldObjectId()); + } + } + assertTrue(foundA); + assertTrue(foundC); + } + @Test public void testLocalTransportWithRelativePath() throws Exception { Repository other = createWorkRepository(); diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/api/PushCommand.java b/org.eclipse.jgit/src/org/eclipse/jgit/api/PushCommand.java index bd4521b51..be15ab13b 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/api/PushCommand.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/api/PushCommand.java @@ -47,9 +47,12 @@ import java.io.OutputStream; import java.net.URISyntaxException; import java.text.MessageFormat; import java.util.ArrayList; +import java.util.Arrays; import java.util.Collection; import java.util.Collections; +import java.util.HashMap; import java.util.List; +import java.util.Map; import org.eclipse.jgit.api.errors.GitAPIException; import org.eclipse.jgit.api.errors.InvalidRemoteException; @@ -65,6 +68,7 @@ import org.eclipse.jgit.lib.ProgressMonitor; import org.eclipse.jgit.lib.Ref; import org.eclipse.jgit.lib.Repository; import org.eclipse.jgit.transport.PushResult; +import org.eclipse.jgit.transport.RefLeaseSpec; import org.eclipse.jgit.transport.RefSpec; import org.eclipse.jgit.transport.RemoteConfig; import org.eclipse.jgit.transport.RemoteRefUpdate; @@ -85,6 +89,8 @@ public class PushCommand extends private final List refSpecs; + private final Map refLeaseSpecs; + private ProgressMonitor monitor = NullProgressMonitor.INSTANCE; private String receivePack = RemoteConfig.DEFAULT_RECEIVE_PACK; @@ -104,6 +110,7 @@ public class PushCommand extends protected PushCommand(Repository repo) { super(repo); refSpecs = new ArrayList(3); + refLeaseSpecs = new HashMap<>(); } /** @@ -155,7 +162,7 @@ public class PushCommand extends configure(transport); final Collection toPush = transport - .findRemoteRefUpdatesFor(refSpecs); + .findRemoteRefUpdatesFor(refSpecs, refLeaseSpecs); try { PushResult result = transport.push(monitor, toPush, out); @@ -270,6 +277,43 @@ public class PushCommand extends return this; } + /** + * @return the ref lease specs + * @since 4.7 + */ + public List getRefLeaseSpecs() { + return new ArrayList(refLeaseSpecs.values()); + } + + /** + * The ref lease specs to be used in the push operation, + * for a force-with-lease push operation. + * + * @param specs + * @return {@code this} + * @since 4.7 + */ + public PushCommand setRefLeaseSpecs(RefLeaseSpec... specs) { + return setRefLeaseSpecs(Arrays.asList(specs)); + } + + /** + * The ref lease specs to be used in the push operation, + * for a force-with-lease push operation. + * + * @param specs + * @return {@code this} + * @since 4.7 + */ + public PushCommand setRefLeaseSpecs(List specs) { + checkCallable(); + this.refLeaseSpecs.clear(); + for (RefLeaseSpec spec : specs) { + refLeaseSpecs.put(spec.getRef(), spec); + } + return this; + } + /** * @return the ref specs */ diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/transport/RefLeaseSpec.java b/org.eclipse.jgit/src/org/eclipse/jgit/transport/RefLeaseSpec.java new file mode 100644 index 000000000..02c3c3cc5 --- /dev/null +++ b/org.eclipse.jgit/src/org/eclipse/jgit/transport/RefLeaseSpec.java @@ -0,0 +1,99 @@ +/* + * Copyright (C) 2017 Two Sigma Open Source + * and other copyright owners as documented in the project's IP log. + * + * This program and the accompanying materials are made available + * under the terms of the Eclipse Distribution License v1.0 which + * accompanies this distribution, is reproduced below, and is + * available at http://www.eclipse.org/org/documents/edl-v10.php + * + * All rights reserved. + * + * Redistribution and use in source and binary forms, with or + * without modification, are permitted provided that the following + * conditions are met: + * + * - Redistributions of source code must retain the above copyright + * notice, this list of conditions and the following disclaimer. + * + * - Redistributions in binary form must reproduce the above + * copyright notice, this list of conditions and the following + * disclaimer in the documentation and/or other materials provided + * with the distribution. + * + * - Neither the name of the Eclipse Foundation, Inc. nor the + * names of its contributors may be used to endorse or promote + * products derived from this software without specific prior + * written permission. + * + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND + * CONTRIBUTORS "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, + * INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES + * OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE + * ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR + * CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, + * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT + * NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; + * LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER + * CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, + * STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) + * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF + * ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. + */ + +package org.eclipse.jgit.transport; + +import java.io.Serializable; + +import org.eclipse.jgit.internal.JGitText; + +/** + * Describes the expected value for a ref being pushed. + * @since 4.7 + */ +public class RefLeaseSpec implements Serializable { + private static final long serialVersionUID = 1L; + + /** Name of the ref whose value we want to check. */ + private final String ref; + + /** Local commitish to get expected value from. */ + private final String expected; + + private RefLeaseSpec(final RefLeaseSpec p) { + ref = p.getRef(); + expected = p.getExpected(); + } + + public RefLeaseSpec(final String ref, final String expected) { + this.ref = ref; + this.expected = expected; + } + + /** + * Get the ref to protect. + * + * @return name of ref to check. + */ + public String getRef() { + return ref; + } + + /** + * Get the expected value of the ref, in the form + * of a local committish + * + * @return expected ref value. + */ + public String getExpected() { + return expected; + } + + public String toString() { + final StringBuilder r = new StringBuilder(); + r.append(getRef()); + r.append(':'); + r.append(getExpected()); + return r.toString(); + } +} diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/transport/Transport.java b/org.eclipse.jgit/src/org/eclipse/jgit/transport/Transport.java index df860695d..64bdbe7d4 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/transport/Transport.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/transport/Transport.java @@ -80,6 +80,7 @@ import org.eclipse.jgit.internal.JGitText; import org.eclipse.jgit.lib.Constants; import org.eclipse.jgit.lib.NullProgressMonitor; import org.eclipse.jgit.lib.ObjectChecker; +import org.eclipse.jgit.lib.ObjectId; import org.eclipse.jgit.lib.ProgressMonitor; import org.eclipse.jgit.lib.Ref; import org.eclipse.jgit.lib.Repository; @@ -603,14 +604,16 @@ public abstract class Transport implements AutoCloseable { * Convert push remote refs update specification from {@link RefSpec} form * to {@link RemoteRefUpdate}. Conversion expands wildcards by matching * source part to local refs. expectedOldObjectId in RemoteRefUpdate is - * always set as null. Tracking branch is configured if RefSpec destination - * matches source of any fetch ref spec for this transport remote - * configuration. + * set when specified in leases. Tracking branch is configured if RefSpec + * destination matches source of any fetch ref spec for this transport + * remote configuration. * * @param db * local database. * @param specs * collection of RefSpec to convert. + * @param leases + * map from ref to lease (containing expected old object id) * @param fetchSpecs * fetch specifications used for finding localtracking refs. May * be null or empty collection. @@ -618,9 +621,11 @@ public abstract class Transport implements AutoCloseable { * @throws IOException * when problem occurred during conversion or specification set * up: most probably, missing objects or refs. + * @since 4.7 */ public static Collection findRemoteRefUpdatesFor( final Repository db, final Collection specs, + final Map leases, Collection fetchSpecs) throws IOException { if (fetchSpecs == null) fetchSpecs = Collections.emptyList(); @@ -652,13 +657,43 @@ public abstract class Transport implements AutoCloseable { final boolean forceUpdate = spec.isForceUpdate(); final String localName = findTrackingRefName(destSpec, fetchSpecs); + final RefLeaseSpec leaseSpec = leases.get(destSpec); + final ObjectId expected = leaseSpec == null ? null : + db.resolve(leaseSpec.getExpected()); final RemoteRefUpdate rru = new RemoteRefUpdate(db, srcSpec, - destSpec, forceUpdate, localName, null); + destSpec, forceUpdate, localName, expected); result.add(rru); } return result; } + /** + * Convert push remote refs update specification from {@link RefSpec} form + * to {@link RemoteRefUpdate}. Conversion expands wildcards by matching + * source part to local refs. expectedOldObjectId in RemoteRefUpdate is + * always set as null. Tracking branch is configured if RefSpec destination + * matches source of any fetch ref spec for this transport remote + * configuration. + * + * @param db + * local database. + * @param specs + * collection of RefSpec to convert. + * @param fetchSpecs + * fetch specifications used for finding localtracking refs. May + * be null or empty collection. + * @return collection of set up {@link RemoteRefUpdate}. + * @throws IOException + * when problem occurred during conversion or specification set + * up: most probably, missing objects or refs. + */ + public static Collection findRemoteRefUpdatesFor( + final Repository db, final Collection specs, + Collection fetchSpecs) throws IOException { + return findRemoteRefUpdatesFor(db, specs, Collections.emptyMap(), + fetchSpecs); + } + private static Collection expandPushWildcardsFor( final Repository db, final Collection specs) throws IOException { @@ -1341,7 +1376,36 @@ public abstract class Transport implements AutoCloseable { */ public Collection findRemoteRefUpdatesFor( final Collection specs) throws IOException { - return findRemoteRefUpdatesFor(local, specs, fetch); + return findRemoteRefUpdatesFor(local, specs, Collections.emptyMap(), + fetch); + } + + /** + * Convert push remote refs update specification from {@link RefSpec} form + * to {@link RemoteRefUpdate}. Conversion expands wildcards by matching + * source part to local refs. expectedOldObjectId in RemoteRefUpdate is + * set according to leases. Tracking branch is configured if RefSpec destination + * matches source of any fetch ref spec for this transport remote + * configuration. + *

+ * Conversion is performed for context of this transport (database, fetch + * specifications). + * + * @param specs + * collection of RefSpec to convert. + * @param leases + * map from ref to lease (containing expected old object id) + * @return collection of set up {@link RemoteRefUpdate}. + * @throws IOException + * when problem occurred during conversion or specification set + * up: most probably, missing objects or refs. + * @since 4.7 + */ + public Collection findRemoteRefUpdatesFor( + final Collection specs, + final Map leases) throws IOException { + return findRemoteRefUpdatesFor(local, specs, leases, + fetch); } /**