Merge "Fix DereferenceWithNullBranch bug pattern flagged by error prone"
diff --git a/Documentation/config-gerrit.txt b/Documentation/config-gerrit.txt
index 927d3fb..6b1dea5 100644
--- a/Documentation/config-gerrit.txt
+++ b/Documentation/config-gerrit.txt
@@ -1456,7 +1456,8 @@
[[change.maxFiles]]change.maxFiles::
+
Maximum number of files allowed per change. Larger changes are rejected and must
-be split up.
+be split up. For merge changes we are comparing against the auto-merge commit,
+so we allow large merges, if they merge cleanly.
+
By default 100,000.
diff --git a/java/com/google/gerrit/server/git/validators/CommitValidators.java b/java/com/google/gerrit/server/git/validators/CommitValidators.java
index cd1e2ee..6adaae2 100644
--- a/java/com/google/gerrit/server/git/validators/CommitValidators.java
+++ b/java/com/google/gerrit/server/git/validators/CommitValidators.java
@@ -31,6 +31,7 @@
import com.google.gerrit.entities.BooleanProjectConfig;
import com.google.gerrit.entities.BranchNameKey;
import com.google.gerrit.entities.Change;
+import com.google.gerrit.entities.Patch;
import com.google.gerrit.entities.RefNames;
import com.google.gerrit.extensions.api.config.ConsistencyCheckInfo.ConsistencyProblemInfo;
import com.google.gerrit.extensions.registration.DynamicItem;
@@ -50,7 +51,10 @@
import com.google.gerrit.server.logging.Metadata;
import com.google.gerrit.server.logging.TraceContext;
import com.google.gerrit.server.logging.TraceContext.TraceTimer;
+import com.google.gerrit.server.patch.DiffNotAvailableException;
import com.google.gerrit.server.patch.DiffOperations;
+import com.google.gerrit.server.patch.DiffOptions;
+import com.google.gerrit.server.patch.filediff.FileDiffOutput;
import com.google.gerrit.server.permissions.PermissionBackend;
import com.google.gerrit.server.permissions.PermissionBackendException;
import com.google.gerrit.server.permissions.RefPermission;
@@ -70,10 +74,10 @@
import java.util.ArrayList;
import java.util.Collections;
import java.util.List;
+import java.util.Map;
import java.util.Optional;
import java.util.regex.Pattern;
-import org.eclipse.jgit.diff.DiffEntry;
-import org.eclipse.jgit.diff.DiffFormatter;
+import java.util.stream.Collectors;
import org.eclipse.jgit.errors.ConfigInvalidException;
import org.eclipse.jgit.lib.Config;
import org.eclipse.jgit.lib.PersonIdent;
@@ -84,7 +88,6 @@
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
@@ -163,7 +166,7 @@
.add(new ProjectStateValidationListener(projectState))
.add(new AmendedGerritMergeCommitValidationListener(perm, gerritIdent))
.add(new AuthorUploaderValidator(user, perm, urlFormatter.get()))
- .add(new FileCountValidator(repoManager, config, urlFormatter.get()))
+ .add(new FileCountValidator(config, urlFormatter.get(), diffOperations))
.add(new CommitterUploaderValidator(user, perm, urlFormatter.get()))
.add(new SignedOffByValidator(user, perm, projectState))
.add(
@@ -195,7 +198,7 @@
.add(new ProjectStateValidationListener(projectState))
.add(new AmendedGerritMergeCommitValidationListener(perm, gerritIdent))
.add(new AuthorUploaderValidator(user, perm, urlFormatter.get()))
- .add(new FileCountValidator(repoManager, config, urlFormatter.get()))
+ .add(new FileCountValidator(config, urlFormatter.get(), diffOperations))
.add(new SignedOffByValidator(user, perm, projectState))
.add(
new ChangeIdValidator(
@@ -392,7 +395,8 @@
String httpHook =
String.format(
- "f=\"$(git rev-parse --git-dir)/hooks/commit-msg\"; curl -o \"$f\" %stools/hooks/commit-msg ; chmod +x \"$f\"",
+ "f=\"$(git rev-parse --git-dir)/hooks/commit-msg\"; curl -o \"$f\""
+ + " %stools/hooks/commit-msg ; chmod +x \"$f\"",
webUrl.get());
if (hostKeys.isEmpty()) {
@@ -426,7 +430,8 @@
String sshHook =
String.format(
- "gitdir=$(git rev-parse --git-dir); scp -p -P %d %s@%s:hooks/commit-msg ${gitdir}/hooks/",
+ "gitdir=$(git rev-parse --git-dir); scp -p -P %d %s@%s:hooks/commit-msg"
+ + " ${gitdir}/hooks/",
sshPort, user.getUserName().orElse("<USERNAME>"), sshHost);
return String.format(" %s\n%s\nor, for http(s):\n %s", sshHook, scpFlagHint, httpHook);
}
@@ -437,13 +442,13 @@
private static final int FILE_COUNT_WARNING_THRESHOLD = 10_000;
- private final GitRepositoryManager repoManager;
private final int maxFileCount;
private final UrlFormatter urlFormatter;
+ private final DiffOperations diffOperations;
- FileCountValidator(GitRepositoryManager repoManager, Config config, UrlFormatter urlFormatter) {
- this.repoManager = repoManager;
+ FileCountValidator(Config config, UrlFormatter urlFormatter, DiffOperations diffOperations) {
this.urlFormatter = urlFormatter;
+ this.diffOperations = diffOperations;
maxFileCount = config.getInt("change", null, "maxFiles", 100_000);
}
@@ -478,7 +483,7 @@
"Warning: Change with %d files on host %s, project %s, ref %s",
changedFiles, host, project, refName);
}
- } catch (IOException e) {
+ } catch (DiffNotAvailableException e) {
// This happens e.g. for cherrypicks.
if (!receiveEvent.command.getRefName().startsWith(REFS_CHANGES)) {
logger.atWarning().withCause(e).log(
@@ -488,20 +493,18 @@
return Collections.emptyList();
}
- private long countChangedFiles(CommitReceivedEvent receiveEvent) throws IOException {
- try (Repository repository = repoManager.openRepository(receiveEvent.project.getNameKey());
- DiffFormatter diffFormatter = new DiffFormatter(DisabledOutputStream.INSTANCE)) {
- diffFormatter.setRepository(repository);
- // Do not detect renames; that would require reading file contents, which is slow for large
- // files.
- diffFormatter.setDetectRenames(false);
- // 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();
- }
+ private int countChangedFiles(CommitReceivedEvent receiveEvent)
+ throws DiffNotAvailableException {
+ // For merge commits this will compare against auto-merge.
+ Map<String, FileDiffOutput> modifiedFiles =
+ diffOperations.listModifiedFilesAgainstParent(
+ receiveEvent.getProjectNameKey(), receiveEvent.commit, 0, DiffOptions.DEFAULTS);
+ // We don't want to count the COMMIT_MSG and MERGE_LIST files.
+ List<FileDiffOutput> modifiedFilesList =
+ modifiedFiles.values().stream()
+ .filter(p -> !Patch.isMagic(p.newPath().orElse("")))
+ .collect(Collectors.toList());
+ return modifiedFilesList.size();
}
}
diff --git a/javatests/com/google/gerrit/acceptance/server/git/receive/ReceiveCommitsLimitsIT.java b/javatests/com/google/gerrit/acceptance/server/git/receive/ReceiveCommitsLimitsIT.java
index 89b5f6e..eab0d39 100644
--- a/javatests/com/google/gerrit/acceptance/server/git/receive/ReceiveCommitsLimitsIT.java
+++ b/javatests/com/google/gerrit/acceptance/server/git/receive/ReceiveCommitsLimitsIT.java
@@ -83,27 +83,18 @@
.create();
testRepo.reset(commitFoo);
- // By convention we diff against the first parent.
-
- // commitFoo is first -> 1 file changed -> OK
+ // compared to AUTO_MERGE only one file is changed -> OK
pushFactory
.create(
- admin.newIdent(),
- testRepo,
- "blah",
- ImmutableMap.of("foo.txt", "same old, same old", "bar.txt", "changed file"))
+ admin.newIdent(), testRepo, "blah", ImmutableMap.of("foo.txt", "same old, same old"))
.setParents(ImmutableList.of(commitFoo, commitBar))
.to("refs/for/master")
.assertOkStatus();
- // commitBar is first -> 2 files changed -> rejected
+ // compared to AUTO_MERGE two files are 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))
+ .create(admin.newIdent(), testRepo, "blah", ImmutableMap.of("foo.txt", "changed"))
+ .setParents(ImmutableList.of(commitFoo, commitBar))
.to("refs/for/master")
.assertErrorStatus("Exceeding maximum number of files per change (2 > 1)");
}
