New issue
Advanced search Search tips

Issue 4158 link

Starred by 4 users

Issue metadata

Status: Released
Owner: ----
Closed: Nov 2016



Sign in to add a comment

RebaseIfNecessary does not merge

Reported by zeck...@gmail.com, Jun 2 2016

Issue 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:

What steps will reproduce the problem?
1. SubmitType Rebease If Necessary, Allow content merges to true
2. Push branch directly based off origin/master, commits already in another branch
3. Try to Submit including parents

What is the expected output? What do you see instead?

https://gerrit.osmocom.org/#/c/170/

Pressing Submit gives "Change is new" (exception raised _after_ merge has been attempted). Patch would be a fast-forward to master right now.


Please provide any additional information below.


[2016-06-02 13:18:23,210] [HTTP-38] DEBUG com.google.gerrit.server.git.MergeOp : [170-1464873503210-cf5177fd]Beginning integration of Change{170 (I93c074bf99db041117c0dc03dc8255879845a875), dest=openbsc,refs/heads/master, status=n}
[2016-06-02 13:18:23,276] [HTTP-38] DEBUG com.google.gerrit.server.git.MergeOp : [170-1464873503210-cf5177fd]Calculated to merge ChangeSet[170]
[2016-06-02 13:18:23,276] [HTTP-38] DEBUG com.google.gerrit.server.git.MergeOp : [170-1464873503210-cf5177fd]Checking submit rules and state
[2016-06-02 13:18:23,278] [HTTP-38] DEBUG com.google.gerrit.server.git.MergeOp : [170-1464873503210-cf5177fd]Beginning merge attempt on ChangeSet[170]
[2016-06-02 13:18:23,278] [HTTP-38] DEBUG com.google.gerrit.server.git.MergeOp : [170-1464873503210-cf5177fd]Perform the merges
[2016-06-02 13:18:23,278] [HTTP-38] DEBUG com.google.gerrit.server.git.MergeOp : [170-1464873503210-cf5177fd]Validating 1 changes
[2016-06-02 13:18:23,296] [HTTP-38] DEBUG com.google.gerrit.server.git.MergeOp : [170-1464873503210-cf5177fd]Updating change status for 0 changes
[2016-06-02 13:18:23,297] [HTTP-38] DEBUG com.google.gerrit.server.git.MergeOp : [170-1464873503210-cf5177fd]Submitting on this run: {REBASE_IF_NECESSARY=[ChangeData{Change{170 (I93c074bf99db041117c0dc03dc8255879845a875), dest=openbsc,refs/heads/master, status=n}}]}
[2016-06-02 13:18:23,355] [HTTP-38] DEBUG com.google.gerrit.server.git.MergeOp : [170-1464873503210-cf5177fd]Opened branch refs/heads/master: commit 62ff38447ce8d24aa1e8b5094153df9e89c986d0 1464782991 -----p
[2016-06-02 13:18:23,362] [HTTP-38] DEBUG com.google.gerrit.server.git.MergeOp : [170-1464873503210-cf5177fd]Found 151 existing heads
[2016-06-02 13:18:23,362] [HTTP-38] DEBUG com.google.gerrit.server.git.MergeOp : [170-1464873503210-cf5177fd]Running submit strategy RebaseIfNecessary for 1 commits [ChangeData{Change{170 (I93c074bf99db041117c0dc03dc8255879845a875), dest=openbsc,refs/heads/master, status=n}}]
[2016-06-02 13:18:23,363] [HTTP-38] DEBUG com.google.gerrit.server.git.MergeOp : [170-1464873503210-cf5177fd]Produced 0 new commits
[2016-06-02 13:18:23,363] [HTTP-38] DEBUG com.google.gerrit.server.git.MergeOp : [170-1464873503210-cf5177fd]Checking change state for 1 changes in a dry run
[2016-06-02 13:18:23,363] [HTTP-38] DEBUG com.google.gerrit.server.git.MergeOp : [170-1464873503210-cf5177fd]Submitted change 170 did not appear in set of new commits produced by merge strategy
[2016-06-02 13:18:23,364] [HTTP-38] DEBUG com.google.gerrit.server.git.MergeOp : [170-1464873503210-cf5177fd]Write out the new branch tips
[2016-06-02 13:18:23,364] [HTTP-38] DEBUG com.google.gerrit.server.git.MergeOp : [170-1464873503210-cf5177fd]Access cached open branch refs/heads/master: commit 62ff38447ce8d24aa1e8b5094153df9e89c986d0 1464782991 ---usp
[2016-06-02 13:18:23,364] [HTTP-38] DEBUG com.google.gerrit.server.git.MergeOp : [170-1464873503210-cf5177fd]Branch already at merge tip 62ff38447ce8d24aa1e8b5094153df9e89c986d0, no update to perform
[2016-06-02 13:18:23,364] [HTTP-38] DEBUG com.google.gerrit.server.git.MergeOp : [170-1464873503210-cf5177fd]Updating change status for 1 changes
[2016-06-02 13:18:23,364] [HTTP-38] DEBUG com.google.gerrit.server.git.MergeOp : [170-1464873503210-cf5177fd]Submitted change 170 did not appear in set of new commits produced by merge strategy
[2016-06-02 13:18:23,364] [HTTP-38] DEBUG com.google.gerrit.server.git.MergeOp : [170-1464873503210-cf5177fd]Updating superprojects
[2016-06-02 13:18:23,365] [HTTP-38] ERROR com.google.gerrit.server.change.Submit : DB com.google.gerrit.reviewdb.server.ReviewDb_Schema_GwtOrm$$20 merge com.google.inject.internal.InjectorImpl$2 merge com.google.gerrit.server.git.MergeOp
 

