Fix change stuck in SUBMITTED state but actually merged

This behavior is caused by submitting a commit that has a tag.
Method MergeUtil.markCleanMerges is used by each merge strategy
to update the status of a change if it was a clean merge. It skips
commits that are identified as accepted and their entire ancestry
chain. If a commit is tagged, method MergeOp.getAlreadyAccepted
identifies it as accepted (even if it is not merged yet).
Fix prevents a commit that is referred by a tag to be included in
alreadyAccepted set. If such commit is already merged then it will
be covered anyway by adding heads to alreadyAccepted.

Add a corresponding test that pushes a commit with a tag

Bug: Issue 600
Change-Id: If00247b809b985eaf60ef5ef09fad0f475fb06b9
(cherry picked from commit b22ee233f1ef880cd84beb5939e332de1d7d704e)

4 files changed, 51 insertions, 12 deletions

diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/GitUtil.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/GitUtil.java
index c8158c07e4..045207c677 100644
--- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/GitUtil.java
+++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/GitUtil.java
@@ -146,9 +146,13 @@ public class GitUtil {
     }
   }
 
-  public static PushResult pushHead(Git git, String ref) throws GitAPIException {
+  public static PushResult pushHead(Git git, String ref, boolean pushTags)
+      throws GitAPIException {
     PushCommand pushCmd = git.push();
     pushCmd.setRefSpecs(new RefSpec("HEAD:" + ref));
+    if (pushTags) {
+      pushCmd.setPushTags();
+    }
     Iterable<PushResult> r = pushCmd.call();
     return Iterables.getOnlyElement(r);
   }
diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/PushOneCommit.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/PushOneCommit.java
index fb1b59206d..4c32f2ff82 100644
--- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/PushOneCommit.java
+++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/PushOneCommit.java
@@ -58,6 +58,7 @@ public class PushOneCommit {
   private final String fileName;
   private final String content;
   private String changeId;
+  private String tagName;
 
   public PushOneCommit(ReviewDb db, PersonIdent i) {
     this(db, i, SUBJECT, FILE_NAME, FILE_CONTENT);
@@ -86,7 +87,14 @@ public class PushOneCommit {
     } else {
       changeId = createCommit(git, i, subject);
     }
-    return new Result(db, ref, pushHead(git, ref), changeId, subject);
+    if (tagName != null) {
+      git.tag().setName(tagName).setAnnotated(false).call();
+    }
+    return new Result(db, ref, pushHead(git, ref, tagName != null), changeId, subject);
+  }
+
+  public void setTag(final String tagName) {
+    this.tagName = tagName;
   }
 
   public static class Result {
diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/SubmitOnPushIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/SubmitOnPushIT.java
index fa85927d8e..ab97d192c2 100644
--- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/SubmitOnPushIT.java
+++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/SubmitOnPushIT.java
@@ -113,7 +113,7 @@ public class SubmitOnPushIT extends AbstractDaemonTest {
   @Test
   public void submitOnPush() throws GitAPIException, OrmException,
       IOException, ConfigInvalidException {
-    grantSubmit(project, "refs/for/refs/heads/master");
+    grant(Permission.SUBMIT, project, "refs/for/refs/heads/master");
     PushOneCommit.Result r = pushTo("refs/for/master%submit");
     r.assertOkStatus();
     r.assertChange(Change.Status.MERGED, null, admin);
@@ -122,9 +122,25 @@ public class SubmitOnPushIT extends AbstractDaemonTest {
   }
 
   @Test
+  public void submitOnPushWithTag() throws GitAPIException, OrmException,
+      IOException, ConfigInvalidException {
+    grant(Permission.SUBMIT, project, "refs/for/refs/heads/master");
+    grant(Permission.CREATE, project, "refs/tags/*");
+    final String tag = "v1.0";
+    PushOneCommit push = new PushOneCommit(db, admin.getIdent());
+    push.setTag(tag);
+    PushOneCommit.Result r = push.to(git, "refs/for/master%submit");
+    r.assertOkStatus();
+    r.assertChange(Change.Status.MERGED, null, admin);
+    assertSubmitApproval(r.getPatchSetId());
+    assertCommit(project, "refs/heads/master");
+    assertTag(project, "refs/heads/master", tag);
+  }
+
+  @Test
   public void submitOnPushToRefsMetaConfig() throws GitAPIException,
       OrmException, IOException, ConfigInvalidException {
-    grantSubmit(project, "refs/for/refs/meta/config");
+    grant(Permission.SUBMIT, project, "refs/for/refs/meta/config");
 
     git.fetch().setRefSpecs(new RefSpec("refs/meta/config:refs/meta/config")).call();
     ObjectId objectId = git.getRepository().getRef("refs/meta/config").getObjectId();
@@ -145,7 +161,7 @@ public class SubmitOnPushIT extends AbstractDaemonTest {
     push(master, "one change", "a.txt", "some content");
     git.checkout().setName(objectId.getName()).call();
 
-    grantSubmit(project, "refs/for/refs/heads/master");
+    grant(Permission.SUBMIT, project, "refs/for/refs/heads/master");
     PushOneCommit.Result r =
         push("refs/for/master%submit", "other change", "a.txt", "other content");
     r.assertOkStatus();
@@ -161,7 +177,7 @@ public class SubmitOnPushIT extends AbstractDaemonTest {
     push(master, "one change", "a.txt", "some content");
     git.checkout().setName(objectId.getName()).call();
 
-    grantSubmit(project, "refs/for/refs/heads/master");
+    grant(Permission.SUBMIT, project, "refs/for/refs/heads/master");
     PushOneCommit.Result r =
         push("refs/for/master%submit", "other change", "b.txt", "other content");
     r.assertOkStatus();
@@ -175,7 +191,7 @@ public class SubmitOnPushIT extends AbstractDaemonTest {
     PushOneCommit.Result r =
         push("refs/for/master", PushOneCommit.SUBJECT, "a.txt", "some content");
 
-    grantSubmit(project, "refs/for/refs/heads/master");
+    grant(Permission.SUBMIT, project, "refs/for/refs/heads/master");
     r = push("refs/for/master%submit", PushOneCommit.SUBJECT, "a.txt",
         "other content", r.getChangeId());
     r.assertOkStatus();
@@ -220,13 +236,13 @@ public class SubmitOnPushIT extends AbstractDaemonTest {
     r.assertErrorStatus("branch " + branchName + " not found");
   }
 
-  private void grantSubmit(Project.NameKey project, String ref)
+  private void grant(String permission, Project.NameKey project, String ref)
       throws RepositoryNotFoundException, IOException, ConfigInvalidException {
     MetaDataUpdate md = metaDataUpdateFactory.create(project);
-    md.setMessage("Grant submit on " + ref);
+    md.setMessage(String.format("Grant %s on %s", permission, ref));
     ProjectConfig config = ProjectConfig.read(md);
     AccessSection s = config.getAccessSection(ref, true);
-    Permission p = s.getPermission(Permission.SUBMIT, true);
+    Permission p = s.getPermission(permission, true);
     AccountGroup adminGroup = groupCache.get(new AccountGroup.NameKey("Administrators"));
     p.add(new PermissionRule(config.resolve(adminGroup)));
     config.commit(md);
@@ -277,6 +293,18 @@ public class SubmitOnPushIT extends AbstractDaemonTest {
     }
   }
 
+  private void assertTag(Project.NameKey project, String branch, String tagName)
+      throws IOException {
+    Repository r = repoManager.openRepository(project);
+    try {
+      ObjectId headCommit = r.getRef(branch).getObjectId();
+      ObjectId taggedCommit = r.getRef(tagName).getObjectId();
+      assertEquals(headCommit, taggedCommit);
+    } finally {
+      r.close();
+    }
+  }
+
   private PushOneCommit.Result pushTo(String ref) throws GitAPIException,
       IOException {
     PushOneCommit push = new PushOneCommit(db, admin.getIdent());
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeOp.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeOp.java
index d86e48c3d2..3a9eaa39b2 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeOp.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeOp.java
@@ -456,8 +456,7 @@ public class MergeOp {
 
     try {
       for (final Ref r : repo.getAllRefs().values()) {
-        if (r.getName().startsWith(Constants.R_HEADS)
-            || r.getName().startsWith(Constants.R_TAGS)) {
+        if (r.getName().startsWith(Constants.R_HEADS)) {
           try {
             alreadyAccepted.add(rw.parseCommit(r.getObjectId()));
           } catch (IncorrectObjectTypeException iote) {