golem15/msd-core
3
0
Fork 0
Code Issues Pull Requests Actions 1 Packages Projects Releases Wiki Activity

fix(#2913): distinguish empty cherry-pick from genuine conflict in hotfix create (#2970)

Browse Source 
* test(#2913): add regression for hotfix empty-cherry-pick discrimination

Two layers: (1) real-git test proving the discrimination logic (check for
unmerged paths → skip if empty, abort if conflict) is correct; (2) source-text
assertions proving the logic and summary heading are in release.yml.

Before the fix: release.yml treats any non-zero cherry-pick exit as a conflict,
so an already-applied commit (empty pick) aborts the entire hotfix create.

* fix(#2913): distinguish empty cherry-pick from genuine conflict

git cherry-pick exits non-zero for BOTH genuine conflicts AND empty picks
(the change is already present by content). The hotfix create job treated
every non-zero exit as a conflict, so an already-applied commit (notably the
structural 'chore: sync next package version' that follows every release
finalize) aborted the entire run.

Now the error handler checks for unmerged paths (git diff --diff-filter=U):
- No unmerged paths → already applied by content → skip, record, continue.
- Unmerged paths present → genuine conflict → existing abort/push/exit-1
  behavior and operator guidance, unchanged.

The job summary now has a separate 'Skipped (already applied by content)'
heading, distinct from 'Skipped (feat/refactor/etc)'.

* fix(#2913): address review — post-skip continuation test + conflict observability

Two findings from isolated adversarial review:
1. MINOR (test gap): no test proved the sequencer is clean after --skip, so a
   regression switching --skip to --quit would pass green. Added a second
   cherry-pick after the skip asserting it succeeds.
2. MINOR (observability): SKIPPED_EMPTY was dropped on the conflict-exit path
   — already-applied commits before a genuine conflict were silently lost from
   the summary. Now the conflict summary emits them under a dedicated heading.

* chore(#2913): add changeset fragment

* chore(#2913): backfill changeset PR number 2970

---------

Co-authored-by: sim <sim@local>
This commit is contained in:
Tom Boucher
2026-08-01 10:21:27 -04:00
committed by GitHub
parent d3305fc3a5
commit 608be0e7cf
3 changed files with 234 additions and 3 deletions
Download Patch File Download Diff File Expand all files Collapse all files

5
.changeset/eager-voles-frolic.md Normal file
View File

@@ -0,0 +1,5 @@
---
type: Fixed
pr: 2970
---
**Hotfix branches with auto cherry-pick no longer abort on already-applied commits** — cutting a hotfix from a tag whose `chore: sync next package version` commit applied empty (already present by content) aborted the entire create run. The cherry-pick error handler now distinguishes empty picks (no unmerged paths → skip) from genuine conflicts (unmerged paths → abort), and the job summary lists skipped-as-empty commits separately. (#2913)

35
.github/workflows/release.yml vendored
View File

@@ -216,6 +216,7 @@ jobs:
INCLUDED=""
SKIPPED=""
SKIPPED_EMPTY=""
while IFS= read -r SHA; do
[ -z "$SHA" ] && continue
SUBJECT=$(git log -1 --format='%s' "$SHA")
@@ -223,9 +224,26 @@ jobs:
if echo "$SUBJECT" | grep -qE '^(fix|chore)(\([^)]+\))?!?: '; then
echo "→ cherry-picking $SHA $SUBJECT"
if ! git cherry-pick -x "$SHA"; then
# Abort restores HEAD to the last successful pick. On real
# runs, push that state so the operator can fetch, resolve
# $SHA manually, and finalize with auto_cherry_pick=false.
# #2913: git cherry-pick exits non-zero for BOTH genuine conflicts
# AND empty picks (the change is already present by content).
# Distinguish them: a genuine conflict has unmerged paths; an
# empty pick has none. `git cherry` compares patch-ids against a
# different base, so a commit already present by content can still
# be marked `+` — and applying it produces an empty diff.
UNMERGED=$(git diff --name-only --diff-filter=U)
if [ -z "$UNMERGED" ]; then
# Already applied by content — skip and continue the loop.
# This is the structural case: every release finalize produces
# a `chore: sync next package version` commit that is a no-op
# against the tag the next hotfix branches from.
git cherry-pick --skip || true
echo " skip $SHA $SUBJECT (already applied by content — empty pick)"
SKIPPED_EMPTY="${SKIPPED_EMPTY}- \`${SHA}\` ${SUBJECT}"$'\n'
continue
fi
# Genuine conflict — abort restores HEAD to the last successful
# pick. On real runs, push that state so the operator can fetch,
# resolve $SHA manually, and finalize with auto_cherry_pick=false.
git cherry-pick --abort || true
if [ "$DRY_RUN" != "true" ]; then
git push --force-with-lease origin "$BRANCH" || git push origin "$BRANCH" || true
@@ -235,6 +253,12 @@ jobs:
echo ""
echo "Failed at: \`${SHA}\` — \`${SUBJECT}\`"
echo ""
if [ -n "$SKIPPED_EMPTY" ]; then
echo "### Skipped before this conflict (already applied by content — empty pick)"
echo ""
echo "$SKIPPED_EMPTY"
echo ""
fi
if [ "$DRY_RUN" = "true" ]; then
echo "**Dry run:** branch was not pushed, so the picks below were discarded with the runner."
if [ -n "$INCLUDED" ]; then
@@ -274,6 +298,11 @@ jobs:
echo "_No fix/chore commits to include._"
echo ""
fi
if [ -n "$SKIPPED_EMPTY" ]; then
echo "### Skipped (already applied by content — empty pick)"
echo ""
echo "$SKIPPED_EMPTY"
fi
if [ -n "$SKIPPED" ]; then
echo "### Skipped (feat/refactor/etc — not auto-included)"
echo ""

197
tests/release-hotfix-empty-cherry-pick.test.cjs Normal file
View File

@@ -0,0 +1,197 @@
// allow-test-rule: source-text-is-the-product (see #2913)
// .github/workflows/release.yml is the deployed CI contract; the cherry-pick
// error handler is bash inside the YAML. This test has two layers:
// 1. Real-git: proves the DISCRIMINATION LOGIC (check for unmerged paths →
// skip if empty, abort if conflict) is correct by exercising it against
// a real git repo with both scenarios.
// 2. Source-text: asserts the discrimination logic AND the separate summary
// heading actually made it into release.yml — so a regression that
// removes the check from the workflow is caught even if the logic test
// still passes.
'use strict';
const { describe, test, before, after } = require('node:test');
const assert = require('node:assert/strict');
const fs = require('node:fs');
const path = require('node:path');
const os = require('node:os');
const { spawnSync } = require('node:child_process');
const { cleanup } = require('./helpers.cjs');
const RELEASE_WORKFLOW = path.join(__dirname, '..', '.github', 'workflows', 'release.yml');
// ─── git helpers ────────────────────────────────────────────────────────────
function git(cwd, ...args) {
const r = spawnSync('git', ['-C', cwd, ...args], { encoding: 'utf8', stdio: ['pipe', 'pipe', 'pipe'] });
if (r.status !== 0) {
throw new Error(`git ${args.join(' ')} failed (status ${r.status}) in ${cwd}:\n${r.stderr || r.stdout}`);
}
return r.stdout.trim();
}
function makeRepo(dir) {
fs.mkdirSync(dir, { recursive: true });
spawnSync('git', ['init', dir], { encoding: 'utf8' });
git(dir, 'config', 'user.email', 'test@test');
git(dir, 'config', 'user.name', 'Test');
git(dir, 'config', 'commit.gpgsign', 'false');
// Ensure the default branch is 'main' regardless of the system's
// init.defaultBranch setting (older Git defaults to 'master').
spawnSync('git', ['-C', dir, 'symbolic-ref', 'HEAD', 'refs/heads/main'], { encoding: 'utf8' });
return dir;
}
function commitFile(dir, file, content, message) {
fs.writeFileSync(path.join(dir, file), content);
git(dir, 'add', file);
git(dir, 'commit', '-m', message);
return git(dir, 'rev-parse', 'HEAD');
}
/**
* The discrimination logic extracted from release.yml's cherry-pick error
* handler. This mirrors what the bash does after `git cherry-pick -x "$SHA"`
* exits non-zero:
* - Check for unmerged paths (`git diff --name-only --diff-filter=U`).
* - Empty → skip (already applied by content), record, continue.
* - Non-empty → genuine conflict, abort, return conflict info.
*
* @returns {{ status: 'empty' | 'conflict', unmerged: string[] }}
*/
function discriminateCherryPick(dir) {
const unmerged = git(dir, 'diff', '--name-only', '--diff-filter=U');
const unmergedFiles = unmerged.split('\n').filter(Boolean);
if (unmergedFiles.length === 0) {
git(dir, 'cherry-pick', '--skip');
return { status: 'empty', unmerged: [] };
}
git(dir, 'cherry-pick', '--abort');
return { status: 'conflict', unmerged: unmergedFiles };
}
// ─── real-git tests: prove the discrimination logic ─────────────────────────
describe('#2913 — cherry-pick empty-vs-conflict discrimination (real git)', () => {
let tmpDir;
before(() => {
tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-2913-'));
});
after(() => {
cleanup(tmpDir);
});
test('an already-applied-by-content commit is detected as empty and skipped', () => {
const repo = makeRepo(path.join(tmpDir, 'empty-pick'));
// Seed: create file.txt with "v1"
commitFile(repo, 'file.txt', 'v1\n', 'init');
const baseSha = git(repo, 'rev-parse', 'HEAD');
// On "next": bump file.txt to v2, then create a chore commit that sets it back to v1 then to v2
git(repo, 'checkout', '-b', 'next');
commitFile(repo, 'file.txt', 'v2\n', 'feat: bump to v2');
// Now create a commit that is a no-op against base by content: set file.txt to "v1\n"
// then immediately set it back — but that creates a non-empty diff. Instead,
// simulate the exact scenario: a commit whose PATCH applies empty because
// the content is already present.
//
// Create the "chore" commit on a throwaway branch from base, then cherry-pick
// it onto next where the change is already present.
git(repo, 'checkout', baseSha);
git(repo, 'checkout', '-b', 'throwaway');
// Apply a change that sets file.txt to v2 (same content as next already has)
const choreSha = commitFile(repo, 'file.txt', 'v2\n', 'chore: sync version to v2');
// Go back to next and try to cherry-pick the chore commit.
// file.txt is already v2 on next → the patch applies empty.
git(repo, 'checkout', 'next');
// Attempt the cherry-pick — it exits non-zero (empty).
const r = spawnSync('git', ['-C', repo, 'cherry-pick', '-x', choreSha], { encoding: 'utf8' });
assert.notStrictEqual(r.status, 0, 'cherry-pick of an already-applied commit must exit non-zero');
// Apply the discrimination logic.
const result = discriminateCherryPick(repo);
assert.strictEqual(result.status, 'empty', 'an empty pick must be classified as "empty", not "conflict"');
assert.deepStrictEqual(result.unmerged, []);
// After skip, HEAD should be back on next (no cherry-pick state).
const headAfter = git(repo, 'rev-parse', 'next');
const currentHead = git(repo, 'rev-parse', 'HEAD');
assert.strictEqual(currentHead, headAfter, 'after skip, HEAD should be unchanged (no new commit applied)');
// #2913 MINOR 1: after --skip, the sequencer must be clean so the next
// iteration's cherry-pick succeeds. A regression that switched --skip to
// --quit (which leaves sequencer state) would fail here.
git(repo, 'checkout', baseSha);
git(repo, 'checkout', '-b', 'throwaway2');
const realSha = commitFile(repo, 'other.txt', 'real change\n', 'fix: a real fix');
git(repo, 'checkout', 'next');
const realPick = spawnSync('git', ['-C', repo, 'cherry-pick', '-x', realSha], { encoding: 'utf8' });
assert.strictEqual(realPick.status, 0,
`after --skip, the sequencer must be clean so the next cherry-pick succeeds; got status ${realPick.status}:\n${realPick.stderr || realPick.stdout}`);
});
test('a genuine cherry-pick conflict is detected as conflict and aborted', () => {
const repo = makeRepo(path.join(tmpDir, 'conflict-pick'));
// Seed: file.txt with "base line"
commitFile(repo, 'file.txt', 'base line\n', 'init');
// On "next": change line 1 to "next version"
git(repo, 'checkout', '-b', 'next');
const conflictingSha = commitFile(repo, 'file.txt', 'next version\n', 'fix: change to next version');
// Go back to main and make a DIFFERENT change to the same line.
git(repo, 'checkout', 'main');
commitFile(repo, 'file.txt', 'main version\n', 'fix: change to main version');
// Attempt to cherry-pick the next commit — it conflicts (same line, different content).
const r = spawnSync('git', ['-C', repo, 'cherry-pick', '-x', conflictingSha], { encoding: 'utf8' });
assert.notStrictEqual(r.status, 0, 'cherry-pick of a conflicting commit must exit non-zero');
// Apply the discrimination logic.
const result = discriminateCherryPick(repo);
assert.strictEqual(result.status, 'conflict', 'a genuine conflict must be classified as "conflict"');
assert.ok(result.unmerged.length > 0, 'a genuine conflict must have unmerged files');
assert.ok(result.unmerged.includes('file.txt'), 'file.txt must be in the unmerged list');
});
});
// ─── source-text assertions: prove the logic is in release.yml ──────────────
describe('#2913 — release.yml cherry-pick error handler discriminates empty from conflict', () => {
const text = fs.existsSync(RELEASE_WORKFLOW)
? fs.readFileSync(RELEASE_WORKFLOW, 'utf8')
: '';
test('the cherry-pick error handler checks for unmerged paths before aborting', () => {
// The fix adds a `git diff --name-only --diff-filter=U` check inside the
// `if ! git cherry-pick` block. Without it, every empty pick aborts the run.
assert.ok(text.length > 0, 'release.yml must exist');
assert.ok(
text.includes('--diff-filter=U'),
'release.yml cherry-pick error handler must check for unmerged paths (git diff --diff-filter=U) before aborting — without this, an already-applied commit aborts the entire hotfix create (#2913)',
);
});
test('empty picks are skipped (git cherry-pick --skip), not aborted', () => {
assert.ok(text.length > 0, 'release.yml must exist');
assert.ok(
text.includes('cherry-pick --skip'),
'release.yml must skip (not abort) already-applied cherry-picks — git cherry-pick --skip continues the loop (#2913)',
);
});
test('the job summary distinguishes already-applied from not-fix-chore skips', () => {
assert.ok(text.length > 0, 'release.yml must exist');
// The fix introduces a separate variable/heading for already-applied commits
// so they are not silently dropped or confused with not-fix/chore skips.
assert.ok(
/already applied|applied by content|SKIPPED_EMPTY|empty pick/i.test(text),
'release.yml job summary must distinguish "skipped (already applied)" from "skipped (not fix/chore)" — a separate heading or variable for empty picks (#2913)',
);
});
});