Re: [EGIT PATCH 1/3] Give NoRemoteRepositoryException better message in BasePackConnection
Marek Zawirski <marek.zawirski@gmail.com> wrote:
Show 22 quoted lines
>
> 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(-)
Show changes to 2 files +34 −1
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 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.
