opendev/gerrit
Code Issues Proposed changes

Merge branch 'stable-2.12'

Browse Source 
* stable-2.12:
  Fix missing query results when label=0 and related relational operators
  OutputStreamQuery: Only return current patch set when visible to user
  Fix NPE in SubmitRuleEvaluator
  Protect the TrackingFooters.extract from null input parameter
  Fix index-activate and index-start documentation
  Reapply: Revert "Use changeRefsById to track existing revisions"

Change-Id: I9f8e82f38c2ec4cf273a6a29c7dc634d8c28b6c1
This commit is contained in:
David Pursehouse 2016-04-27 21:11:13 +09:00
commit d29e0194d6
11 changed files with 209 additions and 60 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

2
Documentation/cmd-index-activate.txt
View File

@ -5,7 +5,7 @@ gerrit index activate - Activate the latest index version available
== SYNOPSIS
--
'ssh' -p @SSH_PORT@ @SSH_HOST@ 'gerrit index activate <index>'
'ssh' -p <port> <host> 'gerrit index activate <index>'
--
== DESCRIPTION

2
Documentation/cmd-index-start.txt
View File

@ -5,7 +5,7 @@ gerrit index start - Start the online indexer
== SYNOPSIS
--
'ssh' -p @SSH_PORT@ @SSH_HOST@ 'gerrit index start <index>'
'ssh' -p <port> <host> 'gerrit index start <index>'
--
== DESCRIPTION

14
Documentation/cmd-query.txt
View File

@ -54,15 +54,17 @@ command line parser in the server).
--current-patch-set::
Include information about the current patch set in the results.
Note that the information will only be included when the current
patch set is visible to the caller.
--patch-sets::
Include information about all patch sets. If combined with
the --current-patch-set flag then the current patch set
information will be output twice, once in each field.
Include information about all patch sets visible to the caller.
If combined with the --current-patch-set flag then the current patch
set information will be output twice, once in each field.
--all-approvals::
Include information about all patch sets along with the
approval information for each patch set. If combined with
Include information about all patch sets visible to the caller along
with the approval information for each patch set. If combined with
the --current-patch-set flag then the current patch set
information will be output twice, once in each field.
@ -76,7 +78,7 @@ command line parser in the server).
--comments::
Include comments for all changes. If combined with the
--patch-sets flag then all inline/file comments are included for
each patch set.
each patch set that is visible to the caller.
--commit-message::
Include the full commit message in the change description.

5
gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/AbstractDaemonTest.java
View File

