[ralflang]

I looked into the test suites of Perl and python diff libraries to look
for edge cases we might want to cover.

At least the MappedDiff looks like it's broken

[github-actions]

🔍 CI Results

Overall: ✅ All 12 lanes passed

TL;DR: ✅ All clean: 88 tests (253 assertions) in 12 lanes, 25 files scanned, 56 files styled.

Summary by PHP Version

PHP	dev	stable
8.1	✅	✅
8.2	✅	✅
8.3	✅	✅
8.4	✅	✅
8.5	✅	✅
8.6	✅	✅

Quality Metrics

• PHPUnit: 88 tests (253 assertions) passed in 12 lanes ✅
• PHPStan: No errors found ✅ - 4 advisories at level 2
• PHP-CS-Fixer: 56 files checked, no issues ✅

---

CI powered by horde-components • View full results

[TDannhauer]

Some Review Findings:

he assertion that cannot fail
testBothSidesIdenticalChange is supposed to prove that the same edit on both sides does not conflict. It searches for the wrong marker:

ThreeWayTest.php
Ln 1773–1787

$this->assertStringNotContainsString(
'<<<,<<<<',
$merged,
'Identical changes on both sides must not produce a conflict'
);
The marker mergedOutput() writes is <<<<<<<. A real conflict between two copies of 2.txt still contains This line is different in 2.txt and does not contain <<<,<<<<, so both assertions pass whether the merge is clean or not. The other three-way tests in this PR use <<<<<<<.

Comments that no longer match the code
The file docblock on test/MappedDiffTest.php still says the constructor is a known PHP 8 bug and "must be fixed in production code." This commit is that fix. The tests assert success. That comment will send the next reader looking for a crash that is gone.

In NativeEngine::diff(), unset($this->seq) became $this->seq = null (same for in_seq and lcs), with a comment that unset is illegal because a subclass might add property hooks. On PHP 8.4 both forms succeed: unset leaves the typed property uninitialized, and the later $this->seq[0] = $yoff - 1 in _diag() initializes it, with no warning. These properties have no hooks, and a subclass cannot add hooks to them. Resetting with $this->seq = [] and $this->in_seq = [] would match how _diag() uses them.

The new $k = null plus is_int($k) guard is also narrower than the comment suggests. _diag() sets in_seq to [] before the loop, and the elseif runs only when in_seq[$y] is already set, which happens in _lcsPos() after $k is assigned. On that path $k is already a positive int, so the guard does not change results. The disabled assert($y <= $this->seq[$k]) (maintaina-com#12) is unchanged, and nothing new hits that elseif.

Tests that pass for a weaker reason
testColoredRendererContextLinesUncolored only rejects the substrings [lightgreen] unchanged and [lightred] unchanged. The renderer colors the whole added or deleted block in one call, so a context line inside that block would not have that exact prefix. The test still passes if context is colored.

testAutoEngineUsesXdiffWhenAvailable skips unless xdiff is loaded, then accepts any diff that contains an AddOperation. NativeEngine produces that too for an appended line, so the test does not show that 'auto' selected XdiffEngine.

EngineTest::testStringEngine() is still the pre-existing trap the new comment describes: expectException() makes every line after the headerless patch unreachable, including the context-patch assertions. testContextPatchRoundtrip covers that case separately. The old method still reports success without running the rest.

[TDannhauer]

FYI @ralflang