Repository navigation
Fix case sensitivity and realpath issues on Windows - #8
Conversation
confused-Techie
left a comment
There was a problem hiding this comment.
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
|
I had to pin to We can go back to |
|
Oh, I considered just declaring That means we could move back to |
|
Bump. Might land this after a few days if there are no objections. |
|
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 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, |
We thought we'd be able to add a feature to this repo, then bump the usage of
git-utilsinppm, 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 ofgit-utils— withlibgit2updated 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 becauseisRepositoryPathis 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 anode:pathmethod, rather than a path that we got fromgit-utilsitself. 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.