Fix status view for very out-of-sync repos #257

bjackman · 2023-05-14T19:36:16Z

Fix for #255. See commit messages for some further details.

However:

It's a little annoying that this means we have to introduce the new
MagitCommitSummary type. I would have liked to just create an object
that satisfies the full Commit interface, but that interface has a parents field, which we can't easily collect without a more extensive operation 😕 .
It seems like the log-parsing logic in getCommitsAheadBehind should
live in gitTextUtils.ts, but it's tightly coupled with the exact git log command.
I'm new to TypeScript, please don't hesitate to let me know if I've
done anything stupid!

One alternative strategy that I haven't tried: Maybe instead of this MagitCommitSummary thing, we could just still use getCommit, but do it only for the commits we are actually going to display in the UI. Probably 256 getCommit calls is pretty OK, especially since it will be cached for later.

The downside of that is that then we need a way to communicate from the model code to the view code that the commit list has been truncated, this seems kinda ugly in its own right. Maybe to fix that we could just pass through the Branch to the view code and then have it do getCommit according to its own knowledge of how many commits will actually get rendered? But then I guess we'd be mixing up model and view code in an awkward way.

So yeah I dunno, let me know what you think.

bjackman · 2023-05-15T08:55:26Z

I would have liked to just create an object
that satisfies the full Commit interface, but that interface has a parents field, which we can't easily collect without a more extensive operation confused .

Oh. git log has a %P format which provides the list of parent hashes, and we only need the hash for Commit.parents. I think this PR can be implemented much more cleanly using that.

bjackman · 2023-05-16T19:48:26Z

OK yeah I tried out building the full Commit object for each commit message - see in my branch if curious. But it has a fatal flaw - for the kind of cases I was looking at, all those full commit messages add up to like half a gigabyte lol.

So yeah I think we need one of:

What I implemented already in this PR
Something similar but where we keep a unified Commit type, but perhaps where information is only retrieved on demand when the view code actually needs it (maybe this could be useful in general?)
An approach where we just have the model code only retrieve a fixed number of commits, and communicate to the view code that the list has been truncated

Each getCommit call ends up as a `git show` subprocess: https://github.com/microsoft/vscode/blob/65a3d201e6a97b56ad0980bf2078cf6381ce3e52/extensions/git/src/git.ts#L2468 In my Linux kernel repo my `master` branch goes unused for months at a time so when I check it out it's normally tens of thousands of commits behind its upstream. Spinning up that many `git show` commands kills the extension! Not sure of the exact mechanics of why it gets killed, but even if it didn't get killed, it would be very slow and power-hungry. I initially tried to fix this by avoiding the `getCommit` calls, instead fetching partial commit data directly from the log. This turned out to be a bit of a pain, it required introducing an extra type to represent that incomplete commit data. Further testing then revealed this wasn't even very fast. So, the new solution is just to have the command code limit the number of commits that we fetch to 50 for each of the ahead/behind gap.

bjackman · 2023-05-18T14:11:45Z

OK sorry for all the noise here... after further usage I discovered that my status view was nowhere near as fast as I first thought. I don't really understand how I came to believe it was rendering in under a second, that's no longer what I observed today.

So I've ended up re-writing this according to approach number 3 in the comment above. I think this actually leads to fairly simple code anyway.

kahole · 2023-05-18T14:15:59Z

Appreciate the work! I will look at this soon, when I have the time

kahole · 2023-05-19T12:18:40Z

LGTM!

kahole · 2023-05-23T11:24:25Z

This PR introduced a serious bug. The same commit appears in behind and ahead in the commit above. @bjackman , does this have a simple fix?

kahole · 2023-05-23T11:25:15Z

Reverting for now

bjackman · 2023-05-23T11:50:55Z

Yep, thanks - suspect the fix will be simple but won't get a chance to take a look for a few weeks so revert is the best option for now.

…

On Tue, May 23, 2023, 8:25 PM Kristian Andersen Hole < ***@***.***> wrote: Reverting for now — Reply to this email directly, view it on GitHub <#257 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAWNWK2AZ7K5UT3ZSCBQMPLXHSNCNANCNFSM6AAAAAAYBK7SLE> . You are receiving this because you were mentioned.Message ID: ***@***.***>

bjackman · 2023-06-28T18:50:33Z

Hey @kahole I just got back to this but I couldn't reproduce it with a straightforward attempt (based on the current tip of this PR's branch i.e. e82a4c3):

Initial repo state per git CLI:

$ git lgg -n 5
*   6aeadf7896bf - (HEAD -> test-branch, origin/master, origin/HEAD, bjackman-bpf/test-branch, master) Merge tag 'docs-arm64-move' of git://git.lwn.net/linux (14 hours ago) <Linus Torvalds>
|\  
| * f40f97aaf7fa - perf arm-spe: Fix a dangling Documentation/arm64 reference (7 days ago) <Jonathan Corbet>
| * c30034054a1a - mm: Fix a dangling Documentation/arm64 reference (7 days ago) <Jonathan Corbet>
| * 6e4596c4038a - arm64: Fix dangling references to Documentation/arm64 (7 days ago) <Jonathan Corbet>
| * a0468d4efc0e - dt-bindings: fix dangling Documentation/arm64 reference (7 days ago) <Jonathan Corbet>

And per edamagit:

Creating a local commit ahead of upstream branch shows the expected unmerged status:

$ git commit --allow-empty -m "test commit please ignore"
[test-branch ba8bfa3acc10] test commit please ignore

