[JGIT PATCH 0/5] Yet another round of transport fixes

[JGIT PATCH 0/5] Yet another round of transport fixes

[Shawn O. Pearce]

Apparently tweaking minor details is never done.  This short
series improves minor issues I have with the transport code
in relation to error and progress reporting.  We also get a
small performance boost for tag fetching over SSH.

Shawn O. Pearce (5):
  Include a progress meter for large uploads to Amazon S3
  Don't display passwords on the console in fetch/push output
  Reuse the same SSH connection when automatically fetching tags
  Report remote SSH execution errors during push via TransportException
  Explicitly capture the stderr from a failed SSH fetch or push

 .../org/spearce/egit/core/op/CloneOperation.java   |   10 ++-
 .../spearce/egit/ui/EclipseSshSessionFactory.java  |    5 +-
 .../egit/ui/internal/clone/SourceBranchPage.java   |    1 +
 .../spearce/jgit/transport/PushProcessTest.java    |    5 +
 .../org/spearce/jgit/transport/TransportTest.java  |   10 ++
 .../jgit/errors/NoRemoteRepositoryException.java   |   59 +++++++++++
 .../src/org/spearce/jgit/pgm/Fetch.java            |   19 +++--
 .../src/org/spearce/jgit/pgm/LsRemote.java         |    1 +
 .../src/org/spearce/jgit/pgm/Push.java             |    2 +-
 .../src/org/spearce/jgit/transport/AmazonS3.java   |   29 +++++-
 .../spearce/jgit/transport/BasePackConnection.java |    3 +-
 .../jgit/transport/DefaultSshSessionFactory.java   |   30 ++++++-
 .../src/org/spearce/jgit/transport/Transport.java  |   10 ++
 .../spearce/jgit/transport/TransportAmazonS3.java  |   12 ++-
 .../spearce/jgit/transport/TransportBundle.java    |    5 +
 .../spearce/jgit/transport/TransportGitAnon.java   |    5 +
 .../spearce/jgit/transport/TransportGitSsh.java    |  103 +++++++++++++-------
 .../org/spearce/jgit/transport/TransportHttp.java  |    5 +
 .../org/spearce/jgit/transport/TransportLocal.java |    5 +
 .../org/spearce/jgit/transport/TransportSftp.java  |   57 ++++++-----
 .../spearce/jgit/transport/WalkPushConnection.java |    5 +-
 .../jgit/transport/WalkRemoteObjectDatabase.java   |   11 ++-
 22 files changed, 303 insertions(+), 89 deletions(-)
 create mode 100644 org.spearce.jgit/src/org/spearce/jgit/errors/NoRemoteRepositoryException.java

[JGIT PATCH 1/5] Include a progress meter for large uploads to Amazon S3

[Shawn O. Pearce]

If we are uploading a sizable pack file or idx file to the S3
service the upload happens after we have finished writing the pack
to a local temporary file.  This causes a long pause for the user
while they wait for the data transfer to complete, with no real
indication of progress happening.

Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---
 .../src/org/spearce/jgit/transport/AmazonS3.java   |   29 +++++++++++++++----
 .../spearce/jgit/transport/TransportAmazonS3.java  |    7 +++-
 .../org/spearce/jgit/transport/TransportSftp.java  |    5 +++-
 .../spearce/jgit/transport/WalkPushConnection.java |    5 ++-
 .../jgit/transport/WalkRemoteObjectDatabase.java   |   11 ++++++-
 5 files changed, 44 insertions(+), 13 deletions(-)

diff --git a/org.spearce.jgit/src/org/spearce/jgit/transport/AmazonS3.java b/org.spearce.jgit/src/org/spearce/jgit/transport/AmazonS3.java
index e8575aa..59337f8 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/transport/AmazonS3.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/transport/AmazonS3.java
@@ -75,6 +75,8 @@ import javax.crypto.spec.SecretKeySpec;
 
 import org.spearce.jgit.awtui.AwtAuthenticator;
 import org.spearce.jgit.lib.Constants;
+import org.spearce.jgit.lib.NullProgressMonitor;
+import org.spearce.jgit.lib.ProgressMonitor;
 import org.spearce.jgit.util.Base64;
 import org.spearce.jgit.util.HttpSupport;
 import org.spearce.jgit.util.TemporaryBuffer;
