Skip to content

Fix case sensitivity and realpath issues on Windows#8

Open
savetheclocktower wants to merge 5 commits into
masterfrom
case-sensitivity-experiments
Open

Fix case sensitivity and realpath issues on Windows#8
savetheclocktower wants to merge 5 commits into
masterfrom
case-sensitivity-experiments

Conversation

@savetheclocktower

@savetheclocktower savetheclocktower commented Jun 13, 2026

Copy link
Copy Markdown

We thought we'd be able to add a feature to this repo, then bump the usage of git-utils in ppm, and then there'd be no step three. Instead, we got a headache of the sort you can see narrated in pulsar-edit/ppm#175. Once @idleberg tried to switch to our new fork of git-utils — with libgit2 updated and bindings rewritten in N-API — they realized that CI was failing in Windows.

It was one symptom of an under-tested code path: isRepositoryPath, which is supposed to tell us whether an arbitrary path is the root of our repository. It even brags that this is better to use than doing straight-up string comparison of paths because isRepositoryPath is supposed to do a lot of realpath and path-separator and case-sensitivity normalization! And yet it wasn't.

To get all the specs to pass, I had to add some new strategies for verifying that two paths are equivalent (even if their strings don't match) and for getting the “real” path of a path that may not even exist. But all the tests pass now and the code is arguably even simpler.

Testing

I was able to break the specs by adding one more call to repo.isRepositoryPath — one that checks a path produced by a node:path method, rather than a path that we got from git-utils itself. And it was stubborn to fix because the CI runner made me do realpath resolution… because long filenames on Windows CI runners can have their directory names truncated to Windows 3.1–style 8.3 filenames. This was annoying and I'm happy that CI is green now.

@confused-Techie confused-Techie 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.

Love to see the love for Windows on this PR! And without a thorough review just wanted to mention there's a few console.log statements left over in spec/git-spec.js that we probably wanna remove

@savetheclocktower

Copy link
Copy Markdown
Author

I had to pin to windows-2022 because of this issue — looks like GitHub just updated the version of Visual Studio they ship with windows-latest.

We can go back to windows-latest once we bump Pulsar to an Electron version that uses Node >= 22, I think.

@savetheclocktower

Copy link
Copy Markdown
Author

Oh, I considered just declaring node-gyp as an explicit entry in devDependencies and upgrading to a version that worked with the latest Visual Studio… except that the earliest version we'd need is 12.1.0, and that version of node-gyp requires a minimum of Node 20.17.0. Right now we're on 20.16.0.

That means we could move back to windows-latest once we get to Electron 33 (Node 20.18.0). But that won't happen until Pulsar 1.134, since we're only bumping as far as Electron 32 for Pulsar 1.133.0. (We need to prepare macOS users for removal of support for Catalina, or else we could upgrade more quickly.)

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.

2 participants