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

Use nodegit #1315

Open
wants to merge 39 commits into
base: master
Choose a base branch
from
Open

Use nodegit #1315

wants to merge 39 commits into from

Conversation

wmertens
Copy link
Contributor

@wmertens wmertens commented Apr 23, 2020

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 :)

@wmertens wmertens marked this pull request as draft April 23, 2020 21:58
@tbranyen
Copy link

Seems like the test failures are around missing support in some of the APIs like /commit? Not sure why the Linux tests failed, maybe apt-get needs a retry mechanism.

Pretty cool to see this!

@campersau
Copy link
Collaborator

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?
Including the prettier changes in this PR makes it really hard to review.

@wmertens
Copy link
Contributor Author

@campersau I added it for now, these are the plugins babel adds for v12:

Using plugins:
  proposal-nullish-coalescing-operator { "node":"12.16.1" }
  proposal-optional-chaining { "node":"12.16.1" }
  syntax-json-strings { "node":"12.16.1" }
  syntax-optional-catch-binding { "node":"12.16.1" }
  syntax-async-generators { "node":"12.16.1" }
  syntax-object-rest-spread { "node":"12.16.1" }
  transform-modules-commonjs { "node":"12.16.1" }
  proposal-dynamic-import { "node":"12.16.1" }

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.

@wmertens wmertens force-pushed the nodegit branch 2 times, most recently from e73ccf1 to f403c58 Compare April 26, 2020 15:40
@wmertens
Copy link
Contributor Author

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.
The advantage of nodegit is that we can traverse any way we like.
I'm thinking to get the 10 latest commits of HEAD, and then some commits for each of the branches, maybe limiting to the 5 most recent branches.
Then the problem is loading more commits, but the current approach is to just load everything from the start again.

Ideally though, the frontend would be ok with commits without metadata, so the entire tree can be loaded in memory.

@wmertens
Copy link
Contributor Author

took out Babel again since my main concern was the async/await.

I'm having a failing test on the checkout:

it('should be possible to checkout with local files that will conflict', (done) => {

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.

@wmertens wmertens force-pushed the nodegit branch 2 times, most recently from 4b9fe01 to a004086 Compare April 27, 2020 16:44
@wmertens
Copy link
Contributor Author

To see the changes without the prettier commits, look at wmertens/ungit@prettier...wmertens:nodegit

@wmertens
Copy link
Contributor Author

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.

@tbranyen
Copy link

@wmertens https://libgit2.org/libgit2/#HEAD/type/git_stash_apply_options has a checkout_options option you can set to force: https://libgit2.org/libgit2/#HEAD/type/git_checkout_strategy_t

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

@wmertens
Copy link
Contributor Author

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

@wmertens
Copy link
Contributor Author

I created libgit2/libgit2#5501 since libgit2 behaves differently from git

@wmertens
Copy link
Contributor Author

wmertens commented May 4, 2020

For autostash, I'll just unstage. For the commits, I'll:

  • let the client request commits from an array of starting commits
  • the first requested commit is the main branch and will get followed for main_num commits and will get the messages and file diffs
  • the others are side branches and get followed for side_num commits and only get ts, author, subject
  • following means: get commit data for current commit, request parents.

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.

@wmertens
Copy link
Contributor Author

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.
There are some extra commits in here from my other PRs, ignore those.

@wmertens
Copy link
Contributor Author

status:

  • All the things for loading the page are handled by nodegit. Scrolling only grabs new commits instead of all commits.
  • Commits don't yet include diff stats, they should really only be loaded when necessary (when showing the commit bubble).
  • Initially, refs aren't attached to nodes, until it recalculates the graph. I need to figure out a way to trigger recalc once /refs has loaded.
  • it doesn't yet use credentialhelper for fetching

@wmertens
Copy link
Contributor Author

status: now it loads very quickly. However, there are some odd bugs:

  • the head branch commits only show the first pageload of commits with messages
  • diffs sometimes don't diff the correct file
  • if /gitlog returns before /refs, the nodes don't show

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.
Also, the commit filediff details are costly to load. Those should be loaded only when we're showing the card with their info.

Would be great to have some eyeballs on this.

@wmertens
Copy link
Contributor Author

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.

@wmertens wmertens marked this pull request as ready for review February 16, 2021 19:33
@ylecuyer
Copy link
Contributor

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

Requires git
5 participants