diff --git a/apps/desktop/electron/main.cjs b/apps/desktop/electron/main.cjs index 7633d71b812..ce42e3474dc 100644 --- a/apps/desktop/electron/main.cjs +++ b/apps/desktop/electron/main.cjs @@ -57,7 +57,7 @@ const { const { gitRootForIpc } = require('./git-root.cjs') const { worktreesForIpc } = require('./git-worktrees.cjs') const { OFFICIAL_REPO_HTTPS_URL, isOfficialSshRemote } = require('./update-remote.cjs') -const { resolveBehindCount } = require('./update-count.cjs') +const { resolveBehindCount, shouldCountCommits } = require('./update-count.cjs') const { runRebuildWithRetry } = require('./update-rebuild.cjs') const { buildPosixCleanupScript, @@ -1707,10 +1707,9 @@ async function checkUpdates() { } const git = args => runGit(args, { cwd: updateRoot }).then(r => r.stdout.trim()) - const [currentSha, targetSha, countStr, dirtyStr, currentBranch, shallowStr, mergeBaseStr] = await Promise.all([ + const [currentSha, targetSha, dirtyStr, currentBranch, shallowStr, mergeBaseStr] = await Promise.all([ git(['rev-parse', 'HEAD']), git(['rev-parse', `origin/${branch}`]), - git(['rev-list', `HEAD..origin/${branch}`, '--count']), git(['status', '--porcelain']), git(['rev-parse', '--abbrev-ref', 'HEAD']), git(['rev-parse', '--is-shallow-repository']), @@ -1719,12 +1718,22 @@ async function checkUpdates() { git(['merge-base', 'HEAD', `origin/${branch}`]) ]) + const isShallow = shallowStr === 'true' + const hasMergeBase = Boolean(mergeBaseStr) + // Only enumerate the commit count when it is meaningful. On a shallow checkout + // with no merge-base, `rev-list --count` walks the entire remote ancestry + // (thousands of commits, see #51922) and resolveBehindCount discards the + // result anyway in favour of a SHA compare — so skip the expensive query. + const countStr = shouldCountCommits({ isShallow, hasMergeBase }) + ? await git(['rev-list', `HEAD..origin/${branch}`, '--count']) + : '' + const behind = resolveBehindCount({ countStr, currentSha, targetSha, - isShallow: shallowStr === 'true', - hasMergeBase: Boolean(mergeBaseStr) + isShallow, + hasMergeBase }) const commits = behind > 0 ? await readCommitLog(updateRoot, branch) : [] diff --git a/apps/desktop/electron/update-count.cjs b/apps/desktop/electron/update-count.cjs index c39deecc3d0..de8d57c4ee6 100644 --- a/apps/desktop/electron/update-count.cjs +++ b/apps/desktop/electron/update-count.cjs @@ -1,20 +1,28 @@ 'use strict' +// Whether `git rev-list HEAD..origin/ --count` produces a meaningful +// number worth computing. On a SHALLOW checkout (installer clones with +// --depth 1) the local history often shares no merge-base with the freshly +// fetched origin tip, so the count enumerates the entire remote ancestry and +// returns a bogus huge number (e.g. 12104) — see #51922. resolveBehindCount +// discards that bogus count in favour of a SHA compare, so the caller should +// SKIP the expensive rev-list entirely in that case rather than run it and +// throw the result away. +function shouldCountCommits({ isShallow, hasMergeBase }) { + return !(isShallow && !hasMergeBase) +} + // Resolve how many commits the local checkout is behind origin for the desktop -// update indicator. On a SHALLOW checkout (installer clones with --depth 1) the -// local history often shares no merge-base with the freshly fetched origin tip, -// so `git rev-list HEAD..origin/ --count` enumerates the entire remote -// ancestry and returns a bogus huge number (e.g. 12104) — see #51922. In that -// case the count is meaningless; fall back to a binary up-to-date check by SHA, -// exactly like the official-SSH path in checkUpdates() and the CLI guard in -// hermes_cli/banner.py. Full clones (developers / Docker dev images) keep the -// exact count path unchanged. +// update indicator. When the count isn't meaningful (shallow + no merge-base) +// fall back to a binary up-to-date check by SHA, exactly like the official-SSH +// path in checkUpdates() and the CLI guard in hermes_cli/banner.py. Full clones +// (developers / Docker dev images) keep the exact count path unchanged. function resolveBehindCount({ countStr, currentSha, targetSha, isShallow, hasMergeBase }) { - if (isShallow && !hasMergeBase) { + if (!shouldCountCommits({ isShallow, hasMergeBase })) { if (currentSha && targetSha && currentSha === targetSha) return 0 return 1 // behind by an unknown amount — show a generic "update available" } return Number.parseInt(countStr, 10) || 0 } -module.exports = { resolveBehindCount } +module.exports = { resolveBehindCount, shouldCountCommits } diff --git a/apps/desktop/electron/update-count.test.cjs b/apps/desktop/electron/update-count.test.cjs index 08c38059f7f..69ee99aa616 100644 --- a/apps/desktop/electron/update-count.test.cjs +++ b/apps/desktop/electron/update-count.test.cjs @@ -1,7 +1,7 @@ 'use strict' const test = require('node:test') const assert = require('node:assert/strict') -const { resolveBehindCount } = require('./update-count.cjs') +const { resolveBehindCount, shouldCountCommits } = require('./update-count.cjs') // FAIL-BEFORE: pre-fix the function did `Number.parseInt(countStr) || 0` // unconditionally, so a shallow checkout with no merge-base surfaced the bogus @@ -47,3 +47,33 @@ test('non-numeric count falls back to 0 (defensive, unchanged behaviour)', () => isShallow: false, hasMergeBase: true, }), 0) }) + +// shouldCountCommits gates the expensive `rev-list --count` in checkUpdates(). +// FAIL-BEFORE: in the shallow + no-merge-base case the caller ran rev-list +// unconditionally and discarded the bogus result; this predicate lets the +// caller SKIP the whole-ancestry enumeration in exactly that case (#51922). +test('shallow checkout with no merge-base SKIPS the rev-list count', () => { + assert.equal(shouldCountCommits({ isShallow: true, hasMergeBase: false }), false) +}) + +test('shallow checkout WITH a merge-base still runs the count', () => { + assert.equal(shouldCountCommits({ isShallow: true, hasMergeBase: true }), true) +}) + +test('full (non-shallow) clone always runs the count', () => { + assert.equal(shouldCountCommits({ isShallow: false, hasMergeBase: true }), true) + assert.equal(shouldCountCommits({ isShallow: false, hasMergeBase: false }), true) +}) + +// The skip path produces an empty countStr; resolveBehindCount must NOT trust +// it and must fall through to the SHA compare (mirrors the live call site). +test('skipped-count path resolves via SHA compare, never via empty countStr', () => { + assert.equal(resolveBehindCount({ + countStr: '', currentSha: 'aaa', targetSha: 'bbb', + isShallow: true, hasMergeBase: false, + }), 1) + assert.equal(resolveBehindCount({ + countStr: '', currentSha: 'same', targetSha: 'same', + isShallow: true, hasMergeBase: false, + }), 0) +})