Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Preserve original letter-casing for displaying #13205

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

wdhongtw
Copy link

@wdhongtw wdhongtw commented Feb 5, 2025

This change will preserve path in it's original letter-case during displaying.

Related to: #6823

Because os.getcwd and os.path.abspath both honors original letter-case,
we should not normalize case again when calculating the relative path.

(Edited) As suggested, we match input path with cwd in normalized-casing,
while returning the relative part in original-casing.
Original path is returned untouched if the match fails.

And this change actually fix the (harmless) bug in Windows that relative path is not shown correctly,
as a side effect.

Before:

PS C:\Users\weida\Projects\pip> pip install luckydep                                      
Requirement already satisfied: luckydep in c:\users\weida\projects\pip\.venv\lib\site-packages (0.2.0)

After:

PS C:\Users\weida\Projects\pip> pip install luckydep
Requirement already satisfied: luckydep in .\.venv\Lib\site-packages (0.2.0)

All the testcase in unit test still passed. (And it should, because it's just a cosmetic change.)


I tried to locate the first commit that use os.path.normcase, but it turns out that it's already there
when this project migrated from svn to git in 2018.

95bcf8c5f6394298035a7332c441868f3b0169f4 `pip\utils\__init__.py`
767d11e49cb916e2d4637421d524efcb8d02ae8d `pip\util.py`
dc6b8393eb13e90edf0f6b9af96516d042b4d1c7 `pip/__init__.py`
ef63f2f48f55ab2e110e07cd069e6c0e6c287a2a `pip.py`
97c152c463713bdaa0c1531a910eeae681035489 `pyinstall.py`

Maybe there are some good reason to normalize case for the path in the past, but it should be ok to remove it now. :D

Comment on lines 193 to 194
path = os.path.abspath(path)
if path.startswith(os.getcwd() + os.path.sep):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comparison would break when path uses a compatible but not canonical casing. Case normalisation should be done for comparision, but not for the return value.

Copy link
Author

@wdhongtw wdhongtw Feb 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Case normalisation should be done for comparision, but not for the return value.

Maybe we can try this solution

--- a/src/pip/_internal/utils/misc.py
+++ b/src/pip/_internal/utils/misc.py
@@ -190,9 +190,10 @@ def rmtree_errorhandler(
 def display_path(path: str) -> str:
     """Gives the display value for a given path, making it relative to cwd
     if possible."""
-    path = os.path.normcase(os.path.abspath(path))
-    if path.startswith(os.getcwd() + os.path.sep):
-        path = "." + path[len(os.getcwd()) :]
+    norm = os.path.normcase
+    abs_path = os.path.abspath(path)
+    if norm(abs_path).startswith(norm(os.getcwd()) + os.path.sep):
+        return "." + abs_path[len(os.getcwd()) :]
     return path

So the original value is return when our heuristic matching fails,
and when the matching successes, we can return the part starting from current path untouched.

Does that look good?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds reasonable to me. Or we can use os.path.commonpath to do the comparison instead.

@@ -0,0 +1 @@
This change will preserve path in it's original letter-case during displaying.
Copy link
Member

@uranusjr uranusjr Feb 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
This change will preserve path in it's original letter-case during displaying.
Better preserve original casing when a path is displayed.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No problem, will be fixed in next revision.

Simplify input path as a relative path to current working directory,
while preserving the original letter-casing at best effort.
@wdhongtw
Copy link
Author

wdhongtw commented Feb 5, 2025

Modified according to review feedbacks.


Since that display_path has implicit dependency on process state. It's not easy to write a
test for this function.
Here is a sample script to validate the change of this PR.

from typing import NamedTuple
import os


def display_path(path: str) -> str:
    """Gives the display value for a given path, making it relative to cwd
    if possible."""
    abs_path, cwd = os.path.abspath(path), os.getcwd()
    common = os.path.commonpath([cwd, abs_path])
    if len(common) == len(cwd):
        return "." + abs_path[len(common) :]
    return path


class Case(NamedTuple):
    input: str
    expected: str


testCases = [
    Case(
        input=r"C:\Users\weida\Projects\pip\main.py",
        expected=r".\main.py",
    ),
    Case(
        input=r"C:\Users\weida\Projects\pip\main.PY",
        expected=r".\main.PY",
    ),
    Case(
        input=r"C:\Users\weida\Projects\PIP\main.py",
        expected=r".\main.py",
    ),
    Case(
        input=r"C:\Users\WeiDa\Projects\ABC\main.PY",
        expected=r"C:\Users\WeiDa\Projects\ABC\main.PY",
    ),
]

# modify this path if needed
assert os.getcwd() == r"C:\Users\weida\Projects\pip"

for idx, test in enumerate(testCases):
    result = display_path(test.input)
    if result == test.expected:
        print(f"Test case {idx} passed.")
    else:
        print(f"Test case {idx} failed: got {result}, expected {test.expected}.")

@wdhongtw wdhongtw changed the title Skip normalizing case for path during displaying Preserve original letter-casing for displaying Feb 5, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants