My favorites | Sign in
Project Home Downloads Wiki Issues Source
New issue   Search
for
  Advanced search   Search tips   Subscriptions
Issue 1258: Gerrit should check commit whether it has been merged before replace a change
2 people starred this issue and may be notified of changes. Back to list
Status:  New
Owner:  ----


Sign in to add a comment
 
Reported by chengwei.yang.cn@gmail.com, Feb 8, 2012
Affected Version: 2.2

What steps will reproduce the problem?
1. assuming there were commitA (merged in remote), commitB (local commit). commitB is current branch HEAD, and commitA is one of commitB's ancestors.
2. push the commitB to Gerrit Code Review
    $ git push origin HEAD:refs/for/master
assuming the above command created a new change idB in Gerrit. After that, we can update change idB like below.
    $ git reset --soft HEAD~1
    /* do some improvements and create a local commit, and then update change idB */
    $ git push origin HEAD:refs/changes/idB
That works fine. However, if we push a commit which is already merged in sometime ago, this will cause problem, at that point, Gerrit just merge is again even there is no side-effect to git tree, but it well confused many people who received email which suggests that change idB has been merged by developer even who has no privileges to submit merge.
For example.
    currently change idB is open, now push commitA to replace change idB.
    $ git push origin HEAD~3:refs/changes/idB
At above, I assuming HEAD~3 is commitA and which has been already merged. This will get change idB merged directly. This is not the expected action. It should reject commitA and notice user. 


What is the expected output?
Reject the commitA and notice user.

What do you see instead?
Gerrit merge commitA directly and noticed all listeners by email and confused all because the developer has no privileges to submit merge. 

Please provide any additional information below.
The below and attachment is my patch to resolve it.

From 9f2959c8ef3c74b0397078169ba3e4bac90168e1 Mon Sep 17 00:00:00 2001
From: Chengwei Yang <chengwei.yang@intel.com>
Date: Thu, 9 Feb 2012 13:54:45 +0800
Subject: [PATCH] check commit before replace change

$ git push ... HEAD:refs/changes/ID
We assuming that HEAD has been merged sometime before and ID is open and
who did the command with no privileges to submit merge in Gerrit.

Currently, the above command will cause Gerrit to close change ID and
get it merged and notice all the listeners. So most of listeners may be
confused why it got merged since the pusher even has no privileges to
submit.

This patch check the commit whether it has been merged already, if so,
just reject it and notice the pusher.
---
 .../google/gerrit/server/git/ReceiveCommits.java   |    9 +++++++++
 1 files changed, 9 insertions(+), 0 deletions(-)

diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java
index 8450dc5..983ec3f 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java
@@ -1090,6 +1090,15 @@ public class ReceiveCommits implements PreReceiveHook, PostReceiveHook {
       return null;
     }
 
+    // If the new commit has already been merged, then reject it
+    for (final PatchSet ps : db.patchSets().byRevision(toRevId(c))) {
+      Change tmpChange = db.changes().get(ps.getId().getParentKey());
+      if (tmpChange.getStatus() == Change.Status.MERGED) {
+        reject(request.cmd, "the commit has already been merged");
+        return null;
+      }
+    }
+
     final PatchSet.Id priorPatchSet = change.currentPatchSetId();
     for (final PatchSet ps : db.patchSets().byChange(request.ontoChange)) {
       if (ps.getRevision() == null) {
-- 
1.7.2.5


0001-check-commit-before-replace-change.patch
1.9 KB   View   Download
Sign in to add a comment

Powered by Google Project Hosting