Multiple Commits with Failed Reviews in Gerrit

Gerrit in review

I’ve been pushing the use of Gerrit at work recently (no pun intended). Originally it was only being used as an authorization tool over our git repositories. We’ve previously used gitosis and gitolite, but I was put onto Gerrit by a friend working on the Rockbox project, and decided to give it a go. Personally, I’m converted, although originally I found its control setup a bit odd, and quite difficult to follow (we’re using 2.2 but the documentation is still very heavily based on 2.1 which had a different model), but once I’d worked out a base set of access rights, I found it very easy to manage.

Within our teams, the level of git knowledge was very limited, which is why we didn’t use Gerrit for full review, but instead allowed direct push to master. However, with the team about to grow and the number of projects due to explode, I decided the git knowledge was enough to introduce the Review process in full so we could iron out any initial problems and working processes.

It’s been a busy week!

This blog (my first serious attempt to add anything of worth outside our internal wiki)  is about a particular problem that appeared very early when a dev asked me how we would fail a review that was part of a series of commits under different reviews that were linked after they had pushed multiple commits instead of rebasing them into a single review.

Multiple Commits with Failed Review

Diag. 2

Diag. 1

Up front, the easiest thing will probably be to fail all the reviews above the failing one, and go back and push each change individually, but here is an alternate that moves changes forward, and gains an understanding of how git and Gerrit both work.

Setup for this case: There are 3 commit objects c1, c2, c3 (Diagram 1).

When the changes are pushed to Gerrit, it will produce 3 Review Changes, each dependent on the previous, as shown in Diagram 2, which also shows there are 3 reviews; R1, R2, R3. The corresponding Gerrit Review number is shown next to this (310, 311, 312).

A failure in review

Diagram 3

Let us assume that R1 is fine, it is verified and approved, and submit in the normal fashion.

However, the reviewer for R2 decides there is a problem with it which requires a change.

Normally, the developer would simply amend the commit, and submit a patch to R2, which can be reviewed and approved in the normal way.However, in this situation, we have R3 which is dependent on the first patch of R2, and this complicates things for the developer.

They no longer have a local master branch referring to c2, they have moved onto c3. A new patch (p2) needs to created in R2 as in Diagram 3.

To do this, the developer would branch (or tag) their current c3, and move to c2 to make changes that will apply to R2.

This can be done as follows (Diagram 4):

# create a point we can get back to
git branch push3
# move back in time to c2 and create a branch for it
git checkout HEAD~1 # alternatively, use the commit id of c2 here
git checkout -b push2

Diag. 4

Diag. 5

The developer does their fix, and commits it on their local branch push2, ensuring the fix is rebased into a single commit. The local repository will look like Diagram 5, with commit “c2b” being the new commit that should be pushed to R2 in Gerrit.

To push the changes to Gerrit, the following needs to be done:
  1. Identify the commit hash for the c2b object
  2. Identify the Review “change” number in Gerrit (e.g. in this case, Review 2’s change number = 311)
  3. Ensure “Change-Id:” values are in each commit message being pushed to review.
$ git log -1 | head -1
commit bfe32dd4f70abb31eea562371dc88994886d416e
$ HASH=bfe32dd4f70abb31eea562371dc88994886d416e
$ CHANGE=311
$ git push gerrit:Project ${HASH}:refs/changes/${CHANGE}

This is a standard git push, but using a particular commit onto a specific branch on the remote (gerrit) repository, and it will appear as Patch 2 on the review R2 (see Diagram 3).

It’s worth noting at this point that Gerrit patches are simply git commit objects that are held under a single review. Notice the similarity between Diagram 3 and 5. All that’s happening is that Gerrit is tracking commit objects under a single umbrella of a review.

When this is done, Review 2 can now be verified, approved and submitted to the main branch.

Fixing the dependent Review

There is now an obvious problem now with Diagram 3. Review 3 (with commit c3) is based off the old commit object in Review 2, but it needs to be based on patch 2 (the fix just pushed).

Attempting to submit Review R3 will fail in Gerrit with an error like:

Change cannot be merged due to unsatisfiable dependencies.
The following dependency errors were found:
Depends on patch set 1 of I337a6b84, however the current patch set is 2.
Please rebase the change and upload a replacement commit.

Diag. 6

As the last line of the message says, the developer now needs to make the changes that were introduced in c3 into our push2 branch.

The easiest way to do this (that I found) is to cherry-pick the changes across into the push2 branch.

The command to cherry-pick is made easy by the fact we branched push3 before moving to push2.

After cherry-picking changes into our branch, the change c3b can be pushed into Review 3 as follows:

$ git cherry-pick push3
$ git log -1 | head -1
commit 522089eaa723b3f8d304ccd5240e8e071f1dd2fa
$ HASH=522089eaa723b3f8d304ccd5240e8e071f1dd2fa
$ CHANGE=312 # this is the change number for review R3
$ git push gerrit:Project ${HASH}:refs/changes/${CHANGE}

Diag. 7

Again, we are pushing into the special branch “refs/changes/312” for a push into that specific review, and using the local commit id as the source.

The final review diagram looks like Diagram 7. Finally, Review R3 can be approved and submit into the main branch.

Well, that’s it for now, and my first blog.

Any feedback appreciated.

Advertisements

7 thoughts on “Multiple Commits with Failed Reviews in Gerrit

  1. Torne says:

    I think you’re overcomplicating this, though your process is perfectly valid. :)

    There’s two things you can do to simplify this:

    1) You don’t need to push to refs/changes/$CHANGE if you have proper change-id lines. The entire point of having the change-id lines in the commit message is so that you can just push to refs/for/master (or whichever branch) and have it automatically recognised as a new patchset for the same change. Nothing is relying on the local branch to know what change is what.

    2) You don’t need to push the changes separately, *or* create another branch for them. When I’m in this situation, I would tend to just `git rebase -i HEAD~2` and set C2 to ‘edit’ in the rebase list. git will give me a tree with just C1 and C2 applied, which I can then edit, compile, test, commit –amend, and then rebase –continue to reapply C3 on top. You can then just push this directly to gerrit again, with no special consideration, just `git push gerrit HEAD:refs/for/master` or whatever you would normally use. This will create a new patchset in C2 and C3, and the new C3 will have the correct dependency.

    • fenrock says:

      Thanks Torne, I’ll try this out and amend the blog accordingly. I’m fairly isolated here in discovering fixes for these situations, so it’s nice to get someone else’s experiences.

      I hadn’t thought of simply interactive rebasing and marking for edit, knowing Gerrit pulls this into the right patches automatically is… obvious now I think about it due to those lovely Change-Id tags.

      • It can be useful to do it on a different branch sometimes still (e.g. if it’s going to take you more than one session to finish, you probably don’t want to leave your branch in the middle of an interactive rebase), and then your process of making a new branch head pointing at C2 and then cherrypicking C3 onto it is probably what you want (and is effectively what rebase -i does anyway, just without the explicit separate steps and actually-recorded progress). So, that’s still worth knowing as a workflow; you just don’t need to actually worry about keeping track of what you’re pushing where.

  2. Venugopal says:

    To rebase push3 on top of push2, I use the following procedure
    1. Create push3 branch
    git checkout -b push3

    2. Fetch review2 from gerrit and checkout the review. Alternatively, you could move 1 commit back with following command
    git checkout HEAD~1

    3. Create push2 branch with the following command
    git checkout -b push2

    4. Make changes to push 2 and push it to gerrit

    5. Cherry picking push3
    git cherry-pick push3

    if there are errors the following message pops up
    error: could not apply ab0eb4d… Support for nick name in payments.
    hint: after resolving the conflicts, mark the corrected paths
    hint: with ‘git add ‘ or ‘git rm ‘
    hint: and commit the result with ‘git commit’

    6. Fix the conflicts
    (Manually check and add/remove necessary changes. Git marks the conflicting changes with “>>>>….”)

    7. Add the conflicted file(s) with following command(s)
    git add
    ……

    8. Continue Cherry pick with following command
    git cherry-pick –continue

    9. Amend the commit with following command
    git commit –amend (sometimes commit message is modified by adding the conflicted files to the last paragraph in the commit message. If it is the case, move the change-id to the last paragraph otherwise git might not allow during the push)

    10. Push the commit
    git push origin HEAD:refs/for/master

    • fenrock says:

      Thanks for your comments.

      It’s been a while since I wrote this blog and never updated it after Torne’s comments.

      We’re still using gerrit heavily, and have found I’ve not once needed to worry about this situation in the 2 years of using it, but I think I’d definitely go the interactive base route with Change-Ids on all commits, and let the natural rebase work its magic and push all changes back once they are tested at all commit point/reviews rather than faffing with all the cherry-pics etc.

      • Venugopal says:

        Well, its up to individual’s way of doing things. With git you have multiple ways of doing.

        Anyways, thanks for your article. Prior to this, I just used to ‘abandon’ the subsequent reviews if there was ‘change request’ (review comment) on a parent review.

  3. bozzwin says:

    Hi
    I newbe in gerrit stuff. I am trying to apply to my project on which I currenty working on. Yesterday I faced with that problem which you described in post. Thank for it, because save me a lot of time:) Good job

Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out /  Change )

Google+ photo

You are commenting using your Google+ account. Log Out /  Change )

Twitter picture

You are commenting using your Twitter account. Log Out /  Change )

Facebook photo

You are commenting using your Facebook account. Log Out /  Change )

Connecting to %s