Page MenuHome

Don't resolve symlinks on packed paths

Authored by Sybren A. Stüvel (sybren) on Jan 26 2020, 10:34 PM.



Previously, path.resolve was used to resolve paths.
This had 3 effects:

  1. It made the path absolute
  2. Normalized the path
  3. Resolve symlinks and network drives

Because no. 3 was undesirable (see D6500), path.resolve was replaced
with os.path.abspath which just does the first two.

Diff Detail

Event Timeline

  • Make resolve_path backwards compatible with 3.5
Sybren A. Stüvel (sybren) requested changes to this revision.EditedMar 2 2020, 11:08 AM

Please don't add files with names like like utils, it doesn't mean anything. The resolve_path function can be placed in

I think it's a good idea to add one or more unittests that test the resolve_path() function (the test you added is good, but it only indirectly tests the behaviour). Most functionality can be tested for easily. It's just the NOT resolving of mapped drive letters to UNC notation that I wouldn't know how to test. Maybe that's possible with some intrusive use of unittest.mock?

As for the function name, I'm not convinced resolve_path is the right name, as it shouldn't resolve symlinks at all. Maybe canonical is a better name, as it in a way returns the canonical path.


bfile_path is already the result of utils.resolve_path(), so it doesn't have to be resolved again.


This line is getting quite long. I think it's cleaner to do this instead:

path = utils.resolve_path(path)
project_path = utils.resolve_path(self.project)

except ValueError:
    return False
return True

Also please don't remove my comment that explains why this is happening at all.

6 ↗(On Diff #21148)

This really needs an explanation as to why it's different than path.resolve(). It should include what it does the same (resolving ../../), and what it does differently (NOT resolving symlinks or mapped drive letters).

This revision now requires changes to proceed.Mar 2 2020, 11:08 AM

I'm taking over this patch to get it into BAT.

This revision now requires review to proceed.Tue, Mar 17, 10:59 AM
  • Cleanup: from pathlib import Path, PosixPath
  • Moved to, renamed function to make_absolute(), and added unit test
  • Rewrote symlinking unittest to be a bit more explicit on what is expected behaviour, and to test the actual files on the filesystem (not just library references in one file).
This revision was not accepted when it landed; it landed in state Needs Review.Tue, Mar 17, 4:05 PM
This revision was automatically updated to reflect the committed changes.

Thanks for the patch @Isaac Weaver (wisaac) . I took it a bit further and committed it as rBASTe4bf2e8e35a8: Improved path handling.