[push] Call the pre-push hook later in the push process

Call the pre-push hook only after having received the remote
advertisement and having determined rejections, like C git does.
Also similar to C git, don't pass rejected or up-to-date updates
to the pre-push hook.

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

diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/transport/PushProcessTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/transport/PushProcessTest.java
index 6928859622bd340636adecf40a927313128c6506..2e8b30f151b9c043079c60c3f215078fae9848af 100644 (file)
--- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/transport/PushProcessTest.java
+++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/transport/PushProcessTest.java
@@ -14,14 +14,19 @@ import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertNotNull;
 import static org.junit.Assert.assertTrue;
 
+import java.io.ByteArrayOutputStream;
 import java.io.IOException;
 import java.io.OutputStream;
+import java.io.PrintStream;
+import java.nio.charset.StandardCharsets;
 import java.util.HashMap;
 import java.util.HashSet;
 import java.util.Map;
 
+import org.eclipse.jgit.api.errors.AbortedByHookException;
 import org.eclipse.jgit.errors.NotSupportedException;
 import org.eclipse.jgit.errors.TransportException;
+import org.eclipse.jgit.hooks.PrePushHook;
 import org.eclipse.jgit.lib.ObjectId;
 import org.eclipse.jgit.lib.ObjectIdRef;
 import org.eclipse.jgit.lib.ProgressMonitor;
@@ -31,6 +36,7 @@ import org.eclipse.jgit.lib.Repository;
 import org.eclipse.jgit.lib.TextProgressMonitor;
 import org.eclipse.jgit.test.resources.SampleDataRepositoryTestCase;
 import org.eclipse.jgit.transport.RemoteRefUpdate.Status;
+import org.eclipse.jgit.util.io.NullOutputStream;
 import org.junit.Before;
 import org.junit.Test;
 
@@ -220,7 +226,17 @@ public class PushProcessTest extends SampleDataRepositoryTestCase {
                                                .fromString("0000000000000000000000000000000000000001"));
                final Ref ref = new ObjectIdRef.Unpeeled(Ref.Storage.LOOSE, "refs/heads/master",
                                ObjectId.fromString("ac7e7e44c1885efb472ad54a78327d66bfc4ecef"));
