Skip to content
Permalink
Browse files
295: jcheck should always use configuration from the target branch
Reviewed-by: rwestberg
  • Loading branch information
edvbld committed Mar 4, 2020
1 parent 3cf0696 commit 9fe63b93625802e0e70ec122adb218b07d66e9e3
Showing 8 changed files with 179 additions and 48 deletions.
@@ -224,8 +224,8 @@ PullRequestCheckIssueVisitor createVisitor(Hash localHash, CensusInstance census
}

void executeChecks(Hash localHash, CensusInstance censusInstance, PullRequestCheckIssueVisitor visitor, List<String> additionalConfiguration) throws Exception {
try (var issues = JCheck.check(localRepo(), censusInstance.census(), CommitMessageParsers.v1, "HEAD^!",
localHash, new HashMap<>(), new HashSet<>(), additionalConfiguration)) {
try (var issues = JCheck.check(localRepo(), censusInstance.census(), CommitMessageParsers.v1, localHash,
targetHash, additionalConfiguration)) {
for (var issue : issues) {
issue.accept(visitor);
}
@@ -44,6 +44,7 @@ public class JCheck {
private final List<CommitCheck> commitChecks;
private final List<RepositoryCheck> repositoryChecks;
private final List<String> additionalConfiguration;
private final JCheckConfiguration overridingConfiguration;
private final Logger log = Logger.getLogger("org.openjdk.skara.jcheck");

private JCheckConfiguration cachedConfiguration = null;
@@ -56,13 +57,15 @@ public class JCheck {
Pattern allowedTags,
Map<String, Set<Hash>> whitelist,
Set<Hash> blacklist,
List<String> additionalConfiguration) throws IOException {
List<String> additionalConfiguration,
JCheckConfiguration overridingConfiguration) throws IOException {
this.repository = repository;
this.census = census;
this.parser = parser;
this.revisionRange = revisionRange;
this.whitelist = whitelist;
this.additionalConfiguration = additionalConfiguration;
this.overridingConfiguration = overridingConfiguration;

var utils = new Utilities();
commitChecks = List.of(
@@ -98,6 +101,9 @@ private static Optional<JCheckConfiguration> parseConfiguration(ReadOnlyReposito
}

private Optional<JCheckConfiguration> getConfigurationFor(Commit c) {
if (overridingConfiguration != null) {
return Optional.of(overridingConfiguration);
}
var confPath = Paths.get(".jcheck/conf");
var changesConfiguration = c.parentDiffs()
.stream()
@@ -223,7 +229,8 @@ private static Issues check(ReadOnlyRepository repository,
String revisionRange,
Map<String, Set<Hash>> whitelist,
Set<Hash> blacklist,
List<String> additionalConfiguration) throws IOException {
List<String> additionalConfiguration,
JCheckConfiguration configuration) throws IOException {

var defaultBranchRegex = "|" + repository.defaultBranch().name();
var allowedBranches = Pattern.compile("^(?:" + branchRegex + defaultBranchRegex + ")$");
@@ -232,60 +239,48 @@ private static Issues check(ReadOnlyRepository repository,
var defaultTagRegex = defaultTag.isPresent() ? "|" + defaultTag.get().name() : "";
var allowedTags = Pattern.compile("^(?:" + tagRegex + defaultTagRegex + ")$");

var jcheck = new JCheck(repository, census, parser, revisionRange, allowedBranches, allowedTags, whitelist, blacklist, additionalConfiguration);
var jcheck = new JCheck(repository, census, parser, revisionRange, allowedBranches, allowedTags, whitelist, blacklist, additionalConfiguration, configuration);
return jcheck.issues();
}

public static Issues check(ReadOnlyRepository repository,
Census census,
CommitMessageParser parser,
String revisionRange,
Hash toCheck,
Hash configuration,
Map<String, Set<Hash>> whitelist,
Set<Hash> blacklist,
List<String> additionalConfiguration) throws IOException {
if (repository.isEmpty()) {
return new Issues(new ArrayList<Issue>().iterator(), null);
}

var conf = parseConfiguration(repository, configuration, additionalConfiguration);
var conf = parseConfiguration(repository, configuration, additionalConfiguration).orElseThrow(() ->
new IllegalArgumentException("No .jcheck/conf present at hash " + configuration.hex())
);

var branchRegex = conf.isPresent() ? conf.get().repository().branches() : ".*";
var tagRegex = conf.isPresent() ? conf.get().repository().tags() : ".*";
var branchRegex = conf.repository().branches();
var tagRegex = conf.repository().tags();

return check(repository, census, parser, branchRegex, tagRegex, revisionRange, whitelist, blacklist, additionalConfiguration);
return check(repository, census, parser, branchRegex, tagRegex, repository.range(toCheck), Map.of(), Set.of(), List.of(), conf);
}

public static Issues check(ReadOnlyRepository repository,
Census census,
CommitMessageParser parser,
String revisionRange,
Hash configuration,
Map<String, Set<Hash>> whitelist,
Set<Hash> blacklist) throws IOException {
return check(repository, census, parser, revisionRange, configuration, whitelist, blacklist, List.of());
}
if (repository.isEmpty()) {
return new Issues(new ArrayList<Issue>().iterator(), null);
}

public static Issues check(ReadOnlyRepository repository,
Census census,
CommitMessageParser parser,
String revisionRange) throws IOException {
var master = repository.resolve(repository.defaultBranch().name())
.orElseThrow(() -> new IllegalStateException("Default branch not found"));
var whitelist = new HashMap<String, Set<Hash>>();
var blacklist = new HashSet<Hash>();
return check(repository, census, parser, revisionRange, master, whitelist, blacklist);
}

public static Issues check(ReadOnlyRepository repository,
Census census,
CommitMessageParser parser,
String revisionRange,
Map<String, Set<Hash>> whitelist,
Set<Hash> blacklist) throws IOException {
var master = repository.resolve(repository.defaultBranch().name())
.orElseThrow(() -> new IllegalStateException("Default branch not found"));
return check(repository, census, parser, revisionRange, master, whitelist, blacklist);
var conf = parseConfiguration(repository, master, List.of());
var branchRegex = conf.isPresent() ? conf.get().repository().branches() : ".*";
var tagRegex = conf.isPresent() ? conf.get().repository().tags() : ".*";

return check(repository, census, parser, branchRegex, tagRegex, revisionRange, whitelist, blacklist, List.of(), null);
}

public static Set<Check> checks(ReadOnlyRepository repository, Census census, Hash hash) throws IOException {
@@ -297,7 +292,8 @@ public static Set<Check> checks(ReadOnlyRepository repository, Census census, Ha
Pattern.compile(".*"),
new HashMap<String, Set<Hash>>(),
new HashSet<Hash>(),
List.of());
List.of(),
null);
return jcheck.checksForCommits();
}
}
@@ -35,7 +35,7 @@
import java.util.*;
import java.util.stream.Collectors;

import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.*;

class JCheckTests {
static class CheckableRepository {
@@ -63,7 +63,7 @@ public static Repository create(Path path, VCS vcs) throws IOException {
}
repo.add(checkConf);

repo.commit("Initial commit", "duke", "duke@openjdk.java.net");
repo.commit("Initial commit\n\nReviewed-by: user2", "user3", "user3@openjdk.java.net");

return repo;
}
@@ -75,10 +75,10 @@ static void populateCensusDirectory(Path censusDir) throws IOException {
var contributorsContent = List.of(
"<?xml version=\"1.0\" encoding=\"UTF-8\" ?>",
"<contributors>",
" <contributor username=\"user_1\" full-name=\"User Number 1\" />",
" <contributor username=\"user_2\" full-name=\"User Number 2\" />",
" <contributor username=\"user_3\" full-name=\"User Number 3\" />",
" <contributor username=\"user_4\" full-name=\"User Number 4\" />",
" <contributor username=\"user1\" full-name=\"User Number 1\" />",
" <contributor username=\"user2\" full-name=\"User Number 2\" />",
" <contributor username=\"user3\" full-name=\"User Number 3\" />",
" <contributor username=\"user4\" full-name=\"User Number 4\" />",
"</contributors>");
Files.write(contributorsFile, contributorsContent);

@@ -90,8 +90,8 @@ static void populateCensusDirectory(Path censusDir) throws IOException {
"<?xml version=\"1.0\" encoding=\"UTF-8\" ?>",
"<group name=\"test\" full-name=\"TEST\">",
" <lead username=\"user_4\" />",
" <member username=\"user_1\" since=\"1\" />",
" <member username=\"user_2\" since=\"1\" />",
" <member username=\"user1\" since=\"0\" />",
" <member username=\"user2\" since=\"0\" />",
"</group>");
Files.write(testGroupFile, testGroupContent);

@@ -102,10 +102,10 @@ static void populateCensusDirectory(Path censusDir) throws IOException {
var testProjectContent = List.of(
"<?xml version=\"1.0\" encoding=\"UTF-8\" ?>",
"<project name=\"test\" full-name=\"TEST\" sponsor=\"test\">",
" <lead username=\"user_1\" since=\"1\" />",
" <reviewer username=\"user_2\" since=\"1\" />",
" <committer username=\"user_3\" since=\"1\" />",
" <author username=\"user_4\" since=\"1\" />",
" <lead username=\"user1\" since=\"0\" />",
" <reviewer username=\"user2\" since=\"0\" />",
" <committer username=\"user3\" since=\"0\" />",
" <author username=\"user4\" since=\"0\" />",
"</project>");
Files.write(testProjectFile, testProjectContent);

@@ -116,8 +116,8 @@ static void populateCensusDirectory(Path censusDir) throws IOException {
var namespaceContent = List.of(
"<?xml version=\"1.0\" encoding=\"UTF-8\" ?>",
"<namespace name=\"github.com\">",
" <user id=\"1234567\" census=\"user_1\" />",
" <user id=\"2345678\" census=\"user_2\" />",
" <user id=\"1234567\" census=\"user1\" />",
" <user id=\"2345678\" census=\"user2\" />",
"</namespace>");
Files.write(namespaceFile, namespaceContent);

@@ -285,7 +285,49 @@ void checkRemoval(VCS vcs) throws Exception {
var census = Census.parse(censusPath);

var visitor = new TestVisitor();
try (var issues = JCheck.check(repo, census, CommitMessageParsers.v1, first.hex() + ".." + second.hex())) {
try (var issues = JCheck.check(repo, census, CommitMessageParsers.v1, first.hex() + ".." + second.hex(), Map.of(), Set.of())) {
for (var issue : issues) {
issue.accept(visitor);
}
}
assertEquals(Set.of("org.openjdk.skara.jcheck.TooFewReviewersIssue"), visitor.issueNames());
}
}

@ParameterizedTest
@EnumSource(VCS.class)
void checkOverridingConfiguration(VCS vcs) throws Exception {
try (var dir = new TemporaryDirectory()) {
var repoPath = dir.path().resolve("repo");
var repo = CheckableRepository.create(repoPath, vcs);

var initialCommit = repo.commits().asList().get(0);

var jcheckConf = repoPath.resolve(".jcheck").resolve("conf");
assertTrue(Files.exists(jcheckConf));
Files.writeString(jcheckConf, "[checks \"reviewers\"]\nminimum = 0\n",
StandardOpenOption.WRITE, StandardOpenOption.APPEND);
repo.add(jcheckConf);
var secondCommit = repo.commit("Do not require reviews", "user3", "user3@openjdk.java.net");

var censusPath = dir.path().resolve("census");
Files.createDirectories(censusPath);
CensusCreator.populateCensusDirectory(censusPath);
var census = Census.parse(censusPath);

// Check the last commit without reviewers, should pass since .jcheck/conf was updated
var range = initialCommit.hash().hex() + ".." + secondCommit.hex();
var visitor = new TestVisitor();
try (var issues = JCheck.check(repo, census, CommitMessageParsers.v1, range, Map.of(), Set.of())) {
for (var issue : issues) {
issue.accept(visitor);
}
}
assertEquals(Set.of(), visitor.issues());

// Check the last commit without reviewers with the initial .jcheck/conf. Should fail
// due to missing reviewers.
try (var issues = JCheck.check(repo, census, CommitMessageParsers.v1, secondCommit, initialCommit.hash(), List.of())) {
for (var issue : issues) {
issue.accept(visitor);
}
@@ -304,4 +304,16 @@ public List<Submodule> submodules() throws IOException {
public Optional<Tag.Annotated> annotate(Tag tag) throws IOException {
return null;
}

public String range(Hash h) {
return null;
}

public String rangeInclusive(Hash from, Hash to) {
return null;
}

public String rangeExclusive(Hash from, Hash to) {
return null;
}
}
@@ -60,6 +60,9 @@ public interface ReadOnlyRepository {
List<CommitMetadata> commitMetadata(Hash from, Hash to, List<Path> paths) throws IOException;
List<CommitMetadata> commitMetadata(String range, List<Path> paths, boolean reverse) throws IOException;
List<CommitMetadata> commitMetadata(Hash from, Hash to, List<Path> paths, boolean reverse) throws IOException;
String range(Hash h);
String rangeInclusive(Hash from, Hash to);
String rangeExclusive(Hash from, Hash to);
Path root() throws IOException;
boolean exists() throws IOException;
boolean isHealthy() throws IOException;
@@ -1318,4 +1318,19 @@ public void config(String section, String key, String value, boolean global) thr
await(p);
}
}

@Override
public String range(Hash h) {
return h.hex() + "^!";
}

@Override
public String rangeInclusive(Hash from, Hash to) {
return from.hex() + "^.." + to.hex();
}

@Override
public String rangeExclusive(Hash from, Hash to) {
return from.hex() + ".." + to.hex();
}
}
@@ -1331,4 +1331,19 @@ public void config(String section, String key, String value, boolean global) thr
}
Files.write(hgrc, lines, StandardOpenOption.WRITE, StandardOpenOption.APPEND);
}

@Override
public String range(Hash h) {
return h.hex();
}

@Override
public String rangeInclusive(Hash from, Hash to) {
return from.hex() + ":" + to.hex();
}

@Override
public String rangeExclusive(Hash from, Hash to) {
return from.hex() + ":" + to.hex() + "-" + from.hex();
}
}
@@ -2176,4 +2176,52 @@ void testUnmergedStatus(VCS vcs) throws IOException {
assertEquals(Hash.zero(), statusEntry.source().hash());
}
}

@ParameterizedTest
@EnumSource(VCS.class)
void testRangeSingle(VCS vcs) throws IOException {
try (var dir = new TemporaryDirectory()) {
var repo = Repository.init(dir.path(), vcs);
var range = repo.range(new Hash("0123456789"));
if (vcs == VCS.GIT) {
assertEquals("0123456789^!", range);
} else if (vcs == VCS.HG) {
assertEquals("0123456789", range);
} else {
fail("Unexpected vcs: " + vcs);
}
}
}

@ParameterizedTest
@EnumSource(VCS.class)
void testRangeInclusive(VCS vcs) throws IOException {
try (var dir = new TemporaryDirectory()) {
var repo = Repository.init(dir.path(), vcs);
var range = repo.rangeInclusive(new Hash("01234"), new Hash("56789"));
if (vcs == VCS.GIT) {
assertEquals("01234^..56789", range);
} else if (vcs == VCS.HG) {
assertEquals("01234:56789", range);
} else {
fail("Unexpected vcs: " + vcs);
}
}
}

@ParameterizedTest
@EnumSource(VCS.class)
void testRangeExclusive(VCS vcs) throws IOException {
try (var dir = new TemporaryDirectory()) {
var repo = Repository.init(dir.path(), vcs);
var range = repo.rangeExclusive(new Hash("01234"), new Hash("56789"));
if (vcs == VCS.GIT) {
assertEquals("01234..56789", range);
} else if (vcs == VCS.HG) {
assertEquals("01234:56789-01234", range);
} else {
fail("Unexpected vcs: " + vcs);
}
}
}
}

0 comments on commit 9fe63b9

Please sign in to comment.