Page MenuHomePhabricator

Side-by-side diff for source code is noisier than other tools
Closed, ResolvedPublic

Description

The side-by-side diff is sometimes very bad at matching up unchanged lines. In particular, indentation changes really screw with things.

Sourcegear's Diffmerge is a desktop application I use locally, and it usually shows a better side-by-side diff for the same review. This gives me hope that Phabricator can do better.

Strangely, the yellow bar in the margin -- the one that indicates which lines were copied -- seems to do a better job than the actual diff highlighting. Maybe I'm imagining things?

Where in the code should I look if I want to mess with the side-by-side diff algorithm?

Event Timeline

cakoose created this task.Dec 19 2014, 8:11 PM
cakoose updated the task description. (Show Details)
cakoose raised the priority of this task from to Needs Triage.
cakoose added a subscriber: cakoose.
epriestley triaged this task as Wishlist priority.Dec 19 2014, 8:13 PM
epriestley added a subscriber: epriestley.

We use diff, git diff, etc. We do not implement our own diff algorithm.

qgil added a subscriber: qgil.Dec 19 2014, 8:16 PM

Are you talking about the diffs in Differential only (for source code) or also the diffs for task descriptions and comments in Maniphest?

(Those diffs are also generated by diff, FWIW.)

T3353 has some discussion of implementing human-text (vs source code) diffing, which would improve the diffs for things like Phriction pages and description edits. This is likely not particularly difficult, but complex and hard to prioritize.

qgil added a comment.Dec 19 2014, 9:07 PM

For what is worth, a similar task was created in https://phabricator.wikimedia.org/T78824 a couple of days ago -- contains screenshots comparing the results with wdiff.

qgil added a subscriber: tgr.Dec 19 2014, 9:08 PM

How do you decide how to apply light-green vs dark-green highlighting? Is that something diff gives you?

And here's a simple diff that Phabricator displays poorly and some other tools display well:

  • Phabricator:
  • BitBucket:
  • Diffmerge:
  • IntelliJ:

With more complicated diffs, the difference is more pronounced. I end up having to arc patch so I can use Diffmerge locally to actually see what's going on.

cakoose renamed this task from Side-by-side diff is noisier than other tools to Side-by-side diff for source code is noisier than other tools.Dec 19 2014, 9:14 PM

Also, Python's built-in difflib also does the right thing (at least for this small example).

Where's the code that decides how to highlight columns?

We compute intraline diffs ourselves, but it's based on the output of the underlying diff tool. All of the other examples have a "better" base diff for this example. We're basically just using the output from git diff or diff in this case:

$ diff -u old new 
--- old	2014-12-19 13:30:13.000000000 -0800
+++ new	2014-12-19 13:30:38.000000000 -0800
@@ -1 +1,6 @@
-print "main program"
+def main():
+    print "main program"
+
+if __name__ == "__main__":
+    main()
+

Assuming that screenshot is from Diffusion, note that we use diff -bwu (or similar) in Differential (at least, by default), which produces a diff more similar to what you're looking for:

We don't ignore whitespace by default in Diffusion because the default configuration of Phabricator uses Differential as the primary code review application, and Diffusion more as a reference. T3498 discusses expanding the options in Diffusion.

chad added a subscriber: chad.Dec 19 2014, 9:47 PM
This comment was removed by chad.

Haha, noobs calling everything "Phabricator".

The screenshot is from Differential, not Diffusion, but I created the diff by copy/pasting the output from diff -u. How did you create yours?

diff -bwu shows something similar to BitBucket's output, but I actually prefer Diffmerge and Intellij -- show whitespace changes but only highlight the parts of the line that have changed.

I don't think the fact that the input comes from diff is a big barrier.

  1. You could still apply a smarter diff algorithm to each individual hunk. That would handle this example better, and probably many other common ones, too.
  2. If the backing repo is available, you could apply the patch and then run a better diff tool. The cost of doing this is probably similar to the cost of doing syntax highlighting, maybe?

Yours is called .py so it gets special rules:

https://secure.phabricator.com/book/phabricator/article/differential_faq/#what-do-the-whitespace-o

If you select "Ignore All", or name it something other than .py, it will diff like mine.

Long ago we did not apply special rules to Python, but there was a great deal of grumbling and complaining and assertion that .py files must always be diffed with all whitespace preserved exactly and any other approach was wrong, so now we do it that way.

This is configurable with differential.whitespace-matters.

Been poking around at the code. The following function looks promising: https://secure.phabricator.com/diffusion/P/browse/master/src/applications/differential/parser/DifferentialHunkParser.php;a366f85c118bf6b857155792446c397dac03e0a3$229

It looks like this function could be modified to align the lines of a hunk to match up better, maybe by running a better diff algorithm first. Does that seem like the right approach? Is there a better place to hook into the diff pipeline?

I've identified that, at least for the diffs used for Maniphest task description changes, a significant part of the noise is caused by the 80 character line wrapping. Which seems to be applied to both old and new content *before* feeding it to the difference engine.

Example: https://phabricator.wikimedia.org/T94149

Note how the word "tests" is broken into tes ts in the before view, and test s in the after view. It's seeing changes to content that didn't at all change. If anything, it should perform such line breaking afterwards. Though ideally it would just leave it to the client instead with natural wrapping through the browser.

See T3353 for discussion of code vs text diffs.