Code review comment for lp:~rls/bzr/transport-link-and-symlink-support

On 3 March 2010 19:40, Neil Santos <email address hidden> wrote:
>> However it seems like paramiko normally translates errors into
>> exceptions itself, so you may not need to worry about this?
>
> Not sure about that part.  I think it raises errors on blatant errors (disconnection, non-support-- stuff that the library can't handle without human intervention, I suppose).  For the most part, it returns error codes.  Crude, but hey, I just work with it. :P

Heh, ok. So on the whole, I'd say if you get a non-ok return code,
just raise an error that includes that value; we could add a method
that specifically translates them if we think any are interesting.

>
>> Actually the thread seemed to conclude that it was better for Transport.stat()
>> to call os.lstat, and then make the higher level code specifically deal with
>> the symlink.
>
> Okay, I've reverted that, then.
>
>> Like that, except the osutils.hardlink_good() check seems redundant.  Just
>> declare the method always and let the AttributeError call handle the case that
>> they're not supported.
>
> Eh.  Your suggestion conflicts with gz's.  I do like the idea of letting the rest of bzrlib's guts tell me whether it's okay to declare a method, though, rather than handle an AttributeError myself.  But you guys are more familiar with the gestalt of Bzr than I am, so perhaps I'm missing something?

I think gz and I agree that there's no point in having redundant
checks. As he says, we have to handle the case that it turns out not
to be possible at runtime, and I guess it just seems a bit cleaner to
me to avoid conditionally declaring functions. Either is ok though,
so let me know when you're ready for this to be rereviewed/merged.

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