From bed840a7ddfbdf928d5122c5c8d92590312e66a3 Mon Sep 17 00:00:00 2001 From: Jinjing <6427696+AmethystLiang@users.noreply.github.com> Date: Tue, 25 Aug 2026 17:51:02 -0700 Subject: [PATCH] Treat sequencer progress as success when HEAD moves MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When `git rebase --continue`, `git merge --continue`, or `git cherry-pick --continue` commit a resolution and encounter the next conflict, Git exits with an error despite having made progress. Read HEAD before and after to detect progress — if it moved, treat the nonzero exit as success. Also shorten bare commit oids in rebase banner to 8 chars. --- src/main/git/sequencer-actions.test.ts | 57 ++++++++++++++++++- src/main/git/sequencer-actions.ts | 40 ++++++++++--- .../listing/conflict-status-cards.tsx | 7 ++- .../listing/operation-banner.test.tsx | 10 ++++ 4 files changed, 103 insertions(+), 11 deletions(-) diff --git a/src/main/git/sequencer-actions.test.ts b/src/main/git/sequencer-actions.test.ts index 6b0a9f43c90..ccdda3ebe81 100644 --- a/src/main/git/sequencer-actions.test.ts +++ b/src/main/git/sequencer-actions.test.ts @@ -27,10 +27,25 @@ const CASES: readonly [string, SequencerAction, string[]][] = [ ['continueCherryPick', continueCherryPick, ['cherry-pick', '--continue']] ] +// The HEAD probe runs before the sequencer step, so calls are matched by argv, not index. +function optionsFor(args: readonly string[]): { env?: NodeJS.ProcessEnv } | undefined { + const call = gitExecFileAsyncMock.mock.calls.find( + (called: unknown[]) => (called[0] as string[]).join(' ') === args.join(' ') + ) + return call?.[1] as { env?: NodeJS.ProcessEnv } | undefined +} + +function headProbe(oid: string) { + return (args: readonly string[]) => + args[0] === 'rev-parse' + ? Promise.resolve({ stdout: `${oid}\n`, stderr: '' }) + : Promise.resolve({ stdout: '', stderr: '' }) +} + describe('git sequencer actions', () => { beforeEach(() => { gitExecFileAsyncMock.mockReset() - gitExecFileAsyncMock.mockResolvedValue({ stdout: '', stderr: '' }) + gitExecFileAsyncMock.mockImplementation(headProbe('abc123')) }) it.each(CASES)('%s runs the matching git command in the worktree', async (_name, run, args) => { @@ -43,10 +58,10 @@ describe('git sequencer actions', () => { }) // Regression guard: without GIT_EDITOR the `--continue` child waits forever on the commit editor. - it.each(CASES)('%s suppresses the commit-message editor', async (_name, run) => { + it.each(CASES)('%s suppresses the commit-message editor', async (_name, run, args) => { await run('/repo') - expect(gitExecFileAsyncMock.mock.calls[0][1].env.GIT_EDITOR).toBe('true') + expect(optionsFor(args)?.env?.GIT_EDITOR).toBe('true') }) it('forwards runtime options such as the WSL distro', async () => { @@ -57,4 +72,40 @@ describe('git sequencer actions', () => { expect.objectContaining({ cwd: '/repo', wslDistro: 'Ubuntu' }) ) }) + + // `git rebase --continue` exits nonzero when it lands the resolution and then stops on the + // NEXT commit's conflict. HEAD moved, so that is the sequencer advancing, not a failed step. + it('treats a stop on the next commit as progress once HEAD has moved', async () => { + let head = 'aaa111' + gitExecFileAsyncMock.mockImplementation((args: string[]) => { + if (args[0] === 'rev-parse') { + return Promise.resolve({ stdout: `${head}\n`, stderr: '' }) + } + head = 'bbb222' + return Promise.reject(new Error('error: could not apply ec9b3362... feat: add thing')) + }) + + await expect(continueRebase('/repo')).resolves.toBeUndefined() + }) + + it('still fails a step that refused to run, leaving HEAD where it was', async () => { + gitExecFileAsyncMock.mockImplementation((args: string[]) => + args[0] === 'rev-parse' + ? Promise.resolve({ stdout: 'aaa111\n', stderr: '' }) + : Promise.reject(new Error('f.txt: needs merge')) + ) + + await expect(continueRebase('/repo')).rejects.toThrow('needs merge') + }) + + // An unborn HEAD (or an unreadable one) proves nothing, so the original failure stands. + it('rethrows when HEAD cannot be read', async () => { + gitExecFileAsyncMock.mockImplementation((args: string[]) => + args[0] === 'rev-parse' + ? Promise.reject(new Error('fatal: bad revision')) + : Promise.reject(new Error('cherry-pick failed')) + ) + + await expect(continueCherryPick('/repo')).rejects.toThrow('cherry-pick failed') + }) }) diff --git a/src/main/git/sequencer-actions.ts b/src/main/git/sequencer-actions.ts index 02751a1654a..46d4d00a0d9 100644 --- a/src/main/git/sequencer-actions.ts +++ b/src/main/git/sequencer-actions.ts @@ -4,6 +4,21 @@ import { gitOptionsForWorktree } from './git-runtime-options' import { gitExecFileAsync } from './runner' import { runWithGitReadCacheInvalidation } from './status' +async function readHeadOid( + worktreePath: string, + options: GitRuntimeOptions +): Promise { + try { + const { stdout } = await gitExecFileAsync( + ['rev-parse', '--verify', 'HEAD'], + gitOptionsForWorktree(worktreePath, options) + ) + return stdout.trim() || null + } catch { + return null + } +} + // Why: every subcommand here predates the Git 2.25 baseline (`merge --continue` 2.12, // the rest older), so no capability probe or fallback is needed. async function runSequencerAction( @@ -11,13 +26,24 @@ async function runSequencerAction( worktreePath: string, options: GitRuntimeOptions ): Promise { - await runWithGitReadCacheInvalidation(() => - gitExecFileAsync([...args], { - ...gitOptionsForWorktree(worktreePath, options), - // Why: `--continue` opens the commit-message editor and would hang with no terminal to close it. - env: editorSuppressedGitEnv() - }) - ) + const headBefore = await readHeadOid(worktreePath, options) + try { + await runWithGitReadCacheInvalidation(() => + gitExecFileAsync([...args], { + ...gitOptionsForWorktree(worktreePath, options), + // Why: `--continue` opens the commit-message editor and would hang with no terminal to close it. + env: editorSuppressedGitEnv() + }) + ) + } catch (error) { + // Why: `--continue` also exits nonzero when it DID commit the resolution and the + // sequencer then stopped on the next commit. A moved HEAD is the proof it advanced; + // only an unmoved one means the step refused and nothing happened. + const headAfter = await readHeadOid(worktreePath, options) + if (!headBefore || !headAfter || headAfter === headBefore) { + throw error + } + } } export async function continueMerge( diff --git a/src/renderer/src/components/right-sidebar/source-control/listing/conflict-status-cards.tsx b/src/renderer/src/components/right-sidebar/source-control/listing/conflict-status-cards.tsx index 3ae05496644..fc9f137588f 100644 --- a/src/renderer/src/components/right-sidebar/source-control/listing/conflict-status-cards.tsx +++ b/src/renderer/src/components/right-sidebar/source-control/listing/conflict-status-cards.tsx @@ -48,6 +48,11 @@ function conflictsHeading(conflictOperation: GitConflictOperation): string { ) } +// Full 40-char oids only get truncated by CSS, which reads as a broken value. +function shortenOnto(onto: string | undefined): string | undefined { + return onto && /^[0-9a-f]{40}$/i.test(onto) ? onto.slice(0, 8) : onto +} + function inProgressHeading(conflictOperation: GitConflictOperation): string { if (conflictOperation === 'merge') { return translate( @@ -155,7 +160,7 @@ export function OperationBannerBody({ onResolveWithAI?: () => void }): React.JSX.Element { const Icon = conflictOperation === 'rebase' ? GitPullRequestArrow : GitMerge - const onto = operationProgress?.onto?.trim() + const onto = shortenOnto(operationProgress?.onto?.trim()) const heading = conflictOperation === 'rebase' && onto ? translate( diff --git a/src/renderer/src/components/right-sidebar/source-control/listing/operation-banner.test.tsx b/src/renderer/src/components/right-sidebar/source-control/listing/operation-banner.test.tsx index 50e92b3244c..0468aec261a 100644 --- a/src/renderer/src/components/right-sidebar/source-control/listing/operation-banner.test.tsx +++ b/src/renderer/src/components/right-sidebar/source-control/listing/operation-banner.test.tsx @@ -42,6 +42,16 @@ describe('OperationBanner heading', () => { expect(render()).toContain('Rebasing onto origin/main') }) + // The banner is narrow; a full oid only gets CSS-truncated, which reads as broken. + it('shortens a bare oid it is replaying onto', () => { + const markup = render({ + operationProgress: { ...progress, onto: 'bc98655a3965fe350f77acb14bf4a53f1e2d3c4b' } + }) + + expect(markup).toContain('Rebasing onto bc98655a') + expect(markup).not.toContain('bc98655a3965fe350f77acb14bf4a53f1e2d3c4b') + }) + // Wire compatibility: a host that predates operationProgress omits it entirely. it('degrades to the plain in-progress banner when the host reported no progress', () => { const markup = render({ operationProgress: null })