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) + }) +})