Skip to content
/ cpython Public
  • Sponsor python/cpython

  • Notifications You must be signed in to change notification settings
  • Fork 32.4k

Handle leading // for posixpath.commonpath #117201

Not planned
@nineteendo

Description

@nineteendo

Bug report

Bug description:

>>> import posixpath
>>> posixpath.commonpath(["//foo/bar", "//foo/baz"])
'/foo'

Expected: //foo with precisely two leading slashes, see: https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap04.html#tag_04_13

This can be quite easily solved using posixpath.splitroot, see the PR.

CPython versions tested on:

CPython main branch

Operating systems tested on:

macOS

Linked PRs

Activity

changed the title [-]`posixpath.commonpath` doesn't handle leading `//` properly[/-] [+]Handle leading `//` for `posixpath.commonpath`[/+] on Mar 26, 2024
serhiy-storchaka

serhiy-storchaka commented on Apr 4, 2024

@serhiy-storchaka
Member

For reference, the original issue that introduced commonpath() is bpo-10395/gh-54604.
And its behavior was discussed at https://mail.python.org/archives/list/python-ideas@python.org/thread/VHQTSU4ERTH262OPVZ6SEO2EYCFQF56G/.

erlend-aasland

erlend-aasland commented on Apr 4, 2024

@erlend-aasland
Contributor
nineteendo

nineteendo commented on Apr 5, 2024

@nineteendo
ContributorAuthor

This is the reason I don't like the current implementation:

from posixpath import abspath, commonpath, splitdrive

def isparent(path1, path2):
    drive1, tail1 = splitdrive(abspath(path1))
    drive2, tail2 = splitdrive(abspath(path2))
    return drive1 == drive2 and commonpath([tail1, tail2]) == tail1

if isparent("//foo", "//foo/bar"):
    print("path1 is a parent of path2")
else:
    print("path1 is not a parent of path2")

Output: path1 is not a parent of path2.

erlend-aasland

erlend-aasland commented on Apr 5, 2024

@erlend-aasland
Contributor

If anything, this looks to me like a commonpath documentation issue.

Moreover, if I were to write such a test, I would normalise tail1 before comparing it, or more probable: use a higher level API.

nineteendo

nineteendo commented on Apr 5, 2024

@nineteendo
ContributorAuthor

If anything, this looks to me like a commonpath documentation issue.

If that's the case, that's fine by me, but that should be confirmed by @ronaldoussoren.

Moreover, if I were to write such a test, I would normalise tail1 before comparing it

The thing is: the paths are already normalised here, but that won't replace precisely 2 leading slashes. Which is intended behaviour.

erlend-aasland

erlend-aasland commented on Apr 5, 2024

@erlend-aasland
Contributor

The thing is: the paths are already normalised here, but that won't eliminate a leading // as intended.

splitdrive does not normalise the paths, if that is what you imply.

nineteendo

nineteendo commented on Apr 5, 2024

@nineteendo
ContributorAuthor

No, the paths I'm passing to splitdrive() are already normalised (I added abspath() to make that clearer), but I would still need to apply commonpath() to the tail to make the comparison work. I shouldn't have to do that.

erlend-aasland

erlend-aasland commented on Apr 5, 2024

@erlend-aasland
Contributor

Well, it is still a backwards-incompatible change.

nineteendo

nineteendo commented on Apr 5, 2024

@nineteendo
ContributorAuthor

Anyway, whether this will be accepted depends on whether this is intentional behaviour or not:

  • If it's intentional, we should document and test this.
  • If it isn't, we should document the new behaviour like how os.path.isabs() was changed to handle paths correctly starting with exactly one (back)slash on Windows.
ronaldoussoren

ronaldoussoren commented on Apr 5, 2024

@ronaldoussoren
Contributor

IMHO we should consider dropping special handling for '//' at the start of a path for our path manipulation APIs. I cannot recall a Unix where '//foo' is different from '/foo', but it is an awfully long time that I've used anything other than Linux and macOS.

The Open Group documents that '//foo' may be special ("may be treated in an implementation defined way"), but AFAIK commonly used UNIX-y systems don't use this possibility.

Because most systems don't treat '//foo' specially we shouldn't change the behaviour of os.path.commonpath as this will only lead to confusing users and might have a security impact for code that expects the current behaviour.

8 remaining items

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    type-bugAn unexpected behavior, bug, or error

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions

      Handle leading `//` for `posixpath.commonpath` · Issue #117201 · python/cpython