From f3dbcec0fd9a88424c7809d8870f519c47db7220 Mon Sep 17 00:00:00 2001 From: claude code agent 227 Date: Mon, 29 Jun 2026 16:10:59 +0300 Subject: [PATCH] refactor(git-sync): DRY the move+body emission; cover M-side ghost-move (F7,F8) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit F8: extract emitMoveWithBody helper (renamesMoves + body update with basePath=oldPath) and call it at all three rename emission sites (ghost-move A, ghost-move M, R/C) — byte-identical behavior, single F4 rationale. Helper placed above computePushActions so the planner JSDoc stays attached. F7: add an M-side ghost-move test (D+M same pageId) asserting the move and the body update carry basePath=oldPath — the previously-untested branch. Co-Authored-By: Claude Opus 4.8 (1M context) --- packages/git-sync/src/engine/push.ts | 98 ++++++++----------- .../test/compute-push-actions.test.ts | 25 +++++ 2 files changed, 67 insertions(+), 56 deletions(-) diff --git a/packages/git-sync/src/engine/push.ts b/packages/git-sync/src/engine/push.ts index 4630c0d1..e8c2d8d5 100644 --- a/packages/git-sync/src/engine/push.ts +++ b/packages/git-sync/src/engine/push.ts @@ -237,6 +237,28 @@ export interface PushActionsInput { currentPageIds?: Set; } +/** + * Emit the action PAIR a move/rename needs (F4): a `renamesMoves` entry (the + * relocation, applied later as a move/rename — those ops are content-free) AND a + * body `updates` entry for the NEW path, so a body edit riding along in the SAME + * diff is not lost. `importPageMarkdown` targets by pageId and is a near-no-op for + * a pure relocation. The merge BASE is taken from the OLD path (`basePath`): the + * file lives at its NEW `path` in the working tree, but at last-pushed it was at + * `oldPath`, so the 3-way base must be looked up THERE — otherwise it resolves to + * null and the merge degrades to a 2-way, clobbering a concurrent Docmost-side + * edit. A PURE relocation is then a no-op that PRESERVES the live page. Shared by + * all three emit sites (ghost-move A, ghost-move M, and R/C). + */ +function emitMoveWithBody( + actions: PushActions, + pageId: string, + oldPath: string, + newPath: string, +): void { + actions.renamesMoves.push({ pageId, oldPath, newPath }); + actions.updates.push({ pageId, path: newPath, basePath: oldPath }); +} + /** * PURE push planner (SPEC §4/§6/§8). Classifies each diff row into a Docmost * action by `pageId` identity, with NO IO (the `metaAt` resolver is injected). @@ -321,25 +343,14 @@ export function computePushActions(input: PushActionsInput): PushActions { const pageId = meta?.pageId; if (pageId && ghostMove.has(pageId)) { // Half of a git-undetected move (a matching DELETE exists): record it - // as a rename/move (like a real `R`); the `D` side is suppressed so the - // page is never soft-deleted. - actions.renamesMoves.push({ + // as a rename/move (like a real `R`) PLUS a body update for the new path + // (F4); the `D` side is suppressed so the page is never soft-deleted. + emitMoveWithBody( + actions, pageId, - oldPath: ghostMove.get(pageId)!.oldPath, - newPath: change.path, - }); - // ...and ALSO push the body as an UPDATE for the new path (F4). A move - // can ride along with a body edit in the SAME diff; the move/rename ops - // never touch page CONTENT, so without this the edit is silently lost. - // `importPageMarkdown` targets by pageId and is a near-no-op for a pure - // relocation whose body is unchanged. The merge BASE is resolved from the - // OLD path (where the file lived at last-pushed) so the 3-way merge has a - // real common ancestor and a concurrent Docmost-side edit is preserved. - actions.updates.push({ - pageId, - path: change.path, - basePath: ghostMove.get(pageId)!.oldPath, - }); + ghostMove.get(pageId)!.oldPath, + change.path, + ); } else if (pageId) { // Added but already carries a pageId (restored/copied file): the page // exists in Docmost, so push content as an UPDATE — never a duplicate. @@ -367,22 +378,14 @@ export function computePushActions(input: PushActionsInput): PushActions { if (pageId && ghostMove.has(pageId)) { // This path's occupant changed pageId: the previous page left and THIS // page relocated here (a reshuffle). Its old file was DELETED elsewhere - // — coalesce into a rename/move so the page is never trashed. - actions.renamesMoves.push({ + // — coalesce into a rename/move (plus a body update for the new path, F4) + // so the page is never trashed. + emitMoveWithBody( + actions, pageId, - oldPath: ghostMove.get(pageId)!.oldPath, - newPath: change.path, - }); - // ...and ALSO push the body as an UPDATE for the new path (F4): the move - // op carries no content, so a body edit accompanying the relocation - // would otherwise be lost. Idempotent for a pure relocation. The merge - // BASE is resolved from the OLD path (the pre-image location at - // last-pushed) for an honest 3-way merge that preserves concurrent edits. - actions.updates.push({ - pageId, - path: change.path, - basePath: ghostMove.get(pageId)!.oldPath, - }); + ghostMove.get(pageId)!.oldPath, + change.path, + ); } else if (pageId) { actions.updates.push({ pageId, path: change.path }); } else { @@ -441,29 +444,12 @@ export function computePushActions(input: PushActionsInput): PushActions { const pageId = meta?.pageId; const oldPath = change.oldPath ?? change.path; if (pageId) { - actions.renamesMoves.push({ - pageId, - oldPath, - newPath: change.path, - }); - // git `-M` reports a rename+edit as a SINGLE `R` row, but the move/rename - // ops never carry page CONTENT — so ALSO emit an UPDATE for the new path - // (F4). Otherwise a body edit made in the same commit as a rename/move is - // silently and permanently lost (the refs advance, the mirror claims the - // new body, the next pull reads the old body back). `importPageMarkdown` - // targets by pageId and is a near-no-op for a pure rename (body unchanged). - // The merge BASE must come from the OLD path: the file lives at the NEW - // `path` now, but at last-pushed it was at `oldPath`, so resolving the - // base there (not at the new path, where it returns null) gives an honest - // 3-way merge — a PURE rename is then a no-op that PRESERVES any - // concurrent Docmost-side edit instead of clobbering it to git's body. - // For a `C` copy `oldPath` is the SOURCE, which exists in the ref tree, - // so the copy's body 3-way-merges against its source's last-pushed text. - actions.updates.push({ - pageId, - path: change.path, - basePath: oldPath, - }); + // Record the rename/move PLUS a body update for the new path (F4): git + // `-M` reports a rename+edit as a SINGLE `R` row, so a body edit made in + // the same commit as the move would otherwise be lost. For a `C` copy + // `oldPath` is the SOURCE (it exists in the ref tree), so the copy's body + // 3-way-merges against its source's last-pushed text. + emitMoveWithBody(actions, pageId, oldPath, change.path); } else { actions.skipped.push({ path: change.path, diff --git a/packages/git-sync/test/compute-push-actions.test.ts b/packages/git-sync/test/compute-push-actions.test.ts index 37305ced..c1965312 100644 --- a/packages/git-sync/test/compute-push-actions.test.ts +++ b/packages/git-sync/test/compute-push-actions.test.ts @@ -304,6 +304,31 @@ describe('computePushActions — ghost-move coalescing (data-loss guard)', () => ]); }); + it('D(old)+M(new) of the SAME pageId -> rename/move, NOT a delete (M-side reshuffle)', () => { + // A reshuffle: an ALREADY-existing path (`New.md`) takes on a new pageId while + // the old path (`Old.md`) is deleted — git reports the surviving side as `M` + // (the path was occupied), not `A`. Same pageId on both sides, so it is one + // page that relocated: the M-side ghost-move coalescing path. + const changes: DiffEntry[] = [ + { status: 'D', path: 'Old.md' }, + { status: 'M', path: 'New.md' }, + ]; + const metaAt = metaTable({ + 'Old.md|prev': meta({ pageId: 'p1', title: 'Old', spaceId: 'sp1' }), + 'New.md|current': meta({ pageId: 'p1', title: 'New', spaceId: 'sp1' }), + }); + const actions = computePushActions({ changes, metaAt }); + expect(actions.deletes).toEqual([]); // the page is NEVER trashed + // The coalesced move ALSO carries a body update for the new path (F4); the + // merge base resolves from the OLD path, not null and not the new path. + expect(actions.updates).toEqual([ + { pageId: 'p1', path: 'New.md', basePath: 'Old.md' }, + ]); + expect(actions.renamesMoves).toEqual([ + { pageId: 'p1', oldPath: 'Old.md', newPath: 'New.md' }, + ]); + }); + it('a real delete (no matching add) is STILL a delete', () => { const changes: DiffEntry[] = [{ status: 'D', path: 'Gone.md' }]; const metaAt = metaTable({