-               testOneUpdateStatus(rru, ref, Status.REJECTED_REMOTE_CHANGED, null);
+               try (ByteArrayOutputStream bytes = new ByteArrayOutputStream();
+                               PrintStream out = new PrintStream(bytes, true,
+                                               StandardCharsets.UTF_8);
+                               PrintStream err = new PrintStream(NullOutputStream.INSTANCE)) {
+                       MockPrePushHook hook = new MockPrePushHook(db, out, err);
+                       testOneUpdateStatus(rru, ref, Status.REJECTED_REMOTE_CHANGED, null,
+                                       hook);
+                       out.flush();
+                       String result = new String(bytes.toString(StandardCharsets.UTF_8));
+                       assertEquals("", result);
+               }
        }
 
        /**
@@ -256,10 +272,22 @@ public class PushProcessTest extends SampleDataRepositoryTestCase {
                refUpdates.add(rruOk);
                refUpdates.add(rruReject);
                advertisedRefs.add(refToChange);
-               executePush();
-               assertEquals(Status.OK, rruOk.getStatus());
-               assertTrue(rruOk.isFastForward());
-               assertEquals(Status.NON_EXISTING, rruReject.getStatus());
+               try (ByteArrayOutputStream bytes = new ByteArrayOutputStream();
+                               PrintStream out = new PrintStream(bytes, true,
+                                               StandardCharsets.UTF_8);
+                               PrintStream err = new PrintStream(NullOutputStream.INSTANCE)) {
+                       MockPrePushHook hook = new MockPrePushHook(db, out, err);
+                       executePush(hook);
+                       assertEquals(Status.OK, rruOk.getStatus());
+                       assertTrue(rruOk.isFastForward());
+                       assertEquals(Status.NON_EXISTING, rruReject.getStatus());
+                       out.flush();
+                       String result = new String(bytes.toString(StandardCharsets.UTF_8));
+                       assertEquals(
+                                       "null 0000000000000000000000000000000000000000 "
+                                                       + "refs/heads/master 2c349335b7f797072cf729c4f3bb0914ecb6dec9\n",
+                                       result);
+               }
        }
 
        /**
@@ -346,10 +374,18 @@ public class PushProcessTest extends SampleDataRepositoryTestCase {
                        final Ref advertisedRef, final Status expectedStatus,
                        Boolean fastForward) throws NotSupportedException,
                        TransportException {
+               return testOneUpdateStatus(rru, advertisedRef, expectedStatus,
+                               fastForward, null);
+       }
+
+       private PushResult testOneUpdateStatus(final RemoteRefUpdate rru,
+                       final Ref advertisedRef, final Status expectedStatus,
+                       Boolean fastForward, PrePushHook hook)
+                       throws NotSupportedException, TransportException {
                refUpdates.add(rru);
                if (advertisedRef != null)
                        advertisedRefs.add(advertisedRef);
-               final PushResult result = executePush();
+               final PushResult result = executePush(hook);
                assertEquals(expectedStatus, rru.getStatus());
                if (fastForward != null)
                        assertEquals(fastForward, Boolean.valueOf(rru.isFastForward()));
@@ -358,7 +394,12 @@ public class PushProcessTest extends SampleDataRepositoryTestCase {
 
        private PushResult executePush() throws NotSupportedException,
                        TransportException {
-               process = new PushProcess(transport, refUpdates);
+               return executePush(null);
+       }
+
+       private PushResult executePush(PrePushHook hook)
+                       throws NotSupportedException, TransportException {
+               process = new PushProcess(transport, refUpdates, hook);
                return process.execute(new TextProgressMonitor());
        }
 
@@ -416,4 +457,20 @@ public class PushProcessTest extends SampleDataRepositoryTestCase {
                        }
                }
        }
+
+       private static class MockPrePushHook extends PrePushHook {
+
+               private final PrintStream output;
+
+               public MockPrePushHook(Repository repo, PrintStream out,
+                               PrintStream err) {
+                       super(repo, out, err);
+                       output = out;
+               }
+
+               @Override
+               protected void doRun() throws AbortedByHookException, IOException {
+                       output.print(getStdinArgs());
+               }
+       }
 }

diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/transport/PushProcess.java b/org.eclipse.jgit/src/org/eclipse/jgit/transport/PushProcess.java
index a244c55a3863d72ae88e2adbd4aefa3799bca932..c4f276988c399e534c2fa9525102fc37e19f2ce3 100644 (file)
--- a/org.eclipse.jgit/src/org/eclipse/jgit/transport/PushProcess.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/transport/PushProcess.java
@@ -18,10 +18,13 @@ import java.util.Collections;
 import java.util.LinkedHashMap;
 import java.util.List;
 import java.util.Map;
+import java.util.stream.Collectors;
 
+import org.eclipse.jgit.api.errors.AbortedByHookException;
 import org.eclipse.jgit.errors.MissingObjectException;
 import org.eclipse.jgit.errors.NotSupportedException;
 import org.eclipse.jgit.errors.TransportException;
+import org.eclipse.jgit.hooks.PrePushHook;
 import org.eclipse.jgit.internal.JGitText;
 import org.eclipse.jgit.lib.ObjectId;
 import org.eclipse.jgit.lib.ProgressMonitor;
@@ -58,6 +61,8 @@ class PushProcess {
        /** A list of option strings associated with this push */
        private List<String> pushOptions;
 
