Skip to content

Fix case sensitivity and realpath issues on Windows - #8

Merged
savetheclocktower merged 10 commits into
masterfrom
case-sensitivity-experiments
Sep 20, 2026
Merged

savetheclocktower merged 10 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.)

@savetheclocktower

Copy link
Copy Markdown
Author

Bump. Might land this after a few days if there are no objections.

Comment thread src/git.js
@savetheclocktower

Copy link
Copy Markdown
Author

OK, I came back to this with fresh eyes. The changes I had made were a bit hacky and the product of some desperation, all of it revolving around the idea that it's ridiculous there aren't better ways to convert between short-name and long-name paths on Windows. I got all the specs to pass, sure, but I did so by applying maximum paranoia and doing a lot of redundant work.

Today's commits tried to untangle all that. Now we generally try to resolve via realpath early on and save the result; we also are a bit more thoughtful about skipping file I/O operations when they're not needed.

Finally, after the specs passed, I had no firm confirmation that they passed because we were handling 8.3 file names correctly… or just because we weren't encountering any in CI. So now the workflow explicitly prints out some of the paths that we encounter (the canonical temp directory, the working directory, the home directory) so we can confirm. On the most recent run (and presumably for a while) we naturally come into contact with an 8.3 path because on Windows, os.tmpdir() resolves to C:\Users\RUNNER~1\AppData\Local\Temp. So now we can be confident that short names are handled properly.

@savetheclocktower
savetheclocktower merged commit faae5d4 into master Sep 20, 2026
8 checks passed
@savetheclocktower
savetheclocktower deleted the case-sensitivity-experiments branch September 20, 2026 18:44
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.

3 participants