-
Notifications
You must be signed in to change notification settings - Fork 638
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
Use nodegit #1315
base: master
Are you sure you want to change the base?
Use nodegit #1315
Conversation
Seems like the test failures are around missing support in some of the APIs like Pretty cool to see this! |
FYI: we removed babel in #1178 because we didn't need it anymore. Our minimum supported node version is 10 which should support async / await already (you noted that in #1323). Are there any other reasons you want to use babel for? |
@campersau I added it for now, these are the plugins babel adds for v12:
I really like optional chaining and object rest spread, the rest I can take or leave. I added prettier because I just can't code without it any more :) I hope the PR passes and then I'll rebase. In the meantime, I try to keep the individual commits clean. |
e73ccf1
to
f403c58
Compare
To implement gitlog I'm a little stuck, since it means iterating over the nodes in some order. Besides, the current implementation is broken too. Ideally though, the frontend would be ok with commits without metadata, so the entire tree can be loaded in memory. |
took out Babel again since my main concern was the async/await. I'm having a failing test on the checkout: ungit/test/spec.git-api.conflict.js Line 107 in 6009c23
this checks out clean for me, presumably because of autostash. If I don't use autostash, the error is about local changes, not a merge conflict. I don't understand how this could pass. |
4b9fe01
to
a004086
Compare
To see the changes without the prettier commits, look at wmertens/ungit@prettier...wmertens:nodegit |
the failing checkout test is because libgit2 doesn't allow applying a stash when the index isn't clean. The workaround would be to merge the stash commit manually and then drop it, but alternatively we could decide that it's ok like that. The failing tests on v14 are because nodegit doesn't have a build for v14 and libkrb5 is missing; it takes really long to build nodegit so I think it's ok to wait for a binary build. |
@wmertens https://libgit2.org/libgit2/#HEAD/type/git_stash_apply_options has a There are quite a few options in: require('nodegit').Checkout.STRATEGY
STRATEGY: {
NONE: 0,
SAFE: 1,
FORCE: 2,
RECREATE_MISSING: 4,
ALLOW_CONFLICTS: 16,
REMOVE_UNTRACKED: 32,
REMOVE_IGNORED: 64,
UPDATE_ONLY: 128,
DONT_UPDATE_INDEX: 256,
NO_REFRESH: 512,
SKIP_UNMERGED: 1024,
USE_OURS: 2048,
USE_THEIRS: 4096,
DISABLE_PATHSPEC_MATCH: 8192,
SKIP_LOCKED_DIRECTORIES: 262144,
DONT_OVERWRITE_IGNORED: 524288,
CONFLICT_STYLE_MERGE: 1048576,
CONFLICT_STYLE_DIFF3: 2097152,
DONT_REMOVE_EXISTING: 4194304,
DONT_WRITE_INDEX: 8388608,
UPDATE_SUBMODULES: 65536,
UPDATE_SUBMODULES_IF_CHANGED: 131072
} You may have seen this already, but if not, something to try |
@tbranyen right, I should have mentioned, I tried all those things, but the problem is this line https://github.com/libgit2/libgit2/blob/66137ff6ea9e516e0fa840134393d5a81d5b86e9/src/stash.c#L900 It unconditionally refuses to apply the stash if there is something in the index. 🤔 I could unstage everything, or I could ask them to remove the restriction. Or we could adhere to the restriction. Or maybe I'm not understanding something. |
I created libgit2/libgit2#5501 since libgit2 behaves differently from git |
For autostash, I'll just unstage. For the commits, I'll:
The client needs to request extra metadata (message, file diffs) when missing while showing a commit and needs to request the logs when it gets a message that refs are updated. |
ec2bddc
to
b03dced
Compare
status: it gets the gitlog via nodegit but the client side needs some changes to optimize fetches. Most GET calls are replaced. They are lots faster. |
status:
|
status: now it loads very quickly. However, there are some odd bugs:
I want to change the gitlog call so that the client keeps track of missing parents, and when you scroll near a missing parent, it loads the missing parents. Nodegit can walk a bunch of commits. Would be great to have some eyeballs on this. |
Status: looking pretty nice. I'll take it out of draft to hopefully get more people looking at it. When you scroll far, the graphics break down, not sure what's at fault. Also, the graph compacting slot choosing algorithm isn't entirely there yet it seems. |
Nice 👍 I'll give it a try |
- diffs: make component+api robust against missing names - diffs: send names anyway even though they don't make sense
+ start keeping context for caching: patchNum instead of paths
* not using index + worktree yet
- /status: add commitMessage - /status: add index and wt diffs
the displayHtml wasn't defined yet at the time Backbone decided to redraw
- /commits endpoint to get a collection of commits and their parents - add date to ref structure for client side sorting - layout nodes to minimize overlap and still remain compact - load nodes when then show up on screen
This is a work in progress - the idea is to replace as many git calls as possible with nodegit calls.
Cc @ylecuyer who started this effort
Fixes #260 if we get rid of git entirely :)