fix(task): auto-skipped empty commits during task-branch cherry-pick
An intermediate commit whose net effect is already on HEAD (redundant change, or 3-way merged to HEAD by "theirs == ours") stopped the sequencer with "The previous cherry-pick is now empty" and was treated as a hard conflict. mergeTaskBranches aborted the whole range, marked the branch failed, and dropped every remaining non-overlapping commit. Add cherryPick.skip and cherryPick.isEmptyError to the git namespace, then in mergeTaskBranches' catch classify the failure before aborting: loop --skip while the error stderr matches the "now empty" phrase so consecutive empties advance the sequencer; fall through to abort/fail on the first non-empty error (genuine conflict with unmerged files). Fixes #4438
This commit is contained in:
roboomp
@@ -2,6 +2,10 @@
|
||||
|
||||
## [Unreleased]
|
||||
|
||||
### Fixed
|
||||
|
||||
- Fixed task-branch merges aborting the whole cherry-pick range when a single commit was empty against HEAD (redundant with an already-applied change, or 3-way merged to HEAD); the sequencer now `--skip`s the empty commit and continues so later non-overlapping work still lands ([#4438](https://github.com/can1357/oh-my-pi/issues/4438)).
|
||||
|
||||
## [16.3.4] - 2026-07-03
|
||||
|
||||
### Fixed
|
||||
|
||||
@@ -855,18 +855,40 @@ export async function mergeTaskBranches(
|
||||
try {
|
||||
const target = baseSha ? `${baseSha}..${branchName}` : branchName;
|
||||
await git.cherryPick(repoRoot, target);
|
||||
} catch (err) {
|
||||
} catch (initialErr) {
|
||||
// Empty cherry-picks are not conflicts: a commit whose net
|
||||
// effect is already on HEAD (redundant change, or 3-way
|
||||
// merge auto-resolved to HEAD) leaves the sequencer stopped
|
||||
// with a "The previous cherry-pick is now empty" message.
|
||||
// Advance past every consecutive empty with `--skip` so the
|
||||
// remaining non-redundant commits in the range still land.
|
||||
// A genuine conflict (unmerged files, no "now empty"
|
||||
// message) falls through to the abort path below.
|
||||
let cursor: unknown = initialErr;
|
||||
while (git.cherryPick.isEmptyError(cursor)) {
|
||||
try {
|
||||
await git.cherryPick.skip(repoRoot);
|
||||
cursor = undefined;
|
||||
break;
|
||||
} catch (skipErr) {
|
||||
cursor = skipErr;
|
||||
}
|
||||
}
|
||||
if (cursor === undefined) {
|
||||
merged.push(branchName);
|
||||
continue;
|
||||
}
|
||||
try {
|
||||
await git.cherryPick.abort(repoRoot);
|
||||
} catch {
|
||||
/* no state to abort */
|
||||
}
|
||||
const stderr =
|
||||
err instanceof git.GitCommandError
|
||||
? err.result.stderr.trim()
|
||||
: err instanceof Error
|
||||
? err.message
|
||||
: String(err);
|
||||
cursor instanceof git.GitCommandError
|
||||
? cursor.result.stderr.trim()
|
||||
: cursor instanceof Error
|
||||
? cursor.message
|
||||
: String(cursor);
|
||||
failed.push(branchName);
|
||||
conflictResult = {
|
||||
merged,
|
||||
|
||||
@@ -1726,6 +1726,29 @@ export const cherryPick = Object.assign(
|
||||
async abort(cwd: string, signal?: AbortSignal): Promise<void> {
|
||||
await runEffect(cwd, ["cherry-pick", "--abort"], { signal });
|
||||
},
|
||||
/**
|
||||
* Skip the current commit of an in-progress cherry-pick sequence and
|
||||
* continue with the rest of the range. Use after {@link isEmptyError}
|
||||
* reports the current attempt collapsed to a no-op — the alternative,
|
||||
* `--abort`, throws away every remaining commit in the range.
|
||||
*/
|
||||
async skip(cwd: string, signal?: AbortSignal): Promise<void> {
|
||||
await runEffect(cwd, ["cherry-pick", "--skip"], { signal });
|
||||
},
|
||||
/**
|
||||
* True when a cherry-pick failure was caused by the current commit
|
||||
* being empty against HEAD — either redundant with an already-applied
|
||||
* change, or auto-resolved to HEAD by a 3-way merge. Callers should
|
||||
* `--skip` in this case to advance the sequencer rather than aborting
|
||||
* the whole range: an empty commit is not a merge conflict, and any
|
||||
* later commits in the range still deserve to land.
|
||||
*/
|
||||
isEmptyError(err: unknown): boolean {
|
||||
return (
|
||||
err instanceof GitCommandError &&
|
||||
/the previous cherry-pick is now empty/i.test(err.result.stderr)
|
||||
);
|
||||
},
|
||||
},
|
||||
);
|
||||
|
||||
|
||||
@@ -398,6 +398,104 @@ describe("worktree isolation helpers", () => {
|
||||
expect(delta.rootPatch).not.toContain("baseline dirty change");
|
||||
expect(delta.rootPatch).not.toContain("preexisting.txt");
|
||||
});
|
||||
|
||||
// Regression for #4438: cherry-picking a range where an intermediate
|
||||
// commit becomes empty (redundant with HEAD, or 3-way merged to HEAD)
|
||||
// used to abort the whole range and mark the branch failed, dropping
|
||||
// every remaining commit. The fixup here restores merged.txt to the
|
||||
// content the shared fixture branch already established on HEAD, so
|
||||
// the sequencer stops with "The previous cherry-pick is now empty".
|
||||
// The follow-up commit contains real, non-overlapping work that MUST
|
||||
// still land.
|
||||
it("auto-skips empty cherry-picks so remaining commits in the range still land", async () => {
|
||||
const REDUNDANT_BRANCH = "task/redundant-then-real";
|
||||
await runGit(repo, ["checkout", "-q", "-b", REDUNDANT_BRANCH, initialSha]);
|
||||
const branchBase = await runGit(repo, ["rev-parse", "HEAD"]);
|
||||
// This commit sets merged.txt to the exact content TASK_BRANCH
|
||||
// establishes on HEAD. Once TASK_BRANCH is cherry-picked, the
|
||||
// 3-way merge for this commit sees theirs == ours == target,
|
||||
// resolves to HEAD, and stops the sequencer with the "The
|
||||
// previous cherry-pick is now empty" message.
|
||||
await fs.writeFile(path.join(repo, "merged.txt"), "task branch change\n");
|
||||
await runGit(repo, ["commit", "-q", "-am", "redundant: same as task branch tip"]);
|
||||
// A non-overlapping follow-up that MUST land even though the
|
||||
// preceding commit collapsed to empty.
|
||||
await fs.writeFile(path.join(repo, "downstream.txt"), "unrelated follow-up\n");
|
||||
await runGit(repo, ["add", "downstream.txt"]);
|
||||
await runGit(repo, ["commit", "-q", "-m", "unrelated follow-up commit"]);
|
||||
await runGit(repo, ["checkout", "-q", BASE_BRANCH]);
|
||||
try {
|
||||
const result = await mergeTaskBranches(repo, [
|
||||
{ branchName: TASK_BRANCH, taskId: "task-1" },
|
||||
{ branchName: REDUNDANT_BRANCH, taskId: "task-2", baseSha: branchBase },
|
||||
]);
|
||||
|
||||
const [status, unmerged, mergedContent, downstreamContent, log] = await Promise.all([
|
||||
runGit(repo, ["status", "--porcelain=v1"]),
|
||||
runGit(repo, ["ls-files", "--unmerged"]),
|
||||
fs.readFile(path.join(repo, "merged.txt"), "utf8"),
|
||||
fs.readFile(path.join(repo, "downstream.txt"), "utf8"),
|
||||
runGit(repo, ["log", "--pretty=%s", `${initialSha}..HEAD`]),
|
||||
]);
|
||||
|
||||
expect(result).toEqual({ failed: [], merged: [TASK_BRANCH, REDUNDANT_BRANCH] });
|
||||
// No cherry-pick sequencer state, no unmerged entries: the
|
||||
// skip advanced cleanly.
|
||||
expect(status).toBe("");
|
||||
expect(unmerged).toBe("");
|
||||
// TASK_BRANCH landed and the empty commit did NOT re-land it
|
||||
// as a duplicate.
|
||||
expect(mergedContent).toBe("task branch change\n");
|
||||
expect(downstreamContent).toBe("unrelated follow-up\n");
|
||||
const subjects = log.split("\n").filter(Boolean);
|
||||
expect(subjects).toContain("unrelated follow-up commit");
|
||||
expect(subjects).toContain("task-change");
|
||||
// The redundant commit MUST NOT survive in history as a
|
||||
// duplicate no-op.
|
||||
expect(subjects).not.toContain("redundant: same as task branch tip");
|
||||
} finally {
|
||||
await cleanupTaskBranches(repo, [REDUNDANT_BRANCH]);
|
||||
}
|
||||
});
|
||||
|
||||
// A genuine content conflict (unmerged files, no "now empty" hint)
|
||||
// must NOT be misclassified as empty. The branch stays in `failed`
|
||||
// and the sequencer aborts cleanly — no lingering cherry-pick state,
|
||||
// no unmerged index entries.
|
||||
it("still aborts on genuine cherry-pick conflicts", async () => {
|
||||
const CONFLICT_BRANCH = "task/genuine-conflict";
|
||||
await runGit(repo, ["checkout", "-q", "-b", CONFLICT_BRANCH, initialSha]);
|
||||
const branchBase = await runGit(repo, ["rev-parse", "HEAD"]);
|
||||
await fs.writeFile(path.join(repo, "merged.txt"), "incompatible edit\n");
|
||||
await runGit(repo, ["commit", "-q", "-am", "incompatible merged.txt edit"]);
|
||||
await runGit(repo, ["checkout", "-q", BASE_BRANCH]);
|
||||
try {
|
||||
const result = await mergeTaskBranches(repo, [
|
||||
{ branchName: TASK_BRANCH, taskId: "task-1" },
|
||||
{ branchName: CONFLICT_BRANCH, taskId: "task-2", baseSha: branchBase },
|
||||
]);
|
||||
|
||||
const [status, unmerged, cherryPickHeadExit] = await Promise.all([
|
||||
runGit(repo, ["status", "--porcelain=v1"]),
|
||||
runGit(repo, ["ls-files", "--unmerged"]),
|
||||
runGit(repo, ["rev-parse", "--verify", "--quiet", "CHERRY_PICK_HEAD"]).then(
|
||||
() => 0,
|
||||
() => 1,
|
||||
),
|
||||
]);
|
||||
|
||||
expect(result.merged).toEqual([TASK_BRANCH]);
|
||||
expect(result.failed).toEqual([CONFLICT_BRANCH]);
|
||||
expect(result.conflict).toContain(CONFLICT_BRANCH);
|
||||
// No stuck sequencer, no unmerged entries: the abort cleaned
|
||||
// up after itself.
|
||||
expect(status).toBe("");
|
||||
expect(unmerged).toBe("");
|
||||
expect(cherryPickHeadExit).toBe(1);
|
||||
} finally {
|
||||
await cleanupTaskBranches(repo, [CONFLICT_BRANCH]);
|
||||
}
|
||||
});
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user