opendev/gerrit
Code Issues Proposed changes

Fix change stuck in SUBMITTED state but actually merged

Browse Source 
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
This commit is contained in:
Jan Opacki 2013-06-02 15:49:12 +02:00
parent 5d259dda9f
commit b22ee233f1
4 changed files with 51 additions and 12 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

6
gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/GitUtil.java
View File

@ -171,9 +171,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);
}

10
gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/PushOneCommit.java
View File

@ -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 {

44
gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/SubmitOnPushIT.java
View File

@ -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());

3
gerrit-server/src/main/java/com/google/gerrit/server/git/MergeOp.java
View File

@ -405,8 +405,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) {