[jgit PATCH] Paper bag fix quoting for SSH transport commands

[jgit PATCH] Paper bag fix quoting for SSH transport commands

[Shawn O. Pearce]

Not all Git-over-SSH servers run a Bourne shell on the remote side
to evaluate the command we are sending.  Some servers run git-shell,
which will fail to execute git-upload-pack if we feed it a quoted
string for the name git-upload-pack.

Testing concludes that git-shell requires the command name to never
be quoted, and the argument name to always be single quoted.  As
this is a long-standing behavior in the wild jgit needs to conform,
as git-shell and all git-shell work-a-likes such as gitosis may be
following the same convention.

Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---

 If there are no arguments I'll push this into the public tree.
 It seems right on the surface, and is necessary to use jgit against
 repo.or.cz, and probably many other sites like it.

 .../spearce/jgit/transport/TransportGitSsh.java    |   22 ++++++++++++++++++-
 1 files changed, 20 insertions(+), 2 deletions(-)

diff --git a/org.spearce.jgit/src/org/spearce/jgit/transport/TransportGitSsh.java b/org.spearce.jgit/src/org/spearce/jgit/transport/TransportGitSsh.java
index d31c525..8944df7 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/transport/TransportGitSsh.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/transport/TransportGitSsh.java
@@ -88,6 +88,24 @@ class TransportGitSsh extends PackTransport {
 		return new SshFetchConnection();
 	}
 
+	private static void sqMinimal(final StringBuilder cmd, final String val) {
+		if (val.matches("^[a-zA-Z0-9._/-]*$")) {
+			// If the string matches only generally safe characters
+			// that the shell is not going to evaluate specially we
+			// should leave the string unquoted. Not all systems
+			// actually run a shell and over-quoting confuses them
+			// when it comes to the command name.
+			//
+			cmd.append(val);
+		} else {
+			sq(cmd, val);
+		}
+	}
+
+	private static void sqAlways(final StringBuilder cmd, final String val) {
+		sq(cmd, val);
+	}
+
 	private static void sq(final StringBuilder cmd, final String val) {
 		int i = 0;
 
@@ -157,9 +175,9 @@ class TransportGitSsh extends PackTransport {
 				path = (uri.getPath().substring(1));
 
 			final StringBuilder cmd = new StringBuilder();
-			sq(cmd, exe);
+			sqMinimal(cmd, exe);
 			cmd.append(' ');
-			sq(cmd, path);
+			sqAlways(cmd, path);
 			channel.setCommand(cmd.toString());
 			channel.setErrStream(System.err);
 			channel.connect();
-- 
1.5.6.74.g8a5e

[PATCH] Take care of errors reported from the server when upload command is started

[Robin Rosenberg]

When JSch cannot launch the command our SSH Channel will close immediately
in connect, resulting in a NullPointerException and a closed System.err
silencing an errors.

Using System.err for errors also means we get no feedback when using from
Eclipse.
---
 .../spearce/egit/ui/EclipseSshSessionFactory.java  |   29 ++++++++++++++++++++
 .../jgit/transport/DefaultSshSessionFactory.java   |    6 ++++
 .../spearce/jgit/transport/SshSessionFactory.java  |   10 +++++++
 .../spearce/jgit/transport/TransportGitSsh.java    |   12 +++++++-
 4 files changed, 55 insertions(+), 2 deletions(-)

diff --git a/org.spearce.egit.ui/src/org/spearce/egit/ui/EclipseSshSessionFactory.java b/org.spearce.egit.ui/src/org/spearce/egit/ui/EclipseSshSessionFactory.java
index 5005494..144d47d 100644
--- a/org.spearce.egit.ui/src/org/spearce/egit/ui/EclipseSshSessionFactory.java
+++ b/org.spearce.egit.ui/src/org/spearce/egit/ui/EclipseSshSessionFactory.java
@@ -8,6 +8,8 @@
  *******************************************************************************/
 package org.spearce.egit.ui;
 
+import java.io.IOException;
+import java.io.OutputStream;
 import java.security.AccessController;
 import java.security.PrivilegedAction;
 
@@ -44,4 +46,31 @@ class EclipseSshSessionFactory extends SshSessionFactory {
 			}
 		});
 	}
+
+	@Override
+	public OutputStream getErrorStream() {
+		return new OutputStream() {
+
+			StringBuilder all = new StringBuilder();
+			StringBuilder sb = new StringBuilder();
+
+			public String toString() {
+				return all.toString();
+			}
+
+			@Override
+			public void write(int b) throws IOException {
+				if (b == '\r')
+					return;
+				sb.append((char) b);
+				if (b == '\n') {
+					String s = sb.toString();
+					all.append(s);
+					sb = new StringBuilder();
+					Activator.logError(s, new Throwable());
+				}
+			}
+		};
+	}
+
 }
