Page MenuHomePhabricator

Obscure end-of-file changes may choke the changeset highlighter
Closed, ResolvedPublic

Description

See PHI1740. Here's a small reproduction case:

  • Create a revision with these two diffs.
  • Intradiff them.
Diff 1
commit 2715e92dd7b71db65639689932ef0a85ce9c9c2c
Author: epriestley <git@epriestley.com>
Date:   Tue May 19 08:07:55 2020 -0700

    WIP

diff --git a/no-newline.txt b/no-newline.txt
index 957fef4..5179482 100644
--- a/no-newline.txt
+++ b/no-newline.txt
@@ -1,4 +1,4 @@
 AAAA
-BBBB
+FFFF
 CCCC
-DDDD
\ No newline at end of file
+EEEE
Diff 2
commit 8392e163b94c3de4ffb6293372e69c741d803d65
Author: epriestley <git@epriestley.com>
Date:   Tue May 19 08:08:14 2020 -0700

    WIP

diff --git a/no-newline.txt b/no-newline.txt
index 957fef4..d48e540 100644
--- a/no-newline.txt
+++ b/no-newline.txt
@@ -1,4 +1,4 @@
 AAAA
-BBBB
+FFFF
 CCCC
-DDDD
\ No newline at end of file
+GGGG
\ No newline at end of file

I think the critical pieces are:

  • Both changes should affect the last line of the file and modify it to have different values.
  • The starting state of the file should have no trailing newline.
    • The first change should add a trailing newline to the file.
    • The second change should remove it ("not add a trailing newline").

This ultimately fails in some very old highlighting code in DifferentialHunkParser->parseHunksForHighlightMasks().

The issue is roughly that each file has 4 real lines so we compute 4 offsets. However, the "No newline at end of file" lines add additional visual lines which aren't counted through properly. Internally, the intradiff ends up having "+/-" lines (from the change to the last line of the file) after it passes through a \ line, and the counts are off.