{"thread":{"id":"18987","subject":"[JGIT PATCH/RFC] Removed possibility to change stderr for ssh sessions","startedAt":"2009-04-21T18:49:56Z","lastAt":"2009-04-22T15:58:00Z","messageCount":4,"participants":["Constantine Plotnikov","Shawn O. Pearce"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"111897","messageId":"85647ef50904211149lc4a4902h554c973017d87adb@mail.gmail.com","threadId":"18987","inReplyTo":null,"subject":"[JGIT PATCH/RFC] Removed possibility to change stderr for ssh sessions","fromName":"Constantine Plotnikov","fromEmail":"constantine.plotnikov@gmail.com","sentAt":"2009-04-21T18:49:56Z","receivedAt":"2009-04-21T18:49:56Z","isPatch":true,"sender":{"key":"constantine.plotnikov@gmail.com","avatar":null},"body":"The current implementation allowed to change stderr for the\nssh sessions. However this functionality is broken. It is used\nonly by GitSshTransport and that class expects a very specific\nbehavior from this class. For example toString() method should\nreturn the entire content of the stream. So only implementation\nfrom SshConfigSessionFactory would have worked anyway. Returning\nSystem.err (as was suggested by javadoc) comment would have broken\nexisting functionality. This patch makes this functionality\nexplicitly private.\n\nIf this functionality is to be reopened, this additional behavior\nshould be documented and there should be additional lifecycle\ncontrol, since the user streams will be interested to know when\nstream will be no more used in order to release resoources.\n\nSigned-off-by: Constantine Plotnikov <constantine.plotnikov@gmail.com>\n---\n .../jgit/transport/SshConfigSessionFactory.java    |   34 -----------------\n .../spearce/jgit/transport/SshSessionFactory.java  |    8 +++--\n .../spearce/jgit/transport/TransportGitSsh.java    |   38 +++++++++++++++++++-\n 3 files changed, 42 insertions(+), 38 deletions(-)\n\ndiff --git a/org.spearce.jgit/src/org/spearce/jgit/transport/SshConfigSessionFactory.java\nb/org.spearce.jgit/src/org/spearce/jgit/transport/SshConfigSessionFactory.java\nindex 4d29829..a87e149 100644\n--- a/org.spearce.jgit/src/org/spearce/jgit/transport/SshConfigSessionFactory.java\n+++ b/org.spearce.jgit/src/org/spearce/jgit/transport/SshConfigSessionFactory.java\n@@ -43,7 +43,6 @@\n import java.io.FileInputStream;\n import java.io.FileNotFoundException;\n import java.io.IOException;\n-import java.io.OutputStream;\n import java.util.HashMap;\n import java.util.Map;\n\n@@ -225,37 +224,4 @@ private static void loadIdentity(final JSch sch,\nfinal File priv) {\n \t\t\t}\n \t\t}\n \t}\n-\n-\t@Override\n-\tpublic OutputStream getErrorStream() {\n-\t\treturn new OutputStream() {\n-\t\t\tprivate StringBuilder all = new StringBuilder();\n-\n-\t\t\tprivate StringBuilder sb = new StringBuilder();\n-\n-\t\t\tpublic String toString() {\n-\t\t\t\tString r = all.toString();\n-\t\t\t\twhile (r.endsWith(\"\\n\"))\n-\t\t\t\t\tr = r.substring(0, r.length() - 1);\n-\t\t\t\treturn r;\n-\t\t\t}\n-\n-\t\t\t@Override\n-\t\t\tpublic void write(final int b) throws IOException {\n-\t\t\t\tif (b == '\\r') {\n-\t\t\t\t\tSystem.err.print('\\r');\n-\t\t\t\t\treturn;\n-\t\t\t\t}\n-\n-\t\t\t\tsb.append((char) b);\n-\n-\t\t\t\tif (b == '\\n') {\n-\t\t\t\t\tfinal String line = sb.toString();\n-\t\t\t\t\tSystem.err.print(line);\n-\t\t\t\t\tall.append(line);\n-\t\t\t\t\tsb = new StringBuilder();\n-\t\t\t\t}\n-\t\t\t}\n-\t\t};\n-\t}\n }\ndiff --git a/org.spearce.jgit/src/org/spearce/jgit/transport/SshSessionFactory.java\nb/org.spearce.jgit/src/org/spearce/jgit/transport/SshSessionFactory.java\nindex f03e80c..bd24d2f 100644\n--- a/org.spearce.jgit/src/org/spearce/jgit/transport/SshSessionFactory.java\n+++ b/org.spearce.jgit/src/org/spearce/jgit/transport/SshSessionFactory.java\n@@ -125,10 +125,12 @@ public void releaseSession(final Session session) {\n \t}\n\n \t/**\n-\t * Find or create an OutputStream for Ssh to use. For a command line client\n-\t * this is probably System.err.\n+\t * The method does not have to be implemented and will be removed in\nfuture versions.\n \t *\n \t * @return an OutputStream to receive the SSH error stream.\n \t */\n-\tpublic abstract OutputStream getErrorStream();\n+\t@Deprecated\n+\tpublic OutputStream getErrorStream() {\n+\t\tthrow new UnsupportedOperationException(\"This method should not be called.\");\n+\t}\n }\ndiff --git a/org.spearce.jgit/src/org/spearce/jgit/transport/TransportGitSsh.java\nb/org.spearce.jgit/src/org/spearce/jgit/transport/TransportGitSsh.java\nindex a24878a..bfe0259 100644\n--- a/org.spearce.jgit/src/org/spearce/jgit/transport/TransportGitSsh.java\n+++ b/org.spearce.jgit/src/org/spearce/jgit/transport/TransportGitSsh.java\n@@ -137,7 +137,7 @@ ChannelExec exec(final String exe) throws\nTransportException {\n \t\t\tcmd.append(' ');\n \t\t\tsqAlways(cmd, path);\n \t\t\tchannel.setCommand(cmd.toString());\n-\t\t\terrStream = SshSessionFactory.getInstance().getErrorStream();\n+\t\t\terrStream = createErrorStream();\n \t\t\tchannel.setErrStream(errStream, true);\n \t\t\tchannel.connect();\n \t\t\treturn channel;\n@@ -146,6 +146,42 @@ ChannelExec exec(final String exe) throws\nTransportException {\n \t\t}\n \t}\n\n+\t/**\n+\t * @return the error stream for the channel, the stream is used to\ndetect specific\n+\t *   error reasons for exceptions.\n+\t */\n+\tprivate static OutputStream createErrorStream() {\n+\t\treturn new OutputStream() {\n+\t\t\tprivate StringBuilder all = new StringBuilder();\n+\n+\t\t\tprivate StringBuilder sb = new StringBuilder();\n+\n+\t\t\tpublic String toString() {\n+\t\t\t\tString r = all.toString();\n+\t\t\t\twhile (r.endsWith(\"\\n\"))\n+\t\t\t\t\tr = r.substring(0, r.length() - 1);\n+\t\t\t\treturn r;\n+\t\t\t}\n+\n+\t\t\t@Override\n+\t\t\tpublic void write(final int b) throws IOException {\n+\t\t\t\tif (b == '\\r') {\n+\t\t\t\t\tSystem.err.print('\\r');\n+\t\t\t\t\treturn;\n+\t\t\t\t}\n+\n+\t\t\t\tsb.append((char) b);\n+\n+\t\t\t\tif (b == '\\n') {\n+\t\t\t\t\tfinal String line = sb.toString();\n+\t\t\t\t\tSystem.err.print(line);\n+\t\t\t\t\tall.append(line);\n+\t\t\t\t\tsb = new StringBuilder();\n+\t\t\t\t}\n+\t\t\t}\n+\t\t};\n+\t}\n+\n \tNoRemoteRepositoryException cleanNotFound(NoRemoteRepositoryException nf) {\n \t\tString why = errStream.toString();\n \t\tif (why == null || why.length() == 0)\n-- \n1.6.0.2.1172.ga5ed0\n"},{"id":"111956","messageId":"20090422154657.GK23604@spearce.org","threadId":"18987","inReplyTo":"85647ef50904211149lc4a4902h554c973017d87adb@mail.gmail.com","subject":"Re: [JGIT PATCH/RFC] Removed possibility to change stderr for ssh sessions","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2009-04-22T15:46:57Z","receivedAt":"2009-04-22T15:46:57Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Constantine Plotnikov <constantine.plotnikov@gmail.com> wrote:\n> The current implementation allowed to change stderr for the\n> ssh sessions. However this functionality is broken.\n\nGood catch.\n\nI applied this, but two comments.\n\nOne, your patch was line wrapped, I had to manually unwrap it\nto apply.  So your MUA is still not able to send patches right.\nThought you'd like to know.\n\nTwo,\n\n> +\t * The method does not have to be implemented and will be removed in\n> future versions.\n>  \t *\n>  \t * @return an OutputStream to receive the SSH error stream.\n>  \t */\n> -\tpublic abstract OutputStream getErrorStream();\n> +\t@Deprecated\n> +\tpublic OutputStream getErrorStream() {\n> +\t\tthrow new UnsupportedOperationException(\"This method should not be called.\");\n> +\t}\n>  }\n\nI think deprecation here is silly.  I just deleted the method.\n\nNobody should be calling this except TransportGitSsh, as you\ndiscovered.\n\nIf they are, getting UnsupportedOperationException at runtime is\nas bad as NoSuchMethodError at runtime, and either is a lot less\nfriendly than a no such method error at compile time.\n\nGiven the method is being broken, I'd rather just remove it outright.\nSo I removed it from your patch when I applied it.\n\n-- \nShawn.\n"},{"id":"111959","messageId":"85647ef50904220855g62890de0r4fee4ea8503aa348@mail.gmail.com","threadId":"18987","inReplyTo":"20090422154657.GK23604@spearce.org","subject":"Re: [JGIT PATCH/RFC] Removed possibility to change stderr for ssh sessions","fromName":"Constantine Plotnikov","fromEmail":"constantine.plotnikov@gmail.com","sentAt":"2009-04-22T15:55:50Z","receivedAt":"2009-04-22T15:55:50Z","isPatch":true,"sender":{"key":"constantine.plotnikov@gmail.com","avatar":null},"body":"On Wed, Apr 22, 2009 at 7:46 PM, Shawn O. Pearce <spearce@spearce.org> wrote:\n> Constantine Plotnikov <constantine.plotnikov@gmail.com> wrote:\n>> The current implementation allowed to change stderr for the\n>> ssh sessions. However this functionality is broken.\n>\n> Good catch.\n>\n> I applied this, but two comments.\n>\n> One, your patch was line wrapped, I had to manually unwrap it\n> to apply.  So your MUA is still not able to send patches right.\n> Thought you'd like to know.\n>\nIt looks like both gmail and thunderbird both have a problem. I will\nlook how opera works next time.\n\n> Two,\n>\n>> +      * The method does not have to be implemented and will be removed in\n>> future versions.\n>>        *\n>>        * @return an OutputStream to receive the SSH error stream.\n>>        */\n>> -     public abstract OutputStream getErrorStream();\n>> +     @Deprecated\n>> +     public OutputStream getErrorStream() {\n>> +             throw new UnsupportedOperationException(\"This method should not be called.\");\n>> +     }\n>>  }\n>\n> I think deprecation here is silly.  I just deleted the method.\n>\n> Nobody should be calling this except TransportGitSsh, as you\n> discovered.\n>\n> If they are, getting UnsupportedOperationException at runtime is\n> as bad as NoSuchMethodError at runtime, and either is a lot less\n> friendly than a no such method error at compile time.\n>\n> Given the method is being broken, I'd rather just remove it outright.\n> So I removed it from your patch when I applied it.\n>\nNo problem with it. I have left it for the the case if someone\noverrides it with @Override annotation. Deprecation would have given a\nwarning rather then an error on annotation.\n\nConstantine\n"},{"id":"111960","messageId":"20090422155800.GM23604@spearce.org","threadId":"18987","inReplyTo":"85647ef50904220855g62890de0r4fee4ea8503aa348@mail.gmail.com","subject":"Re: [JGIT PATCH/RFC] Removed possibility to change stderr for ssh sessions","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2009-04-22T15:58:00Z","receivedAt":"2009-04-22T15:58:00Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Constantine Plotnikov <constantine.plotnikov@gmail.com> wrote:\n> > One, your patch was line wrapped\n> >\n> It looks like both gmail and thunderbird both have a problem. I will\n> look how opera works next time.\n\nGMail supports authenticated SMTP apparently.  Maybe try to get\ngit send-email working with it?\n\n-- \nShawn.\n"}]}