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)
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 c8158c07..045207c 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 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 fb1b592..4c32f2f 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 @@
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 @@
} 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 fa85927..ab97d19 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 @@
@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 @@
}
@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 @@
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 @@
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 @@
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 @@
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 @@
}
}
+ 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 d86e48c..3a9eaa3 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 @@
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) {
