[JGIT PATCH] Disambiguate "push not supported" from "repository not found"

[JGIT PATCH] Disambiguate "push not supported" from "repository not found"

[Shawn O. Pearce]

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.

With this change we now get useful error messages:

  $ ./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.

Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---
 .../spearce/jgit/transport/BasePackConnection.java |   19 ++++++++-------
 .../jgit/transport/BasePackPushConnection.java     |   25 ++++++++++++++++++++
 2 files changed, 35 insertions(+), 9 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 de0c7b6..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) {
@@ -129,15 +133,8 @@ private void readAdvertisedRefsImpl() throws IOException {
 			try {
 				line = pckIn.readString();
 			} catch (EOFException eof) {
-				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.");
-				}
+				if (avail.isEmpty())
+					throw noRepository();
 				throw eof;
 			}
 
@@ -185,6 +182,10 @@ else if (this instanceof FetchConnection)
 		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.174.gd789c

Re: [JGIT PATCH] Disambiguate "push not supported" from "repository not found"

[Robin Rosenberg]

fredagen den 29 augusti 2008 02.18.38 skrev Shawn O. Pearce:
> +				if (avail.isEmpty())
> +					throw noRepository();
>  				throw eof;
>  			}
>  
> @@ -185,6 +182,10 @@ else if (this instanceof FetchConnection)
>  		available(avail);
>  	}
>  
> +	protected TransportException noRepository() {
> +		return new NoRemoteRepositoryException(uri, "not found.");
> +	}
> +

Why an extra method for instantiating the exception?

-- robin

Re: [JGIT PATCH] Disambiguate "push not supported" from "repository not found"

[Marek Zawirski]

Robin Rosenberg wrote:
> fredagen den 29 augusti 2008 02.18.38 skrev Shawn O. Pearce:
>> +				if (avail.isEmpty())
>> +					throw noRepository();
>>  				throw eof;
>>  			}
>>  
>> @@ -185,6 +182,10 @@ else if (this instanceof FetchConnection)
>>  		available(avail);
>>  	}
>>  
>> +	protected TransportException noRepository() {
>> +		return new NoRemoteRepositoryException(uri, "not found.");
>> +	}
>> +
> 
> Why an extra method for instantiating the exception?

Isn't it overrode in subclass - BasePackPushConnection?
-- 
Marek Zawirski [zawir]
marek.zawirski@gmail.com

Re: [JGIT PATCH] Disambiguate "push not supported" from "repository not found"

[Shawn O. Pearce]

Marek Zawirski <marek.zawirski@gmail.com> wrote:
> Robin Rosenberg wrote:
>> fredagen den 29 augusti 2008 02.18.38 skrev Shawn O. Pearce:
>>> +				if (avail.isEmpty())
>>> +					throw noRepository();
>>>  				throw eof;
>>>  			}
>>>  @@ -185,6 +182,10 @@ else if (this instanceof FetchConnection)
>>>  		available(avail);
>>>  	}
>>>  +	protected TransportException noRepository() {
>>> +		return new NoRemoteRepositoryException(uri, "not found.");
>>> +	}
>>> +
>>
>> Why an extra method for instantiating the exception?
>
> Isn't it overrode in subclass - BasePackPushConnection?

Correct.  I introduced the method so the subclass can inject its
own implementation for the catch block.  But its required to give
back a TransportException so the catch block can throw it, as we
do not want the subclass to be able to continue at this point.

-- 
Shawn.

Re: [JGIT PATCH] Disambiguate "push not supported" from "repository not found"

[Robin Rosenberg]

fredagen den 29 augusti 2008 16.31.16 skrev Shawn O. Pearce:
> Marek Zawirski <marek.zawirski@gmail.com> wrote:
> > Robin Rosenberg wrote:
> >>
> >> Why an extra method for instantiating the exception?
> >
> > Isn't it overrode in subclass - BasePackPushConnection?
> 
> Correct.  I introduced the method so the subclass can inject its
> own implementation for the catch block.  But its required to give
> back a TransportException so the catch block can throw it, as we
> do not want the subclass to be able to continue at this point.

Mind if I squash this into the patch?

-- robin

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 e35f850..16e4897 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/transport/BasePackConnection.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/transport/BasePackConnection.java
@@ -182,6 +182,15 @@ private void readAdvertisedRefsImpl() throws IOException {
                available(avail);
        }

+       /**
+        * Create an exception to indicate problems finding a remote repository. The
+        * caller is expected to throw the returned exception.
+        *
+        * Subclasses may override this method to provide better diagnostics.
+        *
+        * @return a TransportException saying a repository cannot be found and
+        *         possibly why.
+        */
        protected TransportException noRepository() {
                return new NoRemoteRepositoryException(uri, "not found.");
        }

Re: [JGIT PATCH] Disambiguate "push not supported" from "repository not found"

[Shawn O. Pearce]

Robin Rosenberg <robin.rosenberg@dewire.com> wrote:
> fredagen den 29 augusti 2008 16.31.16 skrev Shawn O. Pearce:
> > Marek Zawirski <marek.zawirski@gmail.com> wrote:
> > > Robin Rosenberg wrote:
> > >>
> > >> Why an extra method for instantiating the exception?
> > >
> > > Isn't it overrode in subclass - BasePackPushConnection?
> > 
> > Correct.  I introduced the method so the subclass can inject its
> > own implementation for the catch block.  But its required to give
> > back a TransportException so the catch block can throw it, as we
> > do not want the subclass to be able to continue at this point.
> 
> Mind if I squash this into the patch?

No, not at all.  This looks fine.
 

> 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 e35f850..16e4897 100644
> --- a/org.spearce.jgit/src/org/spearce/jgit/transport/BasePackConnection.java
> +++ b/org.spearce.jgit/src/org/spearce/jgit/transport/BasePackConnection.java
> @@ -182,6 +182,15 @@ private void readAdvertisedRefsImpl() throws IOException {
>                 available(avail);
>         }
> 
> +       /**
> +        * Create an exception to indicate problems finding a remote repository. The
> +        * caller is expected to throw the returned exception.
> +        *
> +        * Subclasses may override this method to provide better diagnostics.
> +        *
> +        * @return a TransportException saying a repository cannot be found and
> +        *         possibly why.
> +        */
>         protected TransportException noRepository() {
>                 return new NoRemoteRepositoryException(uri, "not found.");
>         }

-- 
Shawn.