Skip to content

Rewrite in N-API and update libgit2#6

Merged
savetheclocktower merged 22 commits into
pulsar-edit:masterfrom
savetheclocktower:context-aware-napi-and-libgit2-upgrade
Jun 1, 2026
Merged

Rewrite in N-API and update libgit2#6
savetheclocktower merged 22 commits into
pulsar-edit:masterfrom
savetheclocktower:context-aware-napi-and-libgit2-upgrade

Conversation

@savetheclocktower
Copy link
Copy Markdown

git-utils is failing to build altogether on my work laptop (which runs macOS 26, despite my best efforts), so I figured this was a good time to bump libgit2.

This also adopts the needed rewrite of git-utils in N-API; this should probably fix pulsar#1468.

The tests pass for me locally, but I'll open this as a draft to start just to see what CI thinks of it on the other platforms.

@savetheclocktower savetheclocktower force-pushed the context-aware-napi-and-libgit2-upgrade branch from 2509503 to 1723dfa Compare February 28, 2026 00:45
@savetheclocktower
Copy link
Copy Markdown
Author

The size of the diff should be explained.

We used to explicitly vendor libgit2 into deps/libgit2. When I did my N-API rewrite about 16 months ago, I converted that back into a submodule. If the submodule approach works, I'd much prefer it; if not, I can switch back to vendoring.

Either way, this PR would touch a lot of files; if the submodule approach works, then future libgit2 bumps would go much more smoothly.

@savetheclocktower
Copy link
Copy Markdown
Author

OK, CI currently only tests in Linux. Good to know Linux passes, but I should add Windows to the matrix or else I can't be certain that this is a safe change to make! I'll revisit this tomorrow.

@savetheclocktower
Copy link
Copy Markdown
Author

PR #5 has more context about why libgit2 was vendorized, and why it's likely fine to reintroduce the submodule:

Separately: @mauricioszabo indicated that his original reason for vendorizing libgit2 had something to do with yarn. It occurs to me that he was likely running into problems that were specific to how yarn handled dependencies that were git repositories instead of NPM packages. But now that we have a package-publishing strategy, there should be no need for vendorizing, and we can return to the submodule strategy.

@savetheclocktower
Copy link
Copy Markdown
Author

Hey! Got a passing test suite on all platforms. I think some of those Windows bugs had been around for a while.

Taking this out of draft!

@savetheclocktower
Copy link
Copy Markdown
Author

Ping!

Copy link
Copy Markdown
Member

@DeeDeeG DeeDeeG left a comment

Choose a reason for hiding this comment

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

I don't deeply understand the changes, but specs are passing on all three OSes, so...

None of the individual commits look particularly crazy, either. From a quick look at each...

Perhaps we can add ubuntu-24.04-arm to the ci.yml matrix? To try compiling on ARM Linux? Maybe even macos-15-intel or macos-26-intel if feeling adventurous... But uh... optional. Adding Windows and macOS was already a great thing to do. I do think it'd be a good idea, though.

But anyway, probably as close as I'm going to get to "meaningfully reviewing" this ever. I reckon we dogfood it and make sure it behaves IRL, beyond what the specs show (which is looking good, much appreciated there).

Somewhat of a rubber-stamp Approval, admittedly.

But thanks for the efforts, and hopefully this helps us compile on newer macOS versions without breaking other (relevant) macOS/other OS versions? (Compiles on my macOS Catalina x64 machine, by the way! No node-gyp errors, just a few warnings! 👍)

Comment thread .github/workflows/ci.yml
savetheclocktower and others added 2 commits May 31, 2026 22:40
Co-authored-by: DeeDeeG <DeeDeeG@users.noreply.github.com>
@savetheclocktower savetheclocktower merged commit e7cd205 into pulsar-edit:master Jun 1, 2026
4 checks passed
@savetheclocktower savetheclocktower deleted the context-aware-napi-and-libgit2-upgrade branch June 1, 2026 06:32
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.

Occasional Git-related crashes on window reload

2 participants