{"thread":{"id":"15269","subject":"[JGIT PATCH] Disambiguate \"push not supported\" from \"repository not found\"","startedAt":"2008-08-29T00:18:38Z","lastAt":"2008-09-02T05:42:13Z","messageCount":6,"participants":["Shawn O. Pearce","Robin Rosenberg","Marek Zawirski"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"89066","messageId":"1219969118-31672-1-git-send-email-spearce@spearce.org","threadId":"15269","inReplyTo":null,"subject":"[JGIT PATCH] Disambiguate \"push not supported\" from \"repository not found\"","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2008-08-29T00:18:38Z","receivedAt":"2008-08-29T00:18:38Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"If we are pushing to a remote repository the reason why we\nget no refs may be because push is not permitted, or it is\na bad URI and points to a non-existant repository.\n\nTo get a good error message for the user we need to open a\nfetch connection to see if fetch also fails.  If it failed\nwe know the URI is invalid; if fetch succeeds we know that\nthe repository is there but the user is just not allowed to\npush to it over this transport.\n\nWith this change we now get useful error messages:\n\n  $ ./jgit.sh push git://repo.or.cz/egit.git refs/heads/master\n  fatal: git://repo.or.cz/egit.git: push not permitted\n\n  $ ./jgit.sh push git://repo.or.cz/fake.git refs/heads/master\n  fatal: git://repo.or.cz/fake.git: not found.\n\nSigned-off-by: Shawn O. Pearce <spearce@spearce.org>\n---\n .../spearce/jgit/transport/BasePackConnection.java |   19 ++++++++-------\n .../jgit/transport/BasePackPushConnection.java     |   25 ++++++++++++++++++++\n 2 files changed, 35 insertions(+), 9 deletions(-)\n\ndiff --git a/org.spearce.jgit/src/org/spearce/jgit/transport/BasePackConnection.java b/org.spearce.jgit/src/org/spearce/jgit/transport/BasePackConnection.java\nindex de0c7b6..e35f850 100644\n--- a/org.spearce.jgit/src/org/spearce/jgit/transport/BasePackConnection.java\n+++ b/org.spearce.jgit/src/org/spearce/jgit/transport/BasePackConnection.java\n@@ -72,6 +72,9 @@\n \t/** Remote repository location. */\n \tprotected final URIish uri;\n \n+\t/** A transport connected to {@link #uri}. */\n+\tprotected final PackTransport transport;\n+\n \t/** Buffered input stream reading from the remote. */\n \tprotected InputStream in;\n \n@@ -93,6 +96,7 @@\n \tBasePackConnection(final PackTransport packTransport) {\n \t\tlocal = packTransport.local;\n \t\turi = packTransport.uri;\n+\t\ttransport = packTransport;\n \t}\n \n \tprotected void init(final InputStream myIn, final OutputStream myOut) {\n@@ -129,15 +133,8 @@ private void readAdvertisedRefsImpl() throws IOException {\n \t\t\ttry {\n \t\t\t\tline = pckIn.readString();\n \t\t\t} catch (EOFException eof) {\n-\t\t\t\tif (avail.isEmpty()) {\n-\t\t\t\t\tString service = \"unknown\";\n-\t\t\t\t\tif (this instanceof PushConnection)\n-\t\t\t\t\t\tservice = \"push\";\n-\t\t\t\t\telse if (this instanceof FetchConnection)\n-\t\t\t\t\t\tservice = \"fetch\";\n-\t\t\t\t\tthrow new NoRemoteRepositoryException(uri, service\n-\t\t\t\t\t\t\t+ \" service not found.\");\n-\t\t\t\t}\n+\t\t\t\tif (avail.isEmpty())\n+\t\t\t\t\tthrow noRepository();\n \t\t\t\tthrow eof;\n \t\t\t}\n \n@@ -185,6 +182,10 @@ else if (this instanceof FetchConnection)\n \t\tavailable(avail);\n \t}\n \n+\tprotected TransportException noRepository() {\n+\t\treturn new NoRemoteRepositoryException(uri, \"not found.\");\n+\t}\n+\n \tprotected boolean isCapableOf(final String option) {\n \t\treturn remoteCapablities.contains(option);\n \t}\ndiff --git a/org.spearce.jgit/src/org/spearce/jgit/transport/BasePackPushConnection.java b/org.spearce.jgit/src/org/spearce/jgit/transport/BasePackPushConnection.java\nindex a2d5b6f..a6ab9c4 100644\n--- a/org.spearce.jgit/src/org/spearce/jgit/transport/BasePackPushConnection.java\n+++ b/org.spearce.jgit/src/org/spearce/jgit/transport/BasePackPushConnection.java\n@@ -43,6 +43,8 @@\n import java.util.Collection;\n import java.util.Map;\n \n+import org.spearce.jgit.errors.NoRemoteRepositoryException;\n+import org.spearce.jgit.errors.NotSupportedException;\n import org.spearce.jgit.errors.PackProtocolException;\n import org.spearce.jgit.errors.TransportException;\n import org.spearce.jgit.lib.ObjectId;\n@@ -98,6 +100,29 @@ public void push(final ProgressMonitor monitor,\n \t\tdoPush(monitor, refUpdates);\n \t}\n \n+\t@Override\n+\tprotected TransportException noRepository() {\n+\t\t// Sadly we cannot tell the \"invalid URI\" case from \"push not allowed\".\n+\t\t// Opening a fetch connection can help us tell the difference, as any\n+\t\t// useful repository is going to support fetch if it also would allow\n+\t\t// push. So if fetch throws NoRemoteRepositoryException we know the\n+\t\t// URI is wrong. Otherwise we can correctly state push isn't allowed\n+\t\t// as the fetch connection opened successfully.\n+\t\t//\n+\t\ttry {\n+\t\t\ttransport.openFetch().close();\n+\t\t} catch (NotSupportedException e) {\n+\t\t\t// Fall through.\n+\t\t} catch (NoRemoteRepositoryException e) {\n+\t\t\t// Fetch concluded the repository doesn't exist.\n+\t\t\t//\n+\t\t\treturn e;\n+\t\t} catch (TransportException e) {\n+\t\t\t// Fall through.\n+\t\t}\n+\t\treturn new TransportException(uri, \"push not permitted\");\n+\t}\n+\n \tprotected void doPush(final ProgressMonitor monitor,\n \t\t\tfinal Map<String, RemoteRefUpdate> refUpdates)\n \t\t\tthrows TransportException {\n-- \n1.6.0.174.gd789c\n"},{"id":"89113","messageId":"200808291120.44413.robin.rosenberg@dewire.com","threadId":"15269","inReplyTo":"1219969118-31672-1-git-send-email-spearce@spearce.org","subject":"Re: [JGIT PATCH] Disambiguate \"push not supported\" from \"repository not found\"","fromName":"Robin Rosenberg","fromEmail":"robin.rosenberg@dewire.com","sentAt":"2008-08-29T09:20:44Z","receivedAt":"2008-08-29T09:20:44Z","isPatch":true,"sender":{"key":"robin.rosenberg@dewire.com","avatar":"https://avatars.githubusercontent.com/u/46357?v=4"},"body":"fredagen den 29 augusti 2008 02.18.38 skrev Shawn O. Pearce:\n> +\t\t\t\tif (avail.isEmpty())\n> +\t\t\t\t\tthrow noRepository();\n>  \t\t\t\tthrow eof;\n>  \t\t\t}\n>  \n> @@ -185,6 +182,10 @@ else if (this instanceof FetchConnection)\n>  \t\tavailable(avail);\n>  \t}\n>  \n> +\tprotected TransportException noRepository() {\n> +\t\treturn new NoRemoteRepositoryException(uri, \"not found.\");\n> +\t}\n> +\n\nWhy an extra method for instantiating the exception?\n\n-- robin\n"},{"id":"89126","messageId":"48B7E927.2000205@gmail.com","threadId":"15269","inReplyTo":"200808291120.44413.robin.rosenberg@dewire.com","subject":"Re: [JGIT PATCH] Disambiguate \"push not supported\" from \"repository not found\"","fromName":"Marek Zawirski","fromEmail":"marek.zawirski@gmail.com","sentAt":"2008-08-29T12:18:47Z","receivedAt":"2008-08-29T12:18:47Z","isPatch":true,"sender":{"key":"marek.zawirski@gmail.com","avatar":null},"body":"Robin Rosenberg wrote:\n> fredagen den 29 augusti 2008 02.18.38 skrev Shawn O. Pearce:\n>> +\t\t\t\tif (avail.isEmpty())\n>> +\t\t\t\t\tthrow noRepository();\n>>  \t\t\t\tthrow eof;\n>>  \t\t\t}\n>>  \n>> @@ -185,6 +182,10 @@ else if (this instanceof FetchConnection)\n>>  \t\tavailable(avail);\n>>  \t}\n>>  \n>> +\tprotected TransportException noRepository() {\n>> +\t\treturn new NoRemoteRepositoryException(uri, \"not found.\");\n>> +\t}\n>> +\n> \n> Why an extra method for instantiating the exception?\n\nIsn't it overrode in subclass - BasePackPushConnection?\n-- \nMarek Zawirski [zawir]\nmarek.zawirski@gmail.com\n"},{"id":"89121","messageId":"20080829143116.GB7403@spearce.org","threadId":"15269","inReplyTo":"48B7E927.2000205@gmail.com","subject":"Re: [JGIT PATCH] Disambiguate \"push not supported\" from \"repository not found\"","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2008-08-29T14:31:16Z","receivedAt":"2008-08-29T14:31:16Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Marek Zawirski <marek.zawirski@gmail.com> wrote:\n> Robin Rosenberg wrote:\n>> fredagen den 29 augusti 2008 02.18.38 skrev Shawn O. Pearce:\n>>> +\t\t\t\tif (avail.isEmpty())\n>>> +\t\t\t\t\tthrow noRepository();\n>>>  \t\t\t\tthrow eof;\n>>>  \t\t\t}\n>>>  @@ -185,6 +182,10 @@ else if (this instanceof FetchConnection)\n>>>  \t\tavailable(avail);\n>>>  \t}\n>>>  +\tprotected TransportException noRepository() {\n>>> +\t\treturn new NoRemoteRepositoryException(uri, \"not found.\");\n>>> +\t}\n>>> +\n>>\n>> Why an extra method for instantiating the exception?\n>\n> Isn't it overrode in subclass - BasePackPushConnection?\n\nCorrect.  I introduced the method so the subclass can inject its\nown implementation for the catch block.  But its required to give\nback a TransportException so the catch block can throw it, as we\ndo not want the subclass to be able to continue at this point.\n\n-- \nShawn.\n"},{"id":"89296","messageId":"200808311028.59348.robin.rosenberg@dewire.com","threadId":"15269","inReplyTo":"20080829143116.GB7403@spearce.org","subject":"Re: [JGIT PATCH] Disambiguate \"push not supported\" from \"repository not found\"","fromName":"Robin Rosenberg","fromEmail":"robin.rosenberg@dewire.com","sentAt":"2008-08-31T08:28:58Z","receivedAt":"2008-08-31T08:28:58Z","isPatch":true,"sender":{"key":"robin.rosenberg@dewire.com","avatar":"https://avatars.githubusercontent.com/u/46357?v=4"},"body":"fredagen den 29 augusti 2008 16.31.16 skrev Shawn O. Pearce:\n> Marek Zawirski <marek.zawirski@gmail.com> wrote:\n> > Robin Rosenberg wrote:\n> >>\n> >> Why an extra method for instantiating the exception?\n> >\n> > Isn't it overrode in subclass - BasePackPushConnection?\n> \n> Correct.  I introduced the method so the subclass can inject its\n> own implementation for the catch block.  But its required to give\n> back a TransportException so the catch block can throw it, as we\n> do not want the subclass to be able to continue at this point.\n\nMind if I squash this into the patch?\n\n-- robin\n\ndiff --git a/org.spearce.jgit/src/org/spearce/jgit/transport/BasePackConnection.java b/org.spearce.jgit/src/org/spearce/jgit/transport/BasePackConnection.java\nindex e35f850..16e4897 100644\n--- a/org.spearce.jgit/src/org/spearce/jgit/transport/BasePackConnection.java\n+++ b/org.spearce.jgit/src/org/spearce/jgit/transport/BasePackConnection.java\n@@ -182,6 +182,15 @@ private void readAdvertisedRefsImpl() throws IOException {\n                available(avail);\n        }\n\n+       /**\n+        * Create an exception to indicate problems finding a remote repository. The\n+        * caller is expected to throw the returned exception.\n+        *\n+        * Subclasses may override this method to provide better diagnostics.\n+        *\n+        * @return a TransportException saying a repository cannot be found and\n+        *         possibly why.\n+        */\n        protected TransportException noRepository() {\n                return new NoRemoteRepositoryException(uri, \"not found.\");\n        }\n"},{"id":"89479","messageId":"20080902054213.GE13248@spearce.org","threadId":"15269","inReplyTo":"200808311028.59348.robin.rosenberg@dewire.com","subject":"Re: [JGIT PATCH] Disambiguate \"push not supported\" from \"repository not found\"","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2008-09-02T05:42:13Z","receivedAt":"2008-09-02T05:42:13Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Robin Rosenberg <robin.rosenberg@dewire.com> wrote:\n> fredagen den 29 augusti 2008 16.31.16 skrev Shawn O. Pearce:\n> > Marek Zawirski <marek.zawirski@gmail.com> wrote:\n> > > Robin Rosenberg wrote:\n> > >>\n> > >> Why an extra method for instantiating the exception?\n> > >\n> > > Isn't it overrode in subclass - BasePackPushConnection?\n> > \n> > Correct.  I introduced the method so the subclass can inject its\n> > own implementation for the catch block.  But its required to give\n> > back a TransportException so the catch block can throw it, as we\n> > do not want the subclass to be able to continue at this point.\n> \n> Mind if I squash this into the patch?\n\nNo, not at all.  This looks fine.\n \n\n> diff --git a/org.spearce.jgit/src/org/spearce/jgit/transport/BasePackConnection.java b/org.spearce.jgit/src/org/spearce/jgit/transport/BasePackConnection.java\n> index e35f850..16e4897 100644\n> --- a/org.spearce.jgit/src/org/spearce/jgit/transport/BasePackConnection.java\n> +++ b/org.spearce.jgit/src/org/spearce/jgit/transport/BasePackConnection.java\n> @@ -182,6 +182,15 @@ private void readAdvertisedRefsImpl() throws IOException {\n>                 available(avail);\n>         }\n> \n> +       /**\n> +        * Create an exception to indicate problems finding a remote repository. The\n> +        * caller is expected to throw the returned exception.\n> +        *\n> +        * Subclasses may override this method to provide better diagnostics.\n> +        *\n> +        * @return a TransportException saying a repository cannot be found and\n> +        *         possibly why.\n> +        */\n>         protected TransportException noRepository() {\n>                 return new NoRemoteRepositoryException(uri, \"not found.\");\n>         }\n\n-- \nShawn.\n"}]}