[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>

org.eclipse.jgit.test/tst/org/eclipse/jgit/transport/PushProcessTest.java

@@ -14,14 +14,19 @@
 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.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 void testUpdateUnexpectedRemoteVsForce() throws IOException {
 						.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 void testUpdateMixedCases() throws IOException {
 		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 @@ private PushResult testOneUpdateStatus(final RemoteRefUpdate rru,
 			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 @@ private PushResult testOneUpdateStatus(final RemoteRefUpdate rru,
 
 	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 void push(ProgressMonitor monitor,
 			}
 		}
 	}
+
+	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());
+		}
+	}
 }

org.eclipse.jgit/src/org/eclipse/jgit/transport/PushProcess.java

@@ -18,10 +18,13 @@
 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 @@ PushResult execute(ProgressMonitor monitor)
 				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())

org.eclipse.jgit/src/org/eclipse/jgit/transport/Transport.java

@@ -40,7 +40,6 @@
 
 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 PushResult push(final ProgressMonitor monitor,
 			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);
 	}