Code review comment for lp:~mbp/bzr/497274-http-405

2009/12/22 John A Meinel <email address hidden>:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
>
>>     martin> I guess I don't properly understand whether you think
>>     martin> my patch is actually wrong or just needs tests.  I
>>     martin> will try to use your tests with mine and see where we
>>     martin> get to.
>>
>> I hope I clarified that: except for 404, I'm fine with a common
>> error handling, may be I just haven't seen yet a good way to
>> implement that but I didn't spend much time on it either and I
>> trust you to find it ;)
>>
>
> If the only code that uses 404 specially is HEAD, then I would be happy
> to have it trap the exception. #1 because I don't think we use
> "transport.has*" in any live code path. LBYL vs EAFTP.
>
> I think the only code that uses it is probably old-format repositories.
> (Where having a revision was indicated by the presence of the
> '.bzr/revisions/$REVID' file.)
>
> I'm pretty sure everything else would treate 404 as NoSuchFile.

I think it would be reasonable for .has() to just catch NoSuchFile.

Having looked at this a bit more: InvalidHttpResponse mixes both "not
valid http" and "not the http code we expected." So perhaps a good
step would be to separate that into "HttpError" containing all the
right fields, then we can translate that into a specific bzr error,
taking into account the type of operation we were doing.

However, since imports from google are apparently now working, this is
not urgent for me.

--
Martin <http://launchpad.net/~mbp/>