'Review Diff' on merge proposal page can be out-of-date

So, there's always lp:mad to automate the updates of the review diff, but there are no immediate plans to have Launchpad make a moving diff of both branches. As you can imagine, that can require a lot of heavy computational work if it was done on all launchpad projects.

On Fri, Mar 6, 2009 at 10:02 AM, Paul Hummer <email address hidden> wrote:
> So, there's always lp:mad to automate the updates of the review diff,
> but there are no immediate plans to have Launchpad make a moving diff of
> both branches.  As you can imagine, that can require a lot of heavy
> computational work if it was done on all launchpad projects.
>

As a compromise, we could have a "regenerate diff" button, which
wouldn't be too hard to hack.

But we have a lot of other things to do right now.

jml

I think this happened and confused John in <https://code.edge.launchpad.net/~mbp/bzr/340352-rename-lock/+merge/4334>

> No one should review a branch without really getting it, running its tests, exercising the changes, etc.

That's a per-project policy; it might be true for Launchpad but many projects do review just based on the diff.

As another hack, you could show the time the diff was created or something, or the revisions in the relevant branches that were used. It's probably not worth it compared to just eventually fixing this.

I just noticed this again in https://code.edge.launchpad.net/~edwin-grubbs/bzr/bug-384158-passing-body-to-mutt/+merge/7142 where, suprisingly to me, the list of to-be-merged revisions shown above the diff is inconsistent with what's in the diff.

On Mon, 2009-06-08 at 23:35 +0000, Martin Pool wrote:
> I just noticed this again in https://code.edge.launchpad.net/~edwin-
> grubbs/bzr/bug-384158-passing-body-to-mutt/+merge/7142 where,
> suprisingly to me, the list of to-be-merged revisions shown above the
> diff is inconsistent with what's in the diff.

It seems to me that removing the diff if the source branch changes
would lead to less confusion; an incorrect diff is worse than a
missing one.

Obviously updating the diff would be more useful, but if that isn't
going to happen the above would be a better compromise in my opinion.

Thanks,

James

This is biting us on many reviews, so I think its priority needs to be reconsidered. Having a correct diff is pretty fundamental to doing reviews on the web.

Getting more important with more users and UI confusion

I think the right thing is to make resubmit nicer.

Review diffs are meant to be static. They reflect the change that you asked to be merged. That ensures that everyone discussing the change is discussing the same thing.

2009/6/11 Aaron Bentley <email address hidden>:
> I think the right thing is to make resubmit nicer.
>
> Review diffs are meant to be static.  They reflect the change that you
> asked to be merged.  That ensures that everyone discussing the change is
> discussing the same thing.

I wonder if that's a different or larger bug.

It's true that nontrivial proposals may go through several
submissions. Ideally you'd see them all linked together, so you can
see the history of conversation, but also, if you want, see which
particular patch people were talking about with every comment.

However, in practice, I'm not sure that's highly needed. Normally the
updates are fairly small, and it's clear from the context or the
quoted text what people were talking about at any time. Also,
typically the patch is additive: if someone asks for fix X and you see
the patch now fixes X it's only of slight interest most of the time to
see what it was like before.-

For really long running patches or ones where somebody takes a step
back and does a completely new version it may be more complicated, but
it may be enough to just let people make a new proposal in that case.

So, maybe eventually you want the review comments to say eg

 On 2009-06-04 John wrote (_about an older version of this proposal_)

with a link.

--
Martin <http://launchpad.net/~mbp/>

One thing I like about the MOTU REVU system (http://revu.ubuntuwire.com/) is that the submitter can make changes, and you can see and comment on any of the full record of submissions. There are easy links to diff between them. That's sort of what I'd like to see in Launchpad merge proposals - at the moment it's unclear exactly which revision a comment is about, and what changes might have been made since.

https://code.edge.launchpad.net/~spiv/bzr/stacking-friendly-revision-history-verb/+merge/7314
is a kind of interesting contrary case because there spiv has made
later changes on that branch and he specifically doesn't want them
reviewed or merged yet.

(The answer may be 'don't do that.')

--
Martin <http://launchpad.net/~mbp/>

When someone makes a comment on a diff, they're clearly talking about that particular diff. So, if the "latest" diff on the review page gets updated, old comments could be automatically annotated to say that they were reactions to an older version of the diff (a link to which would be provided).

That way the diff displayed is always the most recent, but the history is still there and there's no confusion about which comments are about which iterations of the diff. (The fact that the historical diffs are slightly less convenient to reach is okay, because in practice it's very rare to need to see them anyway. It's enough to know that a comment is about an older diff; one doesn't actually have to *see* the older diff.)

Reviews actually have a pretty nice system by which they can be superseded and have back/forward links to the other versions of that submission. (You could do more, but the basic structure is good.)

So maybe this should fit in with that somehow. Perhaps there is a user choice as to whether these are small changes and stay in the same review, or large changes that need a new review to start. In practice this distinction exists at the moment, except that in the first case the changes are not visible at all.

A merge proposal already has an 'approved revision'. If this could be updated during the review process, this would solve part of the problem. Ideally, it could be set to 'head' which seems to be what most merge proposals want.

Do we actually know if regenerating diffs for in progress merge proposals requires more resources that we can allocate? I don't know how many commits we expect to branches with open merge proposals or how much effort it takes to generate a diff whenever this happens.

https://bugs.edge.launchpad.net/launchpad-code/+bug/392343/comments/1 documents the work-around for this.

The plan is to use the Preview diff as the review diff, and have Launchpad update that when the branch is pushed.