[MERGE] fix blackbox failures when run from windows executable

Mark Hammond mhammond at skippinet.com.au
Mon Aug 11 01:17:33 BST 2008


> It doesn't matter very much, but I'd prefer 3-arg getattr here (if
> getattr(sys,
> "frozen", None) is None) rather than hasattr, because getattr won't
> swallow
> unrelated errors like KeyboardInterrupt.

Good point.

> > === modified file 'bzrlib/tests/test_bzrdir.py'
> > --- bzrlib/tests/test_bzrdir.py	2008-08-01 07:30:20 +0000
> > +++ bzrlib/tests/test_bzrdir.py	2008-08-10 06:55:33 +0000
> > @@ -533,7 +533,7 @@
> >                           self.local_branch_path(branch))
> >          self.assertEqual(
> >              os.path.realpath(os.path.join('topdir', '.bzr',
> 'repository')),
> > -            repo.bzrdir.transport.local_abspath('repository'))
> > +
> os.path.realpath(repo.bzrdir.transport.local_abspath('repository')))
> 
> Transport.local_abspath is supposed to be the absolute path on the
> local filesystem.  I'm a little surprised that local_abspath is 
> different to the realpath.  Is it just a \ vs. / issue?

Exactly - / vs \.  Maybe a path.replace("/", os.path.sep) would be better?
 
> In fact, the docstring for Transport.local_abspath says:
> 
>         This function is quite expensive: it calls realpath which
> resolves symlinks.
> 
> So I think perhaps the right fix might be to fix
> Transport.local_abspath (i.e.
> urlutils._win32_local_path_from_url) rather than these tests?

I'm inclined to agree with you about that - the function name implies native seps should be used - however I brought this up with Robert on IRC and he argued that this was still an 'internal path string', and as it would work with all os functions, it was not a problem, and steered me towards that test fix.

My opinion is that if this function intends to serve as an api for getting a "native" path that applications may call, it should use native seps - but if it is indeed considered internal, I see no problem with it staying as it is.

> (And I wonder if
> doing that would break or fix any other tests?)

Yes, that's another obvious concern.  If we decided that \ should be used on Windows, I'd be inclined to only make that change on .dev and stick with my patch for 1.6 - but obviously I will defer to others on this.

> If Transport.local_abspath isn't changed, then its docstring probably
> needs some
> clarification.

Agreed.

> This change looks a bit weird, particularly how you set
> bzrlib.plugins.__path__
> to [] multiple times.  I think it's correct, but confusing.  I think if
> you
> extract the first determine the default builtin path section into a
> helper
> method it would be a little clearer.

No probs.

> Again, 3-arg getattr would be good.

Cool.  Thanks for the review - I'll submit an updated patch once we come to some closure on that path issue.

Thanks,

Mark




More information about the bazaar mailing list