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

From: Shawn O. Pearce <spearce@spearce.org>
Date: Jun 22, 2008, 23:01 UTC
Message-ID: <20080622230159.GG11793@spearce.org>
In-Reply-To: <1214156797-29186-1-git-send-email-robin.rosenberg@dewire.com>

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.

Show 5 quoted lines

> @@ -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?

Show 6 quoted lines

> @@ -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.

Show 10 quoted lines

> @@ -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.