From 20816ee04203ea9ed35ac53e22d61ce616fec81f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mindaugas=20Kasparavic=CC=8Cius?= Date: Tue, 11 Aug 2026 10:31:58 +0300 Subject: [PATCH] fix(merge): the index decides which marker blocks git actually wrote MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A document ABOUT merge conflicts carries marker-shaped lines of its own. The parser could not tell them from git's, so the view offered the documented example as a decision — and answering it dropped one side of it. Silently: the result pane shows no markers, so there was nothing on screen to notice by. It also claimed two conflicts where git had made one. This is the reference tools' whole reason for not reading $MERGED: JetBrains resolves from the three VCS revisions, so marker-shaped prose is just text on every side. The same source of truth settles it here without replacing git's merge — git never writes markers into the index, so a block that appears verbatim in a pristine side was already in the file, and is put back as stable text. With no index there is nothing to check against and nothing is demoted: guessing there would drop a real conflict. Deliberately NOT recomputing the regions from the three stages, which is what JetBrains does literally. That would replace git's own auto-merge with another algorithm's on the one file this app overwrites, and buys nothing here — git has already decided where the conflicts are, and this only stops us inventing extra ones. The panes now take their seed ranges from the store rather than re-parsing `rawContent`, which is what kept the corrected regions and the seeded decorations from disagreeing — and drops a second parse of the file. Co-Authored-By: Claude Opus 5 --- docs/security.md | 7 ++ e2e/merge-resolve.spec.mjs | 66 +++++++++++++++++++ .../src/features/merge/mergePaneOps.js | 4 +- src/renderer/src/features/merge/mergeStore.js | 17 +++-- src/renderer/src/utils/mergeConflicts.js | 42 ++++++++++++ .../features/merge/mergePaneOps.test.js | 8 +-- tests/renderer/utils/mergeConflicts.test.js | 47 ++++++++++++- 7 files changed, 174 insertions(+), 17 deletions(-) diff --git a/docs/security.md b/docs/security.md index 58655c0..40542b6 100644 --- a/docs/security.md +++ b/docs/security.md @@ -69,6 +69,13 @@ already took arbitrary text, so a renderer that wanted to write something neither side said could always do it. Editing widens what the READER can express, not what the surface accepts. +The index also decides which marker blocks are REAL. A document about merge +conflicts carries marker-shaped lines of its own, and the parser cannot tell +them from git's — offering one as a decision dropped half the document. git +never writes markers into the index, so a block that appears verbatim in a +pristine side is prose and stays put. With no index there is nothing to check +against and nothing is demoted: guessing would drop a real conflict. + The three inputs are read out of git's index (`:1:`/`:2:`/`:3:`) through the same argv fence as any other git call — fixed vectors, `--end-of-options`, the path checked as `readBlobArgs` checks it, and the stage one of three integers diff --git a/e2e/merge-resolve.spec.mjs b/e2e/merge-resolve.spec.mjs index 3348a54..c95b526 100644 --- a/e2e/merge-resolve.spec.mjs +++ b/e2e/merge-resolve.spec.mjs @@ -650,3 +650,69 @@ test('answers a region again after undoing a Neither', async () => { rmSync(dir, { recursive: true, force: true }) } }) + +// A document ABOUT merge conflicts carries marker-shaped lines of its own. The +// parser cannot tell them from git's, so the view offered them as a decision and +// answering it dropped half the document — silently, because the result pane +// shows no markers to notice by. git never writes markers into the index, so the +// pristine sides settle which blocks it actually wrote. +test('leaves marker-shaped prose alone and offers only the real conflict', async () => { + const dir = mkdtempSync(join(tmpdir(), 'diffbro-merge-prose-')) + const env = { + ...process.env, + GIT_AUTHOR_NAME: 'T', + GIT_AUTHOR_EMAIL: 't@e', + GIT_COMMITTER_NAME: 'T', + GIT_COMMITTER_EMAIL: 't@e' + } + const git = (...args) => execFileSync('git', args, { cwd: dir, env }) + const file = join(dir, 'doc.md') + // The markers are BUILT here rather than written literally, so this spec is + // not itself a file full of conflict markers. + const example = [ + '<'.repeat(7) + ' HEAD', + 'the left side', + '='.repeat(7), + 'the right side', + '>'.repeat(7) + ' other' + ] + const doc = (setting) => + ['# how a conflict looks', '', ...example, '', `setting = ${setting}`, 'trailer', ''].join('\n') + + git('init', '-q', '-b', 'main') + writeFileSync(file, doc('base')) + git('add', '.') + git('commit', '-qm', 'base') + git('checkout', '-qb', 'feature') + writeFileSync(file, doc('theirs')) + git('commit', '-qam', 'theirs') + git('checkout', '-q', 'main') + writeFileSync(file, doc('ours')) + git('commit', '-qam', 'ours') + try { + git('merge', 'feature') + } catch { + // Expected. + } + + const userDataDir = freshUserDataDir() + const app = await launchApp(userDataDir) + const page = await firstReadyPage(app) + try { + await runMergetool(userDataDir, dir, file) + await expect(page.locator('.merge-view')).toBeVisible({ timeout: 20000 }) + + // One decision, not two: the document's own example is not one. + await expect(page.locator('.merge-count')).toHaveText('1 conflict left') + + await page.getByTestId('merge-take-theirs').click() + await page.getByTestId('merge-save').click() + await expect(page.locator('.merge-view')).toHaveCount(0, { timeout: 10000 }) + + // The document survives intact, markers and both of its sides. + expect(readFileSync(file, 'utf8')).toBe(doc('theirs')) + } finally { + await app.close().catch(() => {}) + rmSync(dir, { recursive: true, force: true }) + } +}) diff --git a/src/renderer/src/features/merge/mergePaneOps.js b/src/renderer/src/features/merge/mergePaneOps.js index d64cde8..51ada22 100644 --- a/src/renderer/src/features/merge/mergePaneOps.js +++ b/src/renderer/src/features/merge/mergePaneOps.js @@ -6,8 +6,6 @@ import * as monaco from 'monaco-editor' import { gutterAnchors } from './mergeGutter' import { applyChoice, touchedIndexes, wholeLines } from './mergeEdits' import { regionOptions, rulerColors, sideDecorations } from './mergeDecorations' -import { initialRanges } from './threeWay' -import { parseConflicts } from '../../utils/mergeConflicts' export const SIDES = ['ours', 'result', 'theirs'] @@ -69,7 +67,7 @@ export function repaint({ editors, merge, ids, anchors }) { // The regions start where our side put them; from here the editor keeps the // ranges right and nothing re-parses the text. export function seedRegions({ editors, merge, ids, settled }) { - const ranges = initialRanges(parseConflicts(merge.rawContent)) + const ranges = merge.regionLines const colors = rulerColors() const model = editors.result.getModel() ids.result = editors.result.deltaDecorations( diff --git a/src/renderer/src/features/merge/mergeStore.js b/src/renderer/src/features/merge/mergeStore.js index 9925151..3fa2f5d 100644 --- a/src/renderer/src/features/merge/mergeStore.js +++ b/src/renderer/src/features/merge/mergeStore.js @@ -1,6 +1,6 @@ import { defineStore } from 'pinia' -import { parseConflicts } from '../../utils/mergeConflicts' -import { sidesFromConflicts } from './threeWay' +import { parseConflicts, withoutProseConflicts } from '../../utils/mergeConflicts' +import { initialRanges, sidesFromConflicts } from './threeWay' // With no markers left in the result there is nothing for a parser to count, so // resolution is state rather than something re-derived from the text. @@ -30,8 +30,8 @@ export const useMergeStore = defineStore('merge', { theirsName: '', /** The editable middle pane, marker-free from the moment it opens. */ result: '', - /** What git left, kept only to seed the regions' first positions. */ - rawContent: '', + /** Where each region opens, 1-based: the panes anchor their decorations here. */ + regionLines: [], ours: '', theirs: '', base: null, @@ -51,15 +51,18 @@ export const useMergeStore = defineStore('merge', { actions: { /** @param {object} payload from main: the conflicted text and both sides */ begin(payload) { - const parsed = parseConflicts(payload.content) - this.error = parsed ? '' : 'unreadable' + const read = parseConflicts(payload.content) + this.error = read ? '' : 'unreadable' + // The index has the last word on which marker blocks git wrote: a document + // ABOUT conflicts carries its own, and they are prose, not a decision. + const parsed = withoutProseConflicts(read, payload.content, payload) this.regions = (parsed?.segments ?? []) .filter((seg) => seg.type === 'conflict') .map((seg) => ({ ours: seg.ours, theirs: seg.theirs, resolved: false })) // Our side stands in each conflicted place so the file is valid from the // start; the region is tinted unresolved until the reader confirms it. this.result = parsed ? sidesFromConflicts(parsed).ours : payload.content - this.rawContent = payload.content + this.regionLines = initialRanges(parsed) this.mixedEndings = hasMixedEndings(payload.content) const fallback = sidesFromConflicts(parsed) this.ours = payload.ours ?? fallback.ours diff --git a/src/renderer/src/utils/mergeConflicts.js b/src/renderer/src/utils/mergeConflicts.js index 57ffa69..bd2c8bb 100644 --- a/src/renderer/src/utils/mergeConflicts.js +++ b/src/renderer/src/utils/mergeConflicts.js @@ -83,6 +83,48 @@ export function parseConflicts(text) { return { segments } } +/** + * The parse with marker-shaped PROSE put back where it belongs. + * + * A document about merge conflicts contains marker lines, and the parser cannot + * tell them from the ones git wrote — so answering that "conflict" dropped half + * of it, silently, since the result pane shows no markers to notice by. + * + * git never writes markers into the index, so a block that appears verbatim in + * one of the pristine sides was already in the file. That is the whole test. + * With no sides — a mergetool run by hand — nothing is demoted: guessing here + * would drop a real conflict. + * + * @param {object|null} parsed from parseConflicts + * @param {string} text the same text it was given + * @param {{ours?: string, theirs?: string}} sides stages 2 and 3 + */ +export function withoutProseConflicts(parsed, text, sides) { + const pristine = [sides?.ours, sides?.theirs].filter(Boolean) + if (!parsed || !pristine.length) return parsed + const raw = String(text ?? '').split(/(?<=\n)/) + const blockOf = (seg) => raw.slice(seg.startLine - 1, seg.endLine) + const isProse = (seg) => + seg.type === 'conflict' && pristine.some((side) => side.includes(blockOf(seg).join(''))) + return { + segments: parsed.segments.reduce((out, segment) => { + appendSegment(out, isProse(segment) ? stable(blockOf(segment)) : segment) + return out + }, []) + } +} + +// Two stable runs either side of demoted prose are one run. +function appendSegment(segments, next) { + const last = segments[segments.length - 1] + if (next.type !== 'stable' || last?.type !== 'stable') { + segments.push(next) + return + } + last.raw.push(...next.raw) + last.lines.push(...next.lines) +} + const conflicts = (parsed) => (parsed?.segments ?? []).filter((s) => s.type === 'conflict') /** How many regions the reader has to decide. */ diff --git a/tests/renderer/features/merge/mergePaneOps.test.js b/tests/renderer/features/merge/mergePaneOps.test.js index 61a80e4..8fbe4d3 100644 --- a/tests/renderer/features/merge/mergePaneOps.test.js +++ b/tests/renderer/features/merge/mergePaneOps.test.js @@ -11,14 +11,12 @@ import { writeChoice } from '../../../../src/renderer/src/features/merge/mergePaneOps' -const RAW = `head\n<<<<<<< HEAD\nreplicas: 5\n=======\nreplicas: 9\n>>>>>>> feature\ntail\n` - function scene() { const merge = { at: 0, ours: 'head\nreplicas: 5\ntail\n', theirs: 'head\nreplicas: 9\ntail\n', - rawContent: RAW, + regionLines: [{ line: 2, count: 1 }], result: 'head\nreplicas: 5\ntail\n', regions: [{ ours: ['replicas: 5'], theirs: ['replicas: 9'], resolved: false }], markResolved(i) { @@ -160,14 +158,12 @@ describe('reveal', () => { // theirs replaced the first stable line after the conflict with it. The line // was gone from the file git was then handed. describe('a region one side emptied', () => { - const RAW_EMPTY = `head\n<<<<<<< HEAD\n=======\nthey added this\n>>>>>>> feature\ntail\n` - function emptyScene() { const merge = { at: 0, ours: 'head\ntail\n', theirs: 'head\nthey added this\ntail\n', - rawContent: RAW_EMPTY, + regionLines: [{ line: 2, count: 0 }], result: 'head\ntail\n', regions: [{ ours: [], theirs: ['they added this'], resolved: false }], markResolved(i) { diff --git a/tests/renderer/utils/mergeConflicts.test.js b/tests/renderer/utils/mergeConflicts.test.js index 0f475b2..b3578d6 100644 --- a/tests/renderer/utils/mergeConflicts.test.js +++ b/tests/renderer/utils/mergeConflicts.test.js @@ -3,8 +3,10 @@ import { composeMerge, conflictCount, parseConflicts, - unresolvedCount + unresolvedCount, + withoutProseConflicts } from '../../../src/renderer/src/utils/mergeConflicts' +import { sidesFromConflicts } from '../../../src/renderer/src/features/merge/threeWay' const FILE = [ 'top', @@ -173,3 +175,46 @@ describe('parseConflicts — where each region sits', () => { expect(parseConflicts(DIFF3).segments[0].startLine).toBe(1) }) }) + +// A document ABOUT merge conflicts contains marker-shaped lines, and the parser +// cannot tell them from the ones git wrote. Answering the prose "conflict" drops +// half of it — silently, because the result pane shows no markers to notice. +// +// git never writes markers into the index, so the pristine sides settle it. +describe('withoutProseConflicts', () => { + const example = ['<<<<<<< HEAD', 'the left side', '=======', 'the right side', '>>>>>>> other'] + const real = ['<<<<<<< HEAD', 'setting = ours', '=======', 'setting = theirs', '>>>>>>> feature'] + const merged = ['# how a conflict looks', '', ...example, '', ...real, 'trailer', ''].join('\n') + const ours = ['# how a conflict looks', '', ...example, '', 'setting = ours', 'trailer', ''].join( + '\n' + ) + const theirs = ours.replace('setting = ours', 'setting = theirs') + + const kept = (sides) => { + const parsed = withoutProseConflicts(parseConflicts(merged), merged, sides) + return parsed.segments.filter((s) => s.type === 'conflict') + } + + it('keeps only the conflict git actually wrote', () => { + const conflicts = kept({ ours, theirs }) + expect(conflicts).toHaveLength(1) + expect(conflicts[0].ours).toEqual(['setting = ours']) + }) + + it('leaves the prose block in the file, markers and all', () => { + const parsed = withoutProseConflicts(parseConflicts(merged), merged, { ours, theirs }) + expect(sidesFromConflicts(parsed).ours).toBe(ours) + }) + + // No index — a mergetool run by hand — means nothing to check against, and a + // guess here would drop a real conflict. + it('changes nothing when the sides are not available', () => { + expect(kept({ ours: '', theirs: '' })).toHaveLength(2) + expect(kept({})).toHaveLength(2) + }) + + // Only THEIR side has the document; ours never had it. Still prose. + it('accepts a block that only one side carries', () => { + expect(kept({ ours: 'unrelated\n', theirs })).toHaveLength(1) + }) +})