fix(cli): stop using --depth 1 for update-check fetches on shallow installer checkouts - #84597
Closed
ygd58 wants to merge 1 commit into
Closed
fix(cli): stop using --depth 1 for update-check fetches on shallow installer checkouts#84597ygd58 wants to merge 1 commit into
ygd58 wants to merge 1 commit into
Conversation
…staller checkouts Fixes NousResearch#84591. hermes_cli/update_cmd.py's `hermes update --check` and hermes_cli/banner.py's CLI-startup update poller both fetched with `--depth 1` when the repo was already shallow (installer checkouts, `git clone --depth 1`), on the theory that a plain fetch would unshallow the repo and drag in the whole history, making the rev-list count bogus (huge number). Verified against real git behavior (not just reasoning about it) that this theory doesn't hold for the common case: a plain, branch-scoped fetch on an ALREADY-shallow repo extends the existing shallow boundary forward incrementally. Git's fetch negotiation reuses the current boundary as a "have" reference, so only the new commits transfer -- .git doesn't grow, and merge-base/rev-list stay exact. `--depth 1`, in contrast, unconditionally marks the freshly fetched tip as a NEW shallow boundary even when its parent is already local -- permanently breaking `git merge-base HEAD origin/main` from that fetch onward. Not self-healing: every later plain fetch's new tip connects to the now- orphaned boundary, so the indicator sticks at the placeholder "1 commit behind" forever, no matter how far `main` actually advances (real-world report: showed "1" while 61 commits behind). Root cause per the issue: `eecb5b9dd1` (NousResearch#50784) added the --depth-1 check-fetch to avoid a different failure mode (reporting a bogus huge "behind" count on a repo that had never been fetched incrementally at all). That fix's own mechanism is what created this one. Fix: removed `--depth 1` from both check-path fetches. Also changed the post-fetch logic in both files to ATTEMPT the exact rev-list/merge-base count first -- empirically verified this succeeds on a shallow repo as long as the boundary hasn't been poisoned -- falling back to the old presence-only placeholder only if it genuinely can't connect (e.g. a rewritten upstream history orphaning the boundary), rather than always skipping the exact count whenever `is-shallow-repository` reports true. apps/desktop/electron/main.ts's own poller fetch already had no --depth flag (confirmed by reading it) -- it was already correct, just a victim of the CLI-side poisoning; no change needed there. update-count.ts's resolveBehindCount() is a pure function consuming isShallow/hasMergeBase flags computed elsewhere, so it benefits from this fix without any changes of its own. Verification used REAL git repositories (not mocked subprocess calls) -- the bug is in git's own shallow-fetch semantics, which a mock can't meaningfully exercise. Added 4 tests: a sanity check on the shallow-clone fixture; a positive test proving a plain fetch preserves exact merge-base/rev-list; a negative-control test reproducing the --depth 1 poisoning and its non-self-healing behavior (as documented proof of the bug this fix removes); and an end-to-end test of banner.py's _check_via_local_git() against a real shallow checkout, asserting it now returns the exact behind-count (4) instead of the presence-only placeholder (1). Verified as a genuine regression by reverting just the --depth-1 removal in banner.py and confirming the end-to-end test fails with exactly "1" -- the exact reported symptom. 15/15 pass across the new test file plus the four directly related banner test files; 22/22 in the existing test_cmd_update.py file (no regression).
Contributor
|
Thanks @ygd58 — the empirical verification that a branch-scoped fetch preserves the shallow boundary was genuinely useful (it also informed closing #85179). Main took a different direction for the count problem, though: shallow checkouts keep |
alt-glitch
added
type/bug
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
3 participants
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.
Fixes #84591.
Root cause
hermes update --checkand the CLI-startup update poller both fetched with--depth 1when the repo was already shallow, on the theory that a plain fetch would unshallow the repo and drag in the whole history. Verified against real git behavior (not just reasoning) that this doesn't hold: a plain, branch-scoped fetch on an already-shallow repo extends the existing shallow boundary forward incrementally -- only new commits transfer,.gitdoesn't grow, and merge-base/rev-list stay exact.--depth 1, in contrast, unconditionally marks the freshly fetched tip as a NEW boundary even when its parent is already local, permanently breakingmerge-basefrom that fetch onward -- not self-healing, since every later plain fetch's new tip connects to the now-orphaned boundary.Fix
Removed
--depth 1from both check-path fetches (update_cmd.py,banner.py). Changed the post-fetch logic to attempt the exact rev-list/merge-base count first, falling back to the old presence-only placeholder only if it genuinely can't connect.The desktop poller's own fetch already had no
--depthflag (confirmed by reading it) -- already correct, just a victim of the CLI-side poisoning. No TypeScript changes needed.Verification
Used real git repositories (not mocked subprocess calls) -- the bug is in git's own shallow-fetch semantics, which a mock can't meaningfully exercise. Added 4 tests, including a negative-control test reproducing the exact
--depth 1poisoning and its non-self-healing behavior, and an end-to-end test provingbanner.py's check now returns the exact count instead of the placeholder. Verified as a genuine regression by reverting just the--depth-1removal and confirming the test fails with exactly "1" -- the exact reported symptom.15/15 pass across the new test file plus four related banner test files; 22/22 in the existing
test_cmd_update.py(no regression).