+       private final PrePushHook prePush;
+
        /**
         * Create process for specified transport and refs updates specification.
         *
@@ -66,12 +71,15 @@ class PushProcess {
         *            connection.
         * @param toPush
         *            specification of refs updates (and local tracking branches).
-        *
+        * @param prePush
+        *            {@link PrePushHook} to run after the remote advertisement has
+        *            been gotten
         * @throws TransportException
         */
        PushProcess(final Transport transport,
-                       final Collection<RemoteRefUpdate> toPush) throws TransportException {
-               this(transport, toPush, null);
+                       final Collection<RemoteRefUpdate> toPush, PrePushHook prePush)
+                       throws TransportException {
+               this(transport, toPush, prePush, null);
        }
 
        /**
@@ -82,16 +90,20 @@ class PushProcess {
         *            connection.
         * @param toPush
         *            specification of refs updates (and local tracking branches).
+        * @param prePush
+        *            {@link PrePushHook} to run after the remote advertisement has
+        *            been gotten
         * @param out
         *            OutputStream to write messages to
         * @throws TransportException
         */
        PushProcess(final Transport transport,
-                       final Collection<RemoteRefUpdate> toPush, OutputStream out)
-                       throws TransportException {
+                       final Collection<RemoteRefUpdate> toPush, PrePushHook prePush,
+                       OutputStream out) throws TransportException {
                this.walker = new RevWalk(transport.local);
                this.transport = transport;
                this.toPush = new LinkedHashMap<>();
+               this.prePush = prePush;
                this.out = out;
                this.pushOptions = transport.getPushOptions();
                for (RemoteRefUpdate rru : toPush) {
@@ -133,6 +145,30 @@ class PushProcess {
                                monitor.endTask();
 
                                final Map<String, RemoteRefUpdate> preprocessed = prepareRemoteUpdates();
+                               List<RemoteRefUpdate> willBeAttempted = preprocessed.values()
+                                               .stream().filter(u -> {
+                                                       switch (u.getStatus()) {
+                                                       case NON_EXISTING:
+                                                       case REJECTED_NODELETE:
+                                                       case REJECTED_NONFASTFORWARD:
+                                                       case REJECTED_OTHER_REASON:
+                                                       case REJECTED_REMOTE_CHANGED:
+                                                       case UP_TO_DATE:
+                                                               return false;
+                                                       default:
+                                                               return true;
+                                                       }
+                                               }).collect(Collectors.toList());
+                               if (!willBeAttempted.isEmpty()) {
+                                       if (prePush != null) {
+                                               try {
+                                                       prePush.setRefs(willBeAttempted);
+                                                       prePush.call();
+                                               } catch (AbortedByHookException | IOException e) {
+                                                       throw new TransportException(e.getMessage(), e);
+                                               }
+                                       }
+                               }
                                if (transport.isDryRun())
                                        modifyUpdatesForDryRun();
                                else if (!preprocessed.isEmpty())

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 bfe26d9808474f489f1c5c8ff8663c66013fd973..1245f78a6ebc41601d7499e4e0b79fb46f1e8c96 100644 (file)
--- a/org.eclipse.jgit/src/org/eclipse/jgit/transport/Transport.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/transport/Transport.java
@@ -40,7 +40,6 @@ import java.util.concurrent.CopyOnWriteArrayList;
 
 import org.eclipse.jgit.annotations.NonNull;
 import org.eclipse.jgit.annotations.Nullable;
-import org.eclipse.jgit.api.errors.AbortedByHookException;
 import org.eclipse.jgit.errors.NotSupportedException;
 import org.eclipse.jgit.errors.TransportException;
 import org.eclipse.jgit.hooks.Hooks;
@@ -1375,16 +1374,9 @@ public abstract class Transport implements AutoCloseable {
                        if (toPush.isEmpty())
                                throw new TransportException(JGitText.get().nothingToPush);
                }
-               if (prePush != null) {
-                       try {
-                               prePush.setRefs(toPush);
-                               prePush.call();
-                       } catch (AbortedByHookException | IOException e) {
-                               throw new TransportException(e.getMessage(), e);
-                       }
-               }
 
-               final PushProcess pushProcess = new PushProcess(this, toPush, out);
+               final PushProcess pushProcess = new PushProcess(this, toPush, prePush,
+                               out);
                return pushProcess.execute(monitor);
        }