Page MenuHomePhabricator

Put some whitespace behaviors back, but only for "diff alignment", not display

Authored by epriestley on Feb 16 2019, 4:31 PM.



Depends on D20185. Ref T13161. Fixes T6791.

See some discusison in T13161. I want to move to a world where:

  • whitespace changes are always shown, so users writing YAML and Python are happy without adjusting settings;
  • the visual impact of indentation-only whitespace changes is significanlty reduced, so indentation changes are easy to read and users writing Javascript or other flavors of Javascript are happy.

D20181 needs a little more work, but generally tackles these visual changes and lets us always show whitespace changes, but show them in a very low-impact way when they're relatively unimportant.

However, a second aspect to this is how the diff is "aligned". If this file:

A changed to this file:


...diff tools will generally produce this diff:

+ X
+ Y
+ Z

This is good, and easy to read, and what humans expect, and it will "align" in two-up like this:

       1 X
1 A    2 A
       3 Y
       4 Z

However, if the new file looks like this instead:


...we get a diff like this:

- A
+ X
+ A'
+ Y
+ Z

This one aligns like this:

1 A
        1 X
        2 A'
        3 Y
        4 Z

This is correct if A and A' are totally different lines. However, if A' is pretty much the same as A and it just had a whitespace change, human viewers would prefer this alignment:

        1 X
1 A     2 A'
        3 Y
        4 Z

Note that A and A' are different, but we've aligned them on the same line. diff, git diff, etc., won't do this automatically, and a .diff doesn't have a way to say "these lines are more or less the same even though they're different", although some other visual diff tools will do this.

Although diff can't do this for us, we can do it ourselves, and already have the code to do it, because we already nearly did this in the changes removed in D20185: in "Ignore All" or "Ignore Most" mode, we pretty much did this already.

This mostly just restores a bit of the code from D20185, with some adjustments/simplifications. Here's how it works:

  • Rebuild the text of the old and new files from the diff we got out of arc, git diff, etc.
  • Normalize the files (for example, by removing whitespace from each line).
  • Diff the normalized files to produce a second diff.
  • Parse that diff.
  • Take the "alignment" from the normalized diff (whitespace removed) and the actual text from the original diff (whitespace preserved) to build a new diff with the correct text, but also better diff alignment.

Originally, we normalized with diff -bw. I've replaced that with preg_replace() here mostly just so that we have more control over things. I believe the two behaviors are pretty much identical, but this way lets us see more of the pipeline and possibly add more behaviors in the future to improve diff quality (e.g., normalize case? normalize text encoding?).

Test Plan

(Also, fix a unit test.)

Diff Detail

rP Phabricator
Automatic diff as part of commit; lint not applicable.
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

epriestley created this revision.Feb 16 2019, 4:31 PM
epriestley requested review of this revision.Feb 16 2019, 4:32 PM

Builds depend on D20182.

(The actual rendering of that new diff is still a little bit sketchy since some of the pixel alignment isn't quite right and I want to tweak the colors, but all the elements are in the right places.)

amckinley accepted this revision.Feb 19 2019, 9:06 PM
This revision is now accepted and ready to land.Feb 19 2019, 9:06 PM

...since some of the pixel alignment isn't quite right

I think the updated version of D20181 fixed the primary case here.

This revision was automatically updated to reflect the committed changes.