Re: [EGIT PATCH 1/3] Give NoRemoteRepositoryException better message in BasePackConnection

["Shawn O. Pearce" <spearce@spearce.org>]

Marek Zawirski <marek.zawirski@gmail.com> wrote:
> 
> See http://code.google.com/p/egit/issues/detail?id=16
> 
> diff --git a/org.spearce.jgit/src/org/spearce/jgit/transport/BasePackConnection.java b/org.spearce.jgit/src/org/spearce/jgit/transport/BasePackConnection.java
> index 14fffc3..de0c7b6 100644
> --- a/org.spearce.jgit/src/org/spearce/jgit/transport/BasePackConnection.java
> +++ b/org.spearce.jgit/src/org/spearce/jgit/transport/BasePackConnection.java
> @@ -129,8 +129,15 @@ private void readAdvertisedRefsImpl() throws IOException {
>  			try {
>  				line = pckIn.readString();
>  			} catch (EOFException eof) {
> -				if (avail.isEmpty())
> -					throw new NoRemoteRepositoryException(uri, "not found.");
> +				if (avail.isEmpty()) {
> +					String service = "unknown";
> +					if (this instanceof PushConnection)
> +						service = "push";
> +					else if (this instanceof FetchConnection)
> +						service = "fetch";
> +					throw new NoRemoteRepositoryException(uri, service
> +							+ " service not found.");
> +				}

Hmm.  I wonder if we could detect this better.  With the patch
below I can get nice errors:

  $ ./jgit.sh push git://repo.or.cz/egit.git refs/heads/master
  fatal: git://repo.or.cz/egit.git: push not permitted

  $ ./jgit.sh push git://repo.or.cz/fake.git refs/heads/master
  fatal: git://repo.or.cz/fake.git: not found.

--8<--
Disambiguate "push not supported" from "repository not found"

If we are pushing to a remote repository the reason why we
get no refs may be because push is not permitted, or it is
a bad URI and points to a non-existant repository.

To get a good error message for the user we need to open a
fetch connection to see if fetch also fails.  If it failed
we know the URI is invalid; if fetch succeeds we know that
the repository is there but the user is just not allowed to
push to it over this transport.

Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---
 .../spearce/jgit/transport/BasePackConnection.java |   10 +++++++-
 .../jgit/transport/BasePackPushConnection.java     |   25 ++++++++++++++++++++
 2 files changed, 34 insertions(+), 1 deletions(-)

diff --git a/org.spearce.jgit/src/org/spearce/jgit/transport/BasePackConnection.java b/org.spearce.jgit/src/org/spearce/jgit/transport/BasePackConnection.java
index 14fffc3..e35f850 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/transport/BasePackConnection.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/transport/BasePackConnection.java
@@ -72,6 +72,9 @@
 	/** Remote repository location. */
 	protected final URIish uri;
 
+	/** A transport connected to {@link #uri}. */
+	protected final PackTransport transport;
+
 	/** Buffered input stream reading from the remote. */
 	protected InputStream in;
 
@@ -93,6 +96,7 @@
 	BasePackConnection(final PackTransport packTransport) {
 		local = packTransport.local;
 		uri = packTransport.uri;
+		transport = packTransport;
 	}
 
 	protected void init(final InputStream myIn, final OutputStream myOut) {
@@ -130,7 +134,7 @@ private void readAdvertisedRefsImpl() throws IOException {
 				line = pckIn.readString();
 			} catch (EOFException eof) {
 				if (avail.isEmpty())
-					throw new NoRemoteRepositoryException(uri, "not found.");
+					throw noRepository();
 				throw eof;
 			}
 
@@ -178,6 +182,10 @@ if (prior != null)
 		available(avail);
 	}
 
+	protected TransportException noRepository() {
+		return new NoRemoteRepositoryException(uri, "not found.");
+	}
+
 	protected boolean isCapableOf(final String option) {
 		return remoteCapablities.contains(option);
 	}
diff --git a/org.spearce.jgit/src/org/spearce/jgit/transport/BasePackPushConnection.java b/org.spearce.jgit/src/org/spearce/jgit/transport/BasePackPushConnection.java
index a2d5b6f..a6ab9c4 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/transport/BasePackPushConnection.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/transport/BasePackPushConnection.java
@@ -43,6 +43,8 @@
 import java.util.Collection;
 import java.util.Map;
 
+import org.spearce.jgit.errors.NoRemoteRepositoryException;
+import org.spearce.jgit.errors.NotSupportedException;
 import org.spearce.jgit.errors.PackProtocolException;
 import org.spearce.jgit.errors.TransportException;
 import org.spearce.jgit.lib.ObjectId;
@@ -98,6 +100,29 @@ public void push(final ProgressMonitor monitor,
 		doPush(monitor, refUpdates);
 	}
 
+	@Override
+	protected TransportException noRepository() {
+		// Sadly we cannot tell the "invalid URI" case from "push not allowed".
+		// Opening a fetch connection can help us tell the difference, as any
+		// useful repository is going to support fetch if it also would allow
+		// push. So if fetch throws NoRemoteRepositoryException we know the
+		// URI is wrong. Otherwise we can correctly state push isn't allowed
+		// as the fetch connection opened successfully.
+		//
+		try {
+			transport.openFetch().close();
+		} catch (NotSupportedException e) {
+			// Fall through.
+		} catch (NoRemoteRepositoryException e) {
+			// Fetch concluded the repository doesn't exist.
+			//
+			return e;
+		} catch (TransportException e) {
+			// Fall through.
+		}
+		return new TransportException(uri, "push not permitted");
+	}
+
 	protected void doPush(final ProgressMonitor monitor,
 			final Map<String, RemoteRefUpdate> refUpdates)
 			throws TransportException {
-- 
1.6.0.272.g9ab4


-- 
Shawn.