opendev/gerrit
Code Issues Proposed changes

Merge branch 'stable-2.15' into stable-2.16

Browse Source 
* stable-2.15:
  StarredChangesUtil: Fix NPE when ref to be deleted doesn't exist
  StarredChangesUtil: Throw LockFailureException on LOCK_FAILURE
  Add test for creating a change on a non-existing base change
  Rebase: Do not fail with 500 ISE if non-existing change is given as base
  Fix detecting changes of parent trees when computing change kind for merge commit

Change-Id: I8ec9c55d86c3b425e0bce289f4c92cd5b6adbc80
This commit is contained in:
David Pursehouse
2019-08-22 10:57:32 +09:00
parent 89c2b0ddc3 aefd1d5c27
commit c65db0ee1d
6 changed files with 82 additions and 9 deletions
Download Patch File Download Diff File Expand all files Collapse all files

11
java/com/google/gerrit/server/StarredChangesUtil.java
View File

@@ -40,6 +40,7 @@ import com.google.gerrit.server.change.ChangeResource;
import com.google.gerrit.server.config.AllUsersName;
import com.google.gerrit.server.extensions.events.GitReferenceUpdated;
import com.google.gerrit.server.git.GitRepositoryManager;
import com.google.gerrit.server.git.LockFailureException;
import com.google.gerrit.server.index.change.ChangeField;
import com.google.gerrit.server.index.change.ChangeIndexer;
import com.google.gerrit.server.logging.TraceContext;
@@ -248,15 +249,21 @@ public class StarredChangesUtil {
for (Account.Id accountId : byChangeFromIndex(changeId).keySet()) {
String refName = RefNames.refsStarredChanges(changeId, accountId);
Ref ref = repo.getRefDatabase().getRef(refName);
if (ref != null) {
batchUpdate.addCommand(new ReceiveCommand(ref.getObjectId(), ObjectId.zeroId(), refName));
}
}
batchUpdate.execute(rw, NullProgressMonitor.INSTANCE);
for (ReceiveCommand command : batchUpdate.getCommands()) {
if (command.getResult() != ReceiveCommand.Result.OK) {
throw new IOException(
String message =
String.format(
"Unstar change %d failed, ref %s could not be deleted: %s",
changeId.get(), command.getRefName(), command.getResult()));
changeId.get(), command.getRefName(), command.getResult());
if (command.getResult() == ReceiveCommand.Result.LOCK_FAILURE) {
throw new LockFailureException(message, batchUpdate);
}
throw new IOException(message);
}
}
indexer.index(dbProvider.get(), project, changeId);

11
java/com/google/gerrit/server/change/ChangeKindCacheImpl.java
View File

