[MERGE] fix blackbox failures when run from windows executable
John Arbash Meinel
john at arbash-meinel.com
Mon Aug 11 23:54:25 BST 2008
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
Andrew Bennetts wrote:
> Mark Hammond wrote:
> [...]
>>> 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?
>
> I think I'd mildly prefer that. That at least makes it clear that there's no
> fundamental change to the path being represented, just a cosmetic change of
> representation.
I think you are fixing the wrong half of it. I would rather you change:
self.assertEqual(os.path.realpath('blah', 'blah', 'blah'),
...local_abspath('blah'))
to be
self.assertEqual(osutils.realpath('blah', 'blah', 'blah'),
...local_abspath('blah'))
osutils.realpath() should handle the .replace('\\', '/') for you.
I think it is *correct* to assert that local_abspath() returns '/'
separated paths. Which is what osutils.realpath() will do.
>
>>> 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.
>
> Hmm, I think Robert's right that that's for internal use. So in that case the
> only change to Transport.local_abspath we should make is clarifying the
> docstring, because at the moment it implies that it will produce the something
> equivalent to os.path.realpath's output.
>
> -Andrew.
>
So... local_abspath() is meant to be used because Transports work only
in terms of URLs, and sometimes we need to open something up on disk,
and doing open('file:////') doesn't work.
The only callers are in workingtree.py and workingtree_4.py
One could argue about it returning a "C:\Path" rather than "C:/Path",
but ATM I prefer it returning "C:/Path" to be consistent with all other
paths inside bzrlib.
John
=:->
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org
iEYEARECAAYFAkigwyEACgkQJdeBCYSNAANqvgCgtAH/LfkARG7QspAoYa1bJE/f
bW0AmwagwoB/2PyHrRv8/Gy0ltESbWvK
=NpFD
-----END PGP SIGNATURE-----
More information about the bazaar
mailing list