mirror of
https://github.com/garrytan/gstack.git
synced 2026-11-06 00:44:07 +01:00
fix(ship): a metadata-only conflict still holds a landed parent; a post-merge loop head starts at its own fork (GST-1399)
heldBy counts a merge as conflicted only when a conflicted path is inside its pathspec, so a train's VERSION restamp in the squash no longer hides the landed parent. The review-loop exception's atHead uses patchBase, so a fix-check head after the child merged main starts at its own merge base, not at the parent. Review-Finding: p2/specialist-batched#1 reader=specialist-batched Review-Finding: p2/red-team#1 reader=red-team Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com>
This commit is contained in:
1 parent
a09196fc55
commit
3f4c47c5e5
3 files changed
+38
-5
No files matched your search
+8
-4
@@ -329,7 +329,8 @@ export const HELD_WALK = 200;
|
||||
* `pathspec`. The current base is tried first. When that merge conflicts (the base edited the landed lines afterwards),
|
||||
* the commits on its first-parent history since the fork that touched every path of the change are tried newest first,
|
||||
* so a squash the base later edited is still found. A change with no path of its own is held by no commit (any base
|
||||
* trivially holds it), and a conflicted path is never held. Null when none within HELD_WALK holds it or git cannot tell.
|
||||
* trivially holds it), and a conflict inside `pathspec` is never held (one only in excluded release metadata, like a
|
||||
* train's VERSION restamp, is). Null when none within HELD_WALK holds it or git cannot tell.
|
||||
*/
|
||||
export function heldBy(repoRoot: string, commit: string, currentBase: string, pathspec: readonly string[]): string | null {
|
||||
const run = (args: string[]) => spawnSync('git', ['-c', 'core.quotepath=off', ...args], { cwd: repoRoot, encoding: 'utf8', timeout: 120_000, maxBuffer: 64 * 1024 * 1024 });
|
||||
@@ -338,9 +339,12 @@ export function heldBy(repoRoot: string, commit: string, currentBase: string, pa
|
||||
const paths = own?.status === 0 ? own.stdout.split('\0').filter(Boolean) : [];
|
||||
if (!paths.length) return null;
|
||||
const holds = (at: string): 'held' | 'conflict' | 'no' => {
|
||||
const merged = run(['merge-tree', '--write-tree', '--no-messages', at, commit]);
|
||||
const tree = merged.status === 0 || merged.status === 1 ? merged.stdout.split('\n')[0]?.trim() : undefined;
|
||||
if (merged.status === 0 && tree && run(['diff', '--quiet', at, tree, '--', ...pathspec]).status === 0) return 'held';
|
||||
const merged = run(['merge-tree', '--write-tree', '--no-messages', '--name-only', at, commit]);
|
||||
const [tree, ...conflicted] = merged.status === 0 || merged.status === 1 ? merged.stdout.split('\n').filter(Boolean) : [];
|
||||
const mb = conflicted.length ? run(['merge-base', at, commit]).stdout.trim() : '';
|
||||
const theirs = mb ? run(['diff', '-z', '--name-only', '--no-renames', mb, commit, '--', ...pathspec]) : null;
|
||||
const clean = !conflicted.length || (theirs?.status === 0 && !theirs.stdout.split('\0').some(p => conflicted.includes(p)));
|
||||
if (tree && clean && run(['diff', '--quiet', at, tree, '--', ...pathspec]).status === 0) return 'held';
|
||||
return merged.status === 1 ? 'conflict' : 'no';
|
||||
};
|
||||
const tip = holds(currentBase);
|
||||
|
||||
+1
-1
@@ -306,7 +306,7 @@ function route(cwd: string, base: string, reviewed: string, reviewedBase: Review
|
||||
if (loop) {
|
||||
const head = loop.fix_check.head;
|
||||
const since = touchedPaths(cwd, head, now);
|
||||
const fork = parent && ancestor(parent, head) ? parent : git(cwd, ['merge-base', baseRef, head]).stdout.trim();
|
||||
const fork = patchBase(cwd, baseRef, head, parent);
|
||||
const atHead = fork ? touchedPaths(cwd, fork, head) : null;
|
||||
if (!since || !atHead) { r.stop(`git could not list the paths changed since fix-check ${head.slice(0, 12)} or the paths the branch touched at it against ${baseRef}: stage 2 is blocked`); return null; }
|
||||
const listed = loop.fix_check.files;
|
||||
|
||||
@@ -210,6 +210,35 @@ describe('finish after a stacked parent squash-merges (GST-1399)', () => {
|
||||
expect([r.status, lineOf(r.stdout, 'STOP')]).toEqual([3, expect.stringContaining('nor a landed stack parent')]);
|
||||
});
|
||||
|
||||
// Value: protects=a landed parent whose tentative VERSION the train restamped in the squash itself is still landed: a
|
||||
// merge-tree conflict only on excluded release metadata does not count against the parent (specialist p2 on #4021);
|
||||
// fails_when=heldBy rejects any conflicted merge, not only one conflicted inside its pathspec; seam=lib/ship-receipt.ts heldBy
|
||||
test('a parent whose VERSION the train restamped at landing is still landed', () => {
|
||||
const { f, parent, reviewed, post } = stacked({
|
||||
parentFiles: { ...PARENT, VERSION: '1.0.1.0\n' },
|
||||
land: false,
|
||||
afterLand: g => g.advanceBase({ ...PARENT, VERSION: '1.0.2.0\n' }),
|
||||
});
|
||||
expect(f.git('show', 'HEAD:VERSION').stdout).toBe('1.0.2.0\n');
|
||||
const r = finish(f, reviewed, post, ['--reviewed-base', parent]);
|
||||
expect(lineOf(r.stdout, 'REVIEWED_BASE')).toBe(`REVIEWED_BASE: stack parent ${parent.slice(0, 12)} landed on origin/main at ${f.git('rev-parse', 'origin/main').stdout.trim().slice(0, 12)}`);
|
||||
expect(lineOf(r.stdout, 'STOP')).not.toContain('nor a landed stack parent');
|
||||
});
|
||||
|
||||
// Value: protects=a review-loop exception whose fix-check head comes after the child merged main cannot accept a
|
||||
// main-only file edited after it: the "touched at the fix-check head" set starts at the parent only when that head
|
||||
// forked before the parent landed (red-team p2 on #4021); fails_when=the loop's fork is the parent whenever the parent
|
||||
// is an ancestor of the head; seam=route's review-loop exception via patchBase
|
||||
test('a loop exception at a post-merge fix-check head does not accept a main-only file edited after it', () => {
|
||||
const { f, parent, reviewed, post } = stacked({ base: { ...BASE, 'src/other.ts': lines(5) }, mainLater: { ...MAIN_LATER, 'src/other.ts': lines(5, { 2: 'export const l2 = 20;' }) } });
|
||||
const merged = head(f);
|
||||
f.write({ 'src/other.ts': lines(5, { 2: 'export const l2 = 20;', 4: 'export const l4 = 40;' }) }); f.commit('edit a main-only file after the fix-check');
|
||||
const exception = JSON.stringify({ accepted_by: 'GStack Minder', date: '2026-11-05', via: 'test', reason: 'fixture', fix_check: { head: merged, findings: 0 } });
|
||||
const r = finish(f, reviewed, post, ['--reviewed-base', parent, '--review-loop-exception', exception]);
|
||||
expect(lineOf(r.stdout, 'REVIEW_LOOP_EXCEPTION')).toContain('accepted none');
|
||||
expect([r.status, lineOf(r.stdout, 'STOP')]).toEqual([3, behaviorStop('src/other.ts')]);
|
||||
});
|
||||
|
||||
// Value: protects=a reviewed executable bit reverted to main's mode after review is not held by main, so it stops
|
||||
// (outside voice on #4013); fails_when=baseHolds compares blob bytes only; seam=lib/ship-patch.ts baseHolds
|
||||
test('a reverted mode change stops', () => {
|
||||
|
||||
Reference in new issue
Block a user