@@ -214,13 +214,13 @@ public class ChangeKindCacheImpl implements ChangeKindCache {
rw.parseBody(next);
if (!next.getFullMessage().equals(prior.getFullMessage())) {
if (isSameDeltaAndTree(prior, next)) {
if (isSameDeltaAndTree(rw, prior, next)) {
return ChangeKind.NO_CODE_CHANGE;
}
return ChangeKind.REWORK;
}
if (isSameDeltaAndTree(prior, next)) {
if (isSameDeltaAndTree(rw, prior, next)) {
return ChangeKind.NO_CHANGE;
}
@@ -284,7 +284,8 @@ public class ChangeKindCacheImpl implements ChangeKindCache {
return FluentIterable.from(Arrays.asList(parents)).skip(1).toSet();
}
private static boolean isSameDeltaAndTree(RevCommit prior, RevCommit next) {
private static boolean isSameDeltaAndTree(RevWalk rw, RevCommit prior, RevCommit next)
throws IOException {
if (!Objects.equals(next.getTree(), prior.getTree())) {
return false;
}
@@ -298,6 +299,10 @@ public class ChangeKindCacheImpl implements ChangeKindCache {
// Make sure that the prior/next delta is the same - not just the tree.
// This is done by making sure that the parent trees are equal.
for (int i = 0; i < prior.getParentCount(); i++) {
// Parse parent commits so that their trees are available.
rw.parseCommit(prior.getParent(i));
rw.parseCommit(next.getParent(i));
if (!Objects.equals(next.getParent(i).getTree(), prior.getParent(i).getTree())) {
return false;
}

10
java/com/google/gerrit/server/restapi/change/Rebase.java
View File

@@ -24,6 +24,7 @@ import com.google.gerrit.extensions.restapi.AuthException;
import com.google.gerrit.extensions.restapi.ResourceConflictException;
import com.google.gerrit.extensions.restapi.RestApiException;
import com.google.gerrit.extensions.restapi.RestModifyView;
import com.google.gerrit.extensions.restapi.UnprocessableEntityException;
import com.google.gerrit.extensions.webui.UiAction;
import com.google.gerrit.reviewdb.client.Branch;
import com.google.gerrit.reviewdb.client.Change;
@@ -159,10 +160,17 @@ public class Rebase extends RetryingRestModifyView<RevisionResource, RebaseInput
return destRef.getObjectId();
}
Base base = rebaseUtil.parseBase(rsrc, str);
Base base;
try {
base = rebaseUtil.parseBase(rsrc, str);
if (base == null) {
throw new ResourceConflictException("base revision is missing: " + str);
}
} catch (NoSuchChangeException e) {
throw new UnprocessableEntityException(
String.format("Base change not found: %s", input.base));
}
PatchSet.Id baseId = base.patchSet().getId();
if (change.getId().equals(baseId.getParentKey())) {
throw new ResourceConflictException("cannot rebase change onto itself");

10
javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java
View File

@@ -952,6 +952,16 @@ public class ChangeIT extends AbstractDaemonTest {
assertThat(ri2.commit.parents.get(0).commit).isEqualTo(r1.getCommit().name());
}
@Test
public void rebaseOnNonExistingChange() throws Exception {
String changeId = createChange().getChangeId();
RebaseInput in = new RebaseInput();
in.base = "999999";
exception.expect(UnprocessableEntityException.class);
exception.expectMessage("Base change not found: " + in.base);
gApi.changes().id(changeId).rebase(in);
}
@Test
public void rebaseNotAllowedWithoutPermission() throws Exception {
// Create two changes both with the same parent

35
javatests/com/google/gerrit/acceptance/api/change/StickyApprovalsIT.java
View File

@@ -247,6 +247,23 @@ public class StickyApprovalsIT extends AbstractDaemonTest {
assertNotSticky(EnumSet.of(REWORK, NO_CODE_CHANGE, TRIVIAL_REBASE));
}
@Test
public void notStickyWithCopyOnNoChangeWhenSecondParentIsUpdated() throws Exception {
try (ProjectConfigUpdate u = updateProject(project)) {
u.getConfig().getLabelSections().get("Code-Review").setCopyAllScoresIfNoChange(true);
u.save();
}
String changeId = createChangeForMergeCommit();
vote(admin, changeId, 2, 1);
vote(user, changeId, -2, -1);
updateSecondParent(changeId);
ChangeInfo c = detailedChange(changeId);
assertVotes(c, admin, 0, 0, null);
assertVotes(c, user, 0, 0, null);
}
@Test
public void removedVotesNotSticky() throws Exception {
try (ProjectConfigUpdate u = updateProject(project)) {
@@ -512,6 +529,24 @@ public class StickyApprovalsIT extends AbstractDaemonTest {
assertThat(getChangeKind(changeId)).isEqualTo(MERGE_FIRST_PARENT_UPDATE);
}
private void updateSecondParent(String changeId) throws Exception {
ChangeInfo c = detailedChange(changeId);
List<CommitInfo> parents = c.revisions.get(c.currentRevision).commit.parents;
String parent1 = parents.get(0).commit;
String parent2 = parents.get(1).commit;
RevCommit commitParent1 = testRepo.getRevWalk().parseCommit(ObjectId.fromString(parent1));
testRepo.reset(parent2);
PushOneCommit.Result newParent2 = createChange("new parent 2", "p2-2.txt", "content 2-2");
PushOneCommit merge = pushFactory.create(db, admin.getIdent(), testRepo, changeId);
merge.setParents(ImmutableList.of(commitParent1, newParent2.getCommit()));
PushOneCommit.Result result = merge.to("refs/for/master");
result.assertOkStatus();
assertThat(getChangeKind(changeId)).isEqualTo(REWORK);
}
private String cherryPick(String changeId, ChangeKind changeKind) throws Exception {
switch (changeKind) {
case REWORK:

8
javatests/com/google/gerrit/acceptance/rest/change/CreateChangeIT.java
View File

@@ -246,6 +246,14 @@ public class CreateChangeIT extends AbstractDaemonTest {
String.format("Commit %s doesn't exist on ref refs/heads/foo", input.baseCommit));
}
@Test
public void createChangeOnNonExistingBaseChangeFails() throws Exception {
ChangeInput input = newChangeInput(ChangeStatus.NEW);
input.baseChange = "999999";
assertCreateFails(
input, UnprocessableEntityException.class, "Base change not found: " + input.baseChange);
}
@Test
public void createChangeWithoutAccessToParentCommitFails() throws Exception {
Map<String, PushOneCommit.Result> results =