$ git lgg -n 5
* ba8bfa3acc10 - (HEAD -> test-branch) test commit please ignore (36 seconds ago) <Brendan Jackman>
*   6aeadf7896bf - (origin/master, origin/HEAD, bjackman-bpf/test-branch, master) Merge tag 'docs-arm64-move' of git://git.lwn.net/linux (14 hours ago) <Linus Torvalds>
|\  
| * f40f97aaf7fa - perf arm-spe: Fix a dangling Documentation/arm64 reference (7 days ago) <Jonathan Corbet>
| * c30034054a1a - mm: Fix a dangling Documentation/arm64 reference (7 days ago) <Jonathan Corbet>
| * 6e4596c4038a - arm64: Fix dangling references to Documentation/arm64 (7 days ago) <Jonathan Corbet>

Reverse situation where the upstream branch is behind:

$ git push
Enumerating objects: 1, done.
Counting objects: 100% (1/1), done.
Writing objects: 100% (1/1), 200 bytes | 200.00 KiB/s, done.
Total 1 (delta 0), reused 0 (delta 0), pack-reused 0
To github.com:bjackman/linux-bpf.git
   6aeadf7896bf..ba8bfa3acc10  test-branch -> test-branch

$ git reset --hard HEAD^
HEAD is now at 6aeadf7896bf Merge tag 'docs-arm64-move' of git://git.lwn.net/linux

$ git lgg -n 5
*   6aeadf7896bf - (HEAD -> test-branch, origin/master, origin/HEAD, master) Merge tag 'docs-arm64-move' of git://git.lwn.net/linux (14 hours ago) <Linus Torvalds>
|\  
| * f40f97aaf7fa - perf arm-spe: Fix a dangling Documentation/arm64 reference (7 days ago) <Jonathan Corbet>
| * c30034054a1a - mm: Fix a dangling Documentation/arm64 reference (7 days ago) <Jonathan Corbet>
| * 6e4596c4038a - arm64: Fix dangling references to Documentation/arm64 (7 days ago) <Jonathan Corbet>
| * a0468d4efc0e - dt-bindings: fix dangling Documentation/arm64 reference (7 days ago) <Jonathan Corbet>

I also tried it at the commit before the revert i.e. 2bca719 but got the same results as above. So maybe this isn't such a trivial bug.

Any chance you remember what state your repo was in when you took the screenshot in #257 (comment)? Would you be able to reproduce it?

kahole · 2023-06-28T19:02:33Z

Both upstream and push branches active here

kahole · 2023-06-28T19:02:54Z

Might have something to do with it

…c with upstream"" This reverts commit f7cdaee. As noted in kahole#257 (comment), that commit is broken when the current branch is out of sync with its upstream, and a push remote is configured for it. This will be fixed in the next commit.

The three-dot notation is a symmetric operation returning the difference between two commits, i.e. all that are reachable from one but not the other. This didn't turn out to matter for the usage directly in `internalMagitStatus` - we only call `getCommitRange` at most once there anyway because we first check the `.ahead`/`.behind` counters provided by the VSCode Git API. However in `pushRemoteStatus` we call it unconditionall for both "directions", which means the same set of commits show up in the returned `.head` and `.behind` fields. This leads to the bug described in kahole#257 (comment). The fix is just to use the asymmetric range operator: `..`.

bjackman · 2023-07-01T15:18:47Z

Ah nice, thanks, you were right about that. Turns out pushRemoteStatus was basically garbage here. Sending a fixed v2 now.

…c with upstream"" This reverts commit f7cdaee. As noted in kahole#257 (comment), that commit is broken when the current branch is out of sync with its upstream, and a push remote is configured for it. This will be fixed in the next commit.

The three-dot notation is a symmetric operation returning the difference between two commits, i.e. all that are reachable from one but not the other. This didn't turn out to matter for the usage directly in `internalMagitStatus` - we only call `getCommitRange` at most once there anyway because we first check the `.ahead`/`.behind` counters provided by the VSCode Git API. However in `pushRemoteStatus` we call it unconditionall for both "directions", which means the same set of commits show up in the returned `.head` and `.behind` fields. This leads to the bug described in kahole#257 (comment). The fix is just to use the asymmetric range operator: `..`.

bjackman force-pushed the optimise branch from 918a01f to 0a61fd4 Compare May 15, 2023 21:02

bjackman force-pushed the optimise branch from 0a61fd4 to 762e8c1 Compare May 18, 2023 14:09

bjackman force-pushed the optimise branch from 762e8c1 to e82a4c3 Compare May 18, 2023 14:10

kahole merged commit 0c1d951 into kahole:develop May 19, 2023

bjackman mentioned this pull request Jul 1, 2023

Fix status view for very out-of-sync repos [V2] #262

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix status view for very out-of-sync repos #257

Fix status view for very out-of-sync repos #257

bjackman commented May 14, 2023

bjackman commented May 15, 2023

bjackman commented May 16, 2023

bjackman commented May 18, 2023

kahole commented May 18, 2023

kahole commented May 19, 2023

kahole commented May 23, 2023

kahole commented May 23, 2023

bjackman commented May 23, 2023 via email

bjackman commented Jun 28, 2023

kahole commented Jun 28, 2023

kahole commented Jun 28, 2023

bjackman commented Jul 1, 2023

Fix status view for very out-of-sync repos #257

Fix status view for very out-of-sync repos #257

Conversation

bjackman commented May 14, 2023

bjackman commented May 15, 2023

bjackman commented May 16, 2023

bjackman commented May 18, 2023

kahole commented May 18, 2023

kahole commented May 19, 2023

kahole commented May 23, 2023

kahole commented May 23, 2023

bjackman commented May 23, 2023 via email

bjackman commented Jun 28, 2023

kahole commented Jun 28, 2023

kahole commented Jun 28, 2023

bjackman commented Jul 1, 2023