Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

295: jcheck should always use configuration from the target branch #491

Closed
wants to merge 1 commit into from
Closed
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
@@ -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);
}
}
}
}