Allow to specify which group of the comment link holds the issue id This configuration option is useful when reusing the same comment link for different its plugins. Change-Id: I16786b96c388deca5559f57d1d7760a5c8472410
diff --git a/src/main/java/com/googlesource/gerrit/plugins/hooks/its/ItsConfig.java b/src/main/java/com/googlesource/gerrit/plugins/hooks/its/ItsConfig.java index 2be5be8..cdca3e8 100644 --- a/src/main/java/com/googlesource/gerrit/plugins/hooks/its/ItsConfig.java +++ b/src/main/java/com/googlesource/gerrit/plugins/hooks/its/ItsConfig.java
@@ -147,6 +147,10 @@ /** * Gets the regular expression used to identify issue ids. + * <p> + * The index of the group that holds the issue id is + * {@link #getIssuePatternGroupIndex()}. + * * @return the regular expression, or {@code null}, if there is no pattern * to match issue ids. */ @@ -161,6 +165,24 @@ } /** + * Gets the index of the group in the issue pattern that holds the issue id. + * <p> + * The corresponding issue pattern is {@link #getIssuePattern()} + * + * @return the group index for {@link #getIssuePattern()} that holds the + * issue id. The group index is guaranteed to be a valid group index. + */ + public int getIssuePatternGroupIndex() { + Pattern pattern = getIssuePattern(); + int groupCount = pattern.matcher("").groupCount(); + int index = gerritConfig.getInt(pluginName, "commentlinkGroupIndex", 1); + if (index < 0 || index > groupCount) { + index = (groupCount == 0 ? 0 : 1); + } + return index; + } + + /** * Gets how necessary it is to associate commits with issues * @return policy on how necessary association with issues is */
diff --git a/src/main/java/com/googlesource/gerrit/plugins/hooks/util/IssueExtractor.java b/src/main/java/com/googlesource/gerrit/plugins/hooks/util/IssueExtractor.java index 4894492..45892d2 100644 --- a/src/main/java/com/googlesource/gerrit/plugins/hooks/util/IssueExtractor.java +++ b/src/main/java/com/googlesource/gerrit/plugins/hooks/util/IssueExtractor.java
@@ -51,8 +51,8 @@ Set<String> issues = Sets.newHashSet(); Matcher matcher = pattern.matcher(haystack); + int groupIdx = itsConfig.getIssuePatternGroupIndex(); while (matcher.find()) { - int groupIdx = Math.min(matcher.groupCount(), 1); String issueId = matcher.group(groupIdx); if (!Strings.isNullOrEmpty(issueId)) { issues.add(issueId);
diff --git a/src/main/resources/Documentation/config-common.md b/src/main/resources/Documentation/config-common.md index 26da3ca..a111aba 100644 --- a/src/main/resources/Documentation/config-common.md +++ b/src/main/resources/Documentation/config-common.md
@@ -17,8 +17,8 @@ [commentlink][upstream-comment-link-doc]s of ([per default][common-config-commentlink]) name "`@PLUGIN@`". -The first group of `commentlink.@PLUGIN@.match` is considered the -issue id. +The ([per default][common-config-commentlinkGroupIndex]) first group of +`commentlink.@PLUGIN@.match` is considered the issue id. So for example having @@ -148,6 +148,7 @@ ----------------------------------------------------------------------- [common-config-commentlink]: #common-config-commentlink +[common-config-commentlinkGroupIndex]: #common-config-commentlinkGroupIndex <a name="common-config-commentlink">`@PLUGIN@.commentlink` : The name of the comment link to use to extract issue ids. @@ -159,6 +160,13 @@ Default is `@PLUGIN@` +<a name="common-config-commentlinkGroupIndex">`@PLUGIN@.commentlinkGroupIndex` +: The group index within `@PLUGIN@.commentlink` that holds the issue id. + + Default is `1`, if there are are groups within the regular expression for + the `@PLUGIN@.commentlink` comment link, and the default is `0`, if there + are no such groups. + [Back to @PLUGIN@ documentation index][index] [index]: index.html
diff --git a/src/test/java/com/googlesource/gerrit/plugins/hooks/its/ItsConfigTest.java b/src/test/java/com/googlesource/gerrit/plugins/hooks/its/ItsConfigTest.java index 0417c55..88b99fd 100644 --- a/src/test/java/com/googlesource/gerrit/plugins/hooks/its/ItsConfigTest.java +++ b/src/test/java/com/googlesource/gerrit/plugins/hooks/its/ItsConfigTest.java
@@ -465,6 +465,102 @@ } + public void testPatternGroupIndexGroupDefault() { + ItsConfig itsConfig = createItsConfig(); + + expect(serverConfig.getString("ItsTestName", null, "commentlink")) + .andReturn(null).atLeastOnce(); + expect(serverConfig.getString("commentlink", "ItsTestName", "match")) + .andReturn("(foo)(bar)(baz)").atLeastOnce(); + expect(serverConfig.getInt("ItsTestName", "commentlinkGroupIndex", 1)) + .andReturn(1).atLeastOnce(); + + replayMocks(); + + assertEquals("Expected and actual group index do not match", + 1, itsConfig.getIssuePatternGroupIndex()); + } + + public void testPatternGroupIndexGroupDefaultGroupless() { + ItsConfig itsConfig = createItsConfig(); + + expect(serverConfig.getString("ItsTestName", null, "commentlink")) + .andReturn(null).atLeastOnce(); + expect(serverConfig.getString("commentlink", "ItsTestName", "match")) + .andReturn("foo").atLeastOnce(); + expect(serverConfig.getInt("ItsTestName", "commentlinkGroupIndex", 1)) + .andReturn(1).atLeastOnce(); + + replayMocks(); + + assertEquals("Expected and actual group index do not match", + 0, itsConfig.getIssuePatternGroupIndex()); + } + + public void testPatternGroupIndexGroup1() { + ItsConfig itsConfig = createItsConfig(); + + expect(serverConfig.getString("ItsTestName", null, "commentlink")) + .andReturn(null).atLeastOnce(); + expect(serverConfig.getString("commentlink", "ItsTestName", "match")) + .andReturn("(foo)(bar)(baz)").atLeastOnce(); + expect(serverConfig.getInt("ItsTestName", "commentlinkGroupIndex", 1)) + .andReturn(1).atLeastOnce(); + + replayMocks(); + + assertEquals("Expected and actual group index do not match", + 1, itsConfig.getIssuePatternGroupIndex()); + } + + public void testPatternGroupIndexGroup3() { + ItsConfig itsConfig = createItsConfig(); + + expect(serverConfig.getString("ItsTestName", null, "commentlink")) + .andReturn(null).atLeastOnce(); + expect(serverConfig.getString("commentlink", "ItsTestName", "match")) + .andReturn("(foo)(bar)(baz)").atLeastOnce(); + expect(serverConfig.getInt("ItsTestName", "commentlinkGroupIndex", 1)) + .andReturn(3).atLeastOnce(); + + replayMocks(); + + assertEquals("Expected and actual group index do not match", + 3, itsConfig.getIssuePatternGroupIndex()); + } + + public void testPatternGroupIndexGroupTooHigh() { + ItsConfig itsConfig = createItsConfig(); + + expect(serverConfig.getString("ItsTestName", null, "commentlink")) + .andReturn(null).atLeastOnce(); + expect(serverConfig.getString("commentlink", "ItsTestName", "match")) + .andReturn("(foo)(bar)(baz)").atLeastOnce(); + expect(serverConfig.getInt("ItsTestName", "commentlinkGroupIndex", 1)) + .andReturn(5).atLeastOnce(); + + replayMocks(); + + assertEquals("Expected and actual group index do not match", + 1, itsConfig.getIssuePatternGroupIndex()); + } + + public void testPatternGroupIndexGroupTooHighGroupless() { + ItsConfig itsConfig = createItsConfig(); + + expect(serverConfig.getString("ItsTestName", null, "commentlink")) + .andReturn(null).atLeastOnce(); + expect(serverConfig.getString("commentlink", "ItsTestName", "match")) + .andReturn("foo").atLeastOnce(); + expect(serverConfig.getInt("ItsTestName", "commentlinkGroupIndex", 1)) + .andReturn(5).atLeastOnce(); + + replayMocks(); + + assertEquals("Expected and actual group index do not match", + 0, itsConfig.getIssuePatternGroupIndex()); + } + public void testItsAssociationPolicyOptional() { ItsConfig itsConfig = createItsConfig();
diff --git a/src/test/java/com/googlesource/gerrit/plugins/hooks/util/IssueExtractorTest.java b/src/test/java/com/googlesource/gerrit/plugins/hooks/util/IssueExtractorTest.java index f20e955..7a1530d 100644 --- a/src/test/java/com/googlesource/gerrit/plugins/hooks/util/IssueExtractorTest.java +++ b/src/test/java/com/googlesource/gerrit/plugins/hooks/util/IssueExtractorTest.java
@@ -65,6 +65,7 @@ expect(itsConfig.getIssuePattern()).andReturn(Pattern.compile("bug#(\\d+)")) .atLeastOnce(); + expect(itsConfig.getIssuePatternGroupIndex()).andReturn(1).atLeastOnce(); replayMocks(); @@ -79,6 +80,7 @@ expect(itsConfig.getIssuePattern()).andReturn(Pattern.compile("bug#(X*)(\\d+)")) .atLeastOnce(); + expect(itsConfig.getIssuePatternGroupIndex()).andReturn(1).atLeastOnce(); replayMocks(); @@ -93,6 +95,7 @@ expect(itsConfig.getIssuePattern()).andReturn(Pattern.compile("bug#(\\d+)")) .atLeastOnce(); + expect(itsConfig.getIssuePatternGroupIndex()).andReturn(1).atLeastOnce(); replayMocks(); @@ -108,6 +111,7 @@ expect(itsConfig.getIssuePattern()).andReturn(Pattern.compile("bug#(\\d+)")) .atLeastOnce(); + expect(itsConfig.getIssuePatternGroupIndex()).andReturn(1).atLeastOnce(); replayMocks(); @@ -123,6 +127,7 @@ expect(itsConfig.getIssuePattern()).andReturn(Pattern.compile("bug#\\d+")) .atLeastOnce(); + expect(itsConfig.getIssuePatternGroupIndex()).andReturn(0).atLeastOnce(); replayMocks(); @@ -133,11 +138,12 @@ assertLogMessageContains("Matching"); } - public void testIssueIdsMultiGroupMatch() { + public void testIssueIdsMultiGroupMatchGroup1() { IssueExtractor issueExtractor = injector.getInstance(IssueExtractor.class); expect(itsConfig.getIssuePattern()) .andReturn(Pattern.compile("bug#(\\d)(\\d+)")).atLeastOnce(); + expect(itsConfig.getIssuePatternGroupIndex()).andReturn(1).atLeastOnce(); replayMocks(); @@ -148,11 +154,28 @@ assertLogMessageContains("Matching"); } + public void testIssueIdsMultiGroupMatchGroup2() { + IssueExtractor issueExtractor = injector.getInstance(IssueExtractor.class); + + expect(itsConfig.getIssuePattern()) + .andReturn(Pattern.compile("bug#(\\d)(\\d+)")).atLeastOnce(); + expect(itsConfig.getIssuePatternGroupIndex()).andReturn(2).atLeastOnce(); + + replayMocks(); + + String ret[] = issueExtractor.getIssueIds("Foo bug#4711 bar"); + assertEquals("Number of found ids do not match", 1, ret.length); + assertEquals("Found issue id does not match", "711", ret[0]); + + assertLogMessageContains("Matching"); + } + public void testIssueIdsMulipleMatches() { IssueExtractor issueExtractor = injector.getInstance(IssueExtractor.class); expect(itsConfig.getIssuePattern()).andReturn(Pattern.compile("bug#(\\d+)")) .atLeastOnce(); + expect(itsConfig.getIssuePatternGroupIndex()).andReturn(1).atLeastOnce(); replayMocks(); @@ -171,6 +194,7 @@ expect(itsConfig.getIssuePattern()).andReturn(Pattern.compile("bug#(\\d+)")) .atLeastOnce(); + expect(itsConfig.getIssuePatternGroupIndex()).andReturn(1).atLeastOnce(); replayMocks(); @@ -188,6 +212,7 @@ public void testIssueIdsCommitSingleIssue() { expect(itsConfig.getIssuePattern()).andReturn(Pattern.compile("bug#(\\d+)")) .atLeastOnce(); + expect(itsConfig.getIssuePatternGroupIndex()).andReturn(1).atLeastOnce(); expect(commitMessageFetcher.fetchGuarded("testProject", "1234567891123456789212345678931234567894")).andReturn( @@ -215,6 +240,7 @@ public void testIssueIdsCommitMultipleIssues() { expect(itsConfig.getIssuePattern()).andReturn(Pattern.compile("bug#(\\d+)")) .atLeastOnce(); + expect(itsConfig.getIssuePatternGroupIndex()).andReturn(1).atLeastOnce(); expect(commitMessageFetcher.fetchGuarded("testProject", "1234567891123456789212345678931234567894")).andReturn( @@ -243,6 +269,7 @@ public void testIssueIdsCommitMultipleIssuesMultipleTimes() { expect(itsConfig.getIssuePattern()).andReturn(Pattern.compile("bug#(\\d+)")) .atLeastOnce(); + expect(itsConfig.getIssuePatternGroupIndex()).andReturn(1).atLeastOnce(); expect(commitMessageFetcher.fetchGuarded("testProject", "1234567891123456789212345678931234567894")).andReturn( @@ -271,6 +298,7 @@ public void testIssueIdsCommitSingleIssueBody() { expect(itsConfig.getIssuePattern()).andReturn(Pattern.compile("bug#(\\d+)")) .atLeastOnce(); + expect(itsConfig.getIssuePatternGroupIndex()).andReturn(1).atLeastOnce(); expect(commitMessageFetcher.fetchGuarded("testProject", "1234567891123456789212345678931234567894")).andReturn( @@ -301,6 +329,7 @@ public void testIssueIdsCommitSingleIssueFooter() { expect(itsConfig.getIssuePattern()).andReturn(Pattern.compile("bug#(\\d+)")) .atLeastOnce(); + expect(itsConfig.getIssuePatternGroupIndex()).andReturn(1).atLeastOnce(); expect(commitMessageFetcher.fetchGuarded("testProject", "1234567891123456789212345678931234567894")).andReturn( @@ -332,6 +361,7 @@ public void testIssueIdsCommitMultipleIssuesFooter() { expect(itsConfig.getIssuePattern()).andReturn(Pattern.compile("bug#(\\d+)")) .atLeastOnce(); + expect(itsConfig.getIssuePatternGroupIndex()).andReturn(1).atLeastOnce(); expect(commitMessageFetcher.fetchGuarded("testProject", "1234567891123456789212345678931234567894")).andReturn( @@ -372,6 +402,7 @@ public void testIssueIdsCommitDifferentParts() { expect(itsConfig.getIssuePattern()).andReturn(Pattern.compile("bug#(\\d+)")) .atLeastOnce(); + expect(itsConfig.getIssuePatternGroupIndex()).andReturn(1).atLeastOnce(); expect(commitMessageFetcher.fetchGuarded("testProject", "1234567891123456789212345678931234567894")).andReturn( @@ -406,6 +437,7 @@ public void testIssueIdsCommitDifferentPartsEmptySubject() { expect(itsConfig.getIssuePattern()).andReturn(Pattern.compile("bug#(\\d+)")) .atLeastOnce(); + expect(itsConfig.getIssuePatternGroupIndex()).andReturn(1).atLeastOnce(); expect(commitMessageFetcher.fetchGuarded("testProject", "1234567891123456789212345678931234567894")).andReturn( @@ -439,6 +471,7 @@ public void testIssueIdsCommitDifferentPartsLinePastFooter() { expect(itsConfig.getIssuePattern()).andReturn(Pattern.compile("bug#(\\d+)")) .atLeastOnce(); + expect(itsConfig.getIssuePatternGroupIndex()).andReturn(1).atLeastOnce(); expect(commitMessageFetcher.fetchGuarded("testProject", "1234567891123456789212345678931234567894")).andReturn( @@ -473,6 +506,7 @@ public void testIssueIdsCommitDifferentPartsLinesPastFooter() { expect(itsConfig.getIssuePattern()).andReturn(Pattern.compile("bug#(\\d+)")) .atLeastOnce(); + expect(itsConfig.getIssuePatternGroupIndex()).andReturn(1).atLeastOnce(); expect(commitMessageFetcher.fetchGuarded("testProject", "1234567891123456789212345678931234567894")).andReturn( @@ -508,6 +542,7 @@ public void testIssueIdsCommitDifferentPartsNoFooter() { expect(itsConfig.getIssuePattern()).andReturn(Pattern.compile("bug#(\\d+)")) .atLeastOnce(); + expect(itsConfig.getIssuePatternGroupIndex()).andReturn(1).atLeastOnce(); expect(commitMessageFetcher.fetchGuarded("testProject", "1234567891123456789212345678931234567894")).andReturn( @@ -535,6 +570,7 @@ public void testIssueIdsCommitDifferentPartsNoFooterTrailingLine() { expect(itsConfig.getIssuePattern()).andReturn(Pattern.compile("bug#(\\d+)")) .atLeastOnce(); + expect(itsConfig.getIssuePatternGroupIndex()).andReturn(1).atLeastOnce(); expect(commitMessageFetcher.fetchGuarded("testProject", "1234567891123456789212345678931234567894")).andReturn( @@ -562,6 +598,7 @@ public void testIssueIdsCommitDifferentPartsNoFooterTrailingLines() { expect(itsConfig.getIssuePattern()).andReturn(Pattern.compile("bug#(\\d+)")) .atLeastOnce(); + expect(itsConfig.getIssuePatternGroupIndex()).andReturn(1).atLeastOnce(); expect(commitMessageFetcher.fetchGuarded("testProject", "1234567891123456789212345678931234567894")).andReturn( @@ -590,6 +627,7 @@ public void testIssueIdsCommitEmpty() { expect(itsConfig.getIssuePattern()).andReturn(Pattern.compile("bug#(\\d+)")) .atLeastOnce(); + expect(itsConfig.getIssuePatternGroupIndex()).andReturn(1).atLeastOnce(); expect(commitMessageFetcher.fetchGuarded("testProject", "1234567891123456789212345678931234567894")).andReturn(""); @@ -611,6 +649,7 @@ public void testIssueIdsCommitBlankLine() { expect(itsConfig.getIssuePattern()).andReturn(Pattern.compile("bug#(\\d+)")) .atLeastOnce(); + expect(itsConfig.getIssuePatternGroupIndex()).andReturn(1).atLeastOnce(); expect(commitMessageFetcher.fetchGuarded("testProject", "1234567891123456789212345678931234567894")).andReturn("\n"); @@ -630,6 +669,7 @@ public void testIssueIdsCommitBlankLines() { expect(itsConfig.getIssuePattern()).andReturn(Pattern.compile("bug#(\\d+)")) .atLeastOnce(); + expect(itsConfig.getIssuePatternGroupIndex()).andReturn(1).atLeastOnce(); expect(commitMessageFetcher.fetchGuarded("testProject", "1234567891123456789212345678931234567894")).andReturn("\n\n"); @@ -649,6 +689,7 @@ public void testIssueIdsCommitMoreBlankLines() { expect(itsConfig.getIssuePattern()).andReturn(Pattern.compile("bug#(\\d+)")) .atLeastOnce(); + expect(itsConfig.getIssuePatternGroupIndex()).andReturn(1).atLeastOnce(); expect(commitMessageFetcher.fetchGuarded("testProject", "1234567891123456789212345678931234567894")).andReturn("\n\n\n"); @@ -668,6 +709,7 @@ public void testIssueIdsCommitMixed() { expect(itsConfig.getIssuePattern()).andReturn(Pattern.compile("bug#(\\d+)")) .atLeastOnce(); + expect(itsConfig.getIssuePatternGroupIndex()).andReturn(1).atLeastOnce(); expect(commitMessageFetcher.fetchGuarded("testProject", "1234567891123456789212345678931234567894")).andReturn( @@ -708,6 +750,7 @@ public void testIssueIdsCommitWAddedEmptyFirst() { expect(itsConfig.getIssuePattern()).andReturn(Pattern.compile("bug#(\\d+)")) .atLeastOnce(); + expect(itsConfig.getIssuePatternGroupIndex()).andReturn(1).atLeastOnce(); expect(commitMessageFetcher.fetchGuarded("testProject", "1234567891123456789212345678931234567894")).andReturn(""); @@ -730,6 +773,7 @@ public void testIssueIdsCommitWAddedSingleSubjectIssueFirst() { expect(itsConfig.getIssuePattern()).andReturn(Pattern.compile("bug#(\\d+)")) .atLeastOnce(); + expect(itsConfig.getIssuePatternGroupIndex()).andReturn(1).atLeastOnce(); Change.Id changeId = createMock(Change.Id.class); @@ -766,6 +810,7 @@ throws OrmException { expect(itsConfig.getIssuePattern()).andReturn(Pattern.compile("bug#(\\d+)")) .atLeastOnce(); + expect(itsConfig.getIssuePatternGroupIndex()).andReturn(1).atLeastOnce(); Change.Id changeId = createMock(Change.Id.class); @@ -828,6 +873,7 @@ throws OrmException { expect(itsConfig.getIssuePattern()).andReturn(Pattern.compile("bug#(\\d+)")) .atLeastOnce(); + expect(itsConfig.getIssuePatternGroupIndex()).andReturn(1).atLeastOnce(); Change.Id changeId = createMock(Change.Id.class); @@ -889,6 +935,7 @@ throws OrmException { expect(itsConfig.getIssuePattern()).andReturn(Pattern.compile("bug#(\\d+)")) .atLeastOnce(); + expect(itsConfig.getIssuePatternGroupIndex()).andReturn(1).atLeastOnce(); Change.Id changeId = createMock(Change.Id.class); @@ -951,6 +998,7 @@ throws OrmException { expect(itsConfig.getIssuePattern()).andReturn(Pattern.compile("bug#(\\d+)")) .atLeastOnce(); + expect(itsConfig.getIssuePatternGroupIndex()).andReturn(1).atLeastOnce(); Change.Id changeId = createMock(Change.Id.class); @@ -1015,6 +1063,7 @@ throws OrmException { expect(itsConfig.getIssuePattern()).andReturn(Pattern.compile("bug#(\\d+)")) .atLeastOnce(); + expect(itsConfig.getIssuePatternGroupIndex()).andReturn(1).atLeastOnce(); Change.Id changeId = createMock(Change.Id.class); @@ -1083,6 +1132,7 @@ throws OrmException { expect(itsConfig.getIssuePattern()).andReturn(Pattern.compile("bug#(\\d+)")) .atLeastOnce(); + expect(itsConfig.getIssuePatternGroupIndex()).andReturn(1).atLeastOnce(); Change.Id changeId = createMock(Change.Id.class);