@@ -376,7 +378,7 @@ public class AmazonS3 {
 			// We have to copy to produce the cipher text anyway so use
 			// the large object code path as it supports that behavior.
 			//
-			final OutputStream os = beginPut(bucket, key);
+			final OutputStream os = beginPut(bucket, key, null, null);
 			os.write(data);
 			os.close();
 			return;
@@ -430,11 +432,17 @@ public class AmazonS3 {
 	 *            name of the bucket storing the object.
 	 * @param key
 	 *            key of the object within its bucket.
+	 * @param monitor
+	 *            (optional) progress monitor to post upload completion to
+	 *            during the stream's close method.
+	 * @param monitorTask
+	 *            (optional) task name to display during the close method.
 	 * @return a stream which accepts the new data, and transmits once closed.
 	 * @throws IOException
 	 *             if encryption was enabled it could not be configured.
 	 */
-	public OutputStream beginPut(final String bucket, final String key)
+	public OutputStream beginPut(final String bucket, final String key,
+			final ProgressMonitor monitor, final String monitorTask)
 			throws IOException {
 		final MessageDigest md5 = newMD5();
 		final TemporaryBuffer buffer = new TemporaryBuffer() {
@@ -442,7 +450,8 @@ public class AmazonS3 {
 			public void close() throws IOException {
 				super.close();
 				try {
-					putImpl(bucket, key, md5.digest(), this);
+					putImpl(bucket, key, md5.digest(), this, monitor,
+							monitorTask);
 				} finally {
 					destroy();
 				}
@@ -452,7 +461,13 @@ public class AmazonS3 {
 	}
 
 	private void putImpl(final String bucket, final String key,
-			final byte[] csum, final TemporaryBuffer buf) throws IOException {
+			final byte[] csum, final TemporaryBuffer buf,
+			ProgressMonitor monitor, String monitorTask) throws IOException {
+		if (monitor == null)
+			monitor = NullProgressMonitor.INSTANCE;
+		if (monitorTask == null)
+			monitorTask = "Uploading " + key;
+
 		final String md5str = Base64.encodeBytes(csum);
 		final long len = buf.length();
 		final String lenstr = String.valueOf(len);
@@ -465,10 +480,12 @@ public class AmazonS3 {
 			authorize(c);
 			c.setDoOutput(true);
 			c.setFixedLengthStreamingMode((int) len);
+			monitor.beginTask(monitorTask, (int) (len / 1024));
 			final OutputStream os = c.getOutputStream();
 			try {
-				buf.writeTo(os, null);
+				buf.writeTo(os, monitor);
 			} finally {
+				monitor.endTask();
 				os.close();
 			}
 
@@ -641,7 +658,7 @@ public class AmazonS3 {
 		} else if ("rm".equals(op) || "delete".equals(op)) {
 			s3.delete(bucket, key);
 		} else if ("put".equals(op)) {
-			final OutputStream os = s3.beginPut(bucket, key);
+			final OutputStream os = s3.beginPut(bucket, key, null, null);
 			final byte[] tmp = new byte[2048];
 			int n;
 			while ((n = System.in.read(tmp)) > 0)
diff --git a/org.spearce.jgit/src/org/spearce/jgit/transport/TransportAmazonS3.java b/org.spearce.jgit/src/org/spearce/jgit/transport/TransportAmazonS3.java
index d2f4c83..cd62c5b 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/transport/TransportAmazonS3.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/transport/TransportAmazonS3.java
@@ -54,6 +54,7 @@ import java.util.TreeMap;
 import org.spearce.jgit.errors.NotSupportedException;
 import org.spearce.jgit.errors.TransportException;
 import org.spearce.jgit.lib.ObjectId;
+import org.spearce.jgit.lib.ProgressMonitor;
 import org.spearce.jgit.lib.Ref;
 import org.spearce.jgit.lib.Repository;
 import org.spearce.jgit.lib.Ref.Storage;
@@ -236,8 +237,10 @@ class TransportAmazonS3 extends WalkTransport {
 		}
 
 		@Override
-		OutputStream writeFile(final String path) throws IOException {
-			return s3.beginPut(bucket, resolveKey(path));
+		OutputStream writeFile(final String path,
+				final ProgressMonitor monitor, final String monitorTask)
+				throws IOException {
+			return s3.beginPut(bucket, resolveKey(path), monitor, monitorTask);
 		}
 
 		@Override
diff --git a/org.spearce.jgit/src/org/spearce/jgit/transport/TransportSftp.java b/org.spearce.jgit/src/org/spearce/jgit/transport/TransportSftp.java
index a33406b..c2cbe6a 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/transport/TransportSftp.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/transport/TransportSftp.java
@@ -54,6 +54,7 @@ import java.util.TreeMap;
 
 import org.spearce.jgit.errors.TransportException;
 import org.spearce.jgit.lib.ObjectId;
+import org.spearce.jgit.lib.ProgressMonitor;
 import org.spearce.jgit.lib.Ref;
 import org.spearce.jgit.lib.Repository;
 import org.spearce.jgit.lib.Ref.Storage;
@@ -294,7 +295,9 @@ class TransportSftp extends WalkTransport {
 		}
 
 		@Override
-		OutputStream writeFile(final String path) throws IOException {
+		OutputStream writeFile(final String path,
+				final ProgressMonitor monitor, final String monitorTask)
+				throws IOException {
 			try {
 				return ftp.put(path);
 			} catch (SftpException je) {
diff --git a/org.spearce.jgit/src/org/spearce/jgit/transport/WalkPushConnection.java b/org.spearce.jgit/src/org/spearce/jgit/transport/WalkPushConnection.java
index bb5a653..904a699 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/transport/WalkPushConnection.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/transport/WalkPushConnection.java
@@ -230,14 +230,15 @@ class WalkPushConnection extends BaseConnection implements PushConnection {
 			// Write the pack file, then the index, as readers look the
 			// other direction (index, then pack file).
 			//
-			OutputStream os = dest.writeFile(pathPack);
+			final String wt = "Put " + base.substring(0, 12);
+			OutputStream os = dest.writeFile(pathPack, monitor, wt + "..pack");
 			try {
 				pw.writePack(os);
 			} finally {
 				os.close();
 			}
 
-			os = dest.writeFile(pathIdx);
+			os = dest.writeFile(pathIdx, monitor, wt + "..idx");
 			try {
 				pw.writeIndex(os);
 			} finally {
diff --git a/org.spearce.jgit/src/org/spearce/jgit/transport/WalkRemoteObjectDatabase.java b/org.spearce.jgit/src/org/spearce/jgit/transport/WalkRemoteObjectDatabase.java
index 915faac..c5a5199 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/transport/WalkRemoteObjectDatabase.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/transport/WalkRemoteObjectDatabase.java
@@ -52,6 +52,7 @@ import java.util.Map;
 import org.spearce.jgit.errors.TransportException;
 import org.spearce.jgit.lib.Constants;
 import org.spearce.jgit.lib.ObjectId;
+import org.spearce.jgit.lib.ProgressMonitor;
 import org.spearce.jgit.lib.Ref;
 import org.spearce.jgit.util.NB;
 
@@ -213,11 +214,17 @@ abstract class WalkRemoteObjectDatabase {
 	 *         complete the write request. The stream is not buffered and each
 	 *         write may cause a network request/response so callers should
 	 *         buffer to smooth out small writes.
+	 * @param monitor
+	 *            (optional) progress monitor to post write completion to during
+	 *            the stream's close method.
+	 * @param monitorTask
+	 *            (optional) task name to display during the close method.
 	 * @throws IOException
 	 *             writing is not supported, or attempting to write the file
 	 *             failed, possibly due to permissions or remote disk full, etc.
 	 */
-	OutputStream writeFile(final String path) throws IOException {
+	OutputStream writeFile(final String path, final ProgressMonitor monitor,
+			final String monitorTask) throws IOException {
 		throw new IOException("Writing of '" + path + "' not supported.");
 	}
 
@@ -247,7 +254,7 @@ abstract class WalkRemoteObjectDatabase {
 	 *             failed, possibly due to permissions or remote disk full, etc.
 	 */
 	void writeFile(final String path, final byte[] data) throws IOException {
-		final OutputStream os = writeFile(path);
+		final OutputStream os = writeFile(path, null, null);
 		try {
 			os.write(data);
 		} finally {
-- 
1.5.6.2.393.g45096

[JGIT PATCH 2/5] Don't display passwords on the console in fetch/push output

[Shawn O. Pearce]

When we show the URI we just fetched or pushed against there may
be a user password embedded in that URI, as saved in the user's
.git/config file.  We shouldn't display that in public to prying
eyes so nulling it out will give us a copy of the URI without that
field in it.

Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---
 .../src/org/spearce/jgit/pgm/Fetch.java            |    2 +-
 .../src/org/spearce/jgit/pgm/Push.java             |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/org.spearce.jgit/src/org/spearce/jgit/pgm/Fetch.java b/org.spearce.jgit/src/org/spearce/jgit/pgm/Fetch.java
index c9c997e..36a0592 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/pgm/Fetch.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/pgm/Fetch.java
@@ -71,7 +71,7 @@ class Fetch extends TextBuiltin {
 			return;
 
 		out.print("From ");
-		out.print(tn.getURI());
+		out.print(tn.getURI().setPass(null));
 		out.println();
 		for (final TrackingRefUpdate u : r.getTrackingRefUpdates()) {
 			final char type = shortTypeOf(u.getResult());
diff --git a/org.spearce.jgit/src/org/spearce/jgit/pgm/Push.java b/org.spearce.jgit/src/org/spearce/jgit/pgm/Push.java
index cbdf465..8411a11 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/pgm/Push.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/pgm/Push.java
@@ -149,7 +149,7 @@ class Push extends TextBuiltin {
 			final RemoteRefUpdate rru) {
 		if (first) {
 			first = false;
-			out.format("To %s\n", transport.getURI());
+			out.format("To %s\n", transport.getURI().setPass(null));
 		}
 
 		final String remoteName = rru.getRemoteName();
-- 
1.5.6.2.393.g45096

[JGIT PATCH 3/5] Reuse the same SSH connection when automatically fetching tags

[Shawn O. Pearce]

If we have to open a second connection to git-upload-pack in order
to automatically follow and fetch annotated tags we may be able to
reuse the same JSch Session object and simply run the second stream
over the already established, encrypted TCP stream.

Reusing the Session avoids the overheads associated with performing
public/private key authentication and reduces the overall latency
of the fetch process.

To make reuse work we cache the Session at the Transport instance
level and ask Transport API users to close the Transport once they
are completely done talking to this remote repository.  While the
Transport instance is unclosed however multiple FetchConnection
and/or PushConnections can be established, with each subsequent
connection being cheaper to setup.

In the future we may also support reusing the same Session onto
different Transport instances, permitting access to multiple
repositories hosted on the same remote SSH server.

Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---
 .../org/spearce/egit/core/op/CloneOperation.java   |   10 +++-
 .../egit/ui/internal/clone/SourceBranchPage.java   |    1 +
 .../spearce/jgit/transport/PushProcessTest.java    |    5 ++
 .../org/spearce/jgit/transport/TransportTest.java  |   10 +++
 .../src/org/spearce/jgit/pgm/Fetch.java            |   17 ++++--
 .../src/org/spearce/jgit/pgm/LsRemote.java         |    1 +
 .../src/org/spearce/jgit/transport/Transport.java  |   10 +++
 .../spearce/jgit/transport/TransportAmazonS3.java  |    5 ++
 .../spearce/jgit/transport/TransportBundle.java    |    5 ++
 .../spearce/jgit/transport/TransportGitAnon.java   |    5 ++
 .../spearce/jgit/transport/TransportGitSsh.java    |   60 +++++++++-----------
 .../org/spearce/jgit/transport/TransportHttp.java  |    5 ++
 .../org/spearce/jgit/transport/TransportLocal.java |    5 ++
 .../org/spearce/jgit/transport/TransportSftp.java  |   52 +++++++++---------
 14 files changed, 124 insertions(+), 67 deletions(-)

diff --git a/org.spearce.egit.core/src/org/spearce/egit/core/op/CloneOperation.java b/org.spearce.egit.core/src/org/spearce/egit/core/op/CloneOperation.java
index 7600e3b..656f3cb 100644
--- a/org.spearce.egit.core/src/org/spearce/egit/core/op/CloneOperation.java
+++ b/org.spearce.egit.core/src/org/spearce/egit/core/op/CloneOperation.java
@@ -85,8 +85,14 @@ public class CloneOperation implements IRunnableWithProgress {
 
 	private void doFetch(final IProgressMonitor monitor)
 			throws NotSupportedException, TransportException {
-		fetchResult = Transport.open(local, remote).fetch(
-				new EclipseGitProgressTransformer(monitor), null);
+		final Transport tn = Transport.open(local, remote);
+		try {
+			final EclipseGitProgressTransformer pm;
+			pm = new EclipseGitProgressTransformer(monitor);
+			fetchResult = tn.fetch(pm, null);
+		} finally {
+			tn.close();
+		}
 	}
 
 	private void doCheckout(final IProgressMonitor monitor) throws IOException {
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 b704aaa..b0aba1e 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
@@ -241,6 +241,7 @@ class SourceBranchPage extends WizardPage {
 							adv = fn.getRefs();
 						} finally {
 							fn.close();
+							tn.close();
 						}
 
 						final Ref idHEAD = fn.getRef(Constants.HEAD);
diff --git a/org.spearce.jgit.test/tst/org/spearce/jgit/transport/PushProcessTest.java b/org.spearce.jgit.test/tst/org/spearce/jgit/transport/PushProcessTest.java
index fae1cbb..357e6b7 100644
--- a/org.spearce.jgit.test/tst/org/spearce/jgit/transport/PushProcessTest.java
+++ b/org.spearce.jgit.test/tst/org/spearce/jgit/transport/PushProcessTest.java
@@ -379,6 +379,11 @@ public class PushProcessTest extends RepositoryTestCase {
 				TransportException {
 			return new MockPushConnection();
 		}
+
+		@Override
+		public void close() {
+			// nothing here
+		}
 	}
 
 	private class MockPushConnection extends BaseConnection implements
diff --git a/org.spearce.jgit.test/tst/org/spearce/jgit/transport/TransportTest.java b/org.spearce.jgit.test/tst/org/spearce/jgit/transport/TransportTest.java
index 10f6651..dc1cb21 100644
--- a/org.spearce.jgit.test/tst/org/spearce/jgit/transport/TransportTest.java
+++ b/org.spearce.jgit.test/tst/org/spearce/jgit/transport/TransportTest.java
@@ -56,6 +56,16 @@ public class TransportTest extends RepositoryTestCase {
 		final RepositoryConfig config = db.getConfig();
 		remoteConfig = new RemoteConfig(config, "test");
 		remoteConfig.addURI(new URIish("http://everyones.loves.git/u/2"));
+		transport = null;
+	}
+
+	@Override
+	protected void tearDown() throws Exception {
+		if (transport != null) {
+			transport.close();
+			transport = null;
+		}
+		super.tearDown();
 	}
 
 	/**
diff --git a/org.spearce.jgit/src/org/spearce/jgit/pgm/Fetch.java b/org.spearce.jgit/src/org/spearce/jgit/pgm/Fetch.java
index 36a0592..c361c26 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/pgm/Fetch.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/pgm/Fetch.java
@@ -63,12 +63,17 @@ class Fetch extends TextBuiltin {
 			args = new String[] { "origin" };
 
 		final Transport tn = Transport.open(db, args[argi++]);
-		final List<RefSpec> toget = new ArrayList<RefSpec>();
-		for (; argi < args.length; argi++)
-			toget.add(new RefSpec(args[argi]));
-		final FetchResult r = tn.fetch(new TextProgressMonitor(), toget);
-		if (r.getTrackingRefUpdates().isEmpty())
-			return;
+		final FetchResult r;
+		try {
+			final List<RefSpec> toget = new ArrayList<RefSpec>();
+			for (; argi < args.length; argi++)
+				toget.add(new RefSpec(args[argi]));
+			r = tn.fetch(new TextProgressMonitor(), toget);
+			if (r.getTrackingRefUpdates().isEmpty())
+				return;
+		} finally {
+			tn.close();
+		}
 
 		out.print("From ");
 		out.print(tn.getURI().setPass(null));
diff --git a/org.spearce.jgit/src/org/spearce/jgit/pgm/LsRemote.java b/org.spearce.jgit/src/org/spearce/jgit/pgm/LsRemote.java
index dbdfeb3..21e02ec 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/pgm/LsRemote.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/pgm/LsRemote.java
@@ -70,6 +70,7 @@ class LsRemote extends TextBuiltin {
 			}
 		} finally {
 			c.close();
+			tn.close();
 		}
 	}
 
diff --git a/org.spearce.jgit/src/org/spearce/jgit/transport/Transport.java b/org.spearce.jgit/src/org/spearce/jgit/transport/Transport.java
index b962162..5bec4d2 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/transport/Transport.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/transport/Transport.java
@@ -527,6 +527,16 @@ public abstract class Transport {
 	public abstract PushConnection openPush() throws NotSupportedException,
 			TransportException;
 
+	/**
+	 * Close any resources used by this transport.
+	 * <p>
+	 * If the remote repository is contacted by a network socket this method
+	 * must close that network socket, disconnecting the two peers. If the
+	 * remote repository is actually local (same system) this method must close
+	 * any open file handles used to read the "remote" repository.
+	 */
+	public abstract void close();
+
 	private Collection<RefSpec> expandPushWildcardsFor(
 			final Collection<RefSpec> specs) {
 		final Map<String, Ref> localRefs = local.getAllRefs();
diff --git a/org.spearce.jgit/src/org/spearce/jgit/transport/TransportAmazonS3.java b/org.spearce.jgit/src/org/spearce/jgit/transport/TransportAmazonS3.java
index cd62c5b..9aa2567 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/transport/TransportAmazonS3.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/transport/TransportAmazonS3.java
@@ -159,6 +159,11 @@ class TransportAmazonS3 extends WalkTransport {
 		return r;
 	}
 
+	@Override
+	public void close() {
+		// No explicit connections are maintained.
+	}
+
 	class DatabaseS3 extends WalkRemoteObjectDatabase {
 		private final String bucketName;
 
diff --git a/org.spearce.jgit/src/org/spearce/jgit/transport/TransportBundle.java b/org.spearce.jgit/src/org/spearce/jgit/transport/TransportBundle.java
index 6169179..24d49eb 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/transport/TransportBundle.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/transport/TransportBundle.java
@@ -101,6 +101,11 @@ class TransportBundle extends PackTransport {
 				"Push is not supported for bundle transport");
 	}
 
+	@Override
+	public void close() {
+		// Resources must be established per-connection.
+	}
+
 	class BundleFetchConnection extends BaseFetchConnection {
 		FileInputStream in;
 
diff --git a/org.spearce.jgit/src/org/spearce/jgit/transport/TransportGitAnon.java b/org.spearce.jgit/src/org/spearce/jgit/transport/TransportGitAnon.java
index 8a78099..a80c335 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/transport/TransportGitAnon.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/transport/TransportGitAnon.java
@@ -77,6 +77,11 @@ class TransportGitAnon extends PackTransport {
 		return new TcpPushConnection();
 	}
 
+	@Override
+	public void close() {
+		// Resources must be established per-connection.
+	}
+
 	Socket openConnection() throws TransportException {
 		final int port = uri.getPort() > 0 ? uri.getPort() : GIT_PORT;
 		try {
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 1bbdf04..b169f4c 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/transport/TransportGitSsh.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/transport/TransportGitSsh.java
@@ -78,7 +78,10 @@ class TransportGitSsh extends PackTransport {
 		return false;
 	}
 
-	final SshSessionFactory sch;
+	private final SshSessionFactory sch;
+
+	private Session sock;
+
 	OutputStream errStream;
 
 	TransportGitSsh(final Repository local, final URIish uri) {
@@ -96,6 +99,17 @@ class TransportGitSsh extends PackTransport {
 		return new SshPushConnection();
 	}
 
+	@Override
+	public void close() {
+		if (sock != null) {
+			try {
+				sch.releaseSession(sock);
+			} finally {
+				sock = null;
+			}
+		}
+	}
+
 	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
@@ -152,17 +166,18 @@ class TransportGitSsh extends PackTransport {
 		cmd.append('\'');
 	}
 
-	Session openSession() throws TransportException {
+	private void initSession() throws TransportException {
+		if (sock != null)
+			return;
+
 		final String user = uri.getUser();
 		final String pass = uri.getPass();
 		final String host = uri.getHost();
 		final int port = uri.getPort();
 		try {
-			final Session session;
-			session = sch.getSession(user, pass, host, port);
-			if (!session.isConnected())
-				session.connect();
-			return session;
+			sock = sch.getSession(user, pass, host, port);
+			if (!sock.isConnected())
+				sock.connect();
 		} catch (JSchException je) {
 			final Throwable c = je.getCause();
 			if (c instanceof UnknownHostException)
@@ -173,8 +188,9 @@ class TransportGitSsh extends PackTransport {
 		}
 	}
 
-	ChannelExec exec(final Session sock, final String exe)
-			throws TransportException {
+	ChannelExec exec(final String exe) throws TransportException {
+		initSession();
+
 		try {
 			final ChannelExec channel = (ChannelExec) sock.openChannel("exec");
 			String path = uri.getPath();
@@ -202,15 +218,12 @@ class TransportGitSsh extends PackTransport {
 	}
 
 	class SshFetchConnection extends BasePackFetchConnection {
-		private Session session;
-
 		private ChannelExec channel;
 
 		SshFetchConnection() throws TransportException {
 			super(TransportGitSsh.this);
 			try {
-				session = openSession();
-				channel = exec(session, getOptionUploadPack());
+				channel = exec(getOptionUploadPack());
 
 				if (channel.isConnected())
 					init(channel.getInputStream(), channel.getOutputStream());
@@ -240,27 +253,16 @@ class TransportGitSsh extends PackTransport {
 					channel = null;
 				}
 			}
-
-			if (session != null) {
-				try {
-					sch.releaseSession(session);
-				} finally {
-					session = null;
-				}
-			}
 		}
 	}
 
 	class SshPushConnection extends BasePackPushConnection {
-		private Session session;
-
 		private ChannelExec channel;
 
 		SshPushConnection() throws TransportException {
 			super(TransportGitSsh.this);
 			try {
-				session = openSession();
-				channel = exec(session, getOptionReceivePack());
+				channel = exec(getOptionReceivePack());
 				init(channel.getInputStream(), channel.getOutputStream());
 			} catch (TransportException err) {
 				close();
@@ -285,14 +287,6 @@ class TransportGitSsh extends PackTransport {
 					channel = null;
 				}
 			}
-
-			if (session != null) {
-				try {
-					sch.releaseSession(session);
-				} finally {
-					session = null;
-				}
-			}
 		}
 	}
 }
diff --git a/org.spearce.jgit/src/org/spearce/jgit/transport/TransportHttp.java b/org.spearce.jgit/src/org/spearce/jgit/transport/TransportHttp.java
index 9351a12..1357e58 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/transport/TransportHttp.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/transport/TransportHttp.java
@@ -106,6 +106,11 @@ class TransportHttp extends WalkTransport {
 		return r;
 	}
 
+	@Override
+	public void close() {
+		// No explicit connections are maintained.
+	}
+
 	class HttpObjectDB extends WalkRemoteObjectDatabase {
 		private final URL objectsUrl;
 
diff --git a/org.spearce.jgit/src/org/spearce/jgit/transport/TransportLocal.java b/org.spearce.jgit/src/org/spearce/jgit/transport/TransportLocal.java
index 155d59f..d74f1b3 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/transport/TransportLocal.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/transport/TransportLocal.java
@@ -93,6 +93,11 @@ class TransportLocal extends PackTransport {
 		return new LocalPushConnection();
 	}
 
+	@Override
+	public void close() {
+		// Resources must be established per-connection.
+	}
+
 	protected Process startProcessWithErrStream(final String cmd)
 			throws TransportException {
 		try {
diff --git a/org.spearce.jgit/src/org/spearce/jgit/transport/TransportSftp.java b/org.spearce.jgit/src/org/spearce/jgit/transport/TransportSftp.java
index c2cbe6a..6a5df07 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/transport/TransportSftp.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/transport/TransportSftp.java
@@ -91,7 +91,9 @@ class TransportSftp extends WalkTransport {
 		return uri.isRemote() && "sftp".equals(uri.getScheme());
 	}
 
-	final SshSessionFactory sch;
+	private final SshSessionFactory sch;
+
+	private Session sock;
 
 	TransportSftp(final Repository local, final URIish uri) {
 		super(local, uri);
@@ -114,17 +116,29 @@ class TransportSftp extends WalkTransport {
 		return r;
 	}
 
-	Session openSession() throws TransportException {
+	@Override
+	public void close() {
+		if (sock != null) {
+			try {
+				sch.releaseSession(sock);
+			} finally {
+				sock = null;
+			}
+		}
+	}
+
+	private void initSession() throws TransportException {
+		if (sock != null)
+			return;
+
 		final String user = uri.getUser();
 		final String pass = uri.getPass();
 		final String host = uri.getHost();
 		final int port = uri.getPort();
 		try {
-			final Session session;
-			session = sch.getSession(user, pass, host, port);
-			if (!session.isConnected())
-				session.connect();
-			return session;
+			sock = sch.getSession(user, pass, host, port);
+			if (!sock.isConnected())
+				sock.connect();
 		} catch (JSchException je) {
 			final Throwable c = je.getCause();
 			if (c instanceof UnknownHostException)
@@ -135,7 +149,9 @@ class TransportSftp extends WalkTransport {
 		}
 	}
 
-	ChannelSftp open(final Session sock) throws TransportException {
+	ChannelSftp newSftp() throws TransportException {
+		initSession();
+
 		try {
 			final Channel channel = sock.openChannel("sftp");
 			channel.connect();
@@ -148,10 +164,6 @@ class TransportSftp extends WalkTransport {
 	class SftpObjectDB extends WalkRemoteObjectDatabase {
 		private final String objectsPath;
 
-		private final boolean sessionOwner;
-
-		private Session session;
-
 		private ChannelSftp ftp;
 
 		SftpObjectDB(String path) throws TransportException {
@@ -160,9 +172,7 @@ class TransportSftp extends WalkTransport {
 			if (path.startsWith("~/"))
 				path = path.substring(2);
 			try {
-				session = openSession();
-				sessionOwner = true;
-				ftp = TransportSftp.this.open(session);
+				ftp = newSftp();
 				ftp.cd(path);
 				ftp.cd("objects");
 				objectsPath = ftp.pwd();
@@ -177,10 +187,8 @@ class TransportSftp extends WalkTransport {
 
 		SftpObjectDB(final SftpObjectDB parent, final String p)
 				throws TransportException {
-			sessionOwner = false;
-			session = parent.session;
 			try {
-				ftp = TransportSftp.this.open(session);
+				ftp = newSftp();
 				ftp.cd(parent.objectsPath);
 				ftp.cd(p);
 				objectsPath = ftp.pwd();
@@ -452,14 +460,6 @@ class TransportSftp extends WalkTransport {
 					ftp = null;
 				}
 			}
-
-			if (sessionOwner && session != null) {
-				try {
-					sch.releaseSession(session);
-				} finally {
-					session = null;
-				}
-			}
 		}
 	}
 }
-- 
1.5.6.2.393.g45096

[JGIT PATCH 4/5] Report remote SSH execution errors during push via TransportException

[Shawn O. Pearce]

If the remote side failed to execute git-receive-pack we may
have the reason why on the stderr stream of the channel, as
the remote shell may have failed execution.

Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---
 .../spearce/jgit/transport/TransportGitSsh.java    |    9 +++++++--
 1 files changed, 7 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 b169f4c..9a6c719 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/transport/TransportGitSsh.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/transport/TransportGitSsh.java
@@ -228,7 +228,7 @@ class TransportGitSsh extends PackTransport {
 				if (channel.isConnected())
 					init(channel.getInputStream(), channel.getOutputStream());
 				else
-					throw new TransportException(errStream.toString());
+					throw new TransportException(uri, errStream.toString());
 
 			} catch (TransportException err) {
 				close();
@@ -263,7 +263,12 @@ class TransportGitSsh extends PackTransport {
 			super(TransportGitSsh.this);
 			try {
 				channel = exec(getOptionReceivePack());
-				init(channel.getInputStream(), channel.getOutputStream());
+
+				if (channel.isConnected())
+					init(channel.getInputStream(), channel.getOutputStream());
+				else
+					throw new TransportException(uri, errStream.toString());
+
 			} catch (TransportException err) {
 				close();
 				throw err;
-- 
1.5.6.2.393.g45096

[JGIT PATCH 5/5] Explicitly capture the stderr from a failed SSH fetch or push

[Shawn O. Pearce]

If the remote command name is not found on the remote system we are
likely to get a shell error sent to the channel's stderr stream,
and yet the channel is connected.  So we don't see the problem
until we try to read the advertised refs, which is somewhat late.

If we get EOF before the first advertised ref it is a very good
indication that the remote side did not start the process we wanted
it to.  Tossing a specialized exception allows the SSH transport
to offer up the contents of the stderr channel (if any) as possible
indication of why the repository does not exist.

Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---
 .../spearce/egit/ui/EclipseSshSessionFactory.java  |    5 +-
 .../jgit/errors/NoRemoteRepositoryException.java   |   59 ++++++++++++++++++++
 .../spearce/jgit/transport/BasePackConnection.java |    3 +-
 .../jgit/transport/DefaultSshSessionFactory.java   |   30 ++++++++++-
 .../spearce/jgit/transport/TransportGitSsh.java    |   34 +++++++++++-
 5 files changed, 126 insertions(+), 5 deletions(-)
 create mode 100644 org.spearce.jgit/src/org/spearce/jgit/errors/NoRemoteRepositoryException.java

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 144d47d..8f80373 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
@@ -55,7 +55,10 @@ class EclipseSshSessionFactory extends SshSessionFactory {
 			StringBuilder sb = new StringBuilder();
 
 			public String toString() {
-				return all.toString();
+				String r = all.toString();
+				while (r.endsWith("\n"))
+					r = r.substring(0, r.length() - 1);
+				return r;
 			}
 
 			@Override
diff --git a/org.spearce.jgit/src/org/spearce/jgit/errors/NoRemoteRepositoryException.java b/org.spearce.jgit/src/org/spearce/jgit/errors/NoRemoteRepositoryException.java
new file mode 100644
index 0000000..604ec4d
--- /dev/null
+++ b/org.spearce.jgit/src/org/spearce/jgit/errors/NoRemoteRepositoryException.java
@@ -0,0 +1,59 @@
+/*
+ * Copyright (C) 2008, Shawn O. Pearce <spearce@spearce.org>
+ *
+ * All rights reserved.
+ *
+ * Redistribution and use in source and binary forms, with or
+ * without modification, are permitted provided that the following
+ * conditions are met:
+ *
+ * - Redistributions of source code must retain the above copyright
+ *   notice, this list of conditions and the following disclaimer.
+ *
+ * - Redistributions in binary form must reproduce the above
+ *   copyright notice, this list of conditions and the following
+ *   disclaimer in the documentation and/or other materials provided
+ *   with the distribution.
+ *
+ * - Neither the name of the Git Development Community nor the
+ *   names of its contributors may be used to endorse or promote
+ *   products derived from this software without specific prior
+ *   written permission.
+ *
+ * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND
+ * CONTRIBUTORS "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES,
+ * INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES
+ * OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
+ * ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR
+ * CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+ * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT
+ * NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES;
+ * LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER
+ * CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT,
+ * STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
+ * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF
+ * ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+ */
+
+package org.spearce.jgit.errors;
+
+import org.spearce.jgit.transport.URIish;
+
+/**
+ * Indicates a remote repository does not exist.
+ */
+public class NoRemoteRepositoryException extends TransportException {
+	private static final long serialVersionUID = 1L;
+
+	/**
+	 * Constructs an exception indicating a repository does not exist.
+	 *
+	 * @param uri
+	 *            URI used for transport
+	 * @param s
+	 *            message
+	 */
+	public NoRemoteRepositoryException(final URIish uri, final String s) {
+		super(uri, s);
+	}
+}
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 7dc4620..52f3f48 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/transport/BasePackConnection.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/transport/BasePackConnection.java
@@ -49,6 +49,7 @@ import java.util.HashSet;
 import java.util.LinkedHashMap;
 import java.util.Set;
 
+import org.spearce.jgit.errors.NoRemoteRepositoryException;
 import org.spearce.jgit.errors.PackProtocolException;
 import org.spearce.jgit.errors.TransportException;
 import org.spearce.jgit.lib.ObjectId;
@@ -129,7 +130,7 @@ abstract class BasePackConnection extends BaseConnection {
 				line = pckIn.readString();
 			} catch (EOFException eof) {
 				if (avail.isEmpty())
-					throw new TransportException(uri, "not found.");
+					throw new NoRemoteRepositoryException(uri, "not found.");
 				throw eof;
 			}
 
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 5924a04..b4578d4 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/transport/DefaultSshSessionFactory.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/transport/DefaultSshSessionFactory.java
@@ -253,6 +253,34 @@ class DefaultSshSessionFactory extends SshSessionFactory {
 
 	@Override
 	public OutputStream getErrorStream() {
-		return System.err;
+		return new OutputStream() {
+			private StringBuilder all = new StringBuilder();
+
+			private StringBuilder sb = new StringBuilder();
+
+			public String toString() {
+				String r = all.toString();
+				while (r.endsWith("\n"))
+					r = r.substring(0, r.length() - 1);
+				return r;
+			}
+
+			@Override
+			public void write(final int b) throws IOException {
+				if (b == '\r') {
+					System.err.print('\r');
+					return;
+				}
+
+				sb.append((char) b);
+
+				if (b == '\n') {
+					final String line = sb.toString();
+					System.err.print(line);
+					all.append(line);
+					sb = new StringBuilder();
+				}
+			}
+		};
 	}
 }
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 9a6c719..3f2cd37 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/transport/TransportGitSsh.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/transport/TransportGitSsh.java
@@ -44,6 +44,7 @@ import java.io.OutputStream;
 import java.net.ConnectException;
 import java.net.UnknownHostException;
 
+import org.spearce.jgit.errors.NoRemoteRepositoryException;
 import org.spearce.jgit.errors.TransportException;
 import org.spearce.jgit.lib.Repository;
 
@@ -217,6 +218,25 @@ class TransportGitSsh extends PackTransport {
 		}
 	}
 
+	NoRemoteRepositoryException cleanNotFound(NoRemoteRepositoryException nf) {
+		String why = errStream.toString();
+		if (why == null || why.length() == 0)
+			return nf;
+
+		String path = uri.getPath();
+		if (uri.getScheme() != null && uri.getPath().startsWith("/~"))
+			path = uri.getPath().substring(1);
+
+		final StringBuilder pfx = new StringBuilder();
+		pfx.append("fatal: ");
+		sqAlways(pfx, path);
+		pfx.append(": ");
+		if (why.startsWith(pfx.toString()))
+			why = why.substring(pfx.length());
+
+		return new NoRemoteRepositoryException(uri, why);
+	}
+
 	class SshFetchConnection extends BasePackFetchConnection {
 		private ChannelExec channel;
 
@@ -238,7 +258,12 @@ class TransportGitSsh extends PackTransport {
 				throw new TransportException(uri,
 						"remote hung up unexpectedly", err);
 			}
-			readAdvertisedRefs();
+
+			try {
+				readAdvertisedRefs();
+			} catch (NoRemoteRepositoryException notFound) {
+				throw cleanNotFound(notFound);
+			}
 		}
 
 		@Override
@@ -277,7 +302,12 @@ class TransportGitSsh extends PackTransport {
 				throw new TransportException(uri,
 						"remote hung up unexpectedly", err);
 			}
-			readAdvertisedRefs();
+
+			try {
+				readAdvertisedRefs();
+			} catch (NoRemoteRepositoryException notFound) {
+				throw cleanNotFound(notFound);
+			}
 		}
 
 		@Override
-- 
1.5.6.2.393.g45096

Re: [JGIT PATCH 2/5] Don't display passwords on the console in fetch/push output

[Robin Rosenberg]

torsdagen den 10 juli 2008 08.13.20 skrev Shawn O. Pearce:
> When we show the URI we just fetched or pushed against there may
> be a user password embedded in that URI, as saved in the user's
> .git/config file.  We shouldn't display that in public to prying
> eyes so nulling it out will give us a copy of the URI without that
> field in it.
> 
> Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
> ---
>  .../src/org/spearce/jgit/pgm/Fetch.java            |    2 +-
>  .../src/org/spearce/jgit/pgm/Push.java             |    2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/org.spearce.jgit/src/org/spearce/jgit/pgm/Fetch.java b/org.spearce.jgit/src/org/spearce/jgit/pgm/Fetch.java
> index c9c997e..36a0592 100644
> --- a/org.spearce.jgit/src/org/spearce/jgit/pgm/Fetch.java
> +++ b/org.spearce.jgit/src/org/spearce/jgit/pgm/Fetch.java
> @@ -71,7 +71,7 @@ class Fetch extends TextBuiltin {
>  			return;
>  
>  		out.print("From ");
> -		out.print(tn.getURI());
> +		out.print(tn.getURI().setPass(null));

We did this a while ago. Sort of patching broken stuff instead of fixing what's broken, thus we should make URIIsh.toiString
not display the password.

-- robin

Re: [JGIT PATCH 2/5] Don't display passwords on the console in fetch/push output

[Robin Rosenberg]

>From 99c09cf2321f36eb81043aed2fa6834811ee762b Mon Sep 17 00:00:00 2001
From: Robin Rosenberg <robin.rosenberg@dewire.com>
Date: Thu, 10 Jul 2008 22:16:19 +0200
Subject: [PATCH] Avoid password leak from URIIsh

The toString() method is commonly used for dumping information. We
never ever want to use toString when the password is needed. By masking
out the password we avoid unintentional password leaks.

Signed-off-by: Robin Rosenberg <robin.rosenberg@dewire.com>
---
 .../src/org/spearce/jgit/transport/URIish.java     |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/org.spearce.jgit/src/org/spearce/jgit/transport/URIish.java b/org.spearce.jgit/src/org/spearce/jgit/transport/URIish.java
index e022e57..632c8ad 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/transport/URIish.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/transport/URIish.java
@@ -318,7 +318,7 @@ public class URIish {
 			r.append(getUser());
 			if (getPass() != null) {
 				r.append(':');
-				r.append(getPass());
+				r.append("PASSWORD");
 			}
 		}
 
-- 
1.5.6.2.220.g44701

Re: [JGIT PATCH 2/5] Don't display passwords on the console in fetch/push output

[Johannes Schindelin]

Hi,

On Thu, 10 Jul 2008, Robin Rosenberg wrote:

> >From 99c09cf2321f36eb81043aed2fa6834811ee762b Mon Sep 17 00:00:00 2001
> From: Robin Rosenberg <robin.rosenberg@dewire.com>
> Date: Thu, 10 Jul 2008 22:16:19 +0200
> Subject: [PATCH] Avoid password leak from URIIsh

What is this new fashion of sending them headers in the mail body?  Robin, 
I thought you knew better.

Ciao,
Dscho

Re: [JGIT PATCH 2/5] Don't display passwords on the console in fetch/push output

[Robin Rosenberg]

fredagen den 11 juli 2008 00.25.27 skrev Johannes Schindelin:
> Hi,
> 
> On Thu, 10 Jul 2008, Robin Rosenberg wrote:
> 
> > >From 99c09cf2321f36eb81043aed2fa6834811ee762b Mon Sep 17 00:00:00 2001
> > From: Robin Rosenberg <robin.rosenberg@dewire.com>
> > Date: Thu, 10 Jul 2008 22:16:19 +0200
> > Subject: [PATCH] Avoid password leak from URIIsh
> 
> What is this new fashion of sending them headers in the mail body?  Robin, 
> I thought you knew better.

Laziness, I suppose, and that's not a new fashion. I'll try to have better in the future.

-- robin

Re: [JGIT PATCH 2/5] Don't display passwords on the console in fetch/push output

[Shawn O. Pearce]

Robin Rosenberg <robin.rosenberg.lists@dewire.com> wrote:
> diff --git a/org.spearce.jgit/src/org/spearce/jgit/transport/URIish.java b/org.spearce.jgit/src/org/spearce/jgit/transport/URIish.java
> index e022e57..632c8ad 100644
> --- a/org.spearce.jgit/src/org/spearce/jgit/transport/URIish.java
> +++ b/org.spearce.jgit/src/org/spearce/jgit/transport/URIish.java
> @@ -318,7 +318,7 @@ public class URIish {
>  			r.append(getUser());
>  			if (getPass() != null) {
>  				r.append(':');
> -				r.append(getPass());
> +				r.append("PASSWORD");
>  			}
>  		}
>  

I agree in theory, but the implementation isn't quite correct.
How about this instead?

--8<--
Avoid password leak from URIIsh

The toString() method is commonly used for dumping information. We
never ever want to use toString when the password is needed. By
masking out the password we avoid unintentional password leaks.

Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---
 .../tst/org/spearce/jgit/transport/URIishTest.java |    3 ++-
 .../org/spearce/jgit/transport/RemoteConfig.java   |    2 +-
 .../src/org/spearce/jgit/transport/URIish.java     |   15 ++++++++++++++-
 3 files changed, 17 insertions(+), 3 deletions(-)

diff --git a/org.spearce.jgit.test/tst/org/spearce/jgit/transport/URIishTest.java b/org.spearce.jgit.test/tst/org/spearce/jgit/transport/URIishTest.java
index a460418..2e5e847 100644
--- a/org.spearce.jgit.test/tst/org/spearce/jgit/transport/URIishTest.java
+++ b/org.spearce.jgit.test/tst/org/spearce/jgit/transport/URIishTest.java
@@ -214,7 +214,8 @@ public class URIishTest extends TestCase {
 		assertEquals("user", u.getUser());
 		assertEquals("pass", u.getPass());
 		assertEquals(33, u.getPort());
-		assertEquals(str, u.toString());
+		assertEquals(str, u.toPrivateString());
+		assertEquals(u.setPass(null).toPrivateString(), u.toString());
 		assertEquals(u, new URIish(str));
 	}
 }
diff --git a/org.spearce.jgit/src/org/spearce/jgit/transport/RemoteConfig.java b/org.spearce.jgit/src/org/spearce/jgit/transport/RemoteConfig.java
index b0fd5b4..bb21511 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/transport/RemoteConfig.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/transport/RemoteConfig.java
@@ -150,7 +150,7 @@ public class RemoteConfig {
 
 		vlst.clear();
 		for (final URIish u : getURIs())
-			vlst.add(u.toString());
+			vlst.add(u.toPrivateString());
 		rc.setStringList(SECTION, getName(), KEY_URL, vlst);
 
 		vlst.clear();
diff --git a/org.spearce.jgit/src/org/spearce/jgit/transport/URIish.java b/org.spearce.jgit/src/org/spearce/jgit/transport/URIish.java
index e022e57..54a0811 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/transport/URIish.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/transport/URIish.java
@@ -307,7 +307,20 @@ public class URIish {
 		return a.equals(b);
 	}
 
+	/**
+	 * Obtain the string form of the URI, with the password included.
+	 * 
+	 * @return the URI, including its password field, if any.
+	 */
+	public String toPrivateString() {
+		return format(true);
+	}
+
 	public String toString() {
+		return format(false);
+	}
+
+	private String format(final boolean includePassword) {
 		final StringBuilder r = new StringBuilder();
 		if (getScheme() != null) {
 			r.append(getScheme());
@@ -316,7 +329,7 @@ public class URIish {
 
 		if (getUser() != null) {
 			r.append(getUser());
-			if (getPass() != null) {
+			if (includePassword && getPass() != null) {
 				r.append(':');
 				r.append(getPass());
 			}
-- 
1.5.6.2.393.g45096

-- 
Shawn.

Re: [JGIT PATCH 2/5] Don't display passwords on the console in fetch/push output

[Robin Rosenberg]

fredagen den 11 juli 2008 05.20.04 skrev Shawn O. Pearce:
> +	public String toPrivateString() {
> +		return format(true);
> +	}
etc

Yes, that's much better. 

-- robin