threads / patch / 15269

patchDisambiguate "push not supported" from "repository not found"

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

## tl;dr

6 messages between Aug 29, 2008 and Sep 2, 2008. Diffs are folded; open one to read it.

replies: 5people: 3as markdown or json

Shawn O. Pearce· Aug 29, 2008, 00:18 UTC · lore

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(-)
Show changes to 2 files +35 −9

org.spearce.jgit/src/org/spearce/jgit/transport/BasePackConnection.java, org.spearce.jgit/src/org/spearce/jgit/transport/BasePackPushConnection.java

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
Robin Rosenberg· Aug 29, 2008, 09:20 UTC · re: Shawn O. Pearce · lore

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

fredagen den 29 augusti 2008 02.18.38 skrev Shawn O. Pearce:
Show 13 quoted lines
> +				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
Marek Zawirski· Aug 29, 2008, 12:18 UTC · re: Robin Rosenberg · lore

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

Robin Rosenberg wrote:
Show 16 quoted lines
> 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
Shawn O. Pearce· Aug 29, 2008, 14:31 UTC · re: Marek Zawirski · lore

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

Marek Zawirski <marek.zawirski@gmail.com> wrote:
Show 17 quoted lines
> 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.
Robin Rosenberg· Aug 31, 2008, 08:28 UTC · re: Shawn O. Pearce · lore

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

fredagen den 29 augusti 2008 16.31.16 skrev Shawn O. Pearce:
Show 11 quoted lines
> 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
Show changes to org.spearce.jgit/src/org/spearce/jgit/transport/BasePackConnection.java +9 −0
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 O. Pearce· Sep 2, 2008, 05:42 UTC · re: Robin Rosenberg · lore

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

Robin Rosenberg <robin.rosenberg@dewire.com> wrote:
Show 14 quoted lines
> 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.
 
Show 20 quoted lines
> 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.

← back to recent threads