Do not apply pushInsteadOf to existing pushUris

Per the git config documentation[1], pushInsteadOf is ignored when
a remote has explicit pushUris.

Implement this, and adapt tests.

Up to now JGit mistakenly applied pushInsteadOf also to existing
pushUris. If some repositories had relied on this mis-feature,
pushes may newly suddenly fail (the uncritical case; the config
just needs to be fixed) or even still succeed but push to unexpected
places, namely to the non-rewritten pushUrls (the critical case).

The release notes should point out this change.

[1] https://git-scm.com/docs/git-config

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

2 files changed, 25 insertions, 12 deletions

diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/transport/RemoteConfigTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/transport/RemoteConfigTest.java
index b64c1344f2..a0cf0d2dbc 100644
--- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/transport/RemoteConfigTest.java
+++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/transport/RemoteConfigTest.java
@@ -499,19 +499,30 @@ public class RemoteConfigTest {
 	}
 
 	@Test
-	public void singlePushInsteadOf() throws Exception {
+	public void pushInsteadOfNotAppliedToPushUri() throws Exception {
 		config.setString("remote", "origin", "pushurl", "short:project.git");
 		config.setString("url", "https://server/repos/", "pushInsteadOf",
 				"short:");
 		RemoteConfig rc = new RemoteConfig(config, "origin");
 		assertFalse(rc.getPushURIs().isEmpty());
-		assertEquals("https://server/repos/project.git", rc.getPushURIs()
-				.get(0).toASCIIString());
+		assertEquals("short:project.git",
+				rc.getPushURIs().get(0).toASCIIString());
+	}
+
+	@Test
+	public void pushInsteadOfAppliedToUri() throws Exception {
+		config.setString("remote", "origin", "url", "short:project.git");
+		config.setString("url", "https://server/repos/", "pushInsteadOf",
+				"short:");
+		RemoteConfig rc = new RemoteConfig(config, "origin");
+		assertFalse(rc.getPushURIs().isEmpty());
+		assertEquals("https://server/repos/project.git",
+				rc.getPushURIs().get(0).toASCIIString());
 	}
 
 	@Test
 	public void multiplePushInsteadOf() throws Exception {
-		config.setString("remote", "origin", "pushurl", "prefixproject.git");
+		config.setString("remote", "origin", "url", "prefixproject.git");
 		config.setStringList("url", "https://server/repos/", "pushInsteadOf",
 				Arrays.asList("pre", "prefix", "pref", "perf"));
 		RemoteConfig rc = new RemoteConfig(config, "origin");
diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/transport/RemoteConfig.java b/org.eclipse.jgit/src/org/eclipse/jgit/transport/RemoteConfig.java
index f192be1164..a0d81c00f0 100644
--- a/org.eclipse.jgit/src/org/eclipse/jgit/transport/RemoteConfig.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/transport/RemoteConfig.java
@@ -173,20 +173,22 @@ public class RemoteConfig implements Serializable {
 		for (final String s : vlst) {
 			uris.add(new URIish(replaceUri(s, insteadOf)));
 		}
-		Map<String, String> pushInsteadOf = getReplacements(rc,
-				KEY_PUSHINSTEADOF);
 		String[] plst = rc.getStringList(SECTION, name, KEY_PUSHURL);
 		pushURIs = new ArrayList<>(plst.length);
 		for (final String s : plst) {
-			pushURIs.add(new URIish(replaceUri(s, pushInsteadOf)));
+			pushURIs.add(new URIish(s));
 		}
-		if (pushURIs.isEmpty() && !pushInsteadOf.isEmpty()) {
+		if (pushURIs.isEmpty()) {
 			// Would default to the uris. If we have pushinsteadof, we must
 			// supply rewritten push uris.
-			for (String s : vlst) {
-				String replaced = replaceUri(s, pushInsteadOf);
-				if (!s.equals(replaced)) {
-					pushURIs.add(new URIish(replaced));
+			Map<String, String> pushInsteadOf = getReplacements(rc,
+					KEY_PUSHINSTEADOF);
+			if (!pushInsteadOf.isEmpty()) {
+				for (String s : vlst) {
+					String replaced = replaceUri(s, pushInsteadOf);
+					if (!s.equals(replaced)) {
+						pushURIs.add(new URIish(replaced));
+					}
 				}
 			}
 		}