Merge changes I0f7a9dce,I767cb7b5,I8ecffa88
* changes:
Explain why implicit approvals are not always applied for owner/uploader
Parser tests: Add comments for boolean args
Correct find-owners documentation about comments
diff --git a/javatests/com/google/gerrit/plugins/codeowners/backend/findowners/FindOwnersCodeOwnerConfigParserTest.java b/javatests/com/google/gerrit/plugins/codeowners/backend/findowners/FindOwnersCodeOwnerConfigParserTest.java
index cc9cefc..d13d4f6 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/backend/findowners/FindOwnersCodeOwnerConfigParserTest.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/backend/findowners/FindOwnersCodeOwnerConfigParserTest.java
@@ -169,7 +169,7 @@
}
@Test
- public void codeOwnerConfigWithInlineComments() throws Exception {
+ public void codeOwnerConfigWithComment() throws Exception {
assertParseAndFormat(
getCodeOwnerConfig(EMAIL_1, EMAIL_2 + " # some comment", EMAIL_3),
codeOwnerConfig ->
@@ -178,10 +178,66 @@
.onlyElement()
.hasCodeOwnersEmailsThat()
.containsExactly(EMAIL_1, EMAIL_2, EMAIL_3),
+ // inline comments are dropped
getCodeOwnerConfig(EMAIL_1, EMAIL_2, EMAIL_3));
}
@Test
+ public void perFileCodeOwnerConfigWithComment() throws Exception {
+ assertParseAndFormat(
+ "per-file foo=" + EMAIL_1 + "," + EMAIL_2 + "," + EMAIL_3 + " # some comment",
+ codeOwnerConfig -> {
+ CodeOwnerSetSubject codeOwnerSetSubject =
+ assertThat(codeOwnerConfig).hasCodeOwnerSetsThat().onlyElement();
+ codeOwnerSetSubject.hasPathExpressionsThat().containsExactly("foo");
+ codeOwnerSetSubject.hasCodeOwnersEmailsThat().containsExactly(EMAIL_1, EMAIL_2, EMAIL_3);
+ },
+ // inline comments are dropped
+ getCodeOwnerConfig(
+ /* ignoreParentCodeOwners= */ false,
+ CodeOwnerSet.builder()
+ .addPathExpression("foo")
+ .addCodeOwnerEmail(EMAIL_1)
+ .addCodeOwnerEmail(EMAIL_2)
+ .addCodeOwnerEmail(EMAIL_3)
+ .build()));
+ }
+
+ @Test
+ public void setNoParentWithComment() throws Exception {
+ assertParseAndFormat(
+ "set noparent # some comment",
+ codeOwnerConfig -> {
+ assertThat(codeOwnerConfig).hasIgnoreParentCodeOwnersThat().isTrue();
+ assertThat(codeOwnerConfig).hasCodeOwnerSetsThat().isEmpty();
+ },
+ // inline comments are dropped
+ getCodeOwnerConfig(
+ CodeOwnerConfig.builder(
+ CodeOwnerConfig.Key.create(project, "master", "/"), TEST_REVISION)
+ .setIgnoreParentCodeOwners()
+ .build()));
+ }
+
+ @Test
+ public void importCodeOwnerConfigWithComment() throws Exception {
+ Path path = Paths.get("/foo/bar/OWNERS");
+ CodeOwnerConfigReference codeOwnerConfigReference =
+ CodeOwnerConfigReference.builder(CodeOwnerConfigImportMode.ALL, path).build();
+ assertParseAndFormat(
+ "include " + path + " # some comment",
+ codeOwnerConfig -> {
+ CodeOwnerConfigReferenceSubject codeOwnerConfigReferenceSubject =
+ assertThat(codeOwnerConfig).hasImportsThat().onlyElement();
+ codeOwnerConfigReferenceSubject.hasProjectThat().isEmpty();
+ codeOwnerConfigReferenceSubject.hasBranchThat().isEmpty();
+ codeOwnerConfigReferenceSubject.hasFilePathThat().isEqualTo(path);
+ },
+ // inline comments are dropped
+ getCodeOwnerConfig(codeOwnerConfigReference));
+ }
+
+ @Test
public void codeOwnerConfigWithAnnotations() throws Exception {
assertParseAndFormat(
getCodeOwnerConfig(
@@ -238,7 +294,9 @@
@Test
public void setNoParentCanBeSetMultipleTimes() throws Exception {
assertParseAndFormat(
- getCodeOwnerConfig(true, CodeOwnerSet.createWithoutPathExpressions(EMAIL_1))
+ getCodeOwnerConfig(
+ /* ignoreParentCodeOwners= */ true,
+ CodeOwnerSet.createWithoutPathExpressions(EMAIL_1))
+ "\nset noparent\nset noparent",
codeOwnerConfig -> {
assertThat(codeOwnerConfig).hasIgnoreParentCodeOwnersThat().isTrue();
@@ -248,7 +306,9 @@
.hasCodeOwnersEmailsThat()
.containsExactly(EMAIL_1);
},
- getCodeOwnerConfig(true, CodeOwnerSet.createWithoutPathExpressions(EMAIL_1)));
+ getCodeOwnerConfig(
+ /* ignoreParentCodeOwners= */ true,
+ CodeOwnerSet.createWithoutPathExpressions(EMAIL_1)));
}
@Test
@@ -257,14 +317,16 @@
CodeOwnerSet.builder().addPathExpression("foo").addCodeOwnerEmail(EMAIL_2).build();
CodeOwnerSet globalCodeOwnerSet = CodeOwnerSet.createWithoutPathExpressions(EMAIL_1, EMAIL_3);
assertParseAndFormat(
- getCodeOwnerConfig(false, perFileCodeOwnerSet, globalCodeOwnerSet),
+ getCodeOwnerConfig(
+ /* ignoreParentCodeOwners= */ false, perFileCodeOwnerSet, globalCodeOwnerSet),
codeOwnerConfig -> {
assertThat(codeOwnerConfig)
.hasCodeOwnerSetsThat()
.containsExactly(globalCodeOwnerSet, perFileCodeOwnerSet)
.inOrder();
},
- getCodeOwnerConfig(false, globalCodeOwnerSet, perFileCodeOwnerSet));
+ getCodeOwnerConfig(
+ /* ignoreParentCodeOwners= */ false, globalCodeOwnerSet, perFileCodeOwnerSet));
}
@Test
@@ -272,7 +334,7 @@
CodeOwnerSet codeOwnerSet1 = CodeOwnerSet.createWithoutPathExpressions(EMAIL_1, EMAIL_3);
CodeOwnerSet codeOwnerSet2 = CodeOwnerSet.createWithoutPathExpressions(EMAIL_2);
assertParseAndFormat(
- getCodeOwnerConfig(false, codeOwnerSet1, codeOwnerSet2),
+ getCodeOwnerConfig(/* ignoreParentCodeOwners= */ false, codeOwnerSet1, codeOwnerSet2),
codeOwnerConfig -> {
assertThat(codeOwnerConfig)
.hasCodeOwnerSetsThat()
@@ -282,7 +344,8 @@
},
// The code owner sets without path expressions are merged into one code owner set.
getCodeOwnerConfig(
- false, CodeOwnerSet.createWithoutPathExpressions(EMAIL_1, EMAIL_2, EMAIL_3)));
+ /* ignoreParentCodeOwners= */ false,
+ CodeOwnerSet.createWithoutPathExpressions(EMAIL_1, EMAIL_2, EMAIL_3)));
}
@Test
@@ -296,7 +359,7 @@
.addCodeOwnerEmail(EMAIL_2)
.build();
assertParseAndFormat(
- getCodeOwnerConfig(false, codeOwnerSet),
+ getCodeOwnerConfig(/* ignoreParentCodeOwners= */ false, codeOwnerSet),
codeOwnerConfig -> {
// we expect 2 code owner sets:
// 1. code owner set for line "per-file *.md,foo=set noparent"
diff --git a/javatests/com/google/gerrit/plugins/codeowners/backend/proto/ProtoCodeOwnerConfigParserTest.java b/javatests/com/google/gerrit/plugins/codeowners/backend/proto/ProtoCodeOwnerConfigParserTest.java
index 2e768db..5d0d20c 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/backend/proto/ProtoCodeOwnerConfigParserTest.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/backend/proto/ProtoCodeOwnerConfigParserTest.java
@@ -127,12 +127,12 @@
CodeOwnerSet codeOwnerSet1 = CodeOwnerSet.createWithoutPathExpressions(EMAIL_1, EMAIL_3);
CodeOwnerSet codeOwnerSet2 = CodeOwnerSet.createWithoutPathExpressions(EMAIL_2);
assertParseAndFormat(
- getCodeOwnerConfig(false, codeOwnerSet1, codeOwnerSet2),
+ getCodeOwnerConfig(/* ignoreParentCodeOwners= */ false, codeOwnerSet1, codeOwnerSet2),
codeOwnerConfig ->
assertThat(codeOwnerConfig.codeOwnerSets())
.containsExactly(codeOwnerSet1, codeOwnerSet2)
.inOrder(),
- getCodeOwnerConfig(false, codeOwnerSet1, codeOwnerSet2));
+ getCodeOwnerConfig(/* ignoreParentCodeOwners= */ false, codeOwnerSet1, codeOwnerSet2));
}
/**
diff --git a/resources/Documentation/backend-find-owners.md b/resources/Documentation/backend-find-owners.md
index d14ec2a..5a5c3a6 100644
--- a/resources/Documentation/backend-find-owners.md
+++ b/resources/Documentation/backend-find-owners.md
@@ -345,11 +345,8 @@
The '#' character indicates the beginning of a comment. Arbitrary text may be
added in comments.
-Comments are only supported in 2 places:
-
-* comment lines:
- A line starting with '#' (`# <comment-text>`).
-* comments after [user emails](#userEmails) (`<user-email> # <comment-text>`).
+Comments can appear at the end of any line or consume the whole line (a line
+starting with '#', `# <comment-test`).
Comments are not interpreted by the `code-owners` plugin and are intended for
human readers of the `OWNERS` files. However some projects/teams may have own
diff --git a/resources/Documentation/config-guide.md b/resources/Documentation/config-guide.md
index 86b447f..e9d5b88 100644
--- a/resources/Documentation/config-guide.md
+++ b/resources/Documentation/config-guide.md
@@ -200,6 +200,23 @@
To avoid situations like this it is recommended to not enable implicit
approvals.
+**NOTE:** Why are implicit approvals not always applied for the change owner?\
+If implicit approvals would be always applied for the change owner, and not
+only when the change owner is also the last uploader, anyone could upload a new
+patch set to a change that is owned by a code owner and get it implicitly
+approved by the change owner. This would be really bad, as it means that anyone
+could submit arbitrary code without a code owner having actually looked at it
+before the submission.
+
+**NOTE:** Why are implicit approvals not always applied for the last uploader?\
+If implicit approvals would be always applied for the last uploader, and not
+only when the last uploader is also the change owner, changes would get
+implicitly approved whenever a code owner touches a change of somebody else
+(e.g. when editing the commit message, since editing the commit message creates
+a new patch set which has the user editing the commit message as an uploader).
+This would be bad, because code owners are not aware that editing the commit
+message of a change would implictly code-owner approve it.
+
### <a id="securityMergeCommits">Required code owner approvals on merge commits
If any branch doesn't require code owner approvals or if the code owners in any
