Skip to content

fix(stats): handle windows file project roots#453

Open
jojosenthusiast wants to merge 1 commit into
scip-code:mainfrom
jojosenthusiast:fix-windows-file-uri
Open

fix(stats): handle windows file project roots#453
jojosenthusiast wants to merge 1 commit into
scip-code:mainfrom
jojosenthusiast:fix-windows-file-uri

Conversation

@jojosenthusiast

Copy link
Copy Markdown

Fixes #282

Reproduction

scip stats can fail when a SCIP index contains a Windows project root like file://D:\dev\project, because url.Parse treats the drive-letter colon as an invalid host port.

Fix summary

  • Add a shared project-root conversion helper that handles the reported Windows file://D:\... form before falling back to url.Parse(...).Path for normal file URIs.
  • Use the helper in scip stats line-counting and snapshot formatting paths.
  • Also fix the existing countLinesOfCode path where an existing parsed project root left localSource empty before os.Stat("").
  • Add unit coverage for Windows-style and Unix file URI project roots.
  • Add cmd/scip seam tests covering countLinesOfCode behavior.

Tests run

  • git diff --check ? pass
  • gofmt -l on changed Go files ? pass, no output
  • go test ./bindings/go/scip -run TestProjectRootToLocalPath -count=1 ? pass
  • go test ./cmd/scip -run TestCountLinesOfCode -count=1 ? pass
  • go test ./bindings/go/scip ./bindings/go/scip/testutil ./cmd/scip ? fails only at cmd/scip.TestCLIReferenceInSync, unrelated to this diff because CLI help/docs files are not changed

Risk

Low. The special case is limited to the reported non-canonical Windows file://<drive>: form; other file URIs continue through Go's URL parser.

Non-goals

  • No broad URI parser rewrite.
  • No fix for canonical Windows file:///D:/... behavior beyond preserving existing parser behavior.

@jupblb jupblb left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thank you very much for this contribution. I've left some comments. But, to be honest, even if these issues are fixed I'm a bit against merging this PR just yet.

We should first:

  1. Figure out if someone is willing to maintain support for Windows long-term (if that's you @jojosenthusiast please let us know)
  2. Add Windows host to our CI (doesn't have to be the full suite, like the one we're using with Nix, just the essential checks)

If there is interest in having support for Windows then I'm cool with adding it, but the existing maintainers currently do not have the capacity needed to ensure this keeps working.

// For all other inputs it preserves the previous behavior of using
// url.Parse(rawProjectRoot).Path.
func ProjectRootToLocalPath(rawProjectRoot string) (string, error) {
if isWindowsFileURIWithDriveLetter(rawProjectRoot) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can we invert this flow and try url.Parse(rawProjectRoot) first, then fall back only if parsing fails with the known Windows drive-root shape?
Right now the special-case runs before url.Parse, so it catches inputs that previously parsed successfully. For example, file://x:123/share previously produced path /share, but this helper would now return x:123/share. That conflicts with the comment below saying all other inputs preserve url.Parse(rawProjectRoot).Path.

// ponytail: only the "file://X:..." form is handled here; the
// canonical "file:///X:/..." form parses fine via url.Parse and is
// out of scope for this fix.
return rawProjectRoot[len("file://"):], nil

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This should probably run through url.PathUnescape before returning. The normal url.Parse(...).Path branch gives callers the decoded path, but this branch returns the raw URI suffix. A root like file://D:%5Cdev%5Cmy%20project would currently become D:%5Cdev%5Cmy%20project instead of D:\dev\my project.

// isWindowsFileURIWithDriveLetter reports whether s matches "file://X:..."
// where X is an ASCII letter and the position after the scheme is NOT a third
// slash (which would indicate the canonical "file:///..." form).
func isWindowsFileURIWithDriveLetter(s string) bool {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This predicate seems too broad for the stated fix. It accepts any file://<letter>: prefix, including drive-relative paths like file://D:relative, which previously failed parsing and are not absolute project roots.
Could we require the character after the drive colon to be a path separator (\ or /), or otherwise only enter this fallback after url.Parse has failed? That keeps the workaround scoped to the issue shape.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

SCIP CLI misinterprets colon in Windows paths as host port

2 participants