Merge changes I8539a8c7,Ie7e7bc7c,I378abbb9
* changes:
ChangesCollection: Remove unused field
CreateAccessChange: Use same first ChangeMessage as elsewhere
CreateAccessChange: Suppress deprecation warning
diff --git a/Documentation/prolog-change-facts.txt b/Documentation/prolog-change-facts.txt
index 11d17b8..01a1878 100644
--- a/Documentation/prolog-change-facts.txt
+++ b/Documentation/prolog-change-facts.txt
@@ -58,6 +58,10 @@
|`current_user(user(peer_daemon)).`
|`current_user(user(replication)).`
+|`pure_revert/1` |`pure_revert(1).`
+ |link:rest-api-changes.html#get-pure-revert[Pure revert] as integer atom (1 if
+ the change is a pure revert, 0 otherwise)
+
|`uploader/1` |`uploader(user(1000000)).`
|Uploader as `user(ID)` term. ID is the numeric account ID
diff --git a/Documentation/prolog-cookbook.txt b/Documentation/prolog-cookbook.txt
index 1c114e2..40906bd 100644
--- a/Documentation/prolog-cookbook.txt
+++ b/Documentation/prolog-cookbook.txt
@@ -1048,6 +1048,56 @@
indicate to the user that all the comments have to be resolved for the
change to become submittable.
+=== Example 17: Make change submittable if it is a pure revert
+In this example we will use the `pure_revert` fact about a
+change. Our goal is to block the submission of any change that is not a
+pure revert. Basically, it can be achieved by the following rules:
+
+`rules.pl`
+[source,prolog]
+----
+submit_rule(submit(R)) :-
+ gerrit:pure_revert(1),
+ !,
+ gerrit:commit_author(A),
+ R = label('Is-Pure-Revert', ok(A)).
+
+submit_rule(submit(R)) :-
+ gerrit:pure_revert(U),
+ U /= 1,
+ R = label('Is-Pure-Revert', need(_)).
+----
+
+Suppose currently a change is submittable if it gets `+2` for `Code-Review`
+and `+1` for `Verified`. It can be extended to support the above rules as
+follows:
+
+`rules.pl`
+[source,prolog]
+----
+submit_rule(submit(CR, V, R)) :-
+ base(CR, V),
+ gerrit:pure_revert(1),
+ !,
+ gerrit:commit_author(A),
+ R = label('Is-Pure-Revert', ok(A)).
+
+submit_rule(submit(CR, V, R)) :-
+ base(CR, V),
+ gerrit:pure_revert(U),
+ U /= 1,
+ R = label('Is-Pure-Revert', need(_)).
+
+base(CR, V) :-
+ gerrit:max_with_block(-2, 2, 'Code-Review', CR),
+ gerrit:max_with_block(-1, 1, 'Verified', V).
+----
+
+Note that a new label as `Is-Pure-Revert` should not be configured.
+It's only used to show `'Needs Is-Pure-Revert'` in the UI to clearly
+indicate to the user that all the comments have to be resolved for the
+change to become submittable.
+
== Examples - Submit Type
The following examples show how to implement own submit type rules.
diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/change/ChangeIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/change/ChangeIT.java
index 667691b..3bd75c9 100644
--- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/change/ChangeIT.java
+++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/change/ChangeIT.java
@@ -63,7 +63,6 @@
import com.google.gerrit.acceptance.GitUtil;
import com.google.gerrit.acceptance.NoHttpd;
import com.google.gerrit.acceptance.PushOneCommit;
-import com.google.gerrit.acceptance.Sandboxed;
import com.google.gerrit.acceptance.TestAccount;
import com.google.gerrit.acceptance.TestProjectInput;
import com.google.gerrit.common.FooterConstants;
@@ -2963,33 +2962,20 @@
assertThat(approval.permittedVotingRange).isNull();
}
- @Sandboxed
@Test
public void unresolvedCommentsBlocked() throws Exception {
- RevCommit oldHead = getRemoteHead();
- GitUtil.fetch(testRepo, RefNames.REFS_CONFIG + ":config");
- testRepo.reset("config");
- PushOneCommit push =
- pushFactory.create(
- db,
- admin.getIdent(),
- testRepo,
- "Configure",
- "rules.pl",
- "submit_rule(submit(R)) :- \n"
- + "gerrit:unresolved_comments_count(0), \n"
- + "!,"
- + "gerrit:commit_author(A), \n"
- + "R = label('All-Comments-Resolved', ok(A)).\n"
- + "submit_rule(submit(R)) :- \n"
- + "gerrit:unresolved_comments_count(U), \n"
- + "U > 0,"
- + "R = label('All-Comments-Resolved', need(_)). \n\n");
+ modifySubmitRules(
+ "submit_rule(submit(R)) :- \n"
+ + "gerrit:unresolved_comments_count(0), \n"
+ + "!,"
+ + "gerrit:commit_author(A), \n"
+ + "R = label('All-Comments-Resolved', ok(A)).\n"
+ + "submit_rule(submit(R)) :- \n"
+ + "gerrit:unresolved_comments_count(U), \n"
+ + "U > 0,"
+ + "R = label('All-Comments-Resolved', need(_)). \n\n");
- push.to(RefNames.REFS_CONFIG);
- testRepo.reset(oldHead);
-
- oldHead = getRemoteHead();
+ String oldHead = getRemoteHead().name();
PushOneCommit.Result result1 =
pushFactory.create(db, user.getIdent(), testRepo).to("refs/for/master");
testRepo.reset(oldHead);
@@ -3002,13 +2988,61 @@
gApi.changes().id(result1.getChangeId()).current().submit();
exception.expect(ResourceConflictException.class);
- exception.expectMessage(
- "Failed to submit 1 change due to the following problems:\n"
- + "Change 2: needs All-Comments-Resolved");
+ exception.expectMessage("Failed to submit 1 change due to the following problems");
+ exception.expectMessage("needs All-Comments-Resolved");
gApi.changes().id(result2.getChangeId()).current().submit();
}
@Test
+ public void pureRevertFactBlocksSubmissionOfNonReverts() throws Exception {
+ addPureRevertSubmitRule();
+
+ // Create a change that is not a revert of another change
+ PushOneCommit.Result r1 =
+ pushFactory.create(db, user.getIdent(), testRepo).to("refs/for/master");
+ approve(r1.getChangeId());
+
+ exception.expect(ResourceConflictException.class);
+ exception.expectMessage("Failed to submit 1 change due to the following problems");
+ exception.expectMessage("needs Is-Pure-Revert");
+ gApi.changes().id(r1.getChangeId()).current().submit();
+ }
+
+ @Test
+ public void pureRevertFactBlocksSubmissionOfNonPureReverts() throws Exception {
+ PushOneCommit.Result r1 =
+ pushFactory.create(db, user.getIdent(), testRepo).to("refs/for/master");
+ merge(r1);
+
+ addPureRevertSubmitRule();
+
+ // Create a revert and push a content change
+ String revertId = gApi.changes().id(r1.getChangeId()).revert().get().changeId;
+ amendChange(revertId);
+ approve(revertId);
+
+ exception.expect(ResourceConflictException.class);
+ exception.expectMessage("Failed to submit 1 change due to the following problems");
+ exception.expectMessage("needs Is-Pure-Revert");
+ gApi.changes().id(revertId).current().submit();
+ }
+
+ @Test
+ public void pureRevertFactAllowsSubmissionOfPureReverts() throws Exception {
+ // Create a change that we can later revert
+ PushOneCommit.Result r1 =
+ pushFactory.create(db, user.getIdent(), testRepo).to("refs/for/master");
+ merge(r1);
+
+ addPureRevertSubmitRule();
+
+ // Create a revert and submit it
+ String revertId = gApi.changes().id(r1.getChangeId()).revert().get().changeId;
+ approve(revertId);
+ gApi.changes().id(revertId).current().submit();
+ }
+
+ @Test
public void changeCommitMessage() throws Exception {
// Tests mutating the commit message as both the owner of the change and a regular user with
// addPatchSet permission. Asserts that both cases succeed.
@@ -3330,6 +3364,33 @@
}
}
+ private void addPureRevertSubmitRule() throws Exception {
+ modifySubmitRules(
+ "submit_rule(submit(R)) :- \n"
+ + "gerrit:pure_revert(1), \n"
+ + "!,"
+ + "gerrit:commit_author(A), \n"
+ + "R = label('Is-Pure-Revert', ok(A)).\n"
+ + "submit_rule(submit(R)) :- \n"
+ + "gerrit:pure_revert(U), \n"
+ + "U \\= 1,"
+ + "R = label('Is-Pure-Revert', need(_)). \n\n");
+ }
+
+ private void modifySubmitRules(String newContent) throws Exception {
+ try (Repository repo = repoManager.openRepository(project)) {
+ TestRepository testRepo = new TestRepository<>((InMemoryRepository) repo);
+ testRepo
+ .branch(RefNames.REFS_CONFIG)
+ .commit()
+ .author(admin.getIdent())
+ .committer(admin.getIdent())
+ .add("rules.pl", newContent)
+ .message("Modify rules.pl")
+ .create();
+ }
+ }
+
@Test
@GerritConfig(name = "trackingid.jira-bug.footer", value = "Bug:")
@GerritConfig(name = "trackingid.jira-bug.match", value = "JRA\\d{2,8}")
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/GetPureRevert.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/GetPureRevert.java
index 3573ef7..3148872 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/change/GetPureRevert.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/GetPureRevert.java
@@ -93,20 +93,30 @@
.isPatchVisible(currentPatchSet, dbProvider.get())) {
throw new AuthException("current revision not accessible");
}
+ return getPureRevert(rsrc.getNotes());
+ }
+
+ public PureRevertInfo getPureRevert(ChangeNotes notes)
+ throws OrmException, IOException, BadRequestException, AuthException,
+ ResourceConflictException {
+ PatchSet currentPatchSet = psUtil.current(dbProvider.get(), notes);
+ if (currentPatchSet == null) {
+ throw new ResourceConflictException("current revision is missing");
+ }
if (claimedOriginal == null) {
- if (rsrc.getChange().getRevertOf() == null) {
+ if (notes.getChange().getRevertOf() == null) {
throw new BadRequestException("no ID was provided and change isn't a revert");
}
PatchSet ps =
psUtil.current(
dbProvider.get(),
notesFactory.createChecked(
- dbProvider.get(), rsrc.getProject(), rsrc.getChange().getRevertOf()));
+ dbProvider.get(), notes.getProjectName(), notes.getChange().getRevertOf()));
claimedOriginal = ps.getRevision().get();
}
- try (Repository repo = repoManager.openRepository(rsrc.getProject());
+ try (Repository repo = repoManager.openRepository(notes.getProjectName());
ObjectInserter oi = repo.newObjectInserter();
RevWalk rw = new RevWalk(repo)) {
RevCommit claimedOriginalCommit;
@@ -126,7 +136,7 @@
// Rebase claimed revert onto claimed original
ThreeWayMerger merger =
mergeUtilFactory
- .create(projectCache.checkedGet(rsrc.getProject()))
+ .create(projectCache.checkedGet(notes.getProjectName()))
.newThreeWayMerger(oi, repo.getConfig());
merger.setBase(claimedRevertCommit.getParent(0));
merger.merge(claimedRevertCommit, claimedOriginalCommit);
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeData.java b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeData.java
index ce68398..d43edbb 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeData.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeData.java
@@ -35,6 +35,9 @@
import com.google.gerrit.common.data.LabelTypes;
import com.google.gerrit.common.data.SubmitRecord;
import com.google.gerrit.common.data.SubmitTypeRecord;
+import com.google.gerrit.extensions.restapi.AuthException;
+import com.google.gerrit.extensions.restapi.BadRequestException;
+import com.google.gerrit.extensions.restapi.ResourceConflictException;
import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.client.ChangeMessage;
@@ -61,6 +64,7 @@
import com.google.gerrit.server.account.Accounts;
import com.google.gerrit.server.account.Emails;
import com.google.gerrit.server.change.ChangeResource;
+import com.google.gerrit.server.change.GetPureRevert;
import com.google.gerrit.server.change.MergeabilityCache;
import com.google.gerrit.server.config.AllUsersName;
import com.google.gerrit.server.config.TrackingFooters;
@@ -349,7 +353,7 @@
ChangeData cd =
new ChangeData(
null, null, null, null, null, null, null, null, null, null, null, null, null, null,
- null, null, null, null, null, null, project, id, null, null, null);
+ null, null, null, null, null, null, null, project, id, null, null, null);
cd.currentPatchSet = new PatchSet(new PatchSet.Id(id, currentPatchSetId));
return cd;
}
@@ -374,6 +378,7 @@
private final PatchSetUtil psUtil;
private final ProjectCache projectCache;
private final TrackingFooters trackingFooters;
+ private final GetPureRevert pureRevert;
// Required assisted injected fields.
private final ReviewDb db;
@@ -445,6 +450,7 @@
PatchSetUtil psUtil,
ProjectCache projectCache,
TrackingFooters trackingFooters,
+ GetPureRevert pureRevert,
@Assisted ReviewDb db,
@Assisted Project.NameKey project,
@Assisted Change.Id id,
@@ -470,6 +476,7 @@
this.projectCache = projectCache;
this.starredChangesUtil = starredChangesUtil;
this.trackingFooters = trackingFooters;
+ this.pureRevert = pureRevert;
// May be null in tests when created via createForTest above, in which case lazy-loading will
// intentionally fail with NPE. Still not marked @Nullable in the constructor, to force callers
@@ -1270,6 +1277,22 @@
return starsOf.stars();
}
+ /**
+ * @return {@code null} if {@code revertOf} is {@code null}; true if the change is a pure revert;
+ * false otherwise.
+ */
+ @Nullable
+ public Boolean isPureRevert() throws OrmException {
+ if (change().getRevertOf() == null) {
+ return null;
+ }
+ try {
+ return pureRevert.getPureRevert(notes()).isPureRevert;
+ } catch (IOException | BadRequestException | AuthException | ResourceConflictException e) {
+ throw new OrmException("could not compute pure revert", e);
+ }
+ }
+
@Override
public String toString() {
MoreObjects.ToStringHelper h = MoreObjects.toStringHelper(this);
diff --git a/gerrit-server/src/main/java/gerrit/PRED_pure_revert_1.java b/gerrit-server/src/main/java/gerrit/PRED_pure_revert_1.java
new file mode 100644
index 0000000..f3721fb
--- /dev/null
+++ b/gerrit-server/src/main/java/gerrit/PRED_pure_revert_1.java
@@ -0,0 +1,50 @@
+// Copyright (C) 2017 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 gerrit;
+
+import com.google.gerrit.rules.StoredValues;
+import com.google.gwtorm.server.OrmException;
+import com.googlecode.prolog_cafe.exceptions.JavaException;
+import com.googlecode.prolog_cafe.exceptions.PrologException;
+import com.googlecode.prolog_cafe.lang.IntegerTerm;
+import com.googlecode.prolog_cafe.lang.Operation;
+import com.googlecode.prolog_cafe.lang.Predicate;
+import com.googlecode.prolog_cafe.lang.Prolog;
+import com.googlecode.prolog_cafe.lang.Term;
+
+/** Checks if change is a pure revert of the change it references in 'revertOf'. */
+public class PRED_pure_revert_1 extends Predicate.P1 {
+ public PRED_pure_revert_1(Term a1, Operation n) {
+ arg1 = a1;
+ cont = n;
+ }
+
+ @Override
+ public Operation exec(Prolog engine) throws PrologException {
+ engine.setB0();
+ Term a1 = arg1.dereference();
+
+ Boolean isPureRevert;
+ try {
+ isPureRevert = StoredValues.CHANGE_DATA.get(engine).isPureRevert();
+ } catch (OrmException e) {
+ throw new JavaException(this, 1, e);
+ }
+ if (!a1.unify(new IntegerTerm(Boolean.TRUE.equals(isPureRevert) ? 1 : 0), engine.trail)) {
+ return engine.fail();
+ }
+ return cont;
+ }
+}
diff --git a/polygerrit-ui/app/behaviors/gr-tooltip-behavior/gr-tooltip-behavior.js b/polygerrit-ui/app/behaviors/gr-tooltip-behavior/gr-tooltip-behavior.js
index 451fc22..7da82b7 100644
--- a/polygerrit-ui/app/behaviors/gr-tooltip-behavior/gr-tooltip-behavior.js
+++ b/polygerrit-ui/app/behaviors/gr-tooltip-behavior/gr-tooltip-behavior.js
@@ -105,6 +105,9 @@
},
_positionTooltip(tooltip) {
+ // This flush is needed for tooltips to be positioned correctly in Firefox
+ // and Safari.
+ Polymer.dom.flush();
const rect = this.getBoundingClientRect();
const boxRect = tooltip.getBoundingClientRect();
const parentRect = tooltip.parentElement.getBoundingClientRect();
diff --git a/polygerrit-ui/app/elements/change/gr-file-list-header/gr-file-list-header.html b/polygerrit-ui/app/elements/change/gr-file-list-header/gr-file-list-header.html
index edd5389..238db9c 100644
--- a/polygerrit-ui/app/elements/change/gr-file-list-header/gr-file-list-header.html
+++ b/polygerrit-ui/app/elements/change/gr-file-list-header/gr-file-list-header.html
@@ -94,9 +94,6 @@
.separator {
margin: 0 .25em;
}
- .expandInline {
- padding-right: .25em;
- }
.patchSetSelect {
max-width: 8em;
}
diff --git a/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.html b/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.html
index 95bfc70..3464fea 100644
--- a/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.html
+++ b/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.html
@@ -150,6 +150,9 @@
#container.editLoaded .hideOnEdit {
display: none;
}
+ .expandInline {
+ padding-right: .25em;
+ }
@media screen and (max-width: 50em) {
.desktop {
display: none;
diff --git a/polygerrit-ui/app/elements/core/gr-reporting/gr-reporting.js b/polygerrit-ui/app/elements/core/gr-reporting/gr-reporting.js
index 61251af..5a8ddf6 100644
--- a/polygerrit-ui/app/elements/core/gr-reporting/gr-reporting.js
+++ b/polygerrit-ui/app/elements/core/gr-reporting/gr-reporting.js
@@ -198,7 +198,7 @@
*/
timeEnd(name) {
const baseTime = this._baselines[name] || 0;
- const time = this.now() - baseTime;
+ const time = Math.round(this.now() - baseTime) + 'ms';
this.reporter(TIMING.TYPE, TIMING.CATEGORY, name, time);
delete this._baselines[name];
},
diff --git a/polygerrit-ui/app/elements/core/gr-reporting/gr-reporting_test.html b/polygerrit-ui/app/elements/core/gr-reporting/gr-reporting_test.html
index 67cd329..e88096b 100644
--- a/polygerrit-ui/app/elements/core/gr-reporting/gr-reporting_test.html
+++ b/polygerrit-ui/app/elements/core/gr-reporting/gr-reporting_test.html
@@ -85,10 +85,10 @@
nowStub.returns(3.123);
element.timeEnd('foo');
assert.isTrue(element.reporter.calledWithExactly(
- 'timing-report', 'UI Latency', 'foo', 3.123
+ 'timing-report', 'UI Latency', 'foo', '3ms'
));
assert.isTrue(element.reporter.calledWithExactly(
- 'timing-report', 'UI Latency', 'bar', 1
+ 'timing-report', 'UI Latency', 'bar', '1ms'
));
});
@@ -104,7 +104,7 @@
sandbox.stub(element, 'now').returns(42);
element.pluginsLoaded();
assert.isTrue(element.defaultReporter.calledWithExactly(
- 'timing-report', 'UI Latency', 'PluginsLoaded', 42
+ 'timing-report', 'UI Latency', 'PluginsLoaded', '42ms'
));
});
