From cb6edbf448e7878fd25bf6db20df4c18ed8755a7 Mon Sep 17 00:00:00 2001 From: briandevans <252620095+briandevans@users.noreply.github.com> Date: Wed, 24 Jun 2026 12:27:54 -0700 Subject: [PATCH] fix(desktop): skip the rev-list count when it is discarded anyway checkUpdates() ran `git rev-list HEAD..origin/ --count` unconditionally in the parallel probe batch, even on the shallow + no-merge-base path where resolveBehindCount() ignores the result and falls back to a SHA compare. In the #51922 failure mode that count walks the entire remote ancestry (thousands of commits), so the work was pure latency on every update check for the exact case the fix targets. Split the probes into two phases: resolve --is-shallow-repository and merge-base first, then run rev-list --count only when shouldCountCommits says the number is meaningful (full clone, or shallow-with-merge-base). The shallow/no-merge-base SHA fallback is preserved unchanged. --- apps/desktop/electron/main.cjs | 19 ++++++++---- apps/desktop/electron/update-count.cjs | 28 +++++++++++------- apps/desktop/electron/update-count.test.cjs | 32 ++++++++++++++++++++- 3 files changed, 63 insertions(+), 16 deletions(-) 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) +})