Merge "Add rejectEmptyCommit project config"

Documentation/config-project-config.txt

@@ -236,6 +236,10 @@ author last committed. Valid values are 'true', 'false', or 'INHERIT'. The defau
 This option only takes effect in submit strategies which already modify the commit, i.e.
 Cherry Pick, Rebase Always, and (perhaps) Rebase If Necessary.
 
+- 'rejectEmptyCommit': Defines whether empty commits should be rejected when a change is merged.
+Changes might not seem empty at first but when attempting to merge, rebasing can lead to an empty
+commit. If this option is set to 'true' the merge would fail.
+
 Merge strategy
 
 

Documentation/rest-api-projects.txt

@@ -2872,6 +2872,9 @@ entities.
 |`actions`                                 |optional|
 Actions the caller might be able to perform on this project. The
 information is a map of view names to
+|`reject_empty_commit`                     |optional|
+link:#inherited-boolean-info[InheritedBooleanInfo] that tells whether
+empty commits should be rejected when a change is merged.
 link:rest-api-changes.html#action-info[ActionInfo] entities.
 |=======================================================
 
@@ -2936,6 +2939,10 @@ If not set, the project state is not updated.
 |`plugin_config_values`                    |optional|
 Plugin configuration values as map which maps the plugin name to a map
 of parameter names to values.
+|`reject_empty_commit`                     |optional|
+Whether empty commits should be rejected when a change is merged.
+Can be `TRUE`, `FALSE` or `INHERIT`. +
+If not set, this setting is not updated.
 |======================================================
 
 [[config-parameter-info]]
@@ -3269,6 +3276,9 @@ Common unit suffixes of 'k', 'm', or 'g' are supported.
 |`plugin_config_values`      |optional|
 Plugin configuration values as map which maps the plugin name to a map
 of parameter names to values.
+|`reject_empty_commit`       |optional|
+Whether empty commits should be rejected when a change is merged
+(`TRUE`, `FALSE`, `INHERIT`).
 |=========================================
 
 [[project-parent-input]]

java/com/google/gerrit/acceptance/AbstractDaemonTest.java

