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

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

[Marek Zawirski]

Reported-by: Robert <robert_no.spam_m@yahoo.fr>
Signed-off-by: Marek Zawirski <marek.zawirski@gmail.com>
---

See http://code.google.com/p/egit/issues/detail?id=16

 .../spearce/jgit/transport/BasePackConnection.java |   11 +++++++++--
 1 files changed, 9 insertions(+), 2 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..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.");
+				}
 				throw eof;
 			}
 
-- 
1.5.6.3

[EGIT PATCH 2/3] Handle NoRemoteRepositoryException in PushOperation especially

[Marek Zawirski]

We can give user better feedback for this special case, as it
is common and not directly related to protocol error.

Reported-by: Robert <robert_no.spam_m@yahoo.fr>
Signed-off-by: Marek Zawirski <marek.zawirski@gmail.com>
---

Beside of this change, we may consider removing URI part of
TransportException from message, moving it to the field and providing
API for getting this URI. That may allow us showing user cleaner
messages.

 .../src/org/spearce/egit/core/CoreText.java        |    3 +++
 .../src/org/spearce/egit/core/coretext.properties  |    1 +
 .../org/spearce/egit/core/op/PushOperation.java    |    5 +++++
 3 files changed, 9 insertions(+), 0 deletions(-)

diff --git a/org.spearce.egit.core/src/org/spearce/egit/core/CoreText.java b/org.spearce.egit.core/src/org/spearce/egit/core/CoreText.java
index 35e17b9..a750117 100644
--- a/org.spearce.egit.core/src/org/spearce/egit/core/CoreText.java
+++ b/org.spearce.egit.core/src/org/spearce/egit/core/CoreText.java
@@ -108,6 +108,9 @@
 	public static String PushOperation_resultTransportError;
 
 	/** */
+	public static String PushOperation_resultNoServiceError;
+
+	/** */
 	public static String PushOperation_taskNameDryRun;
 
 	/** */
diff --git a/org.spearce.egit.core/src/org/spearce/egit/core/coretext.properties b/org.spearce.egit.core/src/org/spearce/egit/core/coretext.properties
index 94cf4aa..04ca28f 100644
--- a/org.spearce.egit.core/src/org/spearce/egit/core/coretext.properties
+++ b/org.spearce.egit.core/src/org/spearce/egit/core/coretext.properties
@@ -61,5 +61,6 @@ ListRemoteOperation_title=Getting remote branches information
 PushOperation_resultCancelled=Operation was cancelled.
 PushOperation_resultNotSupported=Can't push to {0}
 PushOperation_resultTransportError=Transport error occured during push operation: {0}
+PushOperation_resultNoServiceError=Push service is not available: {0}
 PushOperation_taskNameDryRun=Trying pushing to remote repositories
 PushOperation_taskNameNormalRun=Pushing to remote repositories
diff --git a/org.spearce.egit.core/src/org/spearce/egit/core/op/PushOperation.java b/org.spearce.egit.core/src/org/spearce/egit/core/op/PushOperation.java
index 8811800..a0f2e5c 100644
--- a/org.spearce.egit.core/src/org/spearce/egit/core/op/PushOperation.java
+++ b/org.spearce.egit.core/src/org/spearce/egit/core/op/PushOperation.java
@@ -16,6 +16,7 @@
 import org.eclipse.osgi.util.NLS;
 import org.spearce.egit.core.CoreText;
 import org.spearce.egit.core.EclipseGitProgressTransformer;
+import org.spearce.jgit.errors.NoRemoteRepositoryException;
 import org.spearce.jgit.errors.NotSupportedException;
 import org.spearce.jgit.errors.TransportException;
 import org.spearce.jgit.lib.Repository;
