On 01/26/2016 07:49 PM, Karl Tomlinson wrote:
Boris Zbarsky writes:
On 1/23/16 9:48 PM, Mike Hommey wrote:
Note that if /other/ changes from other bugs have happened to the same
files between the last reviewed iteration and the rebase before landing,
the interdiff will show them without any kind of visual cues.
Ah, that's unfortunate.
I agree that this is a hard problem, though (interdiff across
rebases). I guess that does mean that you can't really use the
final attached thing for the "I want to see the interdiff" use
case; need to push something to MozReview before rebasing to
address that.
Yes, from the reviewing efficiency perspective, it is best to push
MozReview updates for re-review on the same base revision.
i.e. separate from the rebase/landing process.
That may be difficult to do in some cases, though it does seem useful to
keep in mind. If you happen to be using obsolescence markers, is this
fixable? As in, if you have a patch P based on upstream U:
U -> P
that you then amend
U -> P'
U -> [P] => P' # double arrow => shows successor relationship,
[brackets] show obsolete revs
and before re-uploading, rebase onto a new U'
U -> [P']
U -> [P] => [P']
[P'] => P''
U -> U' -> P''
then the interdiff you want is between P and P', which you can get by
looking at the re-uploaded patch (P'') and finding a precursor with a
parent rev matching the parent rev (U) of the previously uploaded patch (P).
Except you might have done it the other way, by rebasing first, in which
case you would have
U -> [P] => [P']
U -> U' -> [P']
[P'] => P''
U' -> P''
in which case you want the interdiff between P' and P''. I think there
you need to categorize successors as due to rebases vs due to changes,
and you can detect a rebase because the parent rev is different. So in
general, you ought to be able to look through the precursor chain to
find a non-rebase precursor, and use that as your interdiff? In this
case, the precursor of P'' is P', and parent(P'')=U' is parent(P') is
also U', so you use that diff. In the previous example, parent(P'')=U'
is not parent(P'), so that's a rebase, so then you ignore that change
and look at P' vs P. Their parents are the same, so you use them. And
last, if you re-upload the same patch, just rebased, then it would get
back to your original patch before finding a pair with the same parent,
and so the interdiff is empty.
I've *almost* managed to convince myself that I'm not completely crazy
with the above algorithm; have I convinced anyone else? Or maybe it's
just a well known fact that if you have precursor/successor relations,
then interdiffs are "solved"? It's never going to be 100% pretty,
because you may end up doing violence to a patch when resolving merge
conflicts from rebasing. Perhaps MR should show all precursor diffs, but
label the rebase ones as such?
_______________________________________________
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform
