threads / rfc / 18987

RFC patchRemoved possibility to change stderr for ssh sessions

Subject: [JGIT PATCH/RFC] Removed possibility to change stderr for ssh sessions

## tl;dr

4 messages between Apr 21, 2009 and Apr 22, 2009. Diffs are folded; open one to read it.

replies: 3people: 2as markdown or json

Constantine Plotnikov· Apr 21, 2009, 18:49 UTC · lore

The current implementation allowed to change stderr for the ssh sessions. However this functionality is broken. It is used only by GitSshTransport and that class expects a very specific behavior from this class. For example toString() method should return the entire content of the stream. So only implementation from SshConfigSessionFactory would have worked anyway. Returning System.err (as was suggested by javadoc) comment would have broken existing functionality. This patch makes this functionality explicitly private.

If this functionality is to be reopened, this additional behavior should be documented and there should be additional lifecycle control, since the user streams will be interested to know when stream will be no more used in order to release resoources.

Signed-off-by: Constantine Plotnikov <constantine.plotnikov@gmail.com>
---
 .../jgit/transport/SshConfigSessionFactory.java    |   34 -----------------
 .../spearce/jgit/transport/SshSessionFactory.java  |    8 +++--
 .../spearce/jgit/transport/TransportGitSsh.java    |   38 +++++++++++++++++++-
 3 files changed, 42 insertions(+), 38 deletions(-)
Show changes to 3 files +42 −38

org.spearce.jgit/src/org/spearce/jgit/transport/SshConfigSessionFactory.java, org.spearce.jgit/src/org/spearce/jgit/transport/SshSessionFactory.java, org.spearce.jgit/src/org/spearce/jgit/transport/TransportGitSsh.java

diff --git a/org.spearce.jgit/src/org/spearce/jgit/transport/SshConfigSessionFactory.java
b/org.spearce.jgit/src/org/spearce/jgit/transport/SshConfigSessionFactory.java
index 4d29829..a87e149 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/transport/SshConfigSessionFactory.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/transport/SshConfigSessionFactory.java
@@ -43,7 +43,6 @@
 import java.io.FileInputStream;
 import java.io.FileNotFoundException;
 import java.io.IOException;
-import java.io.OutputStream;
 import java.util.HashMap;
 import java.util.Map;

@@ -225,37 +224,4 @@ private static void loadIdentity(final JSch sch,
final File priv) {
 			}
 		}
 	}
-
-	@Override
-	public OutputStream getErrorStream() {
-		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/SshSessionFactory.java
b/org.spearce.jgit/src/org/spearce/jgit/transport/SshSessionFactory.java
index f03e80c..bd24d2f 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/transport/SshSessionFactory.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/transport/SshSessionFactory.java
@@ -125,10 +125,12 @@ public void releaseSession(final Session session) {
 	}

 	/**
-	 * Find or create an OutputStream for Ssh to use. For a command line client
-	 * this is probably System.err.
+	 * The method does not have to be implemented and will be removed in
future versions.
 	 *
 	 * @return an OutputStream to receive the SSH error stream.
 	 */
-	public abstract OutputStream getErrorStream();
+	@Deprecated
+	public OutputStream getErrorStream() {
+		throw new UnsupportedOperationException("This method should not be called.");
+	}
 }
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 a24878a..bfe0259 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/transport/TransportGitSsh.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/transport/TransportGitSsh.java
@@ -137,7 +137,7 @@ ChannelExec exec(final String exe) throws
TransportException {
 			cmd.append(' ');
 			sqAlways(cmd, path);
 			channel.setCommand(cmd.toString());
-			errStream = SshSessionFactory.getInstance().getErrorStream();
+			errStream = createErrorStream();
 			channel.setErrStream(errStream, true);
 			channel.connect();
 			return channel;
@@ -146,6 +146,42 @@ ChannelExec exec(final String exe) throws
TransportException {
 		}
 	}

+	/**
+	 * @return the error stream for the channel, the stream is used to
detect specific
+	 *   error reasons for exceptions.
+	 */
+	private static OutputStream createErrorStream() {
+		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();
+				}
+			}
+		};
+	}
+
 	NoRemoteRepositoryException cleanNotFound(NoRemoteRepositoryException nf) {
 		String why = errStream.toString();
 		if (why == null || why.length() == 0)
-- 
1.6.0.2.1172.ga5ed0
Shawn O. Pearce· Apr 22, 2009, 15:46 UTC · re: Constantine Plotnikov · lore

Re: [JGIT PATCH/RFC] Removed possibility to change stderr for ssh sessions

Constantine Plotnikov <constantine.plotnikov@gmail.com> wrote:
> The current implementation allowed to change stderr for the
> ssh sessions. However this functionality is broken.
Good catch.
I applied this, but two comments.

One, your patch was line wrapped, I had to manually unwrap it to apply. So your MUA is still not able to send patches right. Thought you'd like to know.

Two,
Show 11 quoted lines
> +	 * The method does not have to be implemented and will be removed in
> future versions.
>  	 *
>  	 * @return an OutputStream to receive the SSH error stream.
>  	 */
> -	public abstract OutputStream getErrorStream();
> +	@Deprecated
> +	public OutputStream getErrorStream() {
> +		throw new UnsupportedOperationException("This method should not be called.");
> +	}
>  }
I think deprecation here is silly.  I just deleted the method.

Nobody should be calling this except TransportGitSsh, as you discovered.

If they are, getting UnsupportedOperationException at runtime is as bad as NoSuchMethodError at runtime, and either is a lot less friendly than a no such method error at compile time.

Given the method is being broken, I'd rather just remove it outright. So I removed it from your patch when I applied it.

-- 
Shawn.
Constantine Plotnikov· Apr 22, 2009, 15:55 UTC · re: Shawn O. Pearce · lore

Re: [JGIT PATCH/RFC] Removed possibility to change stderr for ssh sessions

On Wed, Apr 22, 2009 at 7:46 PM, Shawn O. Pearce <spearce@spearce.org> wrote:
Show 12 quoted lines
> Constantine Plotnikov <constantine.plotnikov@gmail.com> wrote:
>> The current implementation allowed to change stderr for the
>> ssh sessions. However this functionality is broken.
>
> Good catch.
>
> I applied this, but two comments.
>
> One, your patch was line wrapped, I had to manually unwrap it
> to apply.  So your MUA is still not able to send patches right.
> Thought you'd like to know.
>

It looks like both gmail and thunderbird both have a problem. I will look how opera works next time.

Show 26 quoted lines
> Two,
>
>> +      * The method does not have to be implemented and will be removed in
>> future versions.
>>        *
>>        * @return an OutputStream to receive the SSH error stream.
>>        */
>> -     public abstract OutputStream getErrorStream();
>> +     @Deprecated
>> +     public OutputStream getErrorStream() {
>> +             throw new UnsupportedOperationException("This method should not be called.");
>> +     }
>>  }
>
> I think deprecation here is silly.  I just deleted the method.
>
> Nobody should be calling this except TransportGitSsh, as you
> discovered.
>
> If they are, getting UnsupportedOperationException at runtime is
> as bad as NoSuchMethodError at runtime, and either is a lot less
> friendly than a no such method error at compile time.
>
> Given the method is being broken, I'd rather just remove it outright.
> So I removed it from your patch when I applied it.
>

No problem with it. I have left it for the the case if someone overrides it with @Override annotation. Deprecation would have given a warning rather then an error on annotation.

Constantine
Shawn O. Pearce· Apr 22, 2009, 15:58 UTC · re: Constantine Plotnikov · lore

Re: [JGIT PATCH/RFC] Removed possibility to change stderr for ssh sessions

Constantine Plotnikov <constantine.plotnikov@gmail.com> wrote:
> > One, your patch was line wrapped
> >
> It looks like both gmail and thunderbird both have a problem. I will
> look how opera works next time.

GMail supports authenticated SMTP apparently. Maybe try to get git send-email working with it?

-- 
Shawn.

← back to recent threads