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

Bug #338002 reported by Jamu Kakar
76
This bug affects 9 people
Affects Status Importance Assigned to Milestone
Launchpad itself
High
Aaron Bentley

Bug Description

Today I created a merge proposal [1] for a Storm branch. James was
kind enough to review the branch for me and left a couple of review
comments that I implemented fixes for. I pushed new revisions, went
back to the merge proposal page to leave a comment about my changes
and noticed that the 'Review Diff' on the page was still showing the
original diff.

I figured it must be a latency issue, so I left my comment and
continue on.. After several hours, I checked the page again and was
surprised to see that the diff called 'Review Diff' was still
showing the original diff. I talked to Jonathan and Tim about it
and they explained that it was in fact expected behaviour. I think
it's actually misleading behaviour.

No one should review a branch without really getting it, running its
tests, exercising the changes, etc.; however, once this is done,
reviewing follow-on changes by looking at the diff on the web would
be convenient, but isn't possible with the current arrangement.
It's also misleading to web visitors to see a diff called 'Review
Diff' that isn't the diff that should actually be reviewed.

[1] https://code.edge.launchpad.net/~jkakar/storm/result-set-in-subselects/+merge/4147

Revision history for this message
Paul Hummer (rockstar) 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.

Changed in launchpad-bazaar:
importance: Undecided → Low
status: New → Triaged
Revision history for this message
Jonathan Lange (jml) wrote : Re: [Bug 338002] Re: 'Review Diff' on merge proposal page can be out-of-date

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

Revision history for this message
Martin Pool (mbp) wrote :

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.

Revision history for this message
Martin Pool (mbp) 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.

Revision history for this message
James Westby (james-w) wrote : Re: [Bug 338002] Re: 'Review Diff' on merge proposal page can be out-of-date

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

Revision history for this message
Martin Pool (mbp) wrote :

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.

Revision history for this message
Tim Penhey (thumper) wrote :

Getting more important with more users and UI confusion

Changed in launchpad-code:
importance: Low → High
tags: added: confusing-ui
Revision history for this message
Aaron Bentley (abentley) wrote :

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.

Revision history for this message
Martin Pool (mbp) wrote : Re: [Bug 338002] Re: 'Review Diff' on merge proposal page can be out-of-date

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/>

Revision history for this message
William Grant (wgrant) wrote :

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.

Revision history for this message
Martin Pool (mbp) wrote :

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/>

Revision history for this message
Karl Fogel (kfogel) wrote :

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.)

Revision history for this message
Martin Pool (mbp) wrote :

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.

Revision history for this message
Stuart Bishop (stub) wrote :

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.

Revision history for this message
Jonathan Lange (jml) wrote :
Tim Penhey (thumper)
Changed in launchpad-code:
assignee: nobody → Aaron Bentley (abentley)
milestone: none → 3.0
status: Triaged → In Progress
Revision history for this message
Aaron Bentley (abentley) wrote :

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

Tim Penhey (thumper)
Changed in launchpad-code:
status: In Progress → Fix Released
To post a comment you must log in.
This report contains Public information  Edit
Everyone can see this information.

Duplicates of this bug

Other bug subscribers

Bug attachments