Re: [JGIT RFC PATCH] Improve handling of parsing errors in OpenSshConfig

From: Shawn O. Pearce <spearce@spearce.org>
Date: Sep 25, 2008, 15:16 UTC
Message-ID: <20080925151632.GN3669@spearce.org>
In-Reply-To: <20080925132937.GA16151@diku.dk>

Jonas Fonseca <fonseca@diku.dk> wrote:

Show 11 quoted lines

> Badly quoted entries are now ignored similar to how bad port number are
> currently ignored. A check for negative port numbers is now performed
> so that they also will be ignored.
> 
> Signed-off-by: Jonas Fonseca <fonseca@diku.dk>
> 
> ---
>  .../spearce/jgit/transport/OpenSshConfigTest.java  |   46 +++++++++++++++---
>  .../jgit/transport/DefaultSshSessionFactory.java   |   10 +++-
>  .../org/spearce/jgit/transport/OpenSshConfig.java  |   51 ++++++++++++-------
>  3 files changed, 79 insertions(+), 28 deletions(-)

Well, at first glance the new OpenSshConfigException is missing
from the patch.  We need that class to compile correctly.  ;-)

Show 15 quoted lines

> 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 89beab7..123a9b5 100644
> --- a/org.spearce.jgit/src/org/spearce/jgit/transport/DefaultSshSessionFactory.java
> +++ b/org.spearce.jgit/src/org/spearce/jgit/transport/DefaultSshSessionFactory.java
> @@ -89,7 +90,12 @@
>  	@Override
>  	public synchronized Session getSession(String user, String pass,
>  			String host, int port) throws JSchException {
> -		final OpenSshConfig.Host hc = getConfig().lookup(host);
> +		final OpenSshConfig.Host hc;
> +		try {
> +			hc = getConfig().lookup(host);
> +		} catch (OpenSshConfigException osce) {
> +			throw new JSchException(osce.getMessage());
> +		}

I would perfer to chain the OpenSshConfigException as the cause of the JSchException. That way the caller has a chance to give us a complete stack trace, including the cause of the OSCE being the inner NumberFormatException or ParseException.

Robin or I will need to edit the EclipseSshSessionFactory to add this same sort of try/catch. To keep the tree buildable we'll want to squash that into your patch. Yea, sorry, this is where the egit+jgit within one repository is going to bite us.

Show 15 quoted lines

> diff --git a/org.spearce.jgit/src/org/spearce/jgit/transport/OpenSshConfig.java b/org.spearce.jgit/src/org/spearce/jgit/transport/OpenSshConfig.java
> index b08d5c6..95a37f5 100644
> --- a/org.spearce.jgit/src/org/spearce/jgit/transport/OpenSshConfig.java
> +++ b/org.spearce.jgit/src/org/spearce/jgit/transport/OpenSshConfig.java
> @@ -146,6 +149,10 @@ public Host lookup(final String hostName) {
>  				} finally {
>  					in.close();
>  				}
> +			} catch (NumberFormatException nfe) {
> +				throw new OpenSshConfigException("Parse error", nfe);
> +			} catch (ParseException pe) {
> +				throw new OpenSshConfigException("Parse error", pe);
>  			} catch (FileNotFoundException none) {
>  				hosts = Collections.emptyMap();
>  			} catch (IOException err) {

If we are really going to this level of effort, can we please have the file path and line number of the invalid line in the exception?

We rarely parse the ~/.ssh/config. In general its parsed only once during startup. Going through a bit more work at parse time to get more accurate error messages is acceptable.

I think it may be a good idea to have the exception thrown only when a bad Host block is accessed, or if we try to access an unknown host and there is an unreadable Host block.

So I'm thinking more like we stuff an OSCE instance into the Host block if the host has a bad entry, and during get() or lookup() we test for the exception and rethrow the exception.

E.g. lets say my config file is this:

	$ cat ~/.ssh/config
	Host work
		Hostname internal.google.com"
		Port -1

	Host orcz
		Hostname repo.or.cz
		User spearce
		Port 22

then:

  jgit fetch orcz:foo.git;        # works without error
  jgit fetch work:foo.git;        # throws OSCE due to bad Hostname
  jgit fetch kernel.org:foo.git;  # works as no Host matched

However if my config was more bogus, e.g.:

	$ cat ~/.ssh/config
	Host work"
		User sop

	Host orcz
		Hostname repo.or.cz
		User spearce
		Port 22

then:

  jgit fetch orcz:foo.git;        # still works without error
  jgit fetch work:foo.git;        # fails due to bad Host header
  jgit fetch kernel.org:foo.git;  # fails due to bad Host header

-- 
Shawn.