Re: [JGIT PATCH] Test and fix handling of quotes in ~/.ssh/config

From: Jonas Fonseca <fonseca@diku.dk>
Date: Sep 24, 2008, 23:25 UTC
Message-ID: <20080924232519.GA15318@diku.dk>
In-Reply-To: <20080922210734.GE3669@spearce.org>

Shawn O. Pearce <spearce@spearce.org> wrote Mon, Sep 22, 2008:

Show 13 quoted lines

> Robin Rosenberg <robin.rosenberg.lists@dewire.com> wrote:
> > söndagen den 21 september 2008 13.25.19 skrev Jonas Fonseca:
> > > +		assertEquals("bad.tld\"", osc.lookup("bad").getHostName());
> > This one is really (as you noted) bad so we shouldn't allow it at all. A new 
> > subclass of TransportExcpeption should be thrown to indicate a serious
> > configuration problem when attempting to use the option.
>
> Probably so.
> 
> But then we need to mark that the Host is invalid, because we
> are serving requests from a cache, not from the file itself.
> And TransportException isn't something that the SshSessionFactory
> knows about.  Probably better to use a a subclass of IOException.

Sorry, for not following up on this. I was trying to cook up a patch for this today. Now, it is somehow sad that testing "forces" us to waste time on these stupid corner cases. ;-) On the other hand, this problem might exist (.git/config) or turn up again, so it would be good to have a design principle.

Using exceptions seems a bit harsh, since the quote is not really fatal in anyway. Also, for badly formatted Port values the value is simply ignored. So for bad quoting encountered during non-Host values, I think it is fair to just ignore the value. For Host values it is a bit more non-obvious to me. In terms of invalidating hosts, the API ensures that a lookup will always return a host, so invalid hosts should not return null. I propose to simply remove these hosts from the host map and clear the current host list so that no values will be saved, effectively causing invalid hosts to result in the same as unknown hosts.

-- 
Jonas Fonseca