Status Update
Comments
db...@google.com <db...@google.com> #2
da...@gmail.com <da...@gmail.com> #3
1. The move API does not change the parent, i.e. does not rebase the change onto its destination. In most cases this is obvious in the UI because there is a merge conflict and the change needs to be manually rebased on the new destination branch.
2. Submitting a change with parents that are not in the target branch. This *should* be already prevented by the option that was introduced by Sasa some time back. If that option was enabled, and it still was possible to submit, then it's a bug.
ek...@google.com <ek...@google.com> #4
Isn't this option just about preventing upload of such changes (and not about submit if such a change already exists)?
sv...@axis.com <sv...@axis.com> #6
Even the result of step 2 is counter intuitive to me. Is there any other case where you can have a stack of changes and the individual changes have different target branches? If you explicitly ask for change c2 to move you most likely want to move the entire chain, otherwise you would cherry-pick it (if there were other unmerged changes in the stack not yet merged into the target of the move).
lo...@google.com <lo...@google.com> #7
db...@google.com <db...@google.com> #8
Yes: merge commits.
B1--X--Y--M
B2--Z----/
B1 and B2 are branch tips. X, Y are open changes on B1; Z is an open change on B2. M is on B1.
If X, Y, Z, and M are all submittable (CR+2), then what happens when you submit M?
I would guess (but I haven't verified) that it submits all of (X, Y, Z, M). If that's true, then what is different between this case and the scenario in the original report?
(One difference may be that the UI is unclear about which branches are affected, but building the optimal, least-surprising UI is separate from defining what the backend does when submitting.)
sv...@axis.com <sv...@axis.com> #9
Code Review - Error
Failed to submit 3 changes due to the following problems:
Change 333115: depends on change that was not submitted
So it seems like that assumption was erroneous, at least for 2.15.
> (One difference may be that the UI is unclear about which branches are affected, but building the optimal, least-surprising UI is separate from defining what the backend does when submitting.)
I'm not suggesting this should be fixed in the UI, IMO the "move change" feature does a lot more than what is advertised by its innocuous sounding name.
Here's another example of what you can do with "move change" by moving 797f2bb to master (if you can avoid merge conflicts).
* 56e892c (origin/master) Merge "Innocent documentation change" [Gerrit]
|\
| * 79f5755 Innocent documentation change [User]
| * 797f2bb (origin/other) More feature on other [User]
| * 939ca12 Feature on other [User]
* | b47a28c
I don't even like the current implementation if it only concerns a single change, but when there's a whole stack involved it starts to get really counter-intuitive for me.
IMO two cases would fix the issues and make more sense.
1. Move the change by attempting to cherry-pick it to move-target.
2. Move the entire stack by attempting a rebase on top of move-target
Disallowing to move change if it is dependent on un-merged changes or moving the entire chain of changes to move-target would still cause previous-target to be merged into move-target, without any advertisement, when the moved change(s) is/are merged.
A merge-commit has the advantage that when you review it you are fully aware that it is a merge commit and you should check that the parents are something that should get merged into target as part of the review. In the move change case it is far from apparent what the result of a submit might be to the average Gerrit user.
db...@google.com <db...@google.com> #10
Sorry, I think I constructed an example that was not analogous to your original example. Your original example had "3. Merge change C1 into branch X".
So in the merge example, try submitting Z into B2, then submit M into B1. I am 99% sure this is supposed to work. This is the intended workflow for submitting a merge of a side branch into the main branch.
The check that the submit code is doing for each ancestor Q of the target change is:
1. Is Q submitted into *some branch*? If so, don't block submission.
2. Otherwise, is Q an open, submittable change on *the destination branch*? If so, submit this change along with the target change.
Again, applying this algorithm is what supports the workflow of merging a side branch. I believe this algorithm also explains the behavior you're seeing in the original report.
So: how would you change this algorithm so that it continues to support the merge case, but provides the behavior you expect in the non-merge move case?
da...@gmail.com <da...@gmail.com> #11
Or, somehow remind the user that moving the change doesn't rebase it? I know we have the warning on the dialog in polygerrit, but maybe we can expand the "change was moved" message, or pop up a dialog?
sv...@axis.com <sv...@axis.com> #12
My case is this:
* In the merge-commit case you are reviewing a merge commit, which has the explicit implication that submit would introduce a whole branch into target. It is therefore natural that you review the state of the branch that gets merged into target an you also have the opportunity to review what is introduced by the submit in the diff-view of the merge-commit change.
* In the moved-change case you are *not* reviewing a merge, you are reviewing the tip of a branch that, if the moved change is submitted, will get merged into target by the merge performed by the merge-strategy. There's no easy, straight-forward, way to discern what the submit will actually introduce beyond the diff of the latest patch-set of the change, which is not the entire truth.
To make the comparison analogous you would need to have "Move Change" actually attempt to merge the moved-change into target and create a change for the merge as well, which IMO wouldn't be the best way but it would be better than the current behavior since the effect of submitting the change would be obvious.
I don't think making it work-in-progress would help. In my experience the 'mover' would simply start the review and think no more of it. I'm afraid even a warning message would be ignored by most users.
My concern is that you have less control over what gets introduced to a branch and by whom.
Let's pretend that a company needs to be SOX compliant for business purposes and they are writing some sort of in-house accounting software. They are using Gerrit since it allows for the traceability that is required by SOX.
Should our recommendation be: "Don't use Gerrit because it has the 'Move Change' feature which wrecks traceability'"?
IIUC the use-case you are after with "Move Change" is when moving the change will not have the behavior described be me, so why not just:
tryMoveChange(change, target):
if isMergedInto(change.parent target):
moveChange change
else:
cherryPick change
Or if we want to have the feature move all open changes that 'change' depends on:
moveChange(change, target):
change_list <= getOpenChanges(change)
move_target <= target
for change in change_list.reverseOrder:
tryMoveChange(change, move_target)
move_target <= change
sv...@axis.com <sv...@axis.com> #13
db...@google.com <db...@google.com> #14
Ok, so IIUC you're saying that it's ok to leave the existing submit behavior, which would still have the problem you point out, but we would just change the implementation of the Move endpoint to avoid putting us in this situation. Fair enough. IMO ideally we would still change both, for defense in depth, but I could live with just changing Move.
This sounds reasonable to me but I honestly don't know if it will work for Martin's original use case. I can also see the argument that it should fail instead of cherry-picking, since the whole point of attempting to move in the first place is to preserve review history.
db...@google.com <db...@google.com> #15
Oh, I think this addresses the point about preserving review history. I would probably describe this as "move and change parent" rather than "cherry-pick".
sv...@axis.com <sv...@axis.com> #16
Failing works for me, my problem is with silently moving the commits the change is depending on to the move-target without further warning.
Exactly, "cherry-pick" was an unfortunate choice of word from my side, true for the latest patch-set but not for the change.
mf...@codeaurora.org <mf...@codeaurora.org> #17
I believe the issue should be dealt with by providing information to the user on a change when it is not based on a change currently "on" or "destined for" the current destination of the change, and by potentially providing a config option (by project/branch likely) to elect to prevent submit in this situation. Such a solution is more robust because it will also work for changes that got in this situation by other means then "move".
With the approach proposed above, it the config is enabled to block submit and a user moves a change to a branch to trigger this situation, the change will be blocked from being submitted. The user then has the opportunity to rebase the change using Gerrit, and if that fails they have the option of manually upload a rebased patchset if needed. Other changes in this situation (not as a result of move) can also be fixed this way.
However, with the alternative of rebasing the change on move, changes risk being stuck if the rebase fails and needs manual intervention. If user rebases the change manually on the new proposed destination and pushes it back for review to the old destination, it will likely get blocked on upload because it now contains history from the new proposed destination branch that is not on the old destination branch. This will likely make most changes "unmovable".
sv...@axis.com <sv...@axis.com> #18
Yes, but the commits that were excluded from target due to the force-push would also need to still be included in a different branch for the change to be submitable into target.
Am I really the only one bothered by the fact that we have a feature, available to any user, that you just correctly compared with the effects of a force-push?
We did not have this issue before "Move Change" since we don't allow force-push on production branches.
I have not, however, found any to disable the Move Change feature to prevent getting un-reviewed un-intentional content into the same production branches.
> This will likely make most changes "unmovable".
Probably true for any given change but I'm not so sure that it's correct for changes that are actually "move-candidates".
As long as the issue is fixed I'm all for it, as it is today we'll have to fork Gerrit to remove the "Move Change" feature in order to prevent disasters.
da...@gmail.com <da...@gmail.com> #19
bo...@gmail.com <bo...@gmail.com> #20
lu...@gmail.com <lu...@gmail.com> #21
This problem is still creating trouble right now, see [1].
This is a P1 raised in 2018, we are 5 years after the issue and this is still open. Can we escalate the priority to P0? My 2c: the "Move Change" without a "Rebase" does not really make sense at all. Sven has even forked Gerrit to prevent disasters to happen.
I propose to either FIX or DISABLE the feature altogether right now. It does not make sense to keep a time bomb in our code, isn't it?
My proposed fix options:
- "Move Change" API also triggers a rebase on the new target
- Submission of a "Moved Change" is blocked until there is at least one rebase after the move
@Sven @Ponch what do you think?
[1]
sv...@axis.com <sv...@axis.com> #22
Just semantics but since (IIRC) you can only move 1 change at the time, it would make sense to cherry-pick:
1. Upload a new patch-set to the change that is a cherry-pick on top of move-target.
2. Change parent to move-target.
po...@gmail.com <po...@gmail.com> #23
Can we escalate the priority to P0?
As of today, we should be able to prevent the issue at push time, if receive.rejectImplicitMerges
My proposed fix options:
"Move Change" API also triggers a rebase on the new target Submission of a "Moved Change" is blocked until there is at least one rebase after the move
Both sound good to me (1 being preferable). Definitively they will give more visibility on what it going on. I wonder if
Description
*****************************************************************
***** *****
***** !!!! THIS BUG TRACKER IS FOR GERRIT CODE REVIEW !!!! *****
***** *****
***** DO NOT SUBMIT BUGS FOR CHROME, ANDROID, CYANOGENMOD, *****
***** INTERNAL ISSUES WITH YOUR COMPANY'S GERRIT SETUP, ETC.*****
***** *****
***** THOSE ISSUES BELONG IN DIFFERENT ISSUE TRACKERS *****
***** *****
*****************************************************************
Affected Version:
v2.15.3
What steps will reproduce the problem?
1. Add two dependent changes for branch X C1, C2
(X)
(Y)
2. Move change C2 to branch Y
(X)
(Y)
3. Merge change C1 into branch X
(Y) (X)
What is the expected output?
Not permitted since C2 is dependent on C1 that is not merged into Y.
What do you see instead?
C2 is merged into Y and silently brings along C1.
Now C1 is merged into Y without ever being reviewed for Y.
Please provide any additional information below.
Moving change C2 could move entire stack of changes (C1,C2) to Y or cherry-pick C2 onto Y.