diff --git a/org.spearce.jgit/src/org/spearce/jgit/transport/DefaultSshSessionFactory.java b/org.spearce.jgit/src/org/spearce/jgit/transport/DefaultSshSessionFactory.java
index 8a59904..5924a04 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/transport/DefaultSshSessionFactory.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/transport/DefaultSshSessionFactory.java
@@ -46,6 +46,7 @@ import java.io.File;
 import java.io.FileInputStream;
 import java.io.FileNotFoundException;
 import java.io.IOException;
+import java.io.OutputStream;
 import java.security.AccessController;
 import java.security.PrivilegedAction;
 
@@ -249,4 +250,9 @@ class DefaultSshSessionFactory extends SshSessionFactory {
 			return null; // cancel
 		}
 	}
+
+	@Override
+	public OutputStream getErrorStream() {
+		return System.err;
+	}
 }
diff --git a/org.spearce.jgit/src/org/spearce/jgit/transport/SshSessionFactory.java b/org.spearce.jgit/src/org/spearce/jgit/transport/SshSessionFactory.java
index 7f8136d..4c9eae0 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/transport/SshSessionFactory.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/transport/SshSessionFactory.java
@@ -38,6 +38,8 @@
 
 package org.spearce.jgit.transport;
 
+import java.io.OutputStream;
+
 import com.jcraft.jsch.JSchException;
 import com.jcraft.jsch.Session;
 
@@ -121,4 +123,12 @@ public abstract class SshSessionFactory {
 		if (session.isConnected())
 			session.disconnect();
 	}
+
+	/**
+	 * Find or create an OutputStream for Ssh to use. For a command line client
+	 * this is probably System.err.
+	 * 
+	 * @return an OutputStream to receive the SSH error stream.
+	 */
+	public abstract OutputStream getErrorStream();
 }
diff --git a/org.spearce.jgit/src/org/spearce/jgit/transport/TransportGitSsh.java b/org.spearce.jgit/src/org/spearce/jgit/transport/TransportGitSsh.java
index 8944df7..82723a3 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/transport/TransportGitSsh.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/transport/TransportGitSsh.java
@@ -39,6 +39,7 @@
 package org.spearce.jgit.transport;
 
 import java.io.IOException;
+import java.io.OutputStream;
 import java.net.ConnectException;
 import java.net.UnknownHostException;
 
@@ -77,6 +78,7 @@ class TransportGitSsh extends PackTransport {
 	}
 
 	final SshSessionFactory sch;
