Fix bogus update count in shallow checkouts

Fixes on record

Merged to main. A stamp counts the issues a fix is linked to.

• #52201 fix(desktop): don't report a bogus update count for a shallow checkout

  merged to main

  files: apps/desktop/electron/main.cjs, apps/desktop/electron/update-count.cjs, apps/desktop/electron/update-count.test.cjs, apps/desktop/package.json

  Show the diff

  • apps/desktop/electron/main.cjs

    @@ -57,6 +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, shouldCountCommits } = require('./update-count.cjs')
     const { runRebuildWithRetry } = require('./update-rebuild.cjs')
     const {
       buildPosixCleanupScript,
    @@ -1706,15 +1707,34 @@ async function checkUpdates() {
       }
     
       const git = args => runGit(args, { cwd: updateRoot }).then(r => r.stdout.trim())
    -  const [currentSha, targetSha, countStr, dirtyStr, currentBranch] = 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', '--abbrev-ref', 'HEAD']),
    +    git(['rev-parse', '--is-shallow-repository']),
    +    // merge-base exits non-zero with empty stdout when HEAD shares no common
    +    // ancestor with the freshly fetched tip — exactly the shallow-clone case.
    +    git(['merge-base', 'HEAD', `origin/${branch}`])
       ])
     
    -  const behind = Number.parseInt(countStr, 10) || 0
    +  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,
    +    hasMergeBase
    +  })
       const commits = behind > 0 ? await readCommitLog(updateRoot, branch) : []
     
       return {

  • apps/desktop/electron/update-count.cjs

    @@ -0,0 +1,28 @@
    +'use strict'
    +
    +// Whether `git rev-list HEAD..origin/<branch> --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. 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 (!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, shouldCountCommits }

  • apps/desktop/electron/update-count.test.cjs

    @@ -0,0 +1,79 @@
    +'use strict'
    +const test = require('node:test')
    +const assert = require('node:assert/strict')
    +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
    +// rev-list count (e.g. 12104). This asserts the new shallow/no-merge-base branch.
    +test('shallow checkout with no merge-base does NOT trust the bogus rev-list count', () => {
    +  assert.equal(resolveBehindCount({
    +    countStr: '12104', currentSha: 'aaa', targetSha: 'bbb',
    +    isShallow: true, hasMergeBase: false,
    +  }), 1)
    +})
    +
    +test('shallow checkout with no merge-base but identical SHA reports up-to-date', () => {
    +  assert.equal(resolveBehindCount({
    +    countStr: '12104', currentSha: 'abc', targetSha: 'abc',
    +    isShallow: true, hasMergeBase: false,
    +  }), 0)
    +})
    +
    +test('shallow checkout WITH a merge-base keeps the exact count (reliable)', () => {
    +  assert.equal(resolveBehindCount({
    +    countStr: '3', currentSha: 'aaa', targetSha: 'bbb',
    +    isShallow: true, hasMergeBase: true,
    +  }), 3)
    +})
    +
    +test('full (non-shallow) clone keeps the exact count path unchanged', () => {
    +  assert.equal(resolveBehindCount({
    +    countStr: '7', currentSha: 'aaa', targetSha: 'bbb',
    +    isShallow: false, hasMergeBase: true,
    +  }), 7)
    +})
    +
    +test('up-to-date full clone reports 0', () => {
    +  assert.equal(resolveBehindCount({
    +    countStr: '0', currentSha: 'x', targetSha: 'x',
    +    isShallow: false, hasMergeBase: true,
    +  }), 0)
    +})
    +
    +test('non-numeric count falls back to 0 (defensive, unchanged behaviour)', () => {
    +  assert.equal(resolveBehindCount({
    +    countStr: '', currentSha: 'aaa', targetSha: 'bbb',
    +    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)
    +})

  • apps/desktop/package.json

    @@ -37,7 +37,7 @@
         "test:desktop:nsis": "node scripts/test-desktop.mjs nsis",
         "test:desktop:existing": "node scripts/test-desktop.mjs existing",
         "test:desktop:fresh": "node scripts/test-desktop.mjs fresh",
    -    "test:desktop:platforms": "node --test electron/bootstrap-platform.test.cjs electron/hardening.test.cjs electron/backend-env.test.cjs electron/backend-probes.test.cjs electron/backend-ready.test.cjs electron/bootstrap-runner.test.cjs electron/connection-config.test.cjs electron/dashboard-token.test.cjs electron/gateway-ws-probe.test.cjs electron/oauth-net-request.test.cjs electron/desktop-uninstall.test.cjs electron/session-windows.test.cjs electron/link-title-window.test.cjs electron/workspace-cwd.test.cjs electron/fs-read-dir.test.cjs electron/git-root.test.cjs electron/windows-child-process.test.cjs electron/update-remote.test.cjs electron/update-rebuild.test.cjs electron/update-marker.test.cjs electron/update-relaunch.test.cjs electron/windows-user-env.test.cjs electron/window-state.test.cjs",
    +    "test:desktop:platforms": "node --test electron/bootstrap-platform.test.cjs electron/hardening.test.cjs electron/backend-env.test.cjs electron/backend-probes.test.cjs electron/backend-ready.test.cjs electron/bootstrap-runner.test.cjs electron/connection-config.test.cjs electron/dashboard-token.test.cjs electron/gateway-ws-probe.test.cjs electron/oauth-net-request.test.cjs electron/desktop-uninstall.test.cjs electron/session-windows.test.cjs electron/link-title-window.test.cjs electron/workspace-cwd.test.cjs electron/fs-read-dir.test.cjs electron/git-root.test.cjs electron/windows-child-process.test.cjs electron/update-remote.test.cjs electron/update-count.test.cjs electron/update-rebuild.test.cjs electron/update-marker.test.cjs electron/update-relaunch.test.cjs electron/windows-user-env.test.cjs electron/window-state.test.cjs",
         "typecheck": "tsc -p . --noEmit",
         "lint": "eslint src/ electron/",
         "lint:fix": "eslint src/ electron/ --fix",