fix: cache path checks across Windows volumes - #35
Conversation
WoozyMasta
changed the title
|
Hi @bobg, thanks for maintaining mingo. Workflow is currently awaiting maintainer approval. Could you approve and review it? |
bobg
left a comment
bobg
left a comment
There was a problem hiding this comment.
Thank you for this patch! I have one requested change.
| rel, err := filepath.Rel(dir, filename) | ||
| if err != nil { | ||
| return false, errors.Wrapf(err, "computing relative path from %s to %s", cacheDir, filename) | ||
| return false |
There was a problem hiding this comment.
As you observe, if dir and filename are in different Windows volumes, filepath.Rel will return an error. But that doesn't mean that all errors from filepath.Rel mean "different volumes." I'd rather not ignore those possible other errors.
How about explicitly detecting the separate-volumes condition using filepath.VolumeName?
There was a problem hiding this comment.
You're absolutely right, I missed that. It's now fixed, thanks for pointing it out!
There was a problem hiding this comment.
Pull request overview
This PR fixes Windows scanning failures when GOCACHE is on a different volume than the module being scanned by avoiding filepath.Rel errors across volumes and using a safer lexical containment check to decide whether a file is inside the cache directory.
Changes:
- Replace the cache containment check with a new
isWithinDirhelper that treats different Windows volumes as “outside” and usesfilepath.IsLocalon the relative path. - Add cross-platform and Windows-specific regression tests for directory containment behavior.
- Update CI to run the Go test suite on both Linux and Windows, while keeping coverage/modver steps Linux-only.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| scan.go | Refactors cache containment logic to handle Windows multi-volume paths and separator semantics safely. |
| scan_test.go | Adds general regression tests for lexical containment edge cases (parent/sibling/common-prefix). |
| scan_windows_test.go | Adds Windows-only tests covering different-volume and same-volume containment cases. |
| .github/workflows/go.yml | Runs tests on Windows via a job matrix; gates coverage/modver to Linux to avoid duplicate runs. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Apologies for the failing test - it comes from my Modver tool and, as I've just learned, it's because GitHub refuses to honor the issues: write permission in the workflow file when the PR originates from a fork. I'm going to make a change to the Modver workflow and then ask you to merge it into your PR. It'll take me a day or two, please bear with me. Thanks! (This is separate from the problem with the canceled Windows test, which I don't understand - do you?) |
bobg
left a comment
bobg
left a comment
There was a problem hiding this comment.
Thank you again for this change! My changes to the Modver workflow are taking longer than I planned, but they don't have to block this PR any longer. (Also, I now understand the Windows test failure: it was canceled when the Linux test failed.) Cheers!
filepath.Relreturns an error on Windows when the cache directory and scanned source file are located on different volumes.That condition means the source file is not inside cache and should not abort scan.
Changes:
Fixes #17