Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove `git_commit_tree` which duplicates `commit.getTree()` #1267

Open
wants to merge 2 commits into
base: master
from

Conversation

@rcjsuen
Copy link
Member

rcjsuen commented Apr 11, 2017

libgit2 exposes a git_commit_tree function for retrieving the Tree of a Commit object. In NodeGit, we have our own custom getTree() which does the same thing.

The tree(tree_out) function needs a pointer and this does not make any sense in the JavaScript world. The function should be ignored by the code generator.

@maxkorp
Copy link
Collaborator

maxkorp commented Apr 18, 2017

The tree(tree_out) function needs a pointer and this does not make any sense in the JavaScript world

Shouldnt that pointer be a nodegit commit object which then gets unwrapped?

@rcjsuen
Copy link
Member Author

rcjsuen commented Apr 18, 2017

Shouldnt that pointer be a nodegit commit object which then gets unwrapped?

No, as the wrapped git_commit object is already the thing that's calling the function.

Current libgit2 API:

var result = git_commit_tree(tree_out, commit)

Current NodeGit API:

// generated version
var result = commit.tree(tree_out)
// custom one written in lib/commit.js
commit.getTree().then(function(tree) {
  // use the tree
});

So as you can see, the generated NodeGit API already knows what the commit object is because that's the object that's calling the tree(tree_out) prototype function. However, it's asking for an actual pointer object to "output" the tree into which doesn't make that much sense in NodeGit's object-oriented API and merely duplicates the functionality of the custom one.

I guess there are technically three (or more?) things we can do here.

  1. Remove the generated tree(tree_out) function as originally suggested.
  2. Remove the custom getTree() function and fix the generated tree(tree_out) function to be a simple tree() promise callback.
  3. Generate tree(tree_out) function privately but don't expose it into the public API and then replace the internal implementation of the custom getTree() function to delegate to the generated tree(tree_out) function.
@implausible
Copy link
Member

implausible commented Oct 13, 2017

This doesn't seem correct. Instead we should kill the getTree function and correct the descriptor for the git_commit_tree generated code. @rcjsuen

@rcjsuen
Copy link
Member Author

rcjsuen commented Oct 23, 2017

@implausible Thank you for your comment.

Should git_commit_tree become a commit.tree() JavaScript function or should it be massaged to generate a commit.getTree() instead?

@rcjsuen rcjsuen force-pushed the rcjsuen:remove-commit-tree branch from a9057c0 to 0537fd6 Oct 24, 2017
@rcjsuen
Copy link
Member Author

rcjsuen commented Oct 24, 2017

@implausible The new change that I've pushed kills commit.getTree() outright and replaces it with commit.tree().

@rcjsuen rcjsuen force-pushed the rcjsuen:remove-commit-tree branch from 0537fd6 to e570bdc Nov 29, 2017
@rcjsuen rcjsuen force-pushed the rcjsuen:remove-commit-tree branch from e570bdc to 8fc9dfd Dec 20, 2017
There is a custom handwritte commit.getTree() JavaScript function
which conflicts with the git_commit_tree C function provided by
libgit2. The custom function has been removed in favour of generating
such a function from libgit2's API.

Signed-off-by: Remy Suen <remy.suen@gmail.com>
@rcjsuen rcjsuen force-pushed the rcjsuen:remove-commit-tree branch from 8fc9dfd to 1da88f3 Mar 19, 2018
@implausible implausible force-pushed the nodegit:master branch from c345989 to c67b436 Aug 26, 2019
@implausible implausible force-pushed the nodegit:master branch from 2b7db46 to 69b010a Jul 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.