Merge "Use DiffFormatter instead of PatchListCache to count files"

java/com/google/gerrit/server/git/validators/CommitValidators.java

@@ -45,11 +45,6 @@ import com.google.gerrit.server.events.CommitReceivedEvent;
 import com.google.gerrit.server.git.GitRepositoryManager;
 import com.google.gerrit.server.git.ValidationError;
 import com.google.gerrit.server.git.validators.ValidationMessage.Type;
-import com.google.gerrit.server.patch.DiffSummary;
-import com.google.gerrit.server.patch.DiffSummaryKey;
-import com.google.gerrit.server.patch.PatchListCache;
-import com.google.gerrit.server.patch.PatchListKey;
-import com.google.gerrit.server.patch.PatchListNotAvailableException;
 import com.google.gerrit.server.permissions.PermissionBackend;
 import com.google.gerrit.server.permissions.PermissionBackendException;
 import com.google.gerrit.server.permissions.RefPermission;
@@ -70,6 +65,8 @@ import java.util.Collections;
 import java.util.List;
 import java.util.Optional;
 import java.util.regex.Pattern;
+import org.eclipse.jgit.diff.DiffEntry;
+import org.eclipse.jgit.diff.DiffFormatter;
 import org.eclipse.jgit.errors.ConfigInvalidException;
 import org.eclipse.jgit.lib.Config;
 import org.eclipse.jgit.lib.PersonIdent;
@@ -80,6 +77,7 @@ import org.eclipse.jgit.revwalk.FooterLine;
 import org.eclipse.jgit.revwalk.RevCommit;
 import org.eclipse.jgit.revwalk.RevWalk;
 import org.eclipse.jgit.util.SystemReader;
