Skip to content

Normalize case before comparing#4770

Merged
pfmoore merged 1 commit intopypa:masterfrom
pradyunsg:fix/uninstall-normcase
Oct 7, 2017
Merged

Normalize case before comparing#4770
pfmoore merged 1 commit intopypa:masterfrom
pradyunsg:fix/uninstall-normcase

Conversation

@pradyunsg
Copy link
Copy Markdown
Member

@pradyunsg pradyunsg commented Oct 5, 2017

To the degree that I can figure out, while on a break from doing math problem, this should fix the case problem with #4493.

@pradyunsg pradyunsg added type: maintenance Related to Development and Maintenance Processes skip news Does not need a NEWS file entry (eg: trivial changes) labels Oct 5, 2017
@pradyunsg pradyunsg requested a review from pfmoore October 5, 2017 14:57
@pradyunsg
Copy link
Copy Markdown
Member Author

pradyunsg commented Oct 5, 2017

@pfmoore Could you give this a try and see how this runs?

@pfmoore
Copy link
Copy Markdown
Member

pfmoore commented Oct 5, 2017

Will do. Might not be till tomorrow...

@pradyunsg
Copy link
Copy Markdown
Member Author

pradyunsg commented Oct 5, 2017

Cool. :)

No hurries.

@pradyunsg pradyunsg force-pushed the fix/uninstall-normcase branch from 2001c21 to 5306fbe Compare October 5, 2017 16:18
@pradyunsg pradyunsg mentioned this pull request Oct 5, 2017
Merged
@pradyunsg
Copy link
Copy Markdown
Member Author

pradyunsg commented Oct 7, 2017

Ping! ^.^

@pfmoore
Copy link
Copy Markdown
Member

pfmoore commented Oct 7, 2017

Yeah, as far as I can tell it fixes the issue (and sorry for the delay!)

@pfmoore pfmoore merged commit c81e8ac into pypa:master Oct 7, 2017
@pradyunsg pradyunsg deleted the fix/uninstall-normcase branch October 7, 2017 12:18
@pradyunsg
Copy link
Copy Markdown
Member Author

pradyunsg commented Oct 7, 2017

fixes the issue

Awesome! ^.^

@anntzer
Copy link
Copy Markdown
Contributor

anntzer commented Jul 23, 2018

A minor annoyance is that paths are now all represented as lowercase on Windows when prompting for removal ("c:/users/...") rather than as their canonical value ("C:/Users/..."). Solutions would include re-canonicalizing the paths (https://stackoverflow.com/questions/3692261/in-python-how-can-i-get-the-correctly-cased-path-for-a-file, https://stackoverflow.com/questions/2113822/python-getting-filename-case-as-stored-in-windows) or switch to pathlib as WindowsPath handles the comparisons fine.

On the other hand it is indeed really only a minor annoyance.

@pfmoore
Copy link
Copy Markdown
Member

pfmoore commented Jul 23, 2018

@anntzer Good point, I agree that we should be displaying pathnames to the user in the correct case as held on the filesystem. It's a quality of implementation issue, and as you say only a minor cosmetic annoyance, but I'd be happy to review and approve a PR to fix this.

@anntzer
Copy link
Copy Markdown
Contributor

anntzer commented Jul 23, 2018

It's likely not that hard, I think something like

diff --git i/src/pip/_internal/req/req_uninstall.py w/src/pip/_internal/req/req_uninstall.py
index f7ec3a8f..dd65fe43 100644
--- i/src/pip/_internal/req/req_uninstall.py
+++ w/src/pip/_internal/req/req_uninstall.py
@@ -119,6 +119,7 @@ def compress_for_output_listing(paths):
         if path.endswith("__init__.py") or ".dist-info" in path:
             folders.add(os.path.dirname(path))
         files.add(path)
+    _normcased_files = set(map(os.path.normcase, files))
 
     folders = compact(folders)
 
@@ -130,8 +131,9 @@ def compress_for_output_listing(paths):
                 if fname.endswith(".pyc"):
                     continue
 
-                file_ = os.path.normcase(os.path.join(dirpath, fname))
-                if os.path.isfile(file_) and file_ not in files:
+                file_ = os.path.join(dirpath, fname)
+                if (os.path.isfile(file_)
+                        and os.path.normcase(file_) not in _normcased_files):
                     # We are skipping this file. Add it to the set.
                     will_skip.add(file_)
 
diff --git i/tests/unit/test_req_uninstall.py w/tests/unit/test_req_uninstall.py
index dc851796..8f999457 100644
--- i/tests/unit/test_req_uninstall.py
+++ w/tests/unit/test_req_uninstall.py
@@ -50,9 +50,9 @@ def test_compressed_listing(tmpdir):
     def in_tmpdir(paths):
         li = []
         for path in paths:
-            li.append(str(os.path.normcase(
-                os.path.join(tmpdir, path.replace("/", os.path.sep))
-            )))
+            li.append(
+                str(os.path.join(tmpdir, path.replace("/", os.path.sep)))
+            )
         return li
 
     sample = in_tmpdir([

will work? (The diff on the test is just reverting this PR.)

Feel free to adopt the patch as a PR.

@pfmoore pfmoore mentioned this pull request Jul 23, 2018
Merged
@pfmoore
Copy link
Copy Markdown
Member

pfmoore commented Jul 23, 2018 •
edited
Loading

OK, I made #5642 based on your code. Thanks!

One thing I wonder about - the original code here does normcase() on file_, but doesn't actually seem to normcase the original files list! So the original may have actually been wrong. Tracking back from the original comment from @pradyunsg on this PR, I can't actually work out what "the case problem with #4493" was, so I don't really know how to confirm that suspicion.

Your version avoids that problem, though, so I think it's better in any case.

@lock
Copy link
Copy Markdown

lock bot commented Jun 2, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot added the auto-locked Outdated issues that have been locked by automation label Jun 2, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Jun 2, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@pfmoore pfmoore pfmoore approved these changes

Assignees

No one assigned

Labels

auto-locked Outdated issues that have been locked by automation skip news Does not need a NEWS file entry (eg: trivial changes) type: maintenance Related to Development and Maintenance Processes

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@pradyunsg @pfmoore @anntzer