From 72fa3c242290a962873077c47d0fd82fceda049e Mon Sep 17 00:00:00 2001 From: Jan Opacki Date: Sun, 2 Jun 2013 15:49:12 +0200 Subject: [PATCH] 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) --- .../google/gerrit/acceptance/git/GitUtil.java | 6 ++- .../gerrit/acceptance/git/PushOneCommit.java | 10 ++++- .../gerrit/acceptance/git/SubmitOnPushIT.java | 44 +++++++++++++++---- .../com/google/gerrit/server/git/MergeOp.java | 3 +- 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 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); @@ -121,10 +121,26 @@ public class SubmitOnPushIT extends AbstractDaemonTest { assertCommit(project, "refs/heads/master"); } + @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) {