@@ -436,6 +436,7 @@ public abstract class AbstractDaemonTest {
       in.useContentMerge = ann.useContributorAgreements();
       in.useSignedOffBy = ann.useSignedOffBy();
       in.useContentMerge = ann.useContentMerge();
+      in.rejectEmptyCommit = ann.rejectEmptyCommit();
     } else {
       // Defaults should match TestProjectConfig, omitting nullable values.
       in.createEmptyCommit = true;

java/com/google/gerrit/acceptance/TestProjectInput.java

@@ -45,6 +45,8 @@ public @interface TestProjectInput {
 
   InheritableBoolean requireChangeId() default InheritableBoolean.INHERIT;
 
+  InheritableBoolean rejectEmptyCommit() default InheritableBoolean.INHERIT;
+
   // Fields specific to acceptance test behavior.
 
   /** Username to use for initial clone, passed to {@link AccountCreator}. */

java/com/google/gerrit/extensions/api/projects/ConfigInfo.java

@@ -35,6 +35,7 @@ public class ConfigInfo {
   public InheritedBooleanInfo privateByDefault;
   public InheritedBooleanInfo enableReviewerByEmail;
   public InheritedBooleanInfo matchAuthorToCommitterDate;
+  public InheritedBooleanInfo rejectEmptyCommit;
 
   public MaxObjectSizeLimitInfo maxObjectSizeLimit;
   public SubmitType submitType;

java/com/google/gerrit/extensions/api/projects/ConfigInput.java

@@ -32,6 +32,7 @@ public class ConfigInput {
   public InheritableBoolean privateByDefault;
   public InheritableBoolean enableReviewerByEmail;
   public InheritableBoolean matchAuthorToCommitterDate;
+  public InheritableBoolean rejectEmptyCommit;
   public String maxObjectSizeLimit;
   public SubmitType submitType;
   public ProjectState state;

java/com/google/gerrit/extensions/api/projects/ProjectInput.java

@@ -33,6 +33,7 @@ public class ProjectInput {
   public InheritableBoolean useContentMerge;
   public InheritableBoolean requireChangeId;
   public InheritableBoolean createNewChangeForAllNotInTarget;
+  public InheritableBoolean rejectEmptyCommit;
   public String maxObjectSizeLimit;
   public Map<String, Map<String, ConfigValue>> pluginConfigValues;
 }

java/com/google/gerrit/reviewdb/client/BooleanProjectConfig.java

@@ -39,7 +39,8 @@ public enum BooleanProjectConfig {
   REJECT_IMPLICIT_MERGES("receive", "rejectImplicitMerges"),
   PRIVATE_BY_DEFAULT("change", "privateByDefault"),
   ENABLE_REVIEWER_BY_EMAIL("reviewer", "enableByEmail"),
-  MATCH_AUTHOR_TO_COMMITTER_DATE("project", "matchAuthorToCommitterDate");
+  MATCH_AUTHOR_TO_COMMITTER_DATE("project", "matchAuthorToCommitterDate"),
+  REJECT_EMPTY_COMMIT("submit", "rejectEmptyCommit");
 
   // Git config
   private final String section;

java/com/google/gerrit/server/git/strategy/CherryPick.java

@@ -15,10 +15,12 @@
 package com.google.gerrit.server.git.strategy;
 
 import static com.google.common.base.Preconditions.checkNotNull;
+import static com.google.gerrit.server.git.strategy.CommitMergeStatus.EMPTY_COMMIT;
 import static com.google.gerrit.server.git.strategy.CommitMergeStatus.SKIPPED_IDENTICAL_TREE;
 
 import com.google.common.collect.ImmutableList;
 import com.google.gerrit.extensions.restapi.MergeConflictException;
+import com.google.gerrit.reviewdb.client.BooleanProjectConfig;
 import com.google.gerrit.reviewdb.client.PatchSet;
 import com.google.gerrit.reviewdb.client.PatchSetInfo;
 import com.google.gerrit.server.ChangeUtil;
@@ -123,6 +125,10 @@ public class CherryPick extends SubmitStrategy {
         toMerge.setStatusCode(CommitMergeStatus.PATH_CONFLICT);
         return;
       } catch (MergeIdenticalTreeException mie) {
+        if (args.project.is(BooleanProjectConfig.REJECT_EMPTY_COMMIT)) {
+          toMerge.setStatusCode(EMPTY_COMMIT);
+          return;
+        }
         toMerge.setStatusCode(SKIPPED_IDENTICAL_TREE);
         return;
       }

java/com/google/gerrit/server/git/strategy/CommitMergeStatus.java

@@ -60,7 +60,12 @@ public enum CommitMergeStatus {
   NOT_FAST_FORWARD(
       "Project policy requires all submissions to be a fast-forward.\n"
           + "\n"
-          + "Please rebase the change locally and upload again for review.");
+          + "Please rebase the change locally and upload again for review."),
+
+  EMPTY_COMMIT(
+      "Change could not be merged because the commit is empty.\n"
+          + "\n"
+          + "Project policy requires all commits to contain modifications to at least one file.");
 
   private final String message;
 

java/com/google/gerrit/server/git/strategy/FastForwardOp.java

@@ -14,6 +14,9 @@
 
 package com.google.gerrit.server.git.strategy;
 
+import static com.google.gerrit.server.git.strategy.CommitMergeStatus.EMPTY_COMMIT;
+
+import com.google.gerrit.reviewdb.client.BooleanProjectConfig;
 import com.google.gerrit.server.git.CodeReviewCommit;
 import com.google.gerrit.server.git.IntegrationException;
 import com.google.gerrit.server.update.RepoContext;
@@ -25,6 +28,12 @@ class FastForwardOp extends SubmitStrategyOp {
 
   @Override
   protected void updateRepoImpl(RepoContext ctx) throws IntegrationException {
+    if (args.project.is(BooleanProjectConfig.REJECT_EMPTY_COMMIT)
+        && toMerge.getTree().equals(toMerge.getParent(0).getTree())) {
+      toMerge.setStatusCode(EMPTY_COMMIT);
+      return;
+    }
+
     args.mergeTip.moveTipTo(toMerge, toMerge);
   }
 }

java/com/google/gerrit/server/git/strategy/MergeOneOp.java

@@ -14,6 +14,9 @@
 
 package com.google.gerrit.server.git.strategy;
 
+import static com.google.gerrit.server.git.strategy.CommitMergeStatus.EMPTY_COMMIT;
+
+import com.google.gerrit.reviewdb.client.BooleanProjectConfig;
 import com.google.gerrit.server.git.CodeReviewCommit;
 import com.google.gerrit.server.git.IntegrationException;
 import com.google.gerrit.server.update.RepoContext;
@@ -47,6 +50,11 @@ class MergeOneOp extends SubmitStrategyOp {
             args.destBranch,
             args.mergeTip.getCurrentTip(),
             toMerge);
+    if (args.project.is(BooleanProjectConfig.REJECT_EMPTY_COMMIT)
+        && merged.getTree().equals(merged.getParent(0).getTree())) {
+      toMerge.setStatusCode(EMPTY_COMMIT);
+      return;
+    }
     args.mergeTip.moveTipTo(amendGitlink(merged), toMerge);
   }
 }

java/com/google/gerrit/server/git/strategy/RebaseSubmitStrategy.java

@@ -15,6 +15,7 @@
 package com.google.gerrit.server.git.strategy;
 
 import static com.google.common.base.Preconditions.checkState;
+import static com.google.gerrit.server.git.strategy.CommitMergeStatus.EMPTY_COMMIT;
 import static com.google.gerrit.server.git.strategy.CommitMergeStatus.SKIPPED_IDENTICAL_TREE;
 
 import com.google.common.collect.ImmutableList;
@@ -124,6 +125,12 @@ public class RebaseSubmitStrategy extends SubmitStrategy {
       if (args.mergeUtil.canFastForward(
           args.mergeSorter, args.mergeTip.getCurrentTip(), args.rw, toMerge)) {
         if (!rebaseAlways) {
+          if (args.project.is(BooleanProjectConfig.REJECT_EMPTY_COMMIT)
+              && toMerge.getTree().equals(toMerge.getParent(0).getTree())) {
+            toMerge.setStatusCode(EMPTY_COMMIT);
+            return;
+          }
+
           args.mergeTip.moveTipTo(amendGitlink(toMerge), toMerge);
           toMerge.setStatusCode(CommitMergeStatus.CLEAN_MERGE);
           acceptMergeTip(args.mergeTip);
@@ -192,6 +199,11 @@ public class RebaseSubmitStrategy extends SubmitStrategy {
         newCommit = args.rw.parseCommit(rebaseOp.getRebasedCommit());
         newPatchSetId = rebaseOp.getPatchSetId();
       }
+      if (args.project.is(BooleanProjectConfig.REJECT_EMPTY_COMMIT)
+          && newCommit.getTree().equals(newCommit.getParent(0).getTree())) {
+        toMerge.setStatusCode(EMPTY_COMMIT);
+        return;
+      }
       newCommit = amendGitlink(newCommit);
       newCommit.copyFrom(toMerge);
       newCommit.setPatchsetId(newPatchSetId);

java/com/google/gerrit/server/git/strategy/SubmitStrategyListener.java

@@ -128,6 +128,7 @@ public class SubmitStrategyListener implements BatchUpdateListener {
         case CANNOT_CHERRY_PICK_ROOT:
         case CANNOT_REBASE_ROOT:
         case NOT_FAST_FORWARD:
+        case EMPTY_COMMIT:
           // TODO(dborowitz): Reformat these messages to be more appropriate for
           // short problem descriptions.
           commitStatus.problem(id, CharMatcher.is('\n').collapseFrom(s.getMessage(), ' '));

java/com/google/gerrit/server/project/BooleanProjectConfigTransformations.java

@@ -65,6 +65,9 @@ public class BooleanProjectConfigTransformations {
               BooleanProjectConfig.MATCH_AUTHOR_TO_COMMITTER_DATE,
               new Mapper(
                   i -> i.matchAuthorToCommitterDate, (i, v) -> i.matchAuthorToCommitterDate = v))
+          .put(
+              BooleanProjectConfig.REJECT_EMPTY_COMMIT,
+              new Mapper(i -> i.rejectEmptyCommit, (i, v) -> i.rejectEmptyCommit = v))
           .build();
 
   static {

java/com/google/gerrit/server/project/CreateProjectArgs.java

@@ -34,6 +34,7 @@ public class CreateProjectArgs {
   public InheritableBoolean contentMerge;
   public InheritableBoolean newChangeForAllNotInTarget;
   public InheritableBoolean changeIdRequired;
+  public InheritableBoolean rejectEmptyCommit;
   public boolean createEmptyCommit;
   public String maxObjectSizeLimit;
 

java/com/google/gerrit/server/restapi/project/CreateProject.java

@@ -197,6 +197,8 @@ public class CreateProject implements RestModifyView<TopLevelResource, ProjectIn
             input.createNewChangeForAllNotInTarget, InheritableBoolean.INHERIT);
     args.changeIdRequired =
         MoreObjects.firstNonNull(input.requireChangeId, InheritableBoolean.INHERIT);
+    args.rejectEmptyCommit =
+        MoreObjects.firstNonNull(input.rejectEmptyCommit, InheritableBoolean.INHERIT);
     try {
       args.maxObjectSizeLimit = ProjectConfig.validMaxObjectSizeLimit(input.maxObjectSizeLimit);
     } catch (ConfigInvalidException e) {
@@ -287,6 +289,7 @@ public class CreateProject implements RestModifyView<TopLevelResource, ProjectIn
           BooleanProjectConfig.CREATE_NEW_CHANGE_FOR_ALL_NOT_IN_TARGET,
           args.newChangeForAllNotInTarget);
       newProject.setBooleanConfig(BooleanProjectConfig.REQUIRE_CHANGE_ID, args.changeIdRequired);
+      newProject.setBooleanConfig(BooleanProjectConfig.REJECT_EMPTY_COMMIT, args.rejectEmptyCommit);
       newProject.setMaxObjectSizeLimit(args.maxObjectSizeLimit);
       if (args.newParent != null) {
         newProject.setParentName(args.newParent);

java/com/google/gerrit/sshd/commands/CreateProjectCommand.java

@@ -100,6 +100,9 @@ final class CreateProjectCommand extends SshCommand {
   @Option(name = "--change-id", usage = "if change-id is required")
   private InheritableBoolean requireChangeID = InheritableBoolean.INHERIT;
 
+  @Option(name = "--reject-empty-commit", usage = "if empty commits should be rejected on submit")
+  private InheritableBoolean rejectEmptyCommit = InheritableBoolean.INHERIT;
+
   @Option(
     name = "--new-change-for-all-not-in-target",
     usage = "if a new change will be created for every commit not in target branch"
@@ -201,6 +204,7 @@ final class CreateProjectCommand extends SshCommand {
         input.branches = branch;
         input.createEmptyCommit = createEmptyCommit;
         input.maxObjectSizeLimit = maxObjectSizeLimit;
+        input.rejectEmptyCommit = rejectEmptyCommit;
         if (pluginConfigValues != null) {
           input.pluginConfigValues = parsePluginConfigValues(pluginConfigValues);
         }

javatests/com/google/gerrit/acceptance/rest/change/AbstractSubmit.java

@@ -38,6 +38,7 @@ import com.google.gerrit.acceptance.TestAccount;
 import com.google.gerrit.acceptance.TestProjectInput;
 import com.google.gerrit.common.TimeUtil;
 import com.google.gerrit.common.data.Permission;
+import com.google.gerrit.extensions.api.changes.ChangeApi;
 import com.google.gerrit.extensions.api.changes.SubmitInput;
 import com.google.gerrit.extensions.api.projects.BranchInput;
 import com.google.gerrit.extensions.api.projects.ConfigInput;
@@ -47,6 +48,7 @@ import com.google.gerrit.extensions.client.InheritableBoolean;
 import com.google.gerrit.extensions.client.ListChangesOption;
 import com.google.gerrit.extensions.client.SubmitType;
 import com.google.gerrit.extensions.common.ChangeInfo;
+import com.google.gerrit.extensions.common.ChangeInput;
 import com.google.gerrit.extensions.common.LabelInfo;
 import com.google.gerrit.extensions.registration.DynamicSet;
 import com.google.gerrit.extensions.registration.RegistrationHandle;
@@ -988,6 +990,78 @@ public abstract class AbstractSubmit extends AbstractDaemonTest {
     assertAuthorAndCommitDateEquals(getRemoteHead());
   }
 
+  @Test
+  @TestProjectInput(rejectEmptyCommit = InheritableBoolean.FALSE)
+  public void submitEmptyCommitPatchSetCanNotFastForward_emptyCommitAllowed() throws Exception {
+    assume().that(getSubmitType()).isNotEqualTo(SubmitType.FAST_FORWARD_ONLY);
+
+    PushOneCommit.Result change = createChange("Change 1", "a.txt", "content");
+    submit(change.getChangeId());
+
+    ChangeApi revert1 = gApi.changes().id(change.getChangeId()).revert();
+    approve(revert1.id());
+    revert1.current().submit();
+
+    ChangeApi revert2 = gApi.changes().id(change.getChangeId()).revert();
+    approve(revert2.id());
+    revert2.current().submit();
+  }
+
+  @Test
+  @TestProjectInput(rejectEmptyCommit = InheritableBoolean.TRUE)
+  public void submitEmptyCommitPatchSetCanNotFastForward_emptyCommitNotAllowed() throws Exception {
+    assume().that(getSubmitType()).isNotEqualTo(SubmitType.FAST_FORWARD_ONLY);
+
+    PushOneCommit.Result change = createChange("Change 1", "a.txt", "content");
+    submit(change.getChangeId());
+
+    ChangeApi revert1 = gApi.changes().id(change.getChangeId()).revert();
+    approve(revert1.id());
+    revert1.current().submit();
+
+    ChangeApi revert2 = gApi.changes().id(change.getChangeId()).revert();
+    approve(revert2.id());
+
+    exception.expect(ResourceConflictException.class);
+    exception.expectMessage(
+        "Change "
+            + revert2.get()._number
+            + ": Change could not be merged because the commit is empty. "
+            + "Project policy requires all commits to contain modifications to at least one file.");
+    revert2.current().submit();
+  }
+
+  @Test
+  @TestProjectInput(rejectEmptyCommit = InheritableBoolean.FALSE)
+  public void submitEmptyCommitPatchSetCanFastForward_emptyCommitAllowed() throws Exception {
+    ChangeInput ci = new ChangeInput();
+    ci.subject = "Empty change";
+    ci.project = project.get();
+    ci.branch = "master";
+    ChangeApi change = gApi.changes().create(ci);
+    approve(change.id());
+    change.current().submit();
+  }
+
+  @Test
+  @TestProjectInput(rejectEmptyCommit = InheritableBoolean.TRUE)
+  public void submitEmptyCommitPatchSetCanFastForward_emptyCommitNotAllowed() throws Exception {
+    ChangeInput ci = new ChangeInput();
+    ci.subject = "Empty change";
+    ci.project = project.get();
+    ci.branch = "master";
+    ChangeApi change = gApi.changes().create(ci);
+    approve(change.id());
+
+    exception.expect(ResourceConflictException.class);
+    exception.expectMessage(
+        "Change "
+            + change.get()._number
+            + ": Change could not be merged because the commit is empty. "
+            + "Project policy requires all commits to contain modifications to at least one file.");
+    change.current().submit();
+  }
+
   private void setChangeStatusToNew(PushOneCommit.Result... changes) throws Exception {
     for (PushOneCommit.Result change : changes) {
       try (BatchUpdate bu =