1233076 - Mozreview's interdiff can get confused by rebases and should warn about this instead of potentially lying

Status: RESOLVED WONTFIX

(MozReview Graveyard :: Review Board: Upstream, defect)

Description

[:Gijs (he/him)]

Compare:

https://reviewboard.mozilla.org/r/28129/diff/1
https://reviewboard.mozilla.org/r/28129/diff/2
https://reviewboard.mozilla.org/r/28129/diff/1-2

Note how the .h file has a 1-line removal hunk in version 2 that does not exist in version 1. The interdiff, however, claims there were only whitespace changes to that file (!).

If it's not sure, it should tell you, rather than lying outright. :-)

Comment 1

[:Gijs (he/him)]

Needinfo'ing Steven at his request. :-)

Comment 2

[Steven MacLeod [:smacleod]]

noting other example of broken interdiff:
https://reviewboard.mozilla.org/r/25595/diff/1-3#3

Comment 3

[Steven MacLeod [:smacleod]]

While working on some of these interdiff bugs, particularly the work I've done in https://hellosplat.com/s/beanbag/tickets/4052/ so far, it's become apparent it will be difficult to make the interdiff filtering perfect. That being said, we can definitely do much better and fix a number of the current bugs.

Seeing as the bugs are quite difficult to fix though, we might want to consider a workaround until we've made the interdiff processing a little more reliable:

It should be possible to shut the interdiff rebase processing off completely. This would mean any changes to the files coming from a rebase will show up in the interdiff and be highlighted normally (the interdiff would be a straight diff between the file state from the first patch, to the second patch).

Since the processing works some of the time, I'm not going to go ahead and do this just yet, but it is possible if anyone would like us to move forward here.

Comment 4

[Mark Côté [:mcote]]

(In reply to Steven MacLeod [:smacleod] from comment #3)
> Seeing as the bugs are quite difficult to fix though, we might want to
> consider a workaround until we've made the interdiff processing a little
> more reliable:
> 
> It should be possible to shut the interdiff rebase processing off
> completely. This would mean any changes to the files coming from a rebase
> will show up in the interdiff and be highlighted normally (the interdiff
> would be a straight diff between the file state from the first patch, to the
> second patch).

We need to focus on general UX improvements right now, so we should disable the rebase processing right now (and clearly indicate that this is the case).  I get the feeling that the uncertainty is a bigger problem than including rebased changes.

CCing dbaron since he's encountered several of these misleading interdiffs.

Comment 5

[Masatoshi Kimura [:emk]]

The opposite direction of the breakage:
https://reviewboard.mozilla.org/r/66392/diff/1/
https://reviewboard.mozilla.org/r/66392/diff/2/
https://reviewboard.mozilla.org/r/66392/diff/1-2/
The interdiff displays gazillions of unrelated changes that make interdiff virtually useless.
Even if I back to Splinter, I can't stop others from using MozReview. Really annoying.

Comment 6

[Gregory Szorc [:gps]]

(In reply to Masatoshi Kimura [:emk] from comment #5)
> The opposite direction of the breakage:
> https://reviewboard.mozilla.org/r/66392/diff/1/
> https://reviewboard.mozilla.org/r/66392/diff/2/
> https://reviewboard.mozilla.org/r/66392/diff/1-2/
> The interdiff displays gazillions of unrelated changes that make interdiff
> virtually useless.
> Even if I back to Splinter, I can't stop others from using MozReview. Really
> annoying.

I have difficulty understanding this criticism.

First, interdiffs when rebases are present is a hard problem. You are trying to show the relationship between 4 file states in no more than 2 file views. This either involves diffing 2 commits verbatim (what MozReview currently does) or attempting a "smart" diff, which involves attempting to filter out unwanted changes. Of course this runs the risk of hiding info useful for reviewers ("oh, this function you are calling changed after rebase but you didn't update your code"). So really there is no perfect solution.

But the main reason I find it difficult to understand this criticism is that a) nobody is forcing you to use interdiffs b) interdiffs are broken in Splinter most of the time anyway. I can count on one hand the number of times I was able to successfully use an interdiff in Splinter. MozReview interdiffs, however, always render something. Best case they display exactly what I want. Worst case there is a lot of noise from a rebase and I have to look at the straight diff. But that's no different from looking at the diff the first time. So on average you come out ahead. A far cry from "virtually useless" if you ask me.