Skip to content

add native TS types and a free() method - #27

Open
jboyens wants to merge 2 commits into
masterfrom
jboyens/fix-remove-deprecated-buffer-c/wzuzvkwnrwkw
Open

jboyens wants to merge 2 commits into
masterfrom
jboyens/fix-remove-deprecated-buffer-c/wzuzvkwnrwkw

Conversation

@jboyens

@jboyens jboyens commented Sep 25, 2026

Copy link
Copy Markdown
🚥 Resolves ISSUE_ID

🧰 Changes

  • adds an explicit free() method and tweaks descendant objects to track
  • adds native TS types to resolve drift from an aging @types/nodegit

🧬 QA & Testing

Provide as much information as you can on how to test what you've done.

@jboyens
jboyens force-pushed the jboyens/fix-remove-deprecated-buffer-c/wzuzvkwnrwkw branch from e80c13e to 225bbea Compare September 25, 2026 17:05

@domharrington domharrington 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.

LGTM. The type fixes could've been done separately, but I also think it's fine. Would be curious to see some benchmarks of how this performs on the gitto side.

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.

Idk how much test coverage we should be adding for all of these different error states? Is this a branch that we should be testing?

Comment thread lib/repository.js
Comment on lines -1127 to +1130
.catch(function() {
.catch(function(error) {
if (error.errno !== NodeGit.Error.CODE.ENOTFOUND) {
throw error;
}

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.

New behaviour?

Comment thread test/tests/repository.js
});
});

it("rejects operations on a freed repository without invoking libgit2", function() {

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.

Oh maybe this is the tests I was asking about before?

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