[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