Comment 1 by zeck...@gmail.com, Jun 2 2016

So it "sort()" is throwing away this commit (as it is a fast-forward?)

RebaseIfNecessary.java
  @Override
  protected MergeTip _run(final CodeReviewCommit branchTip,
      final Collection<CodeReviewCommit> toMerge) throws IntegrationException {
    log.error("GOT X commits " + toMerge.size());
    MergeTip mergeTip = new MergeTip(branchTip, toMerge);
    List<CodeReviewCommit> sorted = sort(toMerge);
log.error("GOT X commits " + sorted.size());
    while (!sorted.isEmpty()) {

Comment 2 by zeck...@gmail.com, Jun 2 2016

I hacked this code up to check sorted size being zero and then using toMerge as commits directly. Is that a untested corner case?
Project Member

Comment 3 by marco.mm...@gmail.com, Jun 6 2016

I suspect this issue to be a duplicate of  Issue 4087  fixed by [1] (not released yet).

[1] https://gerrit-review.googlesource.com/#/c/77711/
Status: AwaitingInformation (was: New)
Please confirm which version of Gerrit you're using.
I don't know if it is fixing it but it is certainly a candidate for it.

Comment 6 by nee...@gmail.com, Jun 20 2016

To answer the questions, for my colleague "zeck..."

I've just spent some time trying to re-create the problem, and I've discovered that it is actually related to changing the project merge strategy while a set of changes is already waiting to be submitted.


In version: 09034c03f1f6500c9a88933286fc12f4bb744771
origin/stable-2.12

i.e. the fix for  issue 4087  is already in our code base:
https://gerrit-review.googlesource.com/#/c/77711/2/gerrit-server/src/main/java/com/google/gerrit/server/git/RebaseSorter.java 

* Config:
** Rebase-If-Necessary strategy
** Create a new change for... = TRUE

* Have a branch with two commits off master's tip

* push to a "private" branch (git push gerrit HEAD:refs/heads/test/t1)

* submit the branch (would be a fast-forward)

* Add +2 CR and +1 V votes to commits

* Change the project's merge strategy to Merge-If-Necessary

* On the branch's last revision, click "submit including parents"
  --> the UI greys out and says "Change is new"
  Here I would expect a simple fast-forward merge.

* If you try the rebase button, you get the "Code Review - Rebase Change" dialog
  which seems to accept no input either way.

* Even if you now go back to Rebase-If-Necessary strategy, the submission remains broken
  in the same way.

* Other submissions are not affected, only the one that was active during the
  strategy change.

The fix we thought to have found (quote "I hacked this code up") doesn't actually help here.
The patch may have simulated behavior as with "Create a new change for..."=TRUE, which may be why
we assumed that it helped; or maybe it helps on revision 5fc9ca98ed5d00891d73b883a045a10851323627
that we originally worked on. I can't reproduce anything where this helps ATM, but here it is
anyway, for completeness sake -- it avoids a situation where sort() returns an empty list:

--- a/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/RebaseIfNecessary.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/RebaseIfNecessary.java
@@ -38,6 +38,7 @@ import org.eclipse.jgit.lib.ObjectId;
 import org.eclipse.jgit.lib.PersonIdent;
 
 import java.io.IOException;
+import java.util.ArrayList;
 import java.util.Collection;
 import java.util.HashMap;
 import java.util.List;
@@ -68,6 +69,8 @@ public class RebaseIfNecessary extends SubmitStrategy {
       final Collection<CodeReviewCommit> toMerge) throws IntegrationException {
     MergeTip mergeTip = new MergeTip(branchTip, toMerge);
     List<CodeReviewCommit> sorted = sort(toMerge);
+    if (sorted.isEmpty())
+      sorted = new ArrayList(toMerge);
     while (!sorted.isEmpty()) {
       CodeReviewCommit n = sorted.remove(0);
 

(We originally also needed to fix a NullPointerException which has since been fixed on stable-2.12 by someone else.)

Not sure how to carry on with this report, maybe it should become a new issue?

Comment 7 by nee...@gmail.com, Jun 20 2016

To clarify:

Where I said "submit the branch" above, I meant

git push gerrit HEAD:refs/for/master
Project Member

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

Labels: Priority-3

Comment 9 by nee...@gmail.com, Aug 18 2016

Just this instant I am facing the same problem again.
We are now running 2.12.3.

Take a look at https://gerrit.osmocom.org/711
which I would like to merge using the "Submit including parents" button.
Already the first commit of that branch 705 doesn't work.

I have the same branch pushed as neels/hnbgw-cfg, see
git://git.osmocom.org/osmo-iuh (mirroring the gerrit-internal git repos)
http://git.osmocom.org/osmo-iuh/log/?h=neels/hnbgw-cfg

If you'd like to try, I could provide access rights to that gerrit instance.
Bottom line is: if I click "Submit...", I get greyed out "Change is new".

Comment 10 by nee...@gmail.com, Aug 18 2016

I am able to reproduce the situation in our sandbox repository:
https://gerrit.osmocom.org/718
Refer to 718 instead of the 711 link above, as I will merge 711 manually now;
718 shows identical symptoms.

And I now realize the error only appears when there is a "dangling"
commit on the branch that is not ready for merge yet. The problem does
not appear when I approve all of the patches and submit the branch HEAD.

So the recipe changes to:

* Config:
** Rebase-If-Necessary strategy
** Create a new change for... = TRUE

* Have a branch with 3 commits off master's tip

* push to a "private" branch (git push gerrit HEAD:refs/heads/test/t1)

* submit the branch for review (would be a fast-forward)

* Add +2 CR and +1 V votes to the first two commits, leave the last one.

* on the second commit, click "Submit including parents"

-> "Change is new" error appears

* Add +2 CR and +1 V on the last commit as well

* click "submit including parents" on the last commit

-> now it works.

Even if changing the merge strategy back and forth shows the same problem,
this issue was originally not about changing the strategy (which we don't
do in daily business) but rather for the case described here.

Please excuse the noise, which is due to me struggling to reproduce the
issue reliably.
> push to a "private" branch (git push gerrit HEAD:refs/heads/test/t1)

Can you clarify this step?  In both the example changes (#711 and #718) the changes are pushed for the master branch.

I can't reproduce this on 2.12.3 or on the head of stable-2.12.

- Create a new project, set up the config as described (rebase if necessary, create new change = true)
- Clone the project from https://gerrit.osmocom.org/sandbox
- Push osmocom/master to refs/heads/master
- Fetch and checkout https://gerrit.osmocom.org/#/c/719/1
- push to refs/for/master
- Approve/verify t1 and t2
- submit t2

The changes t1 and t2 are submitted without error.

http://imgur.com/a/OI9nS

Comment 13 by nee...@gmail.com, Aug 22 2016

Thanks for investigating.

The "push to a private branch" step is basically keeping a copy of the branch
that is submitted in the git repository. i.e.

git checkout master
git checkout -b my_feature
git commit -m one
git commit -m two
git commit -m three
git push --set-upstream origin my_feature  # <-- this is the "private" branch
git push origin HEAD:refs/for/master       # <-- submission for review

The result is that the commits sit once in a gerrit submission (as in git ls-remote changes/*),
and at the same time also in a separate branch in the git repository
with the identical change ID (requires the "create new change..." = true option).

The separate branch is for my personal reference, e.g. to be able to tweak the commits
for review from separate machines or collaborate with colleagues.

For the sandbox repos in the example, the "private" branch is called 'change_is_new2'.

In more detail, after the "- push to refs/for/master" step, include these:
 git checkout -b foo
 git push --set-upstream origin foo
and then continue to approve/verify and submit.
Project Member

Comment 14 by sven.sel...@axis.com, Oct 27 2016

We have had the same issue on several occasions since upgrade to v2.12.5 four days ago.

1. push to "private" branch
2. push for review
3. submit review
* User gets "Change is new" error.
* Change still has status 'n' in db
* Commit is merged on target branch

Project Member

Comment 15 by sven.sel...@axis.com, Oct 27 2016

Same issue:
* Commit in private user-branch.
* Different commit with same Change-Id pushed for review.

Project configuration:
State: Active 
Submit Type: Merge if necessary
Allow content merges:	False
Create a new change for every commit not in the target branch:	Inherit(false)
Require Change-Id in commit message: Inherit(true)

Project Member

Comment 16 by sven.sel...@axis.com, Oct 27 2016

Uploaded a testcase that reveals this bug:
https://gerrit-review.googlesource.com/#/c/90094/

Project Member

Comment 17 by sven.sel...@axis.com, Oct 28 2016

So far I've tracked it down to com.google.gerrit.server.git.MergeOp#updateChangeStatus(...)
https://gerrit.googlesource.com/gerrit/+/stable-2.12/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeOp.java#852

      CommitMergeStatus s = commit != null ? commit.getStatusCode() : null;
      if (s == null) {
        // Shouldn't ever happen, but leave the change alone. We'll pick
        // it up on the next pass.
        //
        logDebug("Submitted change {} did not appear in set of new commits"
            + " produced by merge strategy", c.getId());
        continue;
      }

The commit is in fact in commits, it just doesn't have a changeStatus.

p.s.
"Shouldn't ever happen" dangerous claim... :-)
Project Member

Comment 18 by sven.sel...@axis.com, Oct 28 2016

Also, as I wrote in the testcase change.
The bug hits every merge strategy except "Cherry Pick".
Project Member

Comment 19 by sven.sel...@axis.com, Oct 28 2016

With the "Fast Forward Only" strategy the bug causes a different error (other than "change is new" on submission:
    error: Cannot merge <SHA1>
    Project policy requires all submissions to be a fast-forward.

Project Member

Comment 20 by sven.sel...@axis.com, Oct 28 2016

And herein, I believe lies the problem.
Since the commit we are trying to submit is already reachable from a branch tip it is marked as uninteresting in:
https://gerrit.googlesource.com/gerrit/+/stable-2.12/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeUtil.java#659

      for (RevCommit c : alreadyAccepted) {
        rw.markUninteresting(c);
      }

And is never marked as merged.
This would also explain why it will work if the commit is the tip of the topic branch. Since then it is not added to alreadyAccepted.

https://gerrit.googlesource.com/gerrit/+/stable-2.12/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeOp.java#579

          if (!commits.values().contains(aac)) {
            alreadyAccepted.add(aac);
          }

Project Member

Comment 21 by sven.sel...@axis.com, Oct 28 2016

...or should i say: the CodeReviewCommit never gets the CLEAN_MERGE status it has deserved:
https://gerrit.googlesource.com/gerrit/+/stable-2.12/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeUtil.java#663

      CodeReviewCommit c;
      while ((c = (CodeReviewCommit) rw.next()) != null) {
        if (c.getPatchsetId() != null) {
          c.setStatusCode(CommitMergeStatus.CLEAN_MERGE);
        }
      }
Project Member

Comment 22 by sven.sel...@axis.com, Oct 28 2016

If merge strategy is Rebase if Necessary then off course the markUninteresting exclusion of the commit takes place in RebaseSorter.java#52-74 instead.
Project Member

Comment 23 by sven.sel...@axis.com, Oct 28 2016

Uploaded a fix for:
Merge Always
Merge if Necessary
Fast Forward Only
https://gerrit-review.googlesource.com/#/c/90314/

Rebase if Necessary needs to be sorted out in RebaseSorter.
Status: ChangeUnderReview (was: AwaitingInformation)
Labels: FixedIn-2.12.6
Status: Submitted (was: ChangeUnderReview)
Status: Released (was: Submitted)

Sign in to add a comment