@@ -125,6 +126,10 @@ public void run(IProgressMonitor monitor) throws InvocationTargetException {
 				final PushResult pr = transport.push(gitSubMonitor,
 						specification.getRefUpdates(uri));
 				operationResult.addOperationResult(uri, pr);
+			} catch (final NoRemoteRepositoryException e) {
+				operationResult.addOperationResult(uri, NLS.bind(
+						CoreText.PushOperation_resultNoServiceError, e
+								.getMessage()));
 			} catch (final TransportException e) {
 				operationResult.addOperationResult(uri, NLS.bind(
 						CoreText.PushOperation_resultTransportError, e
-- 
1.5.6.3

[EGIT PATCH 3/3] Show ErrorDialog fot fatal connection errors in ConfirmationPage

[Marek Zawirski]

We already do the same in analogous RefSpecPage and PushWizard etc., so
let's do the same here.

Signed-off-by: Marek Zawirski <marek.zawirski@gmail.com>
---
 .../src/org/spearce/egit/ui/UIText.java            |    3 +++
 .../egit/ui/internal/push/ConfirmationPage.java    |   16 ++++++++++++++--
 .../src/org/spearce/egit/ui/uitext.properties      |    1 +
 3 files changed, 18 insertions(+), 2 deletions(-)

diff --git a/org.spearce.egit.ui/src/org/spearce/egit/ui/UIText.java b/org.spearce.egit.ui/src/org/spearce/egit/ui/UIText.java
index b2cb340..b09cc10 100644
--- a/org.spearce.egit.ui/src/org/spearce/egit/ui/UIText.java
+++ b/org.spearce.egit.ui/src/org/spearce/egit/ui/UIText.java
@@ -518,6 +518,9 @@
 	public static String PushWizard_windowTitleWithDestination;
 
 	/** */
+	public static String ConfirmationPage_cantConnectToAnyTitle;
+
+	/** */
 	public static String ConfirmationPage_cantConnectToAny;
 
 	/** */
diff --git a/org.spearce.egit.ui/src/org/spearce/egit/ui/internal/push/ConfirmationPage.java b/org.spearce.egit.ui/src/org/spearce/egit/ui/internal/push/ConfirmationPage.java
index 08d21b3..6e925a7 100644
--- a/org.spearce.egit.ui/src/org/spearce/egit/ui/internal/push/ConfirmationPage.java
+++ b/org.spearce.egit.ui/src/org/spearce/egit/ui/internal/push/ConfirmationPage.java
@@ -13,6 +13,9 @@
 import java.util.Collection;
 import java.util.List;
 
+import org.eclipse.core.runtime.IStatus;
+import org.eclipse.core.runtime.Status;
+import org.eclipse.jface.dialogs.ErrorDialog;
 import org.eclipse.jface.wizard.WizardPage;
 import org.eclipse.osgi.util.NLS;
 import org.eclipse.swt.SWT;
@@ -24,6 +27,7 @@
 import org.spearce.egit.core.op.PushOperation;
 import org.spearce.egit.core.op.PushOperationResult;
 import org.spearce.egit.core.op.PushOperationSpecification;
+import org.spearce.egit.ui.Activator;
 import org.spearce.egit.ui.UIText;
 import org.spearce.egit.ui.internal.components.RefSpecPage;
 import org.spearce.egit.ui.internal.components.RepositorySelection;
@@ -204,8 +208,16 @@ setErrorMessage(NLS.bind(UIText.ConfirmationPage_errorUnexpected, e
 			setPageComplete(true);
 			confirmedResult = result;
 		} else {
-			setErrorMessage(NLS.bind(UIText.ConfirmationPage_cantConnectToAny,
-					result.getErrorStringForAllURis()));
+			final String message = NLS.bind(
+					UIText.ConfirmationPage_cantConnectToAny, result
+							.getErrorStringForAllURis());
+			setErrorMessage(message);
+			ErrorDialog
+					.openError(getShell(),
+							UIText.ConfirmationPage_cantConnectToAnyTitle,
+							null,
+							new Status(IStatus.ERROR, Activator.getPluginId(),
+									message));
 		}
 	}
 }
diff --git a/org.spearce.egit.ui/src/org/spearce/egit/ui/uitext.properties b/org.spearce.egit.ui/src/org/spearce/egit/ui/uitext.properties
index 0590e30..22e29c2 100644
--- a/org.spearce.egit.ui/src/org/spearce/egit/ui/uitext.properties
+++ b/org.spearce.egit.ui/src/org/spearce/egit/ui/uitext.properties
@@ -199,6 +199,7 @@ PushWizard_unexpectedError=Unexpected error occurred.
 PushWizard_windowTitleDefault=Push To Another Repositories
 PushWizard_windowTitleWithDestination=Push To: {0}
 
+ConfirmationPage_cantConnectToAnyTitle=Can't Connect
 ConfirmationPage_cantConnectToAny=Can't connect to any URI: {0}
 ConfirmationPage_description=Confirm following expected push result.
 ConfirmationPage_errorCantResolveSpecs=Can't resolve ref specifications locally or create tracking ref update: {0}
-- 
1.5.6.3

Re: [EGIT PATCH 2/3] Handle NoRemoteRepositoryException in PushOperation especially

[Shawn O. Pearce]

Marek Zawirski <marek.zawirski@gmail.com> wrote:
> diff --git a/org.spearce.egit.core/src/org/spearce/egit/core/coretext.properties b/org.spearce.egit.core/src/org/spearce/egit/core/coretext.properties
> index 94cf4aa..04ca28f 100644
> --- a/org.spearce.egit.core/src/org/spearce/egit/core/coretext.properties
> +++ b/org.spearce.egit.core/src/org/spearce/egit/core/coretext.properties
> @@ -61,5 +61,6 @@ ListRemoteOperation_title=Getting remote branches information
>  PushOperation_resultCancelled=Operation was cancelled.
>  PushOperation_resultNotSupported=Can't push to {0}
>  PushOperation_resultTransportError=Transport error occured during push operation: {0}
> +PushOperation_resultNoServiceError=Push service is not available: {0}
>  PushOperation_taskNameDryRun=Trying pushing to remote repositories
>  PushOperation_taskNameNormalRun=Pushing to remote repositories
> diff --git a/org.spearce.egit.core/src/org/spearce/egit/core/op/PushOperation.java b/org.spearce.egit.core/src/org/spearce/egit/core/op/PushOperation.java
> index 8811800..a0f2e5c 100644
> --- a/org.spearce.egit.core/src/org/spearce/egit/core/op/PushOperation.java
> +++ b/org.spearce.egit.core/src/org/spearce/egit/core/op/PushOperation.java
> @@ -125,6 +126,10 @@ public void run(IProgressMonitor monitor) throws InvocationTargetException {
>  				final PushResult pr = transport.push(gitSubMonitor,
>  						specification.getRefUpdates(uri));
>  				operationResult.addOperationResult(uri, pr);
> +			} catch (final NoRemoteRepositoryException e) {
> +				operationResult.addOperationResult(uri, NLS.bind(
> +						CoreText.PushOperation_resultNoServiceError, e
> +								.getMessage()));

Isn't this when combined with the prior patch going to result in a message like:

	Push service is not available: git://repo.or.cz/egit.git push service is not available

which is sort of redundant and confusingly redundant, isn't it?

-- 
Shawn.

Re: [EGIT PATCH 3/3] Show ErrorDialog fot fatal connection errors in ConfirmationPage

[Shawn O. Pearce]

> Subject: Re: [EGIT PATCH 3/3] Show ErrorDialog fot fatal connection errors

typo ----------------------------------------------*

-- 
Shawn.

Re: [EGIT PATCH 2/3] Handle NoRemoteRepositoryException in PushOperation especially

[Marek Zawirski]

Shawn O. Pearce wrote:
> Marek Zawirski <marek.zawirski@gmail.com> wrote:
>> diff --git a/org.spearce.egit.core/src/org/spearce/egit/core/coretext.properties b/org.spearce.egit.core/src/org/spearce/egit/core/coretext.properties
>> index 94cf4aa..04ca28f 100644
>> --- a/org.spearce.egit.core/src/org/spearce/egit/core/coretext.properties
>> +++ b/org.spearce.egit.core/src/org/spearce/egit/core/coretext.properties
>> @@ -61,5 +61,6 @@ ListRemoteOperation_title=Getting remote branches information
>>  PushOperation_resultCancelled=Operation was cancelled.
>>  PushOperation_resultNotSupported=Can't push to {0}
>>  PushOperation_resultTransportError=Transport error occured during push operation: {0}
>> +PushOperation_resultNoServiceError=Push service is not available: {0}
>>  PushOperation_taskNameDryRun=Trying pushing to remote repositories
>>  PushOperation_taskNameNormalRun=Pushing to remote repositories
>> diff --git a/org.spearce.egit.core/src/org/spearce/egit/core/op/PushOperation.java b/org.spearce.egit.core/src/org/spearce/egit/core/op/PushOperation.java
>> index 8811800..a0f2e5c 100644
>> --- a/org.spearce.egit.core/src/org/spearce/egit/core/op/PushOperation.java
>> +++ b/org.spearce.egit.core/src/org/spearce/egit/core/op/PushOperation.java
>> @@ -125,6 +126,10 @@ public void run(IProgressMonitor monitor) throws InvocationTargetException {
>>  				final PushResult pr = transport.push(gitSubMonitor,
>>  						specification.getRefUpdates(uri));
>>  				operationResult.addOperationResult(uri, pr);
>> +			} catch (final NoRemoteRepositoryException e) {
>> +				operationResult.addOperationResult(uri, NLS.bind(
>> +						CoreText.PushOperation_resultNoServiceError, e
>> +								.getMessage()));
> 
> Isn't this when combined with the prior patch going to result in a message like:
> 
> 	Push service is not available: git://repo.or.cz/egit.git push service is not available
> 
> which is sort of redundant and confusingly redundant, isn't it?

More precisely:
Push service is not available: git://repo.or.cz/egit.git push service 
not found

Yeah, this commit may be removed as well, leaving user with info:
Transport error occured during push operation: git://repo.or.cz/egit.git 
push service not found

-- 
Marek Zawirski [zawir]
marek.zawirski@gmail.com

Re: [EGIT PATCH 3/3] Show ErrorDialog fot fatal connection errors in ConfirmationPage

[Marek Zawirski]

Shawn O. Pearce wrote:
>> Subject: Re: [EGIT PATCH 3/3] Show ErrorDialog fot fatal connection errors
> 
> typo ----------------------------------------------*
> 

Ah, right - thanks. I believe that I don't need to resend that? Will 
edit/rebase on my repo.


-- 
Marek Zawirski [zawir]
marek.zawirski@gmail.com

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

[Shawn O. Pearce]

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.

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

[Marek Zawirski]

Shawn O. Pearce wrote:
(...)
> +	@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");
> +	}
> +

Nice idea, even if it's crazy and time-consuming, it's probably better 
than my previous one.

-- 
Marek Zawirski [zawir]
marek.zawirski@gmail.com

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

[Shawn O. Pearce]

Marek Zawirski <marek.zawirski@gmail.com> wrote:
> Shawn O. Pearce wrote:
> (...)
>> +	@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");
>> +	}
>> +
>
> Nice idea, even if it's crazy and time-consuming, it's probably better  
> than my previous one.

I'm not too worried about the extra time used here.

This happens only after we have already opened a connection and
received no refs at all from the remote peer.  So the user has
already had to wait to get this far.

By asking the same transport to open the fetch we can reuse an
existing SSH tunnel for the new command if this is an SSH connection,
so the setup costs are a lot lower then the original connection.

We are already in a bad error condition; we cannot continue and
the user is about to get an error.  I would rather give them the
best error message we can determine than abort early and give them
something misleading.

-- 
Shawn.