Page MenuHomePhabricator

Unified diffs could do a better job of rendering omitted trailing newlines
Open, WishlistPublic

Description

See PHI1839. When rendering a change like this in 1-up mode:

diff --git a/both-newline.txt b/both-newline.txt
index 7c01088..c420473 100644
--- a/both-newline.txt
+++ b/both-newline.txt
@@ -1,2 +1,2 @@
-both newline
+both newline 2
 
diff --git a/new-newline.txt b/new-newline.txt
index 91a9f22..485061c 100644
--- a/new-newline.txt
+++ b/new-newline.txt
@@ -1 +1 @@
-new-newline
\ No newline at end of file
+new-newline 2
diff --git a/no-newline.txt b/no-newline.txt
index 20cbb4d..5b9b97e 100644
--- a/no-newline.txt
+++ b/no-newline.txt
@@ -1 +1 @@
-no newline
\ No newline at end of file
+no newline 2
\ No newline at end of file
diff --git a/old-newline.txt b/old-newline.txt
index b836bf1..44b5d05 100644
--- a/old-newline.txt
+++ b/old-newline.txt
@@ -1,2 +1 @@
-old newline
-
+old newline 2
\ No newline at end of file

...the behavior after D21433 is:

Screen Shot 2020-08-05 at 9.43.24 AM.png (549×327 px, 35 KB)

This has several minor issues:

  • The "both-newlines.txt" file renders an unchanged newline line at the end of the file (the line that is both old line 2 and new line 2). This is accurate, but unnecessary (its existence is implied by the omission of the "No newline at end of file" text).
  • The "no-newline.txt" file renders two lines representing the missing newline (one is old line 2, the other is new line 2). This is accurate -- or, at least, not especially misleading -- but this is inconsistent with how changes are normally rendered. The rendering I would expect is a single line which is marked as both old line 2 and new line 2.
  • The "old-newline.txt" file renders an empty removed line in the old file and an annotated "grey" line in the new file. This is accurate, although it might be better to put greater emphasis on the removal of the line (e.g., clearly show that the "no newline at end of file" attribute is new), and showing the removed old line isn't strictly necessary (it is implied by the annotation).

Event Timeline

epriestley triaged this task as Wishlist priority.Aug 5 2020, 5:07 PM
epriestley created this task.