Skip to content

Commit 02ae851

Browse files
author
Test
committed
fix(pr-clone): use real cherry-pick conflict state instead of workdir-dirtiness heuristic
isWorkdirHasChanges() can't distinguish "conflict still unresolved" from "conflict legitimately resolved to no diff against HEAD" — both report zero dirty files, so the old heuristic silently skipped (dropping) a commit the user had actually just resolved. Add GitExecutor.getUnresolvedConflicts() (git status --porcelain, filtered to UU/AA/DD/etc unmerged status codes) and commitAllowEmpty() (git commit --allow-empty --no-edit). cherryPickNext(true) now: - reports unresolved conflicts by filename and refuses to proceed - continues the cherry-pick when the result is non-empty - prompts (Skip / Keep as empty commit / Abort clone) when the result is empty, instead of silently skipping Closes #207
1 parent fd3d513 commit 02ae851

3 files changed

Lines changed: 284 additions & 9 deletions

File tree

src/common/git/gitExecutor.ts

Lines changed: 43 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -142,24 +142,36 @@ export function parseStashNameStatusOutput(
142142
}
143143

144144
/**
145-
* Parses `git status --porcelain` (v1) output into the changed/untracked paths it reports.
146-
* Each line is `XY <path>`; rename and copy entries are `XY <old> -> <new>`, and we report the
147-
* destination. Paths containing special characters arrive C-quoted (`"src/\303\251.ts"`) — the
148-
* surrounding quotes and the escapes git adds for `"` and `\` are undone so the path reads
149-
* naturally in UI.
145+
* Parses `git status --porcelain` (v1) output into `{ status, path }` entries. Each line is
146+
* `XY <path>`; rename and copy entries are `XY <old> -> <new>`, and we report the destination.
147+
* Paths containing special characters arrive C-quoted (`"src/\303\251.ts"`) — the surrounding
148+
* quotes and the escapes git adds for `"` and `\` are undone so the path reads naturally in UI.
150149
*/
151-
export function parseStatusPorcelainPaths(output: string): string[] {
150+
export function parseStatusPorcelainEntries(output: string): Array<{ status: string; path: string }> {
152151
return output
153152
.split('\n')
154153
.filter((line) => line.trim().length > 0)
155154
.map((line) => {
155+
const status = line.slice(0, 2);
156156
const pathPart = line.slice(3).trimEnd();
157157
const arrowIndex = pathPart.lastIndexOf(' -> ');
158-
return unquoteStatusPath(arrowIndex === -1 ? pathPart : pathPart.slice(arrowIndex + 4));
158+
const path = unquoteStatusPath(arrowIndex === -1 ? pathPart : pathPart.slice(arrowIndex + 4));
159+
return { status, path };
159160
})
160-
.filter((path) => path.length > 0);
161+
.filter((entry) => entry.path.length > 0);
162+
}
163+
164+
/** Paths of the changed/untracked files reported by `git status --porcelain`. */
165+
export function parseStatusPorcelainPaths(output: string): string[] {
166+
return parseStatusPorcelainEntries(output).map((entry) => entry.path);
161167
}
162168

169+
/**
170+
* `git status --porcelain` XY status codes that indicate an unmerged/conflicted path: either
171+
* side is `U`, or both sides agree on an add/add or delete/delete conflict.
172+
*/
173+
const UNMERGED_STATUS_CODES = new Set(['UU', 'AA', 'DD', 'AU', 'UA', 'UD', 'DU']);
174+
163175
function unquoteStatusPath(path: string): string {
164176
if (!path.startsWith('"') || !path.endsWith('"') || path.length < 2) {
165177
return path;
@@ -976,6 +988,29 @@ export class GitExecutor {
976988
return conflictedFiles.length > 0;
977989
}
978990

991+
/**
992+
* Paths of files still unmerged per `git status --porcelain`'s XY status code (`UU`, `AA`,
993+
* `DD`, `AU`, `UA`, `UD`, `DU` — either side `U`, or both `A`/both `D`). Used to distinguish a
994+
* real unresolved conflict from a conflict the user resolved down to an empty diff, which
995+
* `isWorkdirHasChanges()` alone cannot tell apart (see issue #207).
996+
*/
997+
async getUnresolvedConflicts(): Promise<string[]> {
998+
const { stdout } = await this.#execGitCommand(['status', '--porcelain']);
999+
return parseStatusPorcelainEntries(stdout)
1000+
.filter(({ status }) => UNMERGED_STATUS_CODES.has(status))
1001+
.map(({ path }) => path);
1002+
}
1003+
1004+
/**
1005+
* Finishes an in-progress cherry-pick (`CHERRY_PICK_HEAD` set) whose conflict resolution
1006+
* collapsed to no diff against HEAD, by committing an intentionally empty commit with the
1007+
* original commit message. Equivalent to `cherry-pick --continue` for that case, which would
1008+
* otherwise fail with "previous cherry-pick is now empty".
1009+
*/
1010+
async commitAllowEmpty(): Promise<void> {
1011+
await this.#execGitCommand(['commit', '--allow-empty', '--no-edit']);
1012+
}
1013+
9791014
async cherryPick(
9801015
commitSha: string | string[],
9811016
parseError = false,

src/services/prCloneInPlaceService.ts

Lines changed: 29 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -82,10 +82,38 @@ export class PrCloneInPlaceService extends PrCloneServiceBase {
8282
}
8383

8484
if (isContinue) {
85+
const unresolvedConflicts = await this.git.getUnresolvedConflicts();
86+
if (unresolvedConflicts.length > 0) {
87+
await window.showWarningMessage(
88+
`Cannot continue: the following files still have unresolved conflicts:\n${unresolvedConflicts.join('\n')}`,
89+
{ modal: true }
90+
);
91+
return;
92+
}
93+
8594
if (await this.git.isWorkdirHasChanges()) {
8695
await this.git.cherryPickContinue();
8796
} else {
88-
await this.git.cherryPickSkip();
97+
// The conflict resolution collapsed to no diff against HEAD — cherry-pick --continue
98+
// would fail ("previous cherry-pick is now empty"). Never decide silently: ask the user.
99+
const skipOption = 'Skip this commit';
100+
const emptyOption = 'Keep as empty commit';
101+
const abortOption = 'Abort clone';
102+
const choice = await window.showQuickPick([skipOption, emptyOption, abortOption], {
103+
placeHolder:
104+
'Resolving the conflict produced no changes. What should happen to this commit?',
105+
ignoreFocusOut: true,
106+
});
107+
108+
if (choice === emptyOption) {
109+
await this.git.commitAllowEmpty();
110+
} else if (choice === skipOption) {
111+
await this.git.cherryPickSkip();
112+
} else {
113+
// 'Abort clone' or dismissed (Escape) — never silently skip or continue.
114+
await this.abortClonePR();
115+
return;
116+
}
89117
}
90118
}
91119

Lines changed: 212 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,212 @@
1+
import * as assert from 'assert';
2+
import { window as vscodeWindow } from 'vscode';
3+
4+
import { GitExecutor } from '../../common/git/gitExecutor';
5+
import { PrCloneInPlaceService } from '../../services/prCloneInPlaceService';
6+
import { mockLogService } from '../e2e/helpers/mockLogService';
7+
8+
/**
9+
* Regression tests for issue #207: "Cherry-pick continue-vs-skip decided by workdir
10+
* dirtiness — silently drops commits". `cherryPickNext(true)` must decide continue vs.
11+
* skip vs. prompt from real cherry-pick state (`getUnresolvedConflicts()` +
12+
* `isWorkdirHasChanges()`), never silently skip a legitimately-empty resolution.
13+
*/
14+
describe('PrCloneInPlaceService.cherryPickNext(true) conflict-state decision', () => {
15+
interface GitCalls {
16+
getUnresolvedConflicts: number;
17+
isWorkdirHasChanges: number;
18+
cherryPickContinue: number;
19+
cherryPickSkip: number;
20+
commitAllowEmpty: number;
21+
}
22+
23+
const createGitStub = (opts: { unresolvedConflicts?: string[]; workdirHasChanges?: boolean }) => {
24+
const calls: GitCalls = {
25+
getUnresolvedConflicts: 0,
26+
isWorkdirHasChanges: 0,
27+
cherryPickContinue: 0,
28+
cherryPickSkip: 0,
29+
commitAllowEmpty: 0,
30+
};
31+
32+
const gitStub = {
33+
hasConflicts: async () => false,
34+
getUnresolvedConflicts: async () => {
35+
calls.getUnresolvedConflicts += 1;
36+
return opts.unresolvedConflicts ?? [];
37+
},
38+
isWorkdirHasChanges: async () => {
39+
calls.isWorkdirHasChanges += 1;
40+
return opts.workdirHasChanges ?? false;
41+
},
42+
cherryPickContinue: async () => {
43+
calls.cherryPickContinue += 1;
44+
},
45+
cherryPickSkip: async () => {
46+
calls.cherryPickSkip += 1;
47+
},
48+
commitAllowEmpty: async () => {
49+
calls.commitAllowEmpty += 1;
50+
},
51+
// Reached only after the decision under test; make it fail predictably so the
52+
// remainder of cherryPickNext's "commit landed" flow short-circuits via the
53+
// existing try/catch, without needing to mock the full push/PR-creation chain.
54+
pushBranchToGitHub: async () => {
55+
throw new Error('stop-after-decision (expected in this test)');
56+
},
57+
} as unknown as GitExecutor;
58+
59+
return { gitStub, calls };
60+
};
61+
62+
// A fake commit generator that is immediately "done" — cherryPickNext's post-decision
63+
// code path (push/PR creation) is not what these tests exercise; see pushBranchToGitHub
64+
// stub above for how that path is short-circuited.
65+
const fakeDoneGenerator = () =>
66+
(async function* () {
67+
// no commits to yield
68+
})();
69+
70+
const createService = (gitStub: GitExecutor) => {
71+
const service = new PrCloneInPlaceService(gitStub, {} as any, mockLogService);
72+
(service as any).commitGenerator = fakeDoneGenerator();
73+
return service;
74+
};
75+
76+
let originalShowWarningMessage: typeof vscodeWindow.showWarningMessage;
77+
let originalShowQuickPick: typeof vscodeWindow.showQuickPick;
78+
let originalShowErrorMessage: typeof vscodeWindow.showErrorMessage;
79+
80+
const stubWindow = () => {
81+
const warnings: string[] = [];
82+
let quickPickResponse: string | undefined;
83+
84+
originalShowWarningMessage = vscodeWindow.showWarningMessage;
85+
originalShowQuickPick = vscodeWindow.showQuickPick;
86+
originalShowErrorMessage = vscodeWindow.showErrorMessage;
87+
88+
(vscodeWindow as any).showWarningMessage = async (message: string) => {
89+
warnings.push(message);
90+
return undefined;
91+
};
92+
let quickPickCallCount = 0;
93+
(vscodeWindow as any).showQuickPick = async () => {
94+
quickPickCallCount += 1;
95+
return quickPickResponse;
96+
};
97+
(vscodeWindow as any).showErrorMessage = async () => undefined;
98+
99+
return {
100+
warnings,
101+
setQuickPickResponse: (value: string | undefined) => {
102+
quickPickResponse = value;
103+
},
104+
getQuickPickCallCount: () => quickPickCallCount,
105+
};
106+
};
107+
108+
const restoreWindow = () => {
109+
(vscodeWindow as any).showWarningMessage = originalShowWarningMessage;
110+
(vscodeWindow as any).showQuickPick = originalShowQuickPick;
111+
(vscodeWindow as any).showErrorMessage = originalShowErrorMessage;
112+
};
113+
114+
it('unresolved conflicts remain: warns with the file list, never continues or skips', async () => {
115+
const { gitStub, calls } = createGitStub({ unresolvedConflicts: ['src/foo.ts', 'src/bar.ts'] });
116+
const service = createService(gitStub);
117+
const { warnings } = stubWindow();
118+
119+
try {
120+
await service.cherryPickNext(true);
121+
} finally {
122+
restoreWindow();
123+
}
124+
125+
assert.strictEqual(calls.getUnresolvedConflicts, 1);
126+
assert.strictEqual(calls.cherryPickContinue, 0, 'must not continue with unresolved conflicts');
127+
assert.strictEqual(calls.cherryPickSkip, 0, 'must not skip with unresolved conflicts');
128+
assert.strictEqual(calls.commitAllowEmpty, 0);
129+
assert.strictEqual(calls.isWorkdirHasChanges, 0, 'must not even consult workdir dirtiness');
130+
assert.strictEqual(warnings.length, 1);
131+
assert.match(warnings[0], /src\/foo\.ts/);
132+
assert.match(warnings[0], /src\/bar\.ts/);
133+
});
134+
135+
it('resolved, non-empty result: continues the cherry-pick, no prompt shown', async () => {
136+
const { gitStub, calls } = createGitStub({ unresolvedConflicts: [], workdirHasChanges: true });
137+
const service = createService(gitStub);
138+
const { getQuickPickCallCount } = stubWindow();
139+
140+
try {
141+
await service.cherryPickNext(true);
142+
} catch {
143+
// expected: the post-decision push/PR flow short-circuits via the stubbed
144+
// pushBranchToGitHub rejection — irrelevant to this test.
145+
} finally {
146+
restoreWindow();
147+
}
148+
149+
assert.strictEqual(calls.cherryPickContinue, 1, 'must continue when the result is non-empty');
150+
assert.strictEqual(calls.cherryPickSkip, 0);
151+
assert.strictEqual(calls.commitAllowEmpty, 0);
152+
assert.strictEqual(getQuickPickCallCount(), 0, 'must not prompt when the result is non-empty');
153+
});
154+
155+
describe('resolved, empty result (the issue #207 failure scenario)', () => {
156+
it('never silently skips — prompts, and "Keep as empty commit" commits an empty commit', async () => {
157+
const { gitStub, calls } = createGitStub({ unresolvedConflicts: [], workdirHasChanges: false });
158+
const service = createService(gitStub);
159+
const { setQuickPickResponse, getQuickPickCallCount } = stubWindow();
160+
setQuickPickResponse('Keep as empty commit');
161+
162+
try {
163+
await service.cherryPickNext(true);
164+
} catch {
165+
// expected short-circuit past the decision under test; see stub comment above.
166+
} finally {
167+
restoreWindow();
168+
}
169+
170+
assert.strictEqual(getQuickPickCallCount(), 1, 'must prompt instead of silently skipping');
171+
assert.strictEqual(calls.commitAllowEmpty, 1);
172+
assert.strictEqual(calls.cherryPickSkip, 0);
173+
assert.strictEqual(calls.cherryPickContinue, 0);
174+
});
175+
176+
it('"Skip this commit" explicitly chosen skips the commit', async () => {
177+
const { gitStub, calls } = createGitStub({ unresolvedConflicts: [], workdirHasChanges: false });
178+
const service = createService(gitStub);
179+
const { setQuickPickResponse } = stubWindow();
180+
setQuickPickResponse('Skip this commit');
181+
182+
try {
183+
await service.cherryPickNext(true);
184+
} catch {
185+
// expected short-circuit past the decision under test; see stub comment above.
186+
} finally {
187+
restoreWindow();
188+
}
189+
190+
assert.strictEqual(calls.cherryPickSkip, 1);
191+
assert.strictEqual(calls.commitAllowEmpty, 0);
192+
assert.strictEqual(calls.cherryPickContinue, 0);
193+
});
194+
195+
it('dismissing the prompt (Escape) aborts the clone rather than skipping or continuing', async () => {
196+
const { gitStub, calls } = createGitStub({ unresolvedConflicts: [], workdirHasChanges: false });
197+
const service = createService(gitStub);
198+
const { setQuickPickResponse } = stubWindow();
199+
setQuickPickResponse(undefined);
200+
201+
try {
202+
await service.cherryPickNext(true);
203+
} finally {
204+
restoreWindow();
205+
}
206+
207+
assert.strictEqual(calls.cherryPickSkip, 0);
208+
assert.strictEqual(calls.commitAllowEmpty, 0);
209+
assert.strictEqual(calls.cherryPickContinue, 0);
210+
});
211+
});
212+
});

0 commit comments

Comments
 (0)