fix(desktop): don't trust the behind-count on any shallow checkout - #71992
Closed
AlvaroFGarcia wants to merge 1 commit into
Closed
fix(desktop): don't trust the behind-count on any shallow checkout#71992AlvaroFGarcia wants to merge 1 commit into
AlvaroFGarcia wants to merge 1 commit into
Conversation
The desktop update indicator can report a five-figure "behind" count on a public install: a real Windows client showed `v0.19.0 (+17570)` for a checkout that was 143 commits behind main. NousResearch#51922 already fixed this class for `shallow + no merge-base`, but the predicate it introduced does not cover the state an installer clone actually reaches. On that machine: rev-parse --is-shallow-repository -> true rev-list --count HEAD -> 1 (HEAD is in .git/shallow) rev-list --count origin/main -> 17571 rev-list HEAD..origin/main --count-> 17570 (= 17571 - 1) merge-base HEAD origin/main -> HEAD itself Because a `hermes update` fetch deepens `origin/main`'s ancestry while HEAD stays grafted, `merge-base` resolves to HEAD, so `hasMergeBase` is true, the `isShallow && !hasMergeBase` guard opens, and the bogus count reaches the UI. The count is bogus for the same reason in both cases: the graft boundary stops git from walking HEAD's ancestry, so `rev-list HEAD..origin/<branch>` cannot exclude local history and enumerates nearly the whole remote instead. The reported number is `count(origin/branch) - count(HEAD)`, which is an artifact of truncated history, not a measure of distance. A merge-base probe cannot separate the safe case from the broken one, so gate on shallowness alone — which is what `_check_via_local_git` in hermes_cli/banner.py and the `is_shallow` branch of `cmd_check_update` in hermes_cli/main.py already do. This aligns the desktop with the CLI instead of leaving a third variant. Full clones (developers, Docker dev images) keep the exact count path unchanged, including a full clone with genuinely unrelated history, whose local ancestry is walkable and whose count is therefore real. Dropping `hasMergeBase` also removes one `git merge-base` subprocess from every passive update check. Test: the added regression reproduces the observed shallow + self-as-merge-base state and asserts 1 rather than 17570; it fails against the previous predicate (`17570 !== 1`) and passes with this change. Verified with the desktop `electron` vitest project (727 passing) and prettier --check.
Collaborator
Related: #64469 already covers the same all-shallow presence-only count change and adds shallow local-ahead and commit-log handling. This focused repair is a narrower competing option; maintainers can choose the desired scope. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The desktop update indicator can report a five-figure "behind" count on a public install. A real Windows client showed
v0.19.0 (+17570)for a checkout that was 143 commits behindmain.#51922 already fixed this class for
shallow + no merge-base, but the predicate it introduced does not cover the state an installer clone actually reaches. Probed on the affected machine:rev-parse --is-shallow-repositorytruerev-list --count HEAD.git/shallow)rev-list --count origin/mainrev-list HEAD..origin/main --countmerge-base HEAD origin/main17571 - 1 = 17570. That identity is the tell: the count is not finding new commits, it is failing to exclude local history.Root cause
Because a
hermes updatefetch deepensorigin/main's ancestry while HEAD stays grafted,merge-base HEAD origin/mainresolves to HEAD itself. SohasMergeBaseistrue, theisShallow && !hasMergeBaseguard opens,rev-listruns, and the bogus count reaches the UI.The count is bogus for the same underlying reason in both cases: the graft boundary stops git from walking HEAD's ancestry, so
rev-list HEAD..origin/<branch>cannot subtract local history and enumerates nearly the whole remote instead. A merge-base probe cannot separate the safe case from the broken one — shallowness alone is the reliable signal.The fix
Gate on shallowness alone, which is what the Python side already does —
_check_via_local_gitinhermes_cli/banner.pyand theis_shallowbranch ofcmd_check_updateinhermes_cli/main.pyboth skip counting on any shallow repo and compare tip SHAs. This aligns the desktop with the CLI rather than leaving a third divergent variant. (That divergence is also why the affected user's CLI banner said "update available" while the desktop screamed five figures.)Full clones (developers, Docker dev images) keep the exact count path unchanged — including a full clone with genuinely unrelated history, whose local ancestry is walkable and whose count is therefore real. A test pins that case so the fix isn't over-applied.
Dropping
hasMergeBasealso removes onegit merge-basesubprocess from every passive update check.Test plan
1; against the previous predicate it fails withAssertionError: 17570 !== 1update-count.test.tspass with the changeelectronvitest project: 727 passing, no new failures (2 pre-existing env-only failures — missingsimple-git/node-pty— reproduce identically on untouchedmain)tsc --noEmit: no diagnostics on either changed fileprettier --check: cleangit fetch --unshallow, the count dropped 17570 -> 143, cross-confirmed against an independent full clone of the same repo reporting the same 143Refs #51922