From 3dbdea8b8b6560d6bb43421530727835375c2aae Mon Sep 17 00:00:00 2001 From: metamindedu Date: Tue, 14 Jul 2026 23:47:33 +0900 Subject: [PATCH] fix(desktop): make shallow update status presence-only --- apps/desktop/electron/main.ts | 41 ++--- apps/desktop/electron/update-count.test.ts | 184 +++++++++++++++++---- apps/desktop/electron/update-count.ts | 40 +++-- 3 files changed, 195 insertions(+), 70 deletions(-) diff --git a/apps/desktop/electron/main.ts b/apps/desktop/electron/main.ts index db0f8f2c96..8ca4bf7977 100644 --- a/apps/desktop/electron/main.ts +++ b/apps/desktop/electron/main.ts @@ -206,7 +206,7 @@ import { } from './ssh-connection' import { createStreamThrottle } from './stream-throttle' import { nativeOverlayWidth as computeNativeOverlayWidth, macTitleBarOverlayHeight } from './titlebar-overlay-width' -import { resolveBehindCount, shouldCountCommits } from './update-count' +import { resolveBehindCount, resolveCommitLogSelection, shouldCountCommits } from './update-count' import { waitForUpdateClearance } from './update-gate' import { readLiveUpdateMarker, updateHandoffConflict, writeUpdateMarker } from './update-marker' import { isOfficialSshRemote, OFFICIAL_REPO_HTTPS_URL } from './update-remote' @@ -2603,40 +2603,40 @@ async function checkUpdates() { const git = args => runGit(args, { cwd: updateRoot }).then(r => r.stdout.trim()) - const [currentSha, targetSha, dirtyStr, currentBranch, shallowStr, mergeBaseStr] = await Promise.all([ + const [currentSha, targetSha, dirtyStr, currentBranch, shallowStr] = await Promise.all([ git(['rev-parse', 'HEAD']), git(['rev-parse', `origin/${branch}`]), git(['status', '--porcelain']), 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}`]) + git(['rev-parse', '--is-shallow-repository']) ]) 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']) - : '' + // A shallow graph cannot provide a trustworthy exact count, even when it has + // a visible merge-base. Skip the ancestry walk and use the SHA fallback. + const countStr = shouldCountCommits({ isShallow }) ? await git(['rev-list', `HEAD..origin/${branch}`, '--count']) : '' + + // A positive directional ancestry result remains trustworthy in a shallow + // graph and prevents a local commit on top of origin from looking outdated. + const targetIsAncestorOfHead = + isShallow && + currentSha !== targetSha && + (await runGit(['merge-base', '--is-ancestor', `origin/${branch}`, 'HEAD'], { cwd: updateRoot })).code === 0 const behind = resolveBehindCount({ countStr, currentSha, targetSha, isShallow, - hasMergeBase + targetIsAncestorOfHead }) // behind === null means "update available, exact count unknown" (shallow - // clone without a merge-base): still list what origin offers (the log read - // is capped at 40 entries), so "See what's new" stays useful and honest. - const commits = behind !== 0 ? await readCommitLog(updateRoot, branch) : [] + // clone): still list what origin offers — resolveCommitLogSelection keeps + // the shallow log to the fetched tip so the range walk can't enumerate the + // contaminated ancestry — so "See what's new" stays useful and honest. + const commits = behind !== 0 ? await readCommitLog(updateRoot, branch, isShallow) : [] return { supported: true, @@ -2653,12 +2653,13 @@ async function checkUpdates() { } } -async function readCommitLog(cwd, branch) { +async function readCommitLog(cwd, branch, isShallow) { const SEP = '\x1f' const REC = '\x1e' + const { limit, revision } = resolveCommitLogSelection({ branch, isShallow }) const { stdout } = await runGit( - ['log', `HEAD..origin/${branch}`, `--pretty=format:%H${SEP}%s${SEP}%an${SEP}%at${REC}`, '-n', '40'], + ['log', revision, `--pretty=format:%H${SEP}%s${SEP}%an${SEP}%at${REC}`, '-n', String(limit)], { cwd } ) diff --git a/apps/desktop/electron/update-count.test.ts b/apps/desktop/electron/update-count.test.ts index da50c2cfec..ed6041d1d4 100644 --- a/apps/desktop/electron/update-count.test.ts +++ b/apps/desktop/electron/update-count.test.ts @@ -1,21 +1,45 @@ import assert from 'node:assert/strict' +import { execFileSync } from 'node:child_process' +import fs from 'node:fs' +import os from 'node:os' +import path from 'node:path' import { test } from 'vitest' -import { resolveBehindCount, shouldCountCommits } from './update-count' +import { resolveBehindCount, resolveCommitLogSelection, shouldCountCommits } from './update-count' -// FAIL-BEFORE: the shallow/no-merge-base branch returned the sentinel `1`, -// which the UI rendered as a literal "1 change included" even when the true -// count was far higher (e.g. 90). An update IS available here, but its exact -// size is unknown — the only honest value is `null`. +function createTempGitRepo() { + const cwd = fs.mkdtempSync(path.join(os.tmpdir(), 'hermes-update-count-')) + const git = (...args: string[]) => execFileSync('git', args, { cwd, encoding: 'utf8', timeout: 10_000 }).trim() + + try { + git('init', '--quiet') + git('config', 'commit.gpgSign', 'false') + git('config', 'core.hooksPath', '.git/no-hooks') + git('config', 'user.name', 'Hermes Test') + git('config', 'user.email', 'hermes@example.invalid') + + return { cwd, git } + } catch (error) { + fs.rmSync(cwd, { recursive: true, force: true }) + throw error + } +} + +// 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) — #51922. Later the branch returned the sentinel +// `1`, which the UI rendered as a literal "1 change included" even when the +// true count was far higher (e.g. 90, or the real-world 61 in #84591). An +// update IS available here, but its exact size is unknown — the only honest +// value is `null`. test('shallow checkout with no merge-base reports null (unknown count), not a fake 1', () => { assert.equal( resolveBehindCount({ countStr: '12104', currentSha: 'aaa', targetSha: 'bbb', - isShallow: true, - hasMergeBase: false + isShallow: true }), null ) @@ -27,23 +51,114 @@ test('shallow checkout with no merge-base but identical SHA reports up-to-date', countStr: '12104', currentSha: 'abc', targetSha: 'abc', - isShallow: true, - hasMergeBase: false + isShallow: true }), 0 ) }) -test('shallow checkout WITH a merge-base keeps the exact count (reliable)', () => { +test('shallow local-ahead checkout reports up-to-date when origin is a known ancestor', () => { + assert.equal( + resolveBehindCount({ + countStr: '', + currentSha: 'local-child', + targetSha: 'origin-parent', + isShallow: true, + targetIsAncestorOfHead: true + }), + 0 + ) +}) + +test('shallow Git graph proves the remote tip is an ancestor of a local commit', () => { + const { cwd, git } = createTempGitRepo() + + try { + git('commit', '--allow-empty', '-m', 'origin tip') + + const targetSha = git('rev-parse', 'HEAD') + + git('update-ref', 'refs/remotes/origin/main', targetSha) + fs.writeFileSync(path.join(cwd, '.git', 'shallow'), `${targetSha}\n`) + git('commit', '--allow-empty', '-m', 'local child') + + const currentSha = git('rev-parse', 'HEAD') + + git('merge-base', '--is-ancestor', 'origin/main', 'HEAD') + assert.notEqual(currentSha, targetSha) + assert.equal( + resolveBehindCount({ + countStr: '', + currentSha, + targetSha, + isShallow: true, + targetIsAncestorOfHead: true + }), + 0 + ) + } finally { + fs.rmSync(cwd, { recursive: true, force: true }) + } +}, 30_000) + +test('shallow checkout with a merge-base does not trust an inflated rev-list count', () => { + const { cwd, git } = createTempGitRepo() + + try { + git('commit', '--allow-empty', '-m', 'root') + git('commit', '--allow-empty', '-m', 'ancestor') + + const redundantParent = git('rev-parse', 'HEAD') + + git('commit', '--allow-empty', '-m', 'installed head') + + const currentSha = git('rev-parse', 'HEAD') + const tree = git('rev-parse', 'HEAD^{tree}') + + const targetSha = execFileSync('git', ['commit-tree', tree, '-p', currentSha, '-p', redundantParent], { + cwd, + encoding: 'utf8', + input: 'remote merge\n', + timeout: 10_000 + }).trim() + + git('update-ref', 'refs/remotes/origin/main', targetSha) + + const completeCount = git('rev-list', 'HEAD..origin/main', '--count') + + assert.equal(completeCount, '1') + + fs.writeFileSync(path.join(cwd, '.git', 'shallow'), `${currentSha}\n`) + + assert.equal(git('rev-parse', '--is-shallow-repository'), 'true') + assert.equal(git('merge-base', 'HEAD', 'origin/main'), currentSha) + + const shallowCount = git('rev-list', 'HEAD..origin/main', '--count') + + assert.ok(Number.parseInt(shallowCount, 10) > Number.parseInt(completeCount, 10)) + assert.equal( + resolveBehindCount({ + countStr: shallowCount, + currentSha, + targetSha, + isShallow: true + }), + null + ) + } finally { + fs.rmSync(cwd, { recursive: true, force: true }) + } +}, 30_000) + +test('shallow checkout with a merge-base still uses presence-only status', () => { assert.equal( resolveBehindCount({ countStr: '3', currentSha: 'aaa', targetSha: 'bbb', - isShallow: true, - hasMergeBase: true + isShallow: true }), - 3 + null ) }) @@ -53,8 +168,7 @@ test('full (non-shallow) clone keeps the exact count path unchanged', () => { countStr: '7', currentSha: 'aaa', targetSha: 'bbb', - isShallow: false, - hasMergeBase: true + isShallow: false }), 7 ) @@ -66,8 +180,7 @@ test('up-to-date full clone reports 0', () => { countStr: '0', currentSha: 'x', targetSha: 'x', - isShallow: false, - hasMergeBase: true + isShallow: false }), 0 ) @@ -79,28 +192,35 @@ test('non-numeric count falls back to 0 (defensive, unchanged behaviour)', () => countStr: '', currentSha: 'aaa', targetSha: 'bbb', - isShallow: false, - hasMergeBase: true + isShallow: false }), 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) +// Every shallow graph is incomplete, so a visible merge-base is not enough to +// prove that the count is exact. +test('shallow checkouts skip the rev-list count', () => { + assert.equal(shouldCountCommits({ isShallow: true }), false) }) -test('shallow checkout WITH a merge-base still runs the count', () => { - assert.equal(shouldCountCommits({ isShallow: true, hasMergeBase: true }), true) +test('full (non-shallow) clones run the rev-list count', () => { + assert.equal(shouldCountCommits({ isShallow: false }), 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) +test('shallow commit logs select only the fetched remote tip', () => { + assert.deepEqual(resolveCommitLogSelection({ branch: 'main', isShallow: true }), { + limit: 1, + revision: 'origin/main' + }) +}) + +test('full-clone commit logs keep the complete behind range', () => { + assert.deepEqual(resolveCommitLogSelection({ branch: 'release', isShallow: false }), { + limit: 40, + revision: 'HEAD..origin/release' + }) }) // The skip path produces an empty countStr; resolveBehindCount must NOT trust @@ -111,8 +231,7 @@ test('skipped-count path resolves via SHA compare, never via empty countStr', () countStr: '', currentSha: 'aaa', targetSha: 'bbb', - isShallow: true, - hasMergeBase: false + isShallow: true }), null ) @@ -121,8 +240,7 @@ test('skipped-count path resolves via SHA compare, never via empty countStr', () countStr: '', currentSha: 'same', targetSha: 'same', - isShallow: true, - hasMergeBase: false + isShallow: true }), 0 ) diff --git a/apps/desktop/electron/update-count.ts b/apps/desktop/electron/update-count.ts index f5439c6405..0b5194111c 100644 --- a/apps/desktop/electron/update-count.ts +++ b/apps/desktop/electron/update-count.ts @@ -1,23 +1,20 @@ // 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) +// number worth computing. Installer checkouts are shallow (`--depth 1`), so +// their visible graph is incomplete even when `merge-base` happens to find a +// common commit. A merge can expose ancestry that the local shallow boundary +// hides from HEAD, inflating the count with old commits. Exact counts are only +// trustworthy in full clones; shallow checkouts use presence-only status plus +// any positively proven local-ahead ancestry. +function shouldCountCommits({ isShallow }) { + return !isShallow } // 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) { +// update indicator. Shallow checkouts use SHA equality plus any positively +// proven local-ahead ancestry; exact counts remain exclusive to full clones. +function resolveBehindCount({ countStr, currentSha, targetSha, isShallow, targetIsAncestorOfHead = false }) { + if (!shouldCountCommits({ isShallow })) { + if (currentSha && targetSha && (currentSha === targetSha || targetIsAncestorOfHead)) { return 0 } @@ -31,4 +28,13 @@ function resolveBehindCount({ countStr, currentSha, targetSha, isShallow, hasMer return Number.parseInt(countStr, 10) || 0 } -export { resolveBehindCount, shouldCountCommits } +// Shallow history can also contaminate the changelog range. Trust the fetched +// remote tip itself, but do not walk its ancestry. Full clones retain the +// detailed range used by the existing update overlay. +function resolveCommitLogSelection({ branch, isShallow }) { + const remote = `origin/${branch}` + + return isShallow ? { limit: 1, revision: remote } : { limit: 40, revision: `HEAD..${remote}` } +} + +export { resolveBehindCount, resolveCommitLogSelection, shouldCountCommits }