Use nodegit

[wmertens]

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

[tbranyen]

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]

FYI: we removed babel in https://github.com/FredrikNoren/ungit/pull/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 https://github.com/FredrikNoren/ungit/issues/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]

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

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]

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

I'm having a failing test on the checkout: https://github.com/FredrikNoren/ungit/blob/6009c23e5e9cce78416f9d9ee55fcebdbb8a3920/test/spec.git-api.conflict.js#L107

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]

To see the changes without the prettier commits, look at https://github.com/wmertens/ungit/compare/prettier...wmertens:nodegit

[wmertens]

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]

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

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

I created https://github.com/libgit2/libgit2/issues/5501 since libgit2 behaves differently from git

[wmertens]

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]

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]

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]

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]

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.

[ylecuyer]

Nice :+1: I'll give it a try

[ylecuyer]

I like the branch coloring, you should definitely extract it from this branch and add it to master with a toggle on/off flag in the settings ;)

It worked with ungit repo though I can't say which commands were run by nodegit and which are still executed with git.

But it crashed after loading commits on the main project I'm working on

[wmertens]

@ylecuyer I fixed a bunch of crashes and improved the layout.

The output shows all git commands, the rest is done by nodegit. Almost all querying is done by nodegit and the actions are mostly git.

Feel free to lift the branch coloring and make a PR, I'm concentrating on this one. I think all you need is https://github.com/FredrikNoren/ungit/pull/1315/commits/ce43fd6de8a8920adc94eed48811b449450ea2b5

[wmertens]

The latest commit implemented a very nice layout algorithm. Unfortunately knockout is pretty slow on larger graphs, not sure if that can be improved.

[jung-kim]

This is cool, my knowledge of code base has gone bye bye long time ago and I don't know if I can do a solid code review anymore but this is an exiting change.

Do you think this branch is at stable enough place to be a standalone? What I mean is that maybe we should keep this branch and PR open and begin opening other issues and PRs against this PR. Even if it's not perfect, maybe we can do some small incremental works.

[wmertens]

@jung-kim well, I use it every day :) it has a few small bugs but they are solved by reloading (e.g. simple things like when switching branches it doesn't repaint the graph). I haven't yet lost code with it 😅

It would be really nice to have more eyeballs on it. I tried making it into a nodegit-only branch and a graph branch but a lot of the commit fetching is connected to graph changes.

[wmertens]

The sad thing is that I started this so that I would be able to move commits up and down and do fixups and splits without doing an interactive rebase, but I still haven't gotten to that part :)

[jung-kim]

Hmm I will try to begin using it too. I will open a PR against this branch when I see something

[wmertens]

@jung-kim thanks!

Some bugs/todos:

• diffs - nodegit/libgit isn't very well documented and sometimes I see weird things in the file list. For example, on one repo the diff includes the same file deletion multiple times.
• the caching might be too strong, during a rebase the diff often doesn't show a conflict after the previous step (even though it paused) or it keeps showing a conflict even though I resolved it in the editor (so it missed the change). Note that the watcher library now has better recursive watching, we should update that.
• rebase still shows git errors for a commit that has conflicts. Should be done with nodegit instead.
• the graph loading should load as soon as the user scrolls near the first unloaded commit, not wait until the end of the page

Etc :)

[jung-kim]

Hmmm... Having some trouble installing [email protected]. Error is filed at nodegit/nodegit#1620. Considering it's node-gyp related issue, I'm sure this won't be an easy fix.

My python is 2.7 and don't even have python 3.0. I've tried node@17, node@16, node@14 and even ancient node@8 as recommended by the ticket but all had node-gyp issue for me.

I may dive into this more but even if we get a combination working, broader compatibility maybe a concern for this effort and something we would have to note.

[wmertens]

@jung-kim did you get it working yet?

I'm now struggling to figure out why Knockout isn't displaying new branch names. When I add a branch it shows a branch tag without a name and in the devtools everything looks fine I think.

[wmertens]

ah - nevermind, I figured it out - displayHtml was assigned in constructor but this.node() was called before that and it caused a redraw first and not any more after

[wmertens]

@jung-kim latest changes add a lot of stability and speed, if you'd like to test again

[wmertens]

if someone wants to help out, a fun change would be to figure out the scroll position and load new nodes as soon as a not-yet-loaded commit scrolls into view. Right now it does it when it hits the screen bottom but there's usually unloaded nodes much higher

[jung-kim]

@wmertens That's fantastic! Will give another go at it soon.

Just as FYI, I have been focussing on fixing and stabilizing the tests so we can safely develop but am running into some difficulties. Also, considering some changes I had to make to the code to stabilize the tests, I think there will be conflict with my branch and yours. (i.e. one of the issues is that on the test side we never know when the events such as working-tree-changed is completed processing, and I've changed to async await for all onProgramEvent() functions that will conflict with few places.)

But if and when that PR is ready, I will consult with you before I merge anything to make sure I don't pull entire rug underneath your feet.