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.

Sign up for GitHub

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

Jump to bottom

path/filepath: Clean changed in Go 1.20 #58348

Closed
bep opened this issue Feb 6, 2023 · 6 comments
Closed

path/filepath: Clean changed in Go 1.20 #58348

bep opened this issue Feb 6, 2023 · 6 comments
Labels
Documentation Issues describing a change to documentation. FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Go1.21

Comments

@bep
Copy link
Contributor

bep commented Feb 6, 2023

I have some GitHub action tests that started to fail on Windows after I bumped the build matrix to Go 1.20.

I have distilled the issue into this small repo: https://github.com/bep/go120winpathregression

--- FAIL: TestTo (0.00s)
    lib_test.go:12: 
        error:
          values are not equal
        got:
          "\\\\\\\\post"
        want:
          "\\post"
        stack:
          D:/a/go[12](https://github.com/bep/go120winpathregression/actions/runs/4102512994/jobs/7075566395#step:11:13)0winpathregression/go120winpathregression/lib_test.go:12
            c.Assert(Clean("////post/////"), qt.Equals, filepath.FromSlash("/post"))

I have looked closely in the release notes for Go 1.20 about this, but that does not mention this change. Also, the GoDoc for filepath.Clean is pretty clear about what happens to "multiple Separator elements":

  1. Replace multiple Separator elements with a single one.
@bep bep mentioned this issue Feb 6, 2023
Merged
@seankhliao
Copy link
Member

cc @neild regression from #56336 ?

@seankhliao seankhliao changed the title filepath: Clean changed in Go 1.20 path/filepath: Clean changed in Go 1.20 Feb 6, 2023
@seankhliao seankhliao added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Feb 6, 2023
@bep
Copy link
Contributor Author

bep commented Feb 6, 2023

Looking back at the original failing test, I don't see why that should not use path.Clean, so I'm going to fix that, but I still think the above test case looks like a regression (esp. considering the documentation).

@neild
Copy link
Contributor

neild commented Feb 6, 2023

filepath.Clean doesn't clean the volume name component of Windows paths. This is longstanding behavior:

filepath.Clean(`\\a\b\\c`) = `\\a\b\c`

I see that this isn't documented, however. It should be.

As of Go 1.20, we do a better job of identifying volume names, and we're more lax about accepting invalid volume names. \\\\post is not a valid Windows path, because it contains empty host and share components (the first two path components after a leading \\). Previously, filepath considered this to have a zero-length volume name component; it now considers it to have the (invalid) volume name \\\\.

I don't think this is a regression, since filepath.Clean has always (or at least long) preserved a \\ prefix in valid Windows filenames.

But we should fix the docs.

@bcmills bcmills added Documentation Issues describing a change to documentation. NeedsFix The path to resolution is known, but the work has not been done. labels Feb 6, 2023
@bcmills bcmills added this to the Go1.21 milestone Feb 6, 2023
@gopherbot gopherbot removed the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Feb 6, 2023
@bhavyastar
Copy link

Hey @bep @bcmills , Can I work on this issue?

@ianlancetaylor
Copy link
Member

@bhavyastar Better documentation is always welcome. Thanks.

@antichris antichris mentioned this issue Feb 21, 2023
Closed
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/469955 mentions this issue: path/filepath: document that Clean does not change Windows volume names

johanbrandhorst pushed a commit to Pryz/go that referenced this issue Feb 22, 2023
@neild @johanbrandhorst
path/filepath: document that Clean does not change Windows volume names
14174b7 
Fixes golang#58348

Change-Id: I4aac0285f11618a45aca6b13c2da2a10a803a9b5
Reviewed-on: https://go-review.googlesource.com/c/go/+/469955
Reviewed-by: Bryan Mills <bcmills@google.com>
Auto-Submit: Damien Neil <dneil@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Damien Neil <dneil@google.com>
@imiric imiric mentioned this issue Feb 22, 2023
Closed
@golang golang locked and limited conversation to collaborators Feb 21, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
Documentation Issues describing a change to documentation. FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Go1.21
Development

No branches or pull requests
7 participants
@neild @bep @ianlancetaylor @bcmills @gopherbot @seankhliao @bhavyastar