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

["Shawn O. Pearce" <spearce@spearce.org>]

Jonas Fonseca <fonseca@diku.dk> wrote:
> 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.  ;-)
 
> 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.

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