Merge "Add pure_revert Prolog fact"

Documentation/prolog-change-facts.txt

@@ -58,6 +58,10 @@ of them we must use a qualified name like `gerrit:change_branch(X)`.
                       |`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
 

Documentation/prolog-cookbook.txt

@@ -1048,6 +1048,56 @@ It's only used to show `'Needs All-Comments-Resolved'` in the UI to clearly
 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.
 

gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/change/ChangeIT.java

@@ -63,7 +63,6 @@ import com.google.gerrit.acceptance.GerritConfig;
 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 @@ public class ChangeIT extends AbstractDaemonTest {
     assertThat(approval.permittedVotingRange).isNull();
   }
 
-  @Sandboxed
   @Test
   public void unresolvedCommentsBlocked() throws Exception {
-    RevCommit oldHead = getRemoteHead();
+    modifySubmitRules(
-    GitUtil.fetch(testRepo, RefNames.REFS_CONFIG + ":config");
+        "submit_rule(submit(R)) :- \n"
-    testRepo.reset("config");
+            + "gerrit:unresolved_comments_count(0), \n"
-    PushOneCommit push =
+            + "!,"
-        pushFactory.create(
+            + "gerrit:commit_author(A), \n"
-            db,
+            + "R = label('All-Comments-Resolved', ok(A)).\n"
-            admin.getIdent(),
+            + "submit_rule(submit(R)) :- \n"
-            testRepo,
+            + "gerrit:unresolved_comments_count(U), \n"
-            "Configure",
+            + "U > 0,"
-            "rules.pl",
+            + "R = label('All-Comments-Resolved', need(_)). \n\n");
-            "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);
+    String oldHead = getRemoteHead().name();
-    testRepo.reset(oldHead);
-
-    oldHead = getRemoteHead();
     PushOneCommit.Result result1 =
         pushFactory.create(db, user.getIdent(), testRepo).to("refs/for/master");
     testRepo.reset(oldHead);
@@ -3002,12 +2988,60 @@ public class ChangeIT extends AbstractDaemonTest {
     gApi.changes().id(result1.getChangeId()).current().submit();
 
     exception.expect(ResourceConflictException.class);
-    exception.expectMessage(
+    exception.expectMessage("Failed to submit 1 change due to the following problems");
-        "Failed to submit 1 change due to the following problems:\n"
+    exception.expectMessage("needs All-Comments-Resolved");
-            + "Change 2: 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
@@ -3330,6 +3364,33 @@ public class ChangeIT extends AbstractDaemonTest {
     }
   }
 
+  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}")

gerrit-server/src/main/java/com/google/gerrit/server/change/GetPureRevert.java

@@ -93,20 +93,30 @@ public class GetPureRevert implements RestReadView<ChangeResource> {
         .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 @@ public class GetPureRevert implements RestReadView<ChangeResource> {
       // 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);

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

@@ -35,6 +35,9 @@ import com.google.gerrit.common.Nullable;
 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.AccountCache;
 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 @@ public class ChangeData {
     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 @@ public class ChangeData {
   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 @@ public class ChangeData {
       PatchSetUtil psUtil,
       ProjectCache projectCache,
       TrackingFooters trackingFooters,
+      GetPureRevert pureRevert,
       @Assisted ReviewDb db,
       @Assisted Project.NameKey project,
       @Assisted Change.Id id,
@@ -470,6 +476,7 @@ public class ChangeData {
     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 @@ public class ChangeData {
     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);

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;
+  }
+}