It is not obvious to users how to cause a proposal that was created without a prerequisite branch to become a proposal with a prerequisite branch

Bug #596796 reported by Steve Kowalik
18
This bug affects 2 people
Affects Status Importance Assigned to Milestone
Launchpad itself
Triaged
Low
Unassigned

Bug Description

Original request: I have an MP I'd like to change to depend on an pre-req branch, and I can't edit the MP to do so. This should be possible, and then should tag the diff so it gets regenerated.

Based on code team feedback:
When a user wants to change an MP to have a different prerequisite branch than it was originally filed with, they should be able to see how to do that very easily (and it will probably be implemented by causing a resubmit).

Tags: lp-code
Revision history for this message
Aaron Bentley (abentley) wrote :

We don't want to allow adding a prerequisite branch to an existing merge proposal, because adding (or removing, or changing) a prerequisite branch changes the meaning of the merge proposal, and therefore invalidates any reviews or approval that may have been done.

Instead, we want to support using resubmit to change the involved branches. See bug #504369.

Changed in launchpad-code:
status: New → Invalid
Revision history for this message
Robert Collins (lifeless) wrote :

Aaron, i don't follow the logic here. Adding a preqrequisite branch doesn't change the meaning of an existing proposal, and it doesn't invalidate reviews or approval either - can you explain why you think it does that?

Revision history for this message
Aaron Bentley (abentley) wrote :

The changes introduced in a prerequisite branch are not in the scope of a code review. This is why they are not shown in the diff.

If you could change/add/remove the prerequisite branch, you would be changing the scope of the code review. By deleting a prerequisite branch, you could retroactively cause an approval of changes that the reviewer had never seen. By adding a merge proposal, you could remove changes that the reviewer might consider vital.

Revision history for this message
Martin Pool (mbp) wrote : Re: [Bug 596796] Re: Can't add an pre-req branch to an existing MP

On 24 June 2010 00:28, Aaron Bentley <email address hidden> wrote:
> The changes introduced in a prerequisite branch are not in the scope of
> a code review.  This is why they are not shown in the diff.
>
> If you could change/add/remove the prerequisite branch, you would be
> changing the scope of the code review.  By deleting a prerequisite
> branch, you could retroactively cause an approval of changes that the
> reviewer had never seen.  By adding a merge proposal, you could remove
> changes that the reviewer might consider vital.

It seems to me Launchpad does not have a very clear position on
whether mps are meant to be immutable after creation, or not. You can
change some data fields such as the commit message, and you can change
the code by pushing more. However you can't change the prereq branch.
 I don't understand why the former ought to be permitted and not the
latter.

I think making drastic changes once the code is under review would be
rude or annoying but that might be better handled socially.

If the mp needs to be locked because for example it's approved for
automatic landing, perhaps all changes should be locked out. But for
something still undergoing discussion that seems unnecessary.

--
Martin

Revision history for this message
Robert Collins (lifeless) wrote :

On Thu, Jun 24, 2010 at 12:28 AM, Aaron Bentley <email address hidden> wrote:
> The changes introduced in a prerequisite branch are not in the scope of
> a code review.  This is why they are not shown in the diff.

Agreed,

> If you could change/add/remove the prerequisite branch, you would be
> changing the scope of the code review.  By deleting a prerequisite
> branch, you could retroactively cause an approval of changes that the
> reviewer had never seen.  By adding a merge proposal, you could remove
> changes that the reviewer might consider vital.

You can change the scope of a code review in arbitrary ways if there
is a pre-requisite branch at all.

That is, the change 'alter from prerequisite None to prerequisite
myhack' is equivalent to 'alter from prerequisite
myharmlessbranch@rev-GOOD to myharmlessbranch@rev-BAD'.

So we need to lock down/protect/whatever the revision of the
prerequisite branch rather than the branch that it happens to be
pointing at. We could lock down the branch it points at, but doing so
does not add security. And such a lock down makes it less obvious how
the user changes the setting (unless we have hidden actions - edit
the prerequisite branch -> does a resubmit').

So I'm going to reopen this, because I think its a good idea to make
it obvious to the user how to change the prerequisite branch. If the
code team really don't want a *UI* that lets the user discover how to
make it happen directly, please do close it again.

I am happy with any implementation: if clicking on the edit icon
results in a resubmit, thats fine - the user visible 'how do I make it
work' will be clear.

Details on the attack-without-changing-the-prerequisite setting
follow, in case I wasn't clear enough above.

For instance.
Take a project A
Submit an empty branch ('E') for landing in A
Submit another branch ('G') with good code dependendent on 'E'

Now, you can change the diff in 'G' by changing the content of 'E'.
You can push a 'small change' which is a merge from E containing a sec
vulnerability - for instance - and as long as you push the vuln to E
at the same time, the reviewer won't see it.

Changed in launchpad-code:
status: Invalid → New
summary: - Can't add an pre-req branch to an existing MP
+ It is not obvious to users how to cause a proposal that was created
+ without a prerequisite branch to become a proposal with a prerequisite
+ branch
description: updated
Aaron Bentley (abentley)
Changed in launchpad-code:
status: New → Triaged
importance: Undecided → Medium
Curtis Hovey (sinzui)
Changed in launchpad:
importance: Medium → Low
To post a comment you must log in.
This report contains Public information  
Everyone can see this information.

Duplicates of this bug

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.