+	OutputStream errStream;
 
 	TransportGitSsh(final Repository local, final URIish uri) {
 		super(local, uri);
@@ -179,7 +181,8 @@ class TransportGitSsh extends PackTransport {
 			cmd.append(' ');
 			sqAlways(cmd, path);
 			channel.setCommand(cmd.toString());
-			channel.setErrStream(System.err);
+			errStream = SshSessionFactory.getInstance().getErrorStream();
+			channel.setErrStream(errStream, true); 
 			channel.connect();
 			return channel;
 		} catch (JSchException je) {
@@ -198,7 +201,12 @@ class TransportGitSsh extends PackTransport {
 			try {
 				session = openSession();
 				channel = exec(session, getOptionUploadPack());
-				init(channel.getInputStream(), channel.getOutputStream());
+
+				if (channel.isConnected())
+					init(channel.getInputStream(), channel.getOutputStream());
+				else
+					throw new TransportException(errStream.toString());
+					
 			} catch (TransportException err) {
 				close();
 				throw err;
-- 
1.5.5.1.178.g1f811

[PATCH] Clone: Handle cancel in clone dialog specially

[Robin Rosenberg]

Signed-off-by: Robin Rosenberg <robin.rosenberg@dewire.com>
---
 .../src/org/spearce/egit/ui/UIText.java            |    3 +++
 .../egit/ui/internal/clone/SourceBranchPage.java   |   18 ++++++++++++------
 .../src/org/spearce/egit/ui/uitext.properties      |    2 ++
 3 files changed, 17 insertions(+), 6 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 9ccf606..4adb99c 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
@@ -122,6 +122,9 @@ public class UIText extends NLS {
 	public static String SourceBranchPage_cannotListBranches;
 
 	/** */
+	public static String SourceBranchPage_remoteListingCancelled;
+
+	/** */
 	public static String CloneDestinationPage_title;
 
 	/** */
diff --git a/org.spearce.egit.ui/src/org/spearce/egit/ui/internal/clone/SourceBranchPage.java b/org.spearce.egit.ui/src/org/spearce/egit/ui/internal/clone/SourceBranchPage.java
index b2f1a18..615be32 100644
--- a/org.spearce.egit.ui/src/org/spearce/egit/ui/internal/clone/SourceBranchPage.java
+++ b/org.spearce.egit.ui/src/org/spearce/egit/ui/internal/clone/SourceBranchPage.java
@@ -20,6 +20,7 @@ import java.util.List;
 import org.eclipse.core.runtime.IProgressMonitor;
 import org.eclipse.core.runtime.IStatus;
 import org.eclipse.core.runtime.NullProgressMonitor;
+import org.eclipse.core.runtime.OperationCanceledException;
 import org.eclipse.core.runtime.Status;
 import org.eclipse.jface.dialogs.ErrorDialog;
 import org.eclipse.jface.operation.IRunnableWithProgress;
@@ -269,12 +270,17 @@ class SourceBranchPage extends WizardPage {
 			});
 		} catch (InvocationTargetException e) {
 			Throwable why = e.getCause();
-			ErrorDialog.openError(getShell(),
-					UIText.SourceBranchPage_transportError,
-					UIText.SourceBranchPage_cannotListBranches, new Status(
-							IStatus.ERROR, Activator.getPluginId(), 0, why
-									.getMessage(), why.getCause()));
-			transportError(why.getMessage());
+			if ((why instanceof OperationCanceledException)) {
+				transportError(UIText.SourceBranchPage_remoteListingCancelled);
+				return;
+			} else {
+				ErrorDialog.openError(getShell(),
+						UIText.SourceBranchPage_transportError,
+						UIText.SourceBranchPage_cannotListBranches, new Status(
+								IStatus.ERROR, Activator.getPluginId(), 0, why
+										.getMessage(), why.getCause()));
+				transportError(why.getMessage());
+			}
 			return;
 		} catch (InterruptedException e) {
 			transportError(UIText.SourceBranchPage_interrupted);
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 18f8c28..9516aa0 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
@@ -55,6 +55,8 @@ SourceBranchPage_errorBranchRequired=At least one branch must be selected.
 SourceBranchPage_transportError=Transport Error
 SourceBranchPage_cannotListBranches=Cannot list the available branches.
 SourceBranchPage_interrupted=Connection attempt interrupted.
+SourceBranchPage_remoteListingCancelled=Operation cancelled
+
 
 CloneDestinationPage_title=Local Destination
 CloneDestinationPage_description=Configure the local storage location for {0}.
-- 
1.5.5.1.178.g1f811

[PATCH] Clone: If url is changed was changed, forget the old value

[Robin Rosenberg]

This fixes a bug found by some monkey testing where the dialog
get stuck in a state where changing connection parameters have
no effect.

Signed-off-by: Robin Rosenberg <robin.rosenberg@dewire.com>
---
 .../egit/ui/internal/clone/SourceBranchPage.java   |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/org.spearce.egit.ui/src/org/spearce/egit/ui/internal/clone/SourceBranchPage.java b/org.spearce.egit.ui/src/org/spearce/egit/ui/internal/clone/SourceBranchPage.java
index 615be32..b704aaa 100644
--- a/org.spearce.egit.ui/src/org/spearce/egit/ui/internal/clone/SourceBranchPage.java
+++ b/org.spearce.egit.ui/src/org/spearce/egit/ui/internal/clone/SourceBranchPage.java
@@ -75,8 +75,10 @@ class SourceBranchPage extends WizardPage {
 		setImageDescriptor(UIIcons.WIZBAN_IMPORT_REPO);
 		sourcePage.addURIishChangeListener(new URIishChangeListener() {
 			public void uriishChanged(final URIish newURI) {
-				if (newURI == null || !newURI.equals(validated))
+				if (newURI == null || !newURI.equals(validated)) {
+					validated = null;
 					setPageComplete(false);
+				}
 			}
 		});
 		branchChangeListeners = new ArrayList<BranchChangeListener>(3);
-- 
1.5.5.1.178.g1f811

Re: [PATCH] Take care of errors reported from the server when upload command is started

[Shawn O. Pearce]

Robin Rosenberg <robin.rosenberg@dewire.com> wrote:
> @@ -44,4 +46,31 @@ class EclipseSshSessionFactory extends SshSessionFactory {
> +	@Override
> +	public OutputStream getErrorStream() {
...
> +					Activator.logError(s, new Throwable());

I'm not sure what value the Throwable gives us here; it may be some
call stack deep within JSch, isn't it?  Is it useful to include?

We may also want the log records in Eclipse to say which remote the
message came from, which means passing in the URIish as a parameter.

> @@ -77,6 +78,7 @@ class TransportGitSsh extends PackTransport {
>  	}
>  
>  	final SshSessionFactory sch;
> +	OutputStream errStream;

Could we avoid putting the error stream as an instance member
of the transport by instead using channel.getErrStream() in the
exception case below?
  
> @@ -179,7 +181,8 @@ class TransportGitSsh extends PackTransport {
>  			cmd.append(' ');
>  			sqAlways(cmd, path);
>  			channel.setCommand(cmd.toString());
> -			channel.setErrStream(System.err);
> +			errStream = SshSessionFactory.getInstance().getErrorStream();

Use sch rather than SshSessionFactory.getInstance().  We store it in the
transport so that once the transport instance is created it always goes
to the same SshSessionFactory for anything it needs, even if the caller
has changed the global SshSessionFactory away on us.

> @@ -198,7 +201,12 @@ class TransportGitSsh extends PackTransport {
>  			try {
>  				session = openSession();
>  				channel = exec(session, getOptionUploadPack());
> -				init(channel.getInputStream(), channel.getOutputStream());
> +
> +				if (channel.isConnected())
> +					init(channel.getInputStream(), channel.getOutputStream());
> +				else
> +					throw new TransportException(errStream.toString());

I think you can say channel.getErrStream() here and not need the
instance member.

-- 
Shawn.

Re: [jgit PATCH] Paper bag fix quoting for SSH transport commands

[Robin Rosenberg]

söndagen den 22 juni 2008 03.36.40 skrev Shawn O. Pearce:
> Not all Git-over-SSH servers run a Bourne shell on the remote side
> to evaluate the command we are sending.  Some servers run git-shell,
> which will fail to execute git-upload-pack if we feed it a quoted
> string for the name git-upload-pack.
> 
> Testing concludes that git-shell requires the command name to never
> be quoted, and the argument name to always be single quoted.  As
> this is a long-standing behavior in the wild jgit needs to conform,
> as git-shell and all git-shell work-a-likes such as gitosis may be
> following the same convention.
> 
> Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
> ---
> 
>  If there are no arguments I'll push this into the public tree.
>  It seems right on the surface, and is necessary to use jgit against
>  repo.or.cz, and probably many other sites like it.

Seems ok and works here. Error handling still has a paperbagish feel. See
follow up patches.

Maybe we should have a patch for git too so it will actually work with spaces in file names. What do people on Windows do? (those that actually get an SSH server up and running and sleep well overe it on that platform).

As for pushing and signing. One way is for you (Shawn) and me is to sign-off and push each other's patches. I pushed this one.

-- robin

Re: [jgit PATCH] Paper bag fix quoting for SSH transport commands

[Shawn O. Pearce]

Robin Rosenberg <robin.rosenberg.lists@dewire.com> wrote:
> söndagen den 22 juni 2008 03.36.40 skrev Shawn O. Pearce:
> > 
> > Testing concludes that git-shell requires the command name to never
> > be quoted, and the argument name to always be single quoted.  As
> > this is a long-standing behavior in the wild jgit needs to conform,
> > as git-shell and all git-shell work-a-likes such as gitosis may be
> > following the same convention.
> 
> Seems ok and works here. Error handling still has a paperbagish feel. See
> follow up patches.

Well, I wasn't trying to clean up error handling, I was just trying
to make `jgit fetch` work against repo.or.cz.  But I agree, the
error handling and feedback in the transport code could be improved.

> Maybe we should have a patch for git too so it will actually work
> with spaces in file names. What do people on Windows do? (those that
> actually get an SSH server up and running and sleep well overe it
> on that platform).

The issue only happens with the command name, which most people
use just git-upload-pack/git-receive-pack as they have git in their
$PATH on the remote side, or are running git-shell.  I suspect that
Windows users just don't run Git over SSH with paths that contain
spaces to access git-upload-pack remotely.

I'd patch Git to use the same rule as jgit and only quote the command
when really necessary, but its not high on my list of things to do
this month.  :-)

> As for pushing and signing. One way is for you (Shawn) and me is
> to sign-off and push each other's patches. I pushed this one.

Given that repo.or.cz doesn't show reflogs, I take it this is only
a way to make sure at least someone has reviewed the patch before
it goes into the main tree, since we both have write access?  I
can live with that.

-- 
Shawn.

Re: [jgit PATCH] Paper bag fix quoting for SSH transport commands

[Robin Rosenberg]

måndagen den 23 juni 2008 00.15.45 skrev Shawn O. Pearce:
> Robin Rosenberg <robin.rosenberg.lists@dewire.com> wrote:
> > As for pushing and signing. One way is for you (Shawn) and me is
> > to sign-off and push each other's patches. I pushed this one.
> 
> Given that repo.or.cz doesn't show reflogs, I take it this is only
> a way to make sure at least someone has reviewed the patch before
> it goes into the main tree, since we both have write access?  I
> can live with that.
We'll try that (unless we need pushing desperately).

-- robin