CodeOwnerApprovalCheck: Skip the file path when looking up OWNERS files
When checking code owner approvals we iterate over all files that are
touched in the change and lookup the OWNERS files that are relevant for
them. For each file we start at the file path and then iterate up the
parent folders to look for OWNERS files. E.g. for file
"/foo/bar/baz.md" we check the following OWNERS files:
1. /foo/bar/baz.md/OWNERS
2. /foo/bar/OWNERS
3. /foo/OWNERS
4. /OWNERS
If we know that the given path is a file path, as in
CodeOwnerApprovalCheck, we can skip the first lookup as we know that
this OWNERS file will never exist.
The lower layers (CodeOwnerConfigHierarchy) accept arbitrary paths (file
paths and folder paths). Hence they must continue to check for the
"<path>/OWNERS" file.
Skipping this lookup in CodeOwnersApprovalCheck may look like an
optimization that is not worth doing, but we saw such lookups of
non-existing OWNERS files take up to 10ms, which sums up if you have
changes that touch 1000s of files.
Signed-off-by: Edwin Kempin <ekempin@google.com>
Change-Id: I9be58eb55183ef9177ae0c12b85691b5a26a7abd
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheck.java b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheck.java
index a70fc53..10d869b 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheck.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheck.java
@@ -546,7 +546,7 @@
AtomicBoolean hasRevelantCodeOwnerDefinitions = new AtomicBoolean(false);
AtomicBoolean parentCodeOwnersAreIgnored = new AtomicBoolean(false);
- codeOwnerConfigHierarchy.visit(
+ codeOwnerConfigHierarchy.visitForFile(
branch,
revision,
absolutePath,
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerConfigHierarchy.java b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerConfigHierarchy.java
index 6ce1a87..7ba3352 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerConfigHierarchy.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerConfigHierarchy.java
@@ -132,6 +132,56 @@
Path absolutePath,
PathCodeOwnersVisitor pathCodeOwnersVisitor,
Consumer<CodeOwnerConfig.Key> parentCodeOwnersIgnoredCallback) {
+ visit(
+ branchNameKey,
+ revision,
+ absolutePath,
+ absolutePath,
+ pathCodeOwnersVisitor,
+ parentCodeOwnersIgnoredCallback);
+ }
+
+ /**
+ * Visits the path code owners in the given branch that apply for the given file path by following
+ * the path hierarchy from the given path up to the root folder.
+ *
+ * <p>Same as {@link #visit(BranchNameKey, ObjectId, Path, PathCodeOwnersVisitor, Consumer)} with
+ * the only difference that the provided path must be a file path (no folder path). Knowing that
+ * that the path is a file path allows us to skip checking if there is a code owner config file in
+ * this path (if it's a file it cannot contain a code owner config file). This is a performance
+ * optimization that matters if code owner config files need to be looked up for 1000s of files
+ * (e.g. for large changes).
+ *
+ * @param branchNameKey project and branch from which the code owner configs should be visited
+ * @param revision the branch revision from which the code owner configs should be loaded
+ * @param absoluteFilePath the path for which the code owner configs should be visited; the path
+ * must be absolute; must be the path of a file; the path may or may not exist
+ * @param pathCodeOwnersVisitor visitor that should be invoked for the applying path code owners
+ * @param parentCodeOwnersIgnoredCallback callback that is invoked for the first visited code
+ * owner config that ignores parent code owners
+ */
+ public void visitForFile(
+ BranchNameKey branchNameKey,
+ ObjectId revision,
+ Path absoluteFilePath,
+ PathCodeOwnersVisitor pathCodeOwnersVisitor,
+ Consumer<CodeOwnerConfig.Key> parentCodeOwnersIgnoredCallback) {
+ visit(
+ branchNameKey,
+ revision,
+ absoluteFilePath,
+ absoluteFilePath.getParent(),
+ pathCodeOwnersVisitor,
+ parentCodeOwnersIgnoredCallback);
+ }
+
+ private void visit(
+ BranchNameKey branchNameKey,
+ ObjectId revision,
+ Path absolutePath,
+ Path startFolder,
+ PathCodeOwnersVisitor pathCodeOwnersVisitor,
+ Consumer<CodeOwnerConfig.Key> parentCodeOwnersIgnoredCallback) {
requireNonNull(branchNameKey, "branch");
requireNonNull(revision, "revision");
requireNonNull(absolutePath, "absolutePath");
@@ -143,9 +193,9 @@
"visiting code owner configs for '%s' in branch '%s' in project '%s' (revision = '%s')",
absolutePath, branchNameKey.shortName(), branchNameKey.project(), revision.name());
- // Next path in which we look for a code owner configuration. We start at the given path and
+ // Next path in which we look for a code owner configuration. We start at the given folder and
// then go up the parent hierarchy.
- Path ownerConfigFolder = absolutePath;
+ Path ownerConfigFolder = startFolder;
// Iterate over the parent code owner configurations.
while (ownerConfigFolder != null) {