@ -496,6 +496,11 @@ public abstract class AbstractDaemonTest {
revision(r).submit();
}
protected PushOneCommit.Result amendChangeAsDraft(String changeId)
throws Exception {
return amendChange(changeId, "refs/drafts/master");
}
protected ChangeInfo info(String id)
throws RestApiException {
return gApi.changes().id(id).info();

36
gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/AbstractPushForReview.java
View File

@ -32,6 +32,7 @@ import com.google.gerrit.common.data.LabelType;
import com.google.gerrit.common.data.Permission;
import com.google.gerrit.extensions.api.changes.ReviewInput;
import com.google.gerrit.extensions.api.changes.ReviewInput.NotifyHandling;
import com.google.gerrit.extensions.api.projects.BranchInput;
import com.google.gerrit.extensions.client.InheritableBoolean;
import com.google.gerrit.extensions.common.ChangeInfo;
import com.google.gerrit.extensions.common.ChangeMessageInfo;
@ -46,12 +47,14 @@ import com.google.gerrit.testutil.FakeEmailSender.Message;
import com.google.gerrit.testutil.TestTimeUtil;
import org.eclipse.jgit.revwalk.RevCommit;
import org.eclipse.jgit.transport.PushResult;
import org.junit.AfterClass;
import org.junit.Before;
import org.junit.BeforeClass;
import org.junit.Test;
import java.util.Collection;
import java.util.List;
import java.util.Set;
public abstract class AbstractPushForReview extends AbstractDaemonTest {
@ -488,4 +491,37 @@ public abstract class AbstractPushForReview extends AbstractDaemonTest {
r.assertErrorStatus(
"not Signed-off-by author/committer/uploader in commit message footer");
}
@Test
public void testPushSameCommitTwiceUsingMagicBranchBaseOption()
throws Exception {
grant(Permission.PUSH, project, "refs/heads/master");
PushOneCommit.Result rBase = pushTo("refs/heads/master");
rBase.assertOkStatus();
gApi.projects()
.name(project.get())
.branch("foo")
.create(new BranchInput());
PushOneCommit push =
pushFactory.create(db, admin.getIdent(), testRepo, PushOneCommit.SUBJECT,
"b.txt", "anotherContent");
PushOneCommit.Result r = push.to("refs/for/master");
r.assertOkStatus();
PushResult pr = GitUtil.pushHead(
testRepo, "refs/for/foo%base=" + rBase.getCommit().name(), false, false);
assertThat(pr.getMessages()).contains("changes: new: 1, refs: 1, done");
List<ChangeInfo> changes = query(r.getCommit().name());
assertThat(changes).hasSize(2);
ChangeInfo c1 = get(changes.get(0).id);
ChangeInfo c2 = get(changes.get(1).id);
assertThat(c1.project).isEqualTo(c2.project);
assertThat(c1.branch).isNotEqualTo(c2.branch);
assertThat(c1.changeId).isEqualTo(c2.changeId);
assertThat(c1.currentRevision).isEqualTo(c2.currentRevision);
}
}

38
gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/ssh/QueryIT.java
View File

@ -16,11 +16,13 @@ package com.google.gerrit.acceptance.ssh;
import static com.google.common.truth.Truth.assertThat;
import static com.google.common.truth.Truth.assert_;
import static com.google.gerrit.acceptance.GitUtil.initSsh;
import com.google.common.collect.Lists;
import com.google.gerrit.acceptance.AbstractDaemonTest;
import com.google.gerrit.acceptance.NoHttpd;
import com.google.gerrit.acceptance.PushOneCommit;
import com.google.gerrit.acceptance.SshSession;
import com.google.gerrit.extensions.api.changes.AddReviewerInput;
import com.google.gerrit.extensions.api.changes.ReviewInput;
import com.google.gerrit.extensions.client.Side;
@ -300,13 +302,39 @@ public class QueryIT extends AbstractDaemonTest {
assertThat(changes.get(0).submitRecords.size()).isEqualTo(1);
}
@Test
public void testQueryWithNonVisibleCurrentPatchSet() throws Exception {
String changeId = createChange().getChangeId();
amendChangeAsDraft(changeId);
String query = "--current-patch-set --patch-sets " + changeId;
List<ChangeAttribute> changes = executeSuccessfulQuery(query);
assertThat(changes.size()).isEqualTo(1);
assertThat(changes.get(0).patchSets).isNotNull();
assertThat(changes.get(0).patchSets).hasSize(2);
assertThat(changes.get(0).currentPatchSet).isNotNull();
SshSession userSession = new SshSession(server, user);
initSsh(user);
userSession.open();
changes = executeSuccessfulQuery(query, userSession);
assertThat(changes.size()).isEqualTo(1);
assertThat(changes.get(0).patchSets).hasSize(1);
assertThat(changes.get(0).currentPatchSet).isNull();
userSession.close();
}
private List<ChangeAttribute> executeSuccessfulQuery(String params,
SshSession session) throws Exception {
String rawResponse =
session.exec("gerrit query --format=JSON " + params);
assert_().withFailureMessage(session.getError())
.that(session.hasError()).isFalse();
return getChanges(rawResponse);
}
private List<ChangeAttribute> executeSuccessfulQuery(String params)
throws Exception {
String rawResponse =
adminSshSession.exec("gerrit query --format=JSON " + params);
assert_().withFailureMessage(adminSshSession.getError())
.that(adminSshSession.hasError()).isFalse();
return getChanges(rawResponse);
return executeSuccessfulQuery(params, adminSshSession);
}
private static List<ChangeAttribute> getChanges(String rawResponse) {

27
gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java
View File

@ -1503,8 +1503,8 @@ public class ReceiveCommits {
private void selectNewAndReplacedChangesFromMagicBranch() {
newChanges = Lists.newArrayList();
SetMultimap<ObjectId, Ref> existing = changeRefsById();
GroupCollector groupCollector = GroupCollector.create(refsById, db, psUtil,
SetMultimap<ObjectId, Ref> existing = HashMultimap.create();
GroupCollector groupCollector = GroupCollector.create(changeRefsById(), db, psUtil,
notesFactory, project.getNameKey());
rp.getRevWalk().reset();
@ -1525,6 +1525,7 @@ public class ReceiveCommits {
} else {
markHeadsAsUninteresting(
rp.getRevWalk(),
existing,
magicBranch.ctl != null ? magicBranch.ctl.getRefName() : null);
}
@ -1681,15 +1682,23 @@ public class ReceiveCommits {
}
}
private void markHeadsAsUninteresting(RevWalk rw, @Nullable String forRef) {
private void markHeadsAsUninteresting(
final RevWalk walk,
SetMultimap<ObjectId, Ref> existing,
@Nullable String forRef) {
for (Ref ref : allRefs.values()) {
if ((ref.getName().startsWith(R_HEADS) || ref.getName().equals(forRef))
&& ref.getObjectId() != null) {
if (ref.getObjectId() == null) {
continue;
} else if (ref.getName().startsWith(REFS_CHANGES)) {
existing.put(ref.getObjectId(), ref);
} else if (ref.getName().startsWith(R_HEADS)
|| (forRef != null && forRef.equals(ref.getName()))) {
try {
rw.markUninteresting(rw.parseCommit(ref.getObjectId()));
walk.markUninteresting(walk.parseCommit(ref.getObjectId()));
} catch (IOException e) {
log.warn(String.format("Invalid ref %s in %s",
ref.getName(), project.getName()), e);
continue;
}
}
}
@ -2332,11 +2341,11 @@ public class ReceiveCommits {
if (!(parsedObject instanceof RevCommit)) {
return;
}
SetMultimap<ObjectId, Ref> existing = HashMultimap.create();
walk.markStart((RevCommit)parsedObject);
markHeadsAsUninteresting(walk, cmd.getRefName());
Set<ObjectId> existing = changeRefsById().keySet();
markHeadsAsUninteresting(walk, existing, cmd.getRefName());
for (RevCommit c; (c = walk.next()) != null;) {
if (existing.contains(c)) {
if (existing.keySet().contains(c)) {
continue;
} else if (!validCommit(walk, ctl, cmd, c)) {
break;

22
gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeData.java
View File

@ -21,6 +21,8 @@ import static com.google.gerrit.server.ApprovalsUtil.sortApprovals;
import com.google.auto.value.AutoValue;
import com.google.common.base.MoreObjects;
import com.google.common.base.Optional;
import com.google.common.base.Predicate;
import com.google.common.collect.FluentIterable;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.Iterables;
import com.google.common.collect.ListMultimap;
@ -833,13 +835,29 @@ public class ChangeData {
return patchSets;
}
public void setPatchSets(List<PatchSet> patchSets) {
/**
* @return patches for the change visible to the current user.
* @throws OrmException an error occurred reading the database.
*/
public Collection<PatchSet> visiblePatchSets() throws OrmException {
return FluentIterable.from(patchSets()).filter(new Predicate<PatchSet>() {
@Override
public boolean apply(PatchSet input) {
try {
return changeControl().isPatchVisible(input, db);
} catch (OrmException e) {
return false;
}
}}).toList();
}
public void setPatchSets(Collection<PatchSet> patchSets) {
this.currentPatchSet = null;
this.patchSets = patchSets;
}
/**
* @return patch set with the given ID, or null if it does not exist.
* @return patch with the given ID, or null if it does not exist.
* @throws OrmException an error occurred reading the database.
*/
public PatchSet patchSet(PatchSet.Id psId) throws OrmException {

6
gerrit-server/src/main/java/com/google/gerrit/server/query/change/LabelPredicate.java
View File

@ -141,10 +141,10 @@ public class LabelPredicate extends OrPredicate<ChangeData> {
List<Predicate<ChangeData>> r =
Lists.newArrayListWithCapacity(2 * MAX_LABEL_VALUE);
for (int i = 1; i <= MAX_LABEL_VALUE; i++) {
r.add(not(equalsLabelPredicate(args, label, i)));
r.add(not(equalsLabelPredicate(args, label, -i)));
r.add(equalsLabelPredicate(args, label, i));
r.add(equalsLabelPredicate(args, label, -i));
}
return and(r);
return not(or(r));
}
private static Predicate<ChangeData> equalsLabelPredicate(Args args,

6
gerrit-server/src/main/java/com/google/gerrit/server/query/change/OutputStreamQuery.java
View File

@ -275,14 +275,14 @@ public class OutputStreamQuery {
}
if (includePatchSets) {
eventFactory.addPatchSets(db, rw, c, d.patchSets(),
eventFactory.addPatchSets(db, rw, c, d.visiblePatchSets(),
includeApprovals ? d.approvals().asMap() : null,
includeFiles, d.change(), labelTypes);
}
if (includeCurrentPatchSet) {
PatchSet current = d.currentPatchSet();
if (current != null) {
if (current != null && cc.isPatchVisible(current, d.db())) {
c.currentPatchSet =
eventFactory.asPatchSetAttribute(db, rw, d.change(), current);
eventFactory.addApprovals(c.currentPatchSet,
@ -302,7 +302,7 @@ public class OutputStreamQuery {
if (includeComments) {
eventFactory.addComments(c, d.messages());
if (includePatchSets) {
eventFactory.addPatchSets(db, rw, c, d.patchSets(),
eventFactory.addPatchSets(db, rw, c, d.visiblePatchSets(),
includeApprovals ? d.approvals().asMap() : null,
includeFiles, d.change(), labelTypes);
for (PatchSetAttribute attribute : c.patchSets) {

111
gerrit-server/src/test/java/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java
View File

@ -95,6 +95,7 @@ import org.junit.Test;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Iterator;
import java.util.LinkedHashMap;
import java.util.List;
import java.util.Map;
import java.util.concurrent.TimeUnit;
@ -536,47 +537,97 @@ public abstract class AbstractQueryChangesTest extends GerritServerTests {
public void byLabel() throws Exception {
accountManager.authenticate(AuthRequest.forUser("anotheruser"));
TestRepository<Repo> repo = createProject("repo");
ChangeInserter ins = newChange(repo);
Change change = insert(repo, ins);
ChangeInserter ins = newChange(repo, null, null, null, null);
ChangeInserter ins2 = newChange(repo, null, null, null, null);
ChangeInserter ins3 = newChange(repo, null, null, null, null);
ChangeInserter ins4 = newChange(repo, null, null, null, null);
ChangeInserter ins5 = newChange(repo, null, null, null, null);
Change reviewMinus2Change = insert(repo, ins);
gApi.changes().id(reviewMinus2Change.getId().get()).current()
.review(ReviewInput.reject());
Change reviewMinus1Change = insert(repo, ins2);
gApi.changes().id(reviewMinus1Change.getId().get()).current()
.review(ReviewInput.dislike());
Change noLabelChange = insert(repo, ins3);
Change reviewPlus1Change = insert(repo, ins4);
gApi.changes().id(reviewPlus1Change.getId().get()).current()
.review(ReviewInput.recommend());
Change reviewPlus2Change = insert(repo, ins5);
gApi.changes().id(reviewPlus2Change.getId().get()).current()
.review(ReviewInput.approve());
gApi.changes().id(change.getId().get()).current()
.review(new ReviewInput().label("Code-Review", 1));
Map<String, Short> m = gApi.changes()
.id(change.getId().get())
.id(reviewPlus1Change.getId().get())
.reviewer(user.getAccountId().toString())
.votes();
assertThat(m).hasSize(1);
assertThat(m).containsEntry("Code-Review", new Short((short)1));
assertQuery("label:Code-Review=-2");
assertQuery("label:Code-Review-2");
assertQuery("label:Code-Review=-1");
assertQuery("label:Code-Review-1");
assertQuery("label:Code-Review=0");
assertQuery("label:Code-Review=+1", change);
assertQuery("label:Code-Review=1", change);
assertQuery("label:Code-Review+1", change);
assertQuery("label:Code-Review=+2");
assertQuery("label:Code-Review=2");
assertQuery("label:Code-Review+2");
Map<Integer, Change> changes = new LinkedHashMap<>(5);
changes.put(2, reviewPlus2Change);
changes.put(1, reviewPlus1Change);
changes.put(0, noLabelChange);
changes.put(-1, reviewMinus1Change);
changes.put(-2, reviewMinus2Change);
assertQuery("label:Code-Review>=0", change);
assertQuery("label:Code-Review>0", change);
assertQuery("label:Code-Review>=1", change);
assertQuery("label:Code-Review>1");
assertQuery("label:Code-Review>=2");
assertQuery("label:Code-Review=-2", reviewMinus2Change);
assertQuery("label:Code-Review-2", reviewMinus2Change);
assertQuery("label:Code-Review=-1", reviewMinus1Change);
assertQuery("label:Code-Review-1", reviewMinus1Change);
assertQuery("label:Code-Review=0", noLabelChange);
assertQuery("label:Code-Review=+1", reviewPlus1Change);
assertQuery("label:Code-Review=1", reviewPlus1Change);
assertQuery("label:Code-Review+1", reviewPlus1Change);
assertQuery("label:Code-Review=+2", reviewPlus2Change);
assertQuery("label:Code-Review=2", reviewPlus2Change);
assertQuery("label:Code-Review+2", reviewPlus2Change);
assertQuery("label: Code-Review<=2", change);
assertQuery("label: Code-Review<2", change);
assertQuery("label: Code-Review<=1", change);
assertQuery("label:Code-Review<1");
assertQuery("label:Code-Review<=0");
assertQuery("label:Code-Review>-3", codeReviewInRange(changes, -2, 2));
assertQuery("label:Code-Review>=-2", codeReviewInRange(changes, -2, 2));
assertQuery("label:Code-Review>-2", codeReviewInRange(changes, -1, 2));
assertQuery("label:Code-Review>=-1", codeReviewInRange(changes, -1, 2));
assertQuery("label:Code-Review>-1", codeReviewInRange(changes, 0, 2));
assertQuery("label:Code-Review>=0", codeReviewInRange(changes, 0, 2));
assertQuery("label:Code-Review>0", codeReviewInRange(changes, 1, 2));
assertQuery("label:Code-Review>=1", codeReviewInRange(changes, 1, 2));
assertQuery("label:Code-Review>1", reviewPlus2Change);
assertQuery("label:Code-Review>=2", reviewPlus2Change);
assertQuery("label:Code-Review>2");
assertQuery("label:Code-Review<=2", codeReviewInRange(changes, -2, 2));
assertQuery("label:Code-Review<2", codeReviewInRange(changes, -2, 1));
assertQuery("label:Code-Review<=1", codeReviewInRange(changes, -2, 1));
assertQuery("label:Code-Review<1", codeReviewInRange(changes, -2, 0));
assertQuery("label:Code-Review<=0", codeReviewInRange(changes, -2, 0));
assertQuery("label:Code-Review<0", codeReviewInRange(changes, -2, -1));
assertQuery("label:Code-Review<=-1", codeReviewInRange(changes, -2, -1));
assertQuery("label:Code-Review<-1", reviewMinus2Change);
assertQuery("label:Code-Review<=-2", reviewMinus2Change);
assertQuery("label:Code-Review<-2");
assertQuery("label:Code-Review=+1,anotheruser");
assertQuery("label:Code-Review=+1,user", change);
assertQuery("label:Code-Review=+1,user=user", change);
assertQuery("label:Code-Review=+1,Administrators", change);
assertQuery("label:Code-Review=+1,group=Administrators", change);
assertQuery("label:Code-Review=+1,user", reviewPlus1Change);
assertQuery("label:Code-Review=+1,user=user", reviewPlus1Change);
assertQuery("label:Code-Review=+1,Administrators", reviewPlus1Change);
assertQuery("label:Code-Review=+1,group=Administrators", reviewPlus1Change);
}
private Change[] codeReviewInRange(Map<Integer, Change> changes, int start,
int end) {
int size = 0;
Change[] range = new Change[end - start + 1];
for (int i : changes.keySet()) {
if (i >= start && i <= end) {
range[size] = changes.get(i);
size++;
}
}
return range;
}
private String createGroup(String name, String owner) throws Exception {