From b738929195b3be045240d303fb00e0fa32f425c4 Mon Sep 17 00:00:00 2001 From: Shannon Booth Date: Sun, 10 Sep 2023 18:49:05 +1200 Subject: [PATCH] LibDiff: Fix wrong index used when prepending context lines `i` is used as the index for 'old lines' in diff generation, not 'new lines'. Using the wrong index would mean that for certain diffs the prefixed context information would have wrong content, and could even result in a crash. Fix this, and add a test for an input which was previously crashing. --- Tests/CMakeLists.txt | 1 + Tests/LibDiff/CMakeLists.txt | 7 ++ Tests/LibDiff/TestDiff.cpp | 108 +++++++++++++++++++++++ Userland/Libraries/LibDiff/Generator.cpp | 2 +- 4 files changed, 117 insertions(+), 1 deletion(-) create mode 100644 Tests/LibDiff/CMakeLists.txt create mode 100644 Tests/LibDiff/TestDiff.cpp diff --git a/Tests/CMakeLists.txt b/Tests/CMakeLists.txt index 7176912e1fd..c8b3c504223 100644 --- a/Tests/CMakeLists.txt +++ b/Tests/CMakeLists.txt @@ -5,6 +5,7 @@ add_subdirectory(LibC) add_subdirectory(LibCompress) add_subdirectory(LibCore) add_subdirectory(LibCpp) +add_subdirectory(LibDiff) add_subdirectory(LibEDID) add_subdirectory(LibELF) add_subdirectory(LibGfx) diff --git a/Tests/LibDiff/CMakeLists.txt b/Tests/LibDiff/CMakeLists.txt new file mode 100644 index 00000000000..d1b7cdb562a --- /dev/null +++ b/Tests/LibDiff/CMakeLists.txt @@ -0,0 +1,7 @@ +set(TEST_SOURCES + TestDiff.cpp +) + +foreach(source IN LISTS TEST_SOURCES) + serenity_test("${source}" LibDiff LIBS LibDiff) +endforeach() diff --git a/Tests/LibDiff/TestDiff.cpp b/Tests/LibDiff/TestDiff.cpp new file mode 100644 index 00000000000..1ccfe84b815 --- /dev/null +++ b/Tests/LibDiff/TestDiff.cpp @@ -0,0 +1,108 @@ +/* + * Copyright (c) 2023, Shannon Booth + * + * SPDX-License-Identifier: BSD-2-Clause + */ + +#include +#include +#include +#include + +TEST_CASE(test_generate_unified_diff) +{ + StringView old_text = R"(Viewport <#document> at (0,0) content-size 800x600 children: not-inline + BlockContainer at (0,0) content-size 800x600 [BFC] children: not-inline + BlockContainer at (8,8) content-size 784x150 children: not-inline + BlockContainer <(anonymous)> at (8,8) content-size 784x0 children: inline + TextNode <#text> + TextNode <#text> + BlockContainer
at (8,8) content-size 784x150 children: inline + line 0 width: 300, height: 150, bottom: 150, baseline: 150 + frag 0 from SVGSVGBox start: 0, length: 0, rect: [8,8 300x150] + TextNode <#text> + SVGSVGBox at (8,8) content-size 300x150 [SVG] children: inline + TextNode <#text> + Box at (8,8) content-size 0x0 children: inline + Box at (8,8) content-size 0x0 children: inline + TextNode <#text> + SVGGeometryBox at (92.375,26.75) content-size 131.25x112.15625 children: inline + TextNode <#text> + TextNode <#text> + TextNode <#text> + TextNode <#text> + +ViewportPaintable (Viewport<#document>) [0,0 800x600] + PaintableWithLines (BlockContainer) [0,0 800x600] + PaintableWithLines (BlockContainer) [8,8 784x150] + PaintableWithLines (BlockContainer(anonymous)) [8,8 784x0] + PaintableWithLines (BlockContainer
) [8,8 784x150] + SVGSVGPaintable (SVGSVGBox) [8,8 300x150] + PaintableBox (Box) [8,8 0x0] + PaintableBox (Box#braces) [8,8 0x0] + SVGGeometryPaintable (SVGGeometryBox) [92.375,26.75 131.25x112.15625] + +)"sv; + + StringView new_text = R"(Viewport <#document> at (0,0) content-size 800x600 children: not-inline + BlockContainer at (0,0) content-size 800x600 [BFC] children: not-inline + BlockContainer at (8,8) content-size 784x150 children: not-inline + BlockContainer <(anonymous)> at (8,8) content-size 784x0 children: inline + TextNode <#text> + TextNode <#text> + BlockContainer
at (8,8) content-size 784x150 children: inline + line 0 width: 300, height: 150, bottom: 150, baseline: 150 + frag 0 from SVGSVGBox start: 0, length: 0, rect: [8,8 300x150] + TextNode <#text> + SVGSVGBox at (8,8) content-size 300x150 [SVG] children: inline + TextNode <#text> + Box at (8,8) content-size 0x0 children: not-inline + TextNode <#text> + TextNode <#text> + +ViewportPaintable (Viewport<#document>) [0,0 800x600] + PaintableWithLines (BlockContainer) [0,0 800x600] + PaintableWithLines (BlockContainer) [8,8 784x150] + PaintableWithLines (BlockContainer(anonymous)) [8,8 784x0] + PaintableWithLines (BlockContainer
) [8,8 784x150] + SVGSVGPaintable (SVGSVGBox) [8,8 300x150] + PaintableBox (Box) [8,8 0x0] + +)"sv; + + auto result = MUST(Diff::from_text(old_text, new_text, 3)); + EXPECT_EQ(result.size(), 2U); + + auto hunk1_stream = make(); + MUST(Diff::write_unified(result[0], *hunk1_stream)); + + auto hunk1 = MUST(hunk1_stream->read_until_eof()); + EXPECT_EQ(StringView { hunk1 }, R"(@@ -10,12 +10,7 @@ + TextNode <#text> + SVGSVGBox at (8,8) content-size 300x150 [SVG] children: inline + TextNode <#text> +- Box at (8,8) content-size 0x0 children: inline +- Box at (8,8) content-size 0x0 children: inline +- TextNode <#text> +- SVGGeometryBox at (92.375,26.75) content-size 131.25x112.15625 children: inline +- TextNode <#text> +- TextNode <#text> ++ Box at (8,8) content-size 0x0 children: not-inline + TextNode <#text> + TextNode <#text> + +)"sv); + + auto hunk2_stream = make(); + MUST(Diff::write_unified(result[1], *hunk2_stream)); + + auto hunk2 = MUST(hunk2_stream->read_until_eof()); + EXPECT_EQ(StringView { hunk2 }, R"(@@ -26,6 +21,4 @@ + PaintableWithLines (BlockContainer
) [8,8 784x150] + SVGSVGPaintable (SVGSVGBox) [8,8 300x150] + PaintableBox (Box) [8,8 0x0] +- PaintableBox (Box#braces) [8,8 0x0] +- SVGGeometryPaintable (SVGGeometryBox) [92.375,26.75 131.25x112.15625] + +)"sv); +} diff --git a/Userland/Libraries/LibDiff/Generator.cpp b/Userland/Libraries/LibDiff/Generator.cpp index 1d789d15364..f1a39865ed5 100644 --- a/Userland/Libraries/LibDiff/Generator.cpp +++ b/Userland/Libraries/LibDiff/Generator.cpp @@ -90,7 +90,7 @@ ErrorOr> from_text(StringView old_text, StringView new_text, size_t for (size_t offset = 0; offset < available_context; ++offset) { size_t context_line = i + offset - available_context; - TRY(hunk.lines.try_append(Line { Line::Operation::Context, TRY(String::from_utf8(new_lines[context_line])) })); + TRY(hunk.lines.try_append(Line { Line::Operation::Context, TRY(String::from_utf8(old_lines[context_line])) })); } return {};