+import org.eclipse.jgit.util.io.DisabledOutputStream;
 
 /**
  * Represents a list of {@link CommitValidationListener}s to run for a push to one branch of one
@@ -103,7 +101,6 @@ public class CommitValidators {
     private final AccountValidator accountValidator;
     private final ProjectCache projectCache;
     private final ProjectConfig.Factory projectConfigFactory;
-    private final PatchListCache patchListCache;
     private final Config config;
 
     @Inject
@@ -118,8 +115,7 @@ public class CommitValidators {
         ExternalIdsConsistencyChecker externalIdsConsistencyChecker,
         AccountValidator accountValidator,
         ProjectCache projectCache,
-        ProjectConfig.Factory projectConfigFactory,
-        PatchListCache patchListCache) {
+        ProjectConfig.Factory projectConfigFactory) {
       this.gerritIdent = gerritIdent;
       this.urlFormatter = urlFormatter;
       this.config = config;
@@ -131,7 +127,6 @@ public class CommitValidators {
       this.accountValidator = accountValidator;
       this.projectCache = projectCache;
       this.projectConfigFactory = projectConfigFactory;
-      this.patchListCache = patchListCache;
     }
 
     public CommitValidators forReceiveCommits(
@@ -152,7 +147,7 @@ public class CommitValidators {
               new ProjectStateValidationListener(projectState),
               new AmendedGerritMergeCommitValidationListener(perm, gerritIdent),
               new AuthorUploaderValidator(user, perm, urlFormatter.get()),
-              new FileCountValidator(patchListCache, config),
+              new FileCountValidator(repoManager, config),
               new CommitterUploaderValidator(user, perm, urlFormatter.get()),
               new SignedOffByValidator(user, perm, projectState),
               new ChangeIdValidator(
@@ -181,7 +176,7 @@ public class CommitValidators {
               new ProjectStateValidationListener(projectState),
               new AmendedGerritMergeCommitValidationListener(perm, gerritIdent),
               new AuthorUploaderValidator(user, perm, urlFormatter.get()),
-              new FileCountValidator(patchListCache, config),
+              new FileCountValidator(repoManager, config),
               new SignedOffByValidator(user, perm, projectState),
               new ChangeIdValidator(
                   projectState, user, urlFormatter.get(), config, sshInfo, change),
@@ -392,11 +387,11 @@ public class CommitValidators {
   /** Limits the number of files per change. */
   private static class FileCountValidator implements CommitValidationListener {
 
-    private final PatchListCache patchListCache;
+    private final GitRepositoryManager repoManager;
     private final int maxFileCount;
 
-    FileCountValidator(PatchListCache patchListCache, Config config) {
-      this.patchListCache = patchListCache;
+    FileCountValidator(GitRepositoryManager repoManager, Config config) {
+      this.repoManager = repoManager;
       maxFileCount = config.getInt("change", null, "maxFiles", 100_000);
     }
 
@@ -414,20 +409,17 @@ public class CommitValidators {
         return Collections.emptyList();
       }
 
-      PatchListKey patchListKey =
-          PatchListKey.againstBase(
-              receiveEvent.commit.getId(), receiveEvent.commit.getParentCount());
-      DiffSummaryKey diffSummaryKey = DiffSummaryKey.fromPatchListKey(patchListKey);
+      // Use DiffFormatter to compute the number of files in the change. This should be faster than
+      // the previous approach of using the PatchListCache.
       try {
-        DiffSummary diffSummary =
-            patchListCache.getDiffSummary(diffSummaryKey, receiveEvent.project.getNameKey());
-        if (diffSummary.getPaths().size() > maxFileCount) {
+        long changedFiles = countChangedFiles(receiveEvent);
+        if (changedFiles > maxFileCount) {
           throw new CommitValidationException(
               String.format(
                   "Exceeding maximum number of files per change (%d > %d)",
-                  diffSummary.getPaths().size(), maxFileCount));
+                  changedFiles, maxFileCount));
         }
-      } catch (PatchListNotAvailableException e) {
+      } catch (IOException e) {
         // This happens e.g. for cherrypicks.
         if (!receiveEvent.command.getRefName().startsWith(REFS_CHANGES)) {
           logger.atWarning().withCause(e).log(
@@ -436,6 +428,21 @@ public class CommitValidators {
       }
       return Collections.emptyList();
     }
+
+    private long countChangedFiles(CommitReceivedEvent receiveEvent) throws IOException {
+      try (Repository repository = repoManager.openRepository(receiveEvent.project.getNameKey());
+          RevWalk revWalk = new RevWalk(repository);
+          DiffFormatter diffFormatter = new DiffFormatter(DisabledOutputStream.INSTANCE)) {
+        diffFormatter.setReader(revWalk.getObjectReader(), repository.getConfig());
+        diffFormatter.setDetectRenames(true);
+        // For merge commits, i.e. >1 parents, we use parent #0 by convention.
+        List<DiffEntry> diffEntries =
+            diffFormatter.scan(
+                receiveEvent.commit.getParentCount() > 0 ? receiveEvent.commit.getParent(0) : null,
+                receiveEvent.commit);
+        return diffEntries.stream().map(DiffEntry::getNewPath).distinct().count();
+      }
+    }
   }
 
   /** If this is the special project configuration branch, validate the config. */

javatests/com/google/gerrit/acceptance/server/git/receive/ReceiveCommitsLimitsIT.java

@@ -14,54 +14,97 @@
 
 package com.google.gerrit.acceptance.server.git.receive;
 
-import static com.google.common.truth.Truth.assertThat;
-
-import com.google.common.cache.Cache;
+import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
 import com.google.gerrit.acceptance.AbstractDaemonTest;
-import com.google.gerrit.acceptance.PushOneCommit;
 import com.google.gerrit.acceptance.config.GerritConfig;
-import com.google.gerrit.server.patch.DiffSummary;
-import com.google.gerrit.server.patch.DiffSummaryKey;
-import com.google.inject.Inject;
-import com.google.inject.name.Named;
+import org.eclipse.jgit.revwalk.RevCommit;
 import org.junit.Test;
 
 /** Tests for applying limits to e.g. number of files per change. */
 public class ReceiveCommitsLimitsIT extends AbstractDaemonTest {
+  @Test
+  @GerritConfig(name = "change.maxFiles", value = "2")
+  public void limitFileCount() throws Exception {
+    // Create the parent.
+    RevCommit parent =
+        commitBuilder()
+            .add("foo.txt", "same old, same old")
+            .add("bar.txt", "bar")
+            .message("blah")
+            .create();
+    testRepo.reset(parent);
 
-  @Inject
-  private @Named("diff_summary") Cache<DiffSummaryKey, DiffSummary> diffSummaryCache;
+    // A commit with 2 files is OK.
+    pushFactory
+        .create(
+            admin.newIdent(),
+            testRepo,
+            "blah",
+            ImmutableMap.of(
+                "foo.txt", "same old, same old", "bar.txt", "changed file", "baz.txt", "new file"))
+        .setParent(parent)
+        .to("refs/for/master")
+        .assertOkStatus();
+
+    // A commit with 3 files is rejected.
+    pushFactory
+        .create(
+            admin.newIdent(),
+            testRepo,
+            "blah",
+            ImmutableMap.of(
+                "foo.txt",
+                "same old, same old",
+                "bar.txt",
+                "changed file",
+                "baz.txt",
+                "new file",
+                "boom.txt",
+                "boom!"))
+        .setParent(parent)
+        .to("refs/for/master")
+        .assertErrorStatus("Exceeding maximum number of files per change (3 > 2)");
+  }
 
   @Test
   @GerritConfig(name = "change.maxFiles", value = "1")
-  public void limitFileCount() throws Exception {
-    PushOneCommit.Result result =
-        pushFactory
-            .create(
-                admin.newIdent(),
-                testRepo,
-                "foo",
-                ImmutableMap.of("foo", "foo-1.0", "bar", "bar-1.0"))
-            .to("refs/for/master");
-    result.assertErrorStatus("Exceeding maximum number of files per change (2 > 1)");
-  }
+  public void limitFileCount_merge() throws Exception {
+    // Create the parents.
+    RevCommit commitFoo =
+        commitBuilder().add("foo.txt", "same old, same old").message("blah").create();
+    RevCommit commitBar =
+        testRepo
+            .branch("branch")
+            .commit()
+            .insertChangeId()
+            .add("bar.txt", "bar")
+            .message("blah")
+            .create();
+    testRepo.reset(commitFoo);
 
-  @Test
-  public void cacheKeyMatches() throws Exception {
-    int cacheSizeBefore = diffSummaryCache.asMap().size();
-    PushOneCommit.Result result =
-        pushFactory
-            .create(
-                admin.newIdent(),
-                testRepo,
-                "foo",
-                ImmutableMap.of("foo", "foo-1.0", "bar", "bar-1.0"))
-            .to("refs/for/master");
-    result.assertOkStatus();
+    // By convention we diff against the first parent.
 
-    // Assert that we only performed the diff computation once. This would e.g. catch
-    // bugs/deviations in the computation of the cache key.
-    assertThat(diffSummaryCache.asMap()).hasSize(cacheSizeBefore + 1);
+    // commitFoo is first -> 1 file changed -> OK
+    pushFactory
+        .create(
+            admin.newIdent(),
+            testRepo,
+            "blah",
+            ImmutableMap.of("foo.txt", "same old, same old", "bar.txt", "changed file"))
+        .setParents(ImmutableList.of(commitFoo, commitBar))
+        .to("refs/for/master")
+        .assertOkStatus();
+
+    // commitBar is first -> 2 files changed -> rejected
+    pushFactory
+        .create(
+            admin.newIdent(),
+            testRepo,
+            "blah",
+            ImmutableMap.of("foo.txt", "same old, same old", "bar.txt", "changed file"))
+        .setParents(ImmutableList.of(commitBar, commitFoo))
+        .to("refs/for/master")
+        .assertErrorStatus("Exceeding maximum number of files per change (2 > 1)");
   }
 }