Merge changes from topic "rebase-on-closed-change"

* changes:
  Prevent related changes when rebasing on the tip of a branch
  ChangeIT: Check related changes of rebased change
  Support Get Related in extension API
  CommitInfo: Handle root commits in toString()
This commit is contained in:
Dave Borowitz
2018-10-16 15:29:17 +00:00
committed by Gerrit Code Review
10 changed files with 227 additions and 93 deletions

View File

@@ -50,6 +50,7 @@ import com.google.gerrit.common.data.Permission;
import com.google.gerrit.common.data.PermissionRule;
import com.google.gerrit.common.data.PermissionRule.Action;
import com.google.gerrit.extensions.api.GerritApi;
import com.google.gerrit.extensions.api.changes.RelatedChangeAndCommitInfo;
import com.google.gerrit.extensions.api.changes.ReviewInput;
import com.google.gerrit.extensions.api.changes.RevisionApi;
import com.google.gerrit.extensions.api.changes.SubmittedTogetherInfo;
@@ -1714,4 +1715,13 @@ public abstract class AbstractDaemonTest {
comments.sort(Comparator.comparing(c -> c.id));
return comments;
}
protected List<RelatedChangeAndCommitInfo> getRelated(PatchSet.Id ps) throws Exception {
return getRelated(ps.getParentKey(), ps.get());
}
protected List<RelatedChangeAndCommitInfo> getRelated(Change.Id changeId, int ps)
throws Exception {
return gApi.changes().id(changeId.get()).revision(ps).related().changes;
}
}

View File

@@ -0,0 +1,55 @@
// Copyright (C) 2018 The Android Open Source Project
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.
package com.google.gerrit.extensions.api.changes;
import com.google.common.base.MoreObjects;
import com.google.gerrit.extensions.common.CommitInfo;
public class RelatedChangeAndCommitInfo {
public String project;
public String changeId;
public CommitInfo commit;
public Integer _changeNumber;
public Integer _revisionNumber;
public Integer _currentRevisionNumber;
public String status;
public RelatedChangeAndCommitInfo() {}
@Override
public String toString() {
return MoreObjects.toStringHelper(this)
.add("project", project)
.add("changeId", changeId)
.add("commit", toString(commit))
.add("_changeNumber", _changeNumber)
.add("_revisionNumber", _revisionNumber)
.add("_currentRevisionNumber", _currentRevisionNumber)
.add("status", status)
.toString();
}
private static String toString(CommitInfo commit) {
return MoreObjects.toStringHelper(commit)
.add("commit", commit.commit)
.add("parent", commit.parents)
.add("author", commit.author)
.add("committer", commit.committer)
.add("subject", commit.subject)
.add("message", commit.message)
.add("webLinks", commit.webLinks)
.toString();
}
}

View File

@@ -0,0 +1,21 @@
// Copyright (C) 2018 The Android Open Source Project
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.
package com.google.gerrit.extensions.api.changes;
import java.util.List;
public class RelatedChangesInfo {
public List<RelatedChangeAndCommitInfo> changes;
}

View File

@@ -133,6 +133,8 @@ public interface RevisionApi {
MergeListRequest getMergeList() throws RestApiException;
RelatedChangesInfo related() throws RestApiException;
abstract class MergeListRequest {
private boolean addLinks;
private int uninterestingParent = 1;
@@ -370,6 +372,11 @@ public interface RevisionApi {
throw new NotImplementedException();
}
@Override
public RelatedChangesInfo related() throws RestApiException {
throw new NotImplementedException();
}
@Override
public void description(String description) throws RestApiException {
throw new NotImplementedException();

View File

@@ -53,7 +53,9 @@ public class CommitInfo {
// Using something like the raw commit format might be nice, but we can't depend on JGit here.
StringBuilder sb = new StringBuilder().append(getClass().getSimpleName()).append('{');
sb.append(commit);
sb.append(", parents=").append(parents.stream().map(p -> p.commit).collect(joining(", ")));
if (parents != null) {
sb.append(", parents=").append(parents.stream().map(p -> p.commit).collect(joining(", ")));
}
sb.append(", author=").append(author);
sb.append(", committer=").append(committer);
sb.append(", subject=").append(subject);

View File

@@ -26,6 +26,7 @@ import com.google.gerrit.extensions.api.changes.DraftApi;
import com.google.gerrit.extensions.api.changes.DraftInput;
import com.google.gerrit.extensions.api.changes.FileApi;
import com.google.gerrit.extensions.api.changes.RebaseInput;
import com.google.gerrit.extensions.api.changes.RelatedChangesInfo;
import com.google.gerrit.extensions.api.changes.ReviewInput;
import com.google.gerrit.extensions.api.changes.ReviewResult;
import com.google.gerrit.extensions.api.changes.RevisionApi;
@@ -64,6 +65,7 @@ import com.google.gerrit.server.restapi.change.GetCommit;
import com.google.gerrit.server.restapi.change.GetDescription;
import com.google.gerrit.server.restapi.change.GetMergeList;
import com.google.gerrit.server.restapi.change.GetPatch;
import com.google.gerrit.server.restapi.change.GetRelated;
import com.google.gerrit.server.restapi.change.GetRevisionActions;
import com.google.gerrit.server.restapi.change.ListRevisionComments;
import com.google.gerrit.server.restapi.change.ListRevisionDrafts;
@@ -129,6 +131,7 @@ class RevisionApiImpl implements RevisionApi {
private final TestSubmitType.Get getSubmitType;
private final Provider<TestSubmitRule> testSubmitRule;
private final Provider<GetMergeList> getMergeList;
private final GetRelated getRelated;
private final PutDescription putDescription;
private final GetDescription getDescription;
@@ -169,6 +172,7 @@ class RevisionApiImpl implements RevisionApi {
TestSubmitType.Get getSubmitType,
Provider<TestSubmitRule> testSubmitRule,
Provider<GetMergeList> getMergeList,
GetRelated getRelated,
PutDescription putDescription,
GetDescription getDescription,
@Assisted RevisionResource r) {
@@ -207,6 +211,7 @@ class RevisionApiImpl implements RevisionApi {
this.getSubmitType = getSubmitType;
this.testSubmitRule = testSubmitRule;
this.getMergeList = getMergeList;
this.getRelated = getRelated;
this.putDescription = putDescription;
this.getDescription = getDescription;
this.revision = r;
@@ -589,6 +594,15 @@ class RevisionApiImpl implements RevisionApi {
};
}
@Override
public RelatedChangesInfo related() throws RestApiException {
try {
return getRelated.apply(revision);
} catch (Exception e) {
throw asRestApiException("Cannot get related changes", e);
}
}
@Override
public void description(String description) throws RestApiException {
DescriptionInput in = new DescriptionInput();

View File

@@ -20,12 +20,14 @@ import com.google.gerrit.extensions.api.changes.NotifyHandling;
import com.google.gerrit.extensions.restapi.MergeConflictException;
import com.google.gerrit.extensions.restapi.ResourceConflictException;
import com.google.gerrit.extensions.restapi.RestApiException;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.client.PatchSet;
import com.google.gerrit.reviewdb.client.RevId;
import com.google.gerrit.server.ChangeUtil;
import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.change.RebaseUtil.Base;
import com.google.gerrit.server.git.GroupCollector;
import com.google.gerrit.server.git.MergeUtil;
import com.google.gerrit.server.notedb.ChangeNotes;
import com.google.gerrit.server.permissions.PermissionBackendException;
@@ -199,8 +201,14 @@ public class RebaseChangeOp implements BatchUpdateOp {
+ " was rebased");
}
if (base != null) {
patchSetInserter.setGroups(base.patchSet().getGroups());
if (base != null && base.notes().getChange().getStatus() != Change.Status.MERGED) {
if (base.notes().getChange().getStatus() != Change.Status.MERGED) {
// Add to end of relation chain for open base change.
patchSetInserter.setGroups(base.patchSet().getGroups());
} else {
// If the base is merged, start a new relation chain.
patchSetInserter.setGroups(GroupCollector.getDefaultGroups(rebasedCommit));
}
}
patchSetInserter.updateRepo(ctx);
}

View File

@@ -17,9 +17,10 @@ package com.google.gerrit.server.restapi.change;
import static java.util.stream.Collectors.toSet;
import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.MoreObjects;
import com.google.common.collect.Lists;
import com.google.gerrit.common.Nullable;
import com.google.gerrit.extensions.api.changes.RelatedChangeAndCommitInfo;
import com.google.gerrit.extensions.api.changes.RelatedChangesInfo;
import com.google.gerrit.extensions.common.CommitInfo;
import com.google.gerrit.extensions.restapi.RestReadView;
import com.google.gerrit.index.IndexConfig;
@@ -70,15 +71,15 @@ public class GetRelated implements RestReadView<RevisionResource> {
}
@Override
public RelatedInfo apply(RevisionResource rsrc)
public RelatedChangesInfo apply(RevisionResource rsrc)
throws RepositoryNotFoundException, IOException, OrmException, NoSuchProjectException,
PermissionBackendException {
RelatedInfo relatedInfo = new RelatedInfo();
relatedInfo.changes = getRelated(rsrc);
return relatedInfo;
RelatedChangesInfo relatedChangesInfo = new RelatedChangesInfo();
relatedChangesInfo.changes = getRelated(rsrc);
return relatedChangesInfo;
}
private List<ChangeAndCommit> getRelated(RevisionResource rsrc)
private List<RelatedChangeAndCommitInfo> getRelated(RevisionResource rsrc)
throws OrmException, IOException, PermissionBackendException {
Set<String> groups = getAllGroups(rsrc.getNotes(), db.get(), psUtil);
if (groups.isEmpty()) {
@@ -94,7 +95,7 @@ public class GetRelated implements RestReadView<RevisionResource> {
if (cds.size() == 1 && cds.get(0).getId().equals(rsrc.getChange().getId())) {
return Collections.emptyList();
}
List<ChangeAndCommit> result = new ArrayList<>(cds.size());
List<RelatedChangeAndCommitInfo> result = new ArrayList<>(cds.size());
boolean isEdit = rsrc.getEdit().isPresent();
PatchSet basePs = isEdit ? rsrc.getEdit().get().getBasePatchSet() : rsrc.getPatchSet();
@@ -111,11 +112,11 @@ public class GetRelated implements RestReadView<RevisionResource> {
} else {
commit = d.commit();
}
result.add(new ChangeAndCommit(rsrc.getProject(), d.data().change(), ps, commit));
result.add(newChangeAndCommit(rsrc.getProject(), d.data().change(), ps, commit));
}
if (result.size() == 1) {
ChangeAndCommit r = result.get(0);
RelatedChangeAndCommitInfo r = result.get(0);
if (r.commit != null && r.commit.commit.equals(rsrc.getPatchSet().getRevision().get())) {
return Collections.emptyList();
}
@@ -143,69 +144,30 @@ public class GetRelated implements RestReadView<RevisionResource> {
}
}
public static class RelatedInfo {
public List<ChangeAndCommit> changes;
}
static RelatedChangeAndCommitInfo newChangeAndCommit(
Project.NameKey project, @Nullable Change change, @Nullable PatchSet ps, RevCommit c) {
RelatedChangeAndCommitInfo info = new RelatedChangeAndCommitInfo();
info.project = project.get();
public static class ChangeAndCommit {
public String project;
public String changeId;
public CommitInfo commit;
public Integer _changeNumber;
public Integer _revisionNumber;
public Integer _currentRevisionNumber;
public String status;
public ChangeAndCommit() {}
ChangeAndCommit(
Project.NameKey project, @Nullable Change change, @Nullable PatchSet ps, RevCommit c) {
this.project = project.get();
if (change != null) {
changeId = change.getKey().get();
_changeNumber = change.getChangeId();
_revisionNumber = ps != null ? ps.getPatchSetId() : null;
PatchSet.Id curr = change.currentPatchSetId();
_currentRevisionNumber = curr != null ? curr.get() : null;
status = change.getStatus().asChangeStatus().toString();
}
commit = new CommitInfo();
commit.commit = c.name();
commit.parents = Lists.newArrayListWithCapacity(c.getParentCount());
for (int i = 0; i < c.getParentCount(); i++) {
CommitInfo p = new CommitInfo();
p.commit = c.getParent(i).name();
commit.parents.add(p);
}
commit.author = CommonConverters.toGitPerson(c.getAuthorIdent());
commit.subject = c.getShortMessage();
if (change != null) {
info.changeId = change.getKey().get();
info._changeNumber = change.getChangeId();
info._revisionNumber = ps != null ? ps.getPatchSetId() : null;
PatchSet.Id curr = change.currentPatchSetId();
info._currentRevisionNumber = curr != null ? curr.get() : null;
info.status = change.getStatus().asChangeStatus().toString();
}
@Override
public String toString() {
return MoreObjects.toStringHelper(this)
.add("project", project)
.add("changeId", changeId)
.add("commit", toString(commit))
.add("_changeNumber", _changeNumber)
.add("_revisionNumber", _revisionNumber)
.add("_currentRevisionNumber", _currentRevisionNumber)
.add("status", status)
.toString();
}
private static String toString(CommitInfo commit) {
return MoreObjects.toStringHelper(commit)
.add("commit", commit.commit)
.add("parent", commit.parents)
.add("author", commit.author)
.add("committer", commit.committer)
.add("subject", commit.subject)
.add("message", commit.message)
.add("webLinks", commit.webLinks)
.toString();
info.commit = new CommitInfo();
info.commit.commit = c.name();
info.commit.parents = Lists.newArrayListWithCapacity(c.getParentCount());
for (int i = 0; i < c.getParentCount(); i++) {
CommitInfo p = new CommitInfo();
p.commit = c.getParent(i).name();
info.commit.parents.add(p);
}
info.commit.author = CommonConverters.toGitPerson(c.getAuthorIdent());
info.commit.subject = c.getShortMessage();
return info;
}
}

View File

@@ -80,6 +80,7 @@ import com.google.gerrit.extensions.api.changes.NotifyHandling;
import com.google.gerrit.extensions.api.changes.NotifyInfo;
import com.google.gerrit.extensions.api.changes.RebaseInput;
import com.google.gerrit.extensions.api.changes.RecipientType;
import com.google.gerrit.extensions.api.changes.RelatedChangeAndCommitInfo;
import com.google.gerrit.extensions.api.changes.RevertInput;
import com.google.gerrit.extensions.api.changes.ReviewInput;
import com.google.gerrit.extensions.api.changes.ReviewInput.DraftHandling;
@@ -943,13 +944,77 @@ public class ChangeIT extends AbstractDaemonTest {
RevisionInfo ri2 = ci2.revisions.get(ci2.currentRevision);
assertThat(ri2.commit.parents.get(0).commit).isEqualTo(branchTip);
Change.Id id1 = r1.getChange().getId();
RebaseInput in = new RebaseInput();
in.base = Integer.toString(r1.getChange().getId().get());
in.base = id1.toString();
gApi.changes().id(r2.getChangeId()).rebase(in);
Change.Id id2 = r2.getChange().getId();
ci2 = get(r2.getChangeId(), CURRENT_REVISION, CURRENT_COMMIT);
ri2 = ci2.revisions.get(ci2.currentRevision);
assertThat(ri2.commit.parents.get(0).commit).isEqualTo(r1.getCommit().name());
List<RelatedChangeAndCommitInfo> related = getRelated(id2, ri2._number);
assertThat(related).hasSize(2);
assertThat(related.get(0)._changeNumber).isEqualTo(id2.get());
assertThat(related.get(0)._revisionNumber).isEqualTo(2);
assertThat(related.get(1)._changeNumber).isEqualTo(id1.get());
assertThat(related.get(1)._revisionNumber).isEqualTo(1);
}
@Test
public void rebaseOnClosedChange() throws Exception {
String branchTip = testRepo.getRepository().exactRef("HEAD").getObjectId().name();
PushOneCommit.Result r1 = createChange();
testRepo.reset("HEAD~1");
PushOneCommit.Result r2 = createChange();
ChangeInfo ci2 = get(r2.getChangeId(), CURRENT_REVISION, CURRENT_COMMIT);
RevisionInfo ri2 = ci2.revisions.get(ci2.currentRevision);
assertThat(ri2.commit.parents.get(0).commit).isEqualTo(branchTip);
// Submit first change.
Change.Id id1 = r1.getChange().getId();
gApi.changes().id(id1.get()).current().review(ReviewInput.approve());
gApi.changes().id(id1.get()).current().submit();
// Rebase second change on first change.
RebaseInput in = new RebaseInput();
in.base = id1.toString();
gApi.changes().id(r2.getChangeId()).rebase(in);
Change.Id id2 = r2.getChange().getId();
ci2 = get(r2.getChangeId(), CURRENT_REVISION, CURRENT_COMMIT);
ri2 = ci2.revisions.get(ci2.currentRevision);
assertThat(ri2.commit.parents.get(0).commit).isEqualTo(r1.getCommit().name());
assertThat(getRelated(id2, ri2._number)).isEmpty();
}
@Test
public void rebaseFromRelationChainToClosedChange() throws Exception {
PushOneCommit.Result r1 = createChange();
testRepo.reset("HEAD~1");
createChange();
PushOneCommit.Result r3 = createChange();
// Submit first change.
Change.Id id1 = r1.getChange().getId();
gApi.changes().id(id1.get()).current().review(ReviewInput.approve());
gApi.changes().id(id1.get()).current().submit();
// Rebase third change on first change.
RebaseInput in = new RebaseInput();
in.base = id1.toString();
gApi.changes().id(r3.getChangeId()).rebase(in);
Change.Id id3 = r3.getChange().getId();
ChangeInfo ci3 = get(r3.getChangeId(), CURRENT_REVISION, CURRENT_COMMIT);
RevisionInfo ri3 = ci3.revisions.get(ci3.currentRevision);
assertThat(ri3.commit.parents.get(0).commit).isEqualTo(r1.getCommit().name());
assertThat(getRelated(id3, ri3._number)).isEmpty();
}
@Test

View File

@@ -24,9 +24,10 @@ import com.google.common.collect.ImmutableList;
import com.google.common.collect.Iterables;
import com.google.gerrit.acceptance.AbstractDaemonTest;
import com.google.gerrit.acceptance.GerritConfig;
import com.google.gerrit.acceptance.NoHttpd;
import com.google.gerrit.acceptance.PushOneCommit;
import com.google.gerrit.acceptance.RestResponse;
import com.google.gerrit.common.RawInputUtil;
import com.google.gerrit.extensions.api.changes.RelatedChangeAndCommitInfo;
import com.google.gerrit.extensions.common.CommitInfo;
import com.google.gerrit.extensions.common.EditInfo;
import com.google.gerrit.index.IndexConfig;
@@ -35,8 +36,6 @@ import com.google.gerrit.reviewdb.client.PatchSet;
import com.google.gerrit.server.query.change.ChangeData;
import com.google.gerrit.server.restapi.change.ChangesCollection;
import com.google.gerrit.server.restapi.change.GetRelated;
import com.google.gerrit.server.restapi.change.GetRelated.ChangeAndCommit;
import com.google.gerrit.server.restapi.change.GetRelated.RelatedInfo;
import com.google.gerrit.server.update.BatchUpdate;
import com.google.gerrit.server.update.BatchUpdateOp;
import com.google.gerrit.server.update.ChangeContext;
@@ -56,6 +55,7 @@ import org.junit.After;
import org.junit.Before;
import org.junit.Test;
@NoHttpd
public class GetRelatedIT extends AbstractDaemonTest {
private static final int MAX_TERMS = 10;
@@ -577,17 +577,6 @@ public class GetRelatedIT extends AbstractDaemonTest {
assertRelated(cd.change().currentPatchSetId());
}
private List<ChangeAndCommit> getRelated(PatchSet.Id ps) throws Exception {
return getRelated(ps.getParentKey(), ps.get());
}
private List<ChangeAndCommit> getRelated(Change.Id changeId, int ps) throws Exception {
String url = String.format("/changes/%d/revisions/%d/related", changeId.get(), ps);
RestResponse r = adminRestSession.get(url);
r.assertOK();
return newGson().fromJson(r.getReader(), RelatedInfo.class).changes;
}
private RevCommit parseBody(RevCommit c) throws Exception {
testRepo.getRevWalk().parseBody(c);
return c;
@@ -601,9 +590,9 @@ public class GetRelatedIT extends AbstractDaemonTest {
return Iterables.getOnlyElement(queryProvider.get().byCommit(c));
}
private ChangeAndCommit changeAndCommit(
private RelatedChangeAndCommitInfo changeAndCommit(
PatchSet.Id psId, ObjectId commitId, int currentRevisionNum) {
ChangeAndCommit result = new ChangeAndCommit();
RelatedChangeAndCommitInfo result = new RelatedChangeAndCommitInfo();
result.project = project.get();
result._changeNumber = psId.getParentKey().get();
result.commit = new CommitInfo();
@@ -631,13 +620,14 @@ public class GetRelatedIT extends AbstractDaemonTest {
}
}
private void assertRelated(PatchSet.Id psId, ChangeAndCommit... expected) throws Exception {
List<ChangeAndCommit> actual = getRelated(psId);
private void assertRelated(PatchSet.Id psId, RelatedChangeAndCommitInfo... expected)
throws Exception {
List<RelatedChangeAndCommitInfo> actual = getRelated(psId);
assertThat(actual).named("related to " + psId).hasSize(expected.length);
for (int i = 0; i < actual.size(); i++) {
String name = "index " + i + " related to " + psId;
ChangeAndCommit a = actual.get(i);
ChangeAndCommit e = expected[i];
RelatedChangeAndCommitInfo a = actual.get(i);
RelatedChangeAndCommitInfo e = expected[i];
assertThat(a.project).named("project of " + name).isEqualTo(e.project);
assertThat(a._changeNumber).named("change ID of " + name).isEqualTo(e._changeNumber);
// Don't bother checking changeId; assume _changeNumber is sufficient.