New issue
Advanced search Search tips

Issue 1107 link

Starred by 45 users

Issue metadata

Status: Released
Owner: ----
Closed: Sep 2016



Sign in to add a comment

Gerrit accepted and merged change Id's that had already been merged to a different branch

Reported by dree...@gmail.com, Aug 23 2011

Issue description

Affected Version:
2.2.1

What steps will reproduce the problem?
1. Branch B is created from Branch A
2. Branch A gets a new commit and is submitted to Gerrit with a new change ID (#123) and is merged
3. Branch B needs a code fix but the developer mistakenly starts with the Branch A code, then submits two commits to Gerrit Branch B (his new fix (#456) and ,erroneously, the recent Branch A commit along with it's Change ID (#123)) 
 

What is the expected output? 

Expected Gerrit to not accept the change since the Change ID (#123) was already merged but just on a different branch.

What do you see instead?

Gerrit took the Change on Branch A (#456). Didn't show the file diffs from change #123 but DID merge the changes in when submitted.  


Please provide any additional information below.

Gerrit should not accept the change since the new change branch had change id's that at already been merged BUT to a DIFFERENT branch.

 

Comment 1 by dree...@gmail.com, Aug 24 2011

This is a critical issue for us as it's caused code to make it into a branch that was un-intended and not at all obvious that it even happened.

Comment 2 by sop@google.com, Aug 24 2011

Status: AwaitingInformation
What was the commit relationship here?

In step 3 the developer uploaded a new change for review (you called this change ID #456). Was the parent commit SHA-1 for change #123 identical to the commit SHA-1 that was already merged into branch A?

Comment 3 by dree...@gmail.com, Aug 24 2011

Correct, so for example

Branch A = 66c1bbaa
Branch B = 66c1bbaa

then Branch A gets a new commit:
66c1bbaa -> ab8db2c7     (Gerrit change Id #123 to branch A)

then Branch B needs a new commit and it should look like this:
66c1bbaa -> be64dd43

but instead, Branch B looks like this
66c1bbaa -> ab8db2c7 -> be64dd43 (Gerrit change Id #456 to branch B)

because the developer made a mistake and submitted a Gerrit change that included commit ab8db2c7


Comment 4 by sop@google.com, Aug 24 2011

Labels: -Priority-Minor Priority-Critical
Status: Accepted
OK, I agree with you, this is a bug. The server should not have submitted be64dd43, as ab8db2c7 was not already merged into the destination branch B.
What is the expected behavior in this case?  Should Gerrit have made a new change  ab8db2c7 for Branch B and required it to be submitted before submitting be64dd43?

Comment 6 by dree...@gmail.com, Aug 24 2011

My preference would be (if it's possible/feasible to implement) have Gerrit reject the push of change Id #456 (be64dd43) with an error that says it had uncommitted change ID's pointing to a different branch

If thats not feasible or desired then what you stated would work for me but I'm not sure what you do with the change id numbers as the one on ab8db2c7 (#123) already exists.  Would it have to create a new change id and what does it do with the one in the commit message already?   
This bug is really killing us.  Our development shop does not code with java but I may need to find a java develop that can fix this for us.
Project Member

Comment 8 by mf...@codeaurora.org, Jan 1 2012

dreed10: Gerrit already supports multiply branches with the same changeid, so simply adding the dependencies (ab8db2c7) to the new branch (B) should be ok.  

It would be somewhat inconsistent with the way Gerrit works with changes submitted to a single branch if it were to reject dependencies since Gerrit normally treats unmerged commits as changes even if they are not the last commit in a series.  this allows a user to submit more than one change in a single push.  Is there some special condition that I am missing which would let Gerrit distinguish between your case and the normal case where a user wants to submit multiple changes to a branch where some of them just happen to already be merged on another branch?

Comment 9 by dree...@gmail.com, Jan 1 2012

I think adding the dependency (ab8db2c7) to the new branch (B) should be fine.  
Normally when a sequence of new commits are being pushed, they're all flagged up for code review. This should always be the case for new changes within a branch. 

Gerrit is finding these commits elsewhere and automatically merging them, which may be useful in some situations but it seems that even then, it's good to be prompted as to what's being merged along with any code review for that commit that has been done elsewhere (if any!).

Comment 11 by sop@google.com, May 18 2012

 Issue 1387  has been merged into this issue.
Hello, Has this issue already been fixed in the master?
@szymon.w...@gmail.com

I am planning on working on this change.
Can you please explain --

Gerrit is finding these commits elsewhere and automatically merging them, which may be useful in some situations but it seems that even then, it's good to be prompted as to what's being merged along with any code review for that commit that has been done elsewhere.

Do you mean to give a notification mail when a change is merged which already has a change id existing in different branch?
Please clarify your requirement?

Comment 14 by dree...@gmail.com, Jan 20 2014

I guess I don't have a strong opinion on what Gerrit should do in these cases I just think that Gerrit should not do it without somehow telling the user that it in fact happened.   
@sop@google.com, mf...@codeaurora.org, dreed10, david.re...@gmail.com
I have submitted the above 2 changes so users can get some kind of notification when this issue happens. Can you please help to review it. 
We're definitely being bitten by this too. Our ACLs disallow pushing of merge commits, but our backport workflow is currently for a developer to cherry-pick a commit from master, preserving the original change-id, and propose it as a new change to a stable branch. If the developer accidentally cherry-picks the change right back onto the master branch instead of a checkout of the stable branch, but then proposes that change to the stable branch in Gerrit, it's not disallowed even though it depends on a merge commit from another branch (the one created by Gerrit employing its "merge if necessary" strategy) which is not present in the target branch. There's no indication at all in the UI that this has happened, short of browsing the parent commit SHA to discover that it's only present on another branch entirely (it's not displayed in the dependencies list since its a merge commit in the repository rather than a real Gerrit change).
For reference, how it looks like in reality:
https://review.openstack.org/#/c/122364/ is the change.
https://github.com/stackforge/fuel-main/commit/f183e1837d26d3794764be5dfff0ab5a54a1849b is the resulting merge onto the stable branch from that change

Comment 19 by ocroque...@free.fr, Jan 29 2015

I think I just hit this bug too. Here are the steps:

# Just get a clean state:
git checkout master
git fetch
git reset --hard origin/master

# Modify files
...

# Commit unacceptable changes 
git commit -a -m "Unacceptable changes that should never end up in the mainline"

# Push directly to feature branch without code review (this is the trigger of the issue)
git push origin HEAD:refs/heads/feature/branch

# Modify files
...

# Commit Acceptable changes 
git commit -a -m "Acceptable changes"

# Push commit for review
git push origin HEAD:refs/heads/for/master

Since there are 2 commits between my HEAD and origin/master, I expect Gerrit to open 2 code review ("Unacceptable" + "Acceptable"). But that doesn't happen. Gerrit opens only one code review for the "Acceptable" commit, the parent displayed in the WUI is the "Unacceptable" commit). If the reviewer doesn't notice the wrong parent, and submits the changes, both changes end up in master.

If I don't push to the feature branch ("trigger of the issue" above), then it works as expected, 2 code reviews get open and the world is fine.

It's a huge loophole, especially in Gerrit instances where developers can create their own refs.

@mani.cha...@gmail.com: it's a good step to be notified or highlight the situation, but the real fix is to prohibit the merge of unwanted/unreviewed commits.


For the record, we are using 2.8.4. @dreed10: it would be great if you could update the "Affected Version". My understanding is that all 2.x versions behave this way.

Comment 21 by ocroque...@free.fr, Jan 13 2016

One year later, we just got bitten again by this bug :(

Comment 22 by ndjen...@gmail.com, Jul 20 2016

This is really bad when it happens.  With a team of developers, some don't realize they're going down the path of this bug and then extra steps must be taken to reverse the problem.  This bug should really be fixed.

Comment 23 by ocroque...@free.fr, Jul 20 2016

We are currently evaluating the new option "new-change-for-all-not-in-target", it looks like a solution (or workaround).

See https://gerrit-review.googlesource.com/Documentation/project-configuration.html
Project Member

Comment 24 by logan@google.com, Aug 17 2016

Labels: Priority-1
Project Member

Comment 25 by huga...@gmail.com, Sep 1 2016

Labels: FixedIn-2.13
Status: Submitted (was: Accepted)
https://gerrit-review.googlesource.com/#/c/85233/
Project Member

Comment 26 by huga...@gmail.com, Sep 22 2016

Status: Released (was: Submitted)
 Issue 2294  has been merged into this issue.

Sign in to add a comment