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

375: Improve Merge PR RFR email #591

Closed
wants to merge 8 commits into from
Closed
Changes from 1 commit
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
@@ -41,8 +41,7 @@ private ArchiveItem(ArchiveItem parent, String id, ZonedDateTime created, ZonedD
private static Optional<Commit> mergeCommit(PullRequest pr, Repository localRepo, Hash head) {
try {
var author = new Author("duke", "duke@openjdk.org");
var prUtils = new PullRequestUtils(pr);
var hash = prUtils.createCommit(localRepo, head, author, author, pr.title());
var hash = PullRequestUtils.createCommit(pr, localRepo, head, author, author, pr.title());
return localRepo.lookup(hash);
} catch (IOException | CommitFailure e) {
return Optional.empty();
@@ -60,8 +59,7 @@ static ArchiveItem from(PullRequest pr, Repository localRepo, HostUserToEmailAut
() -> "",
() -> ArchiveMessages.composeConversation(pr),
() -> {
var prUtils = new PullRequestUtils(pr);
if (prUtils.isMerge()) {
if (PullRequestUtils.isMerge(pr)) {
//TODO: Try to merge in target - generate possible conflict webrev
var mergeCommit = mergeCommit(pr, localRepo, head);
var mergeWebrevs = new ArrayList<WebrevDescription>();
@@ -292,9 +292,8 @@ public void run(Path scratchPath) {
// Materialize the PR's source and target ref
var seedPath = bot.seedStorage().orElse(scratchPath.resolve("seeds"));
var hostedRepositoryPool = new HostedRepositoryPool(seedPath);
var localRepoPath = scratchPath.resolve("mlbridge-mergebase");
var localRepo = hostedRepositoryPool.checkout(pr, localRepoPath.resolve(pr.repository().name()));
localRepo.fetch(pr.repository().url(), "+" + pr.targetRef() + ":archiveworkitem", false);
var localRepoPath = scratchPath.resolve("mlbridge-mergebase").resolve(pr.repository().name());
var localRepo = PullRequestUtils.materialize(hostedRepositoryPool, pr, localRepoPath);

var webrevPath = scratchPath.resolve("mlbridge-webrevs");
var listServer = MailingListServerFactory.createMailmanServer(bot.listArchive(), bot.smtpServer(), bot.sendInterval());
@@ -93,8 +93,7 @@ private List<ArchiveItem> generateArchiveItems(List<Email> sentEmails, Repositor
}

// Check if we're at a revision not previously reported
var prUtils = new PullRequestUtils(pr);
var baseHash = prUtils.baseHash(localRepo);
var baseHash = PullRequestUtils.baseHash(pr, localRepo);
if (!baseHash.equals(lastBase) || !pr.headHash().equals(lastHead)) {
if (generated.isEmpty()) {
var first = ArchiveItem.from(pr, localRepo, hostUserToEmailAuthor, issueTracker, issuePrefix, webrevGenerator, webrevNotification, pr.createdAt(), pr.updatedAt(), baseHash, pr.headHash(), subjectPrefix, threadPrefix);
@@ -71,8 +71,7 @@ private CheckRun(CheckWorkItem workItem, PullRequest pr, Repository localRepo, L
this.censusInstance = censusInstance;
this.ignoreStaleReviews = ignoreStaleReviews;

var prUtils = new PullRequestUtils(pr);
baseHash = prUtils.baseHash(localRepo);
baseHash = PullRequestUtils.baseHash(pr, localRepo);
checkablePullRequest = new CheckablePullRequest(pr, localRepo, ignoreStaleReviews);
}

@@ -150,9 +150,8 @@ public void run(Path scratchPath) {
try {
var seedPath = bot.seedStorage().orElse(scratchPath.resolve("seeds"));
var hostedRepositoryPool = new HostedRepositoryPool(seedPath);
var localRepoPath = scratchPath.resolve("pr").resolve("check");
var localRepo = hostedRepositoryPool.checkout(pr, localRepoPath.resolve(pr.repository().name()));
localRepo.fetch(pr.repository().url(), "+" + pr.targetRef() + ":checkworkitem", false);
var localRepoPath = scratchPath.resolve("pr").resolve("check").resolve(pr.repository().name());
var localRepo = PullRequestUtils.materialize(hostedRepositoryPool, pr, localRepoPath);

CheckRun.execute(this, pr, localRepo, comments, allReviews, activeReviews, labels, census, bot.ignoreStaleReviews());
} catch (IOException e) {
@@ -94,10 +94,8 @@ Hash commit(Hash finalHead, Namespace namespace, String censusDomain, String spo
Author author;
var contributor = namespace.get(pr.author().id());

var prUtils = new PullRequestUtils(pr);

if (contributor == null) {
if (prUtils.isMerge()) {
if (PullRequestUtils.isMerge(pr)) {
throw new CommitFailure("Merges can only be performed by Committers.");
}

@@ -117,7 +115,7 @@ Hash commit(Hash finalHead, Namespace namespace, String censusDomain, String spo

var activeReviews = filterActiveReviews(pr.reviews());
var commitMessage = commitMessage(activeReviews, namespace);
return prUtils.createCommit(localRepo, finalHead, author, committer, commitMessage);
return PullRequestUtils.createCommit(pr, localRepo, finalHead, author, committer, commitMessage);
}

PullRequestCheckIssueVisitor createVisitor(Hash localHash, CensusInstance censusInstance) throws IOException {
@@ -27,8 +27,6 @@
import org.openjdk.skara.vcs.Hash;

import java.io.*;
import java.net.URLEncoder;
import java.nio.charset.StandardCharsets;
import java.nio.file.Path;
import java.util.*;
import java.util.logging.Logger;
@@ -88,12 +86,10 @@ public void handle(PullRequestBot bot, PullRequest pr, CensusInstance censusInst

// Run a final jcheck to ensure the change has been properly reviewed
try {
var sanitizedUrl = URLEncoder.encode(pr.repository().webUrl().toString(), StandardCharsets.UTF_8);
var path = scratchPath.resolve("integrate").resolve(sanitizedUrl);
var path = scratchPath.resolve("integrate").resolve(pr.repository().name());
var seedPath = bot.seedStorage().orElse(scratchPath.resolve("seeds"));
var hostedRepositoryPool = new HostedRepositoryPool(seedPath);
var localRepo = hostedRepositoryPool.checkout(pr, path);
localRepo.fetch(pr.repository().url(), "+" + pr.targetRef() + ":integratecommand", false);
var localRepo = PullRequestUtils.materialize(hostedRepositoryPool, pr, path);
var checkablePr = new CheckablePullRequest(pr, localRepo, bot.ignoreStaleReviews());

// Validate the target hash if requested
@@ -43,8 +43,7 @@ public String toString() {

private Set<String> getLabels(Repository localRepo) throws IOException {
var labels = new HashSet<String>();
var prUtils = new PullRequestUtils(pr);
var files = prUtils.changedFiles(localRepo);
var files = PullRequestUtils.changedFiles(pr, localRepo);
for (var file : files) {
for (var label : bot.labelPatterns().entrySet()) {
for (var pattern : label.getValue()) {
@@ -68,8 +67,7 @@ public void run(Path scratchPath) {
var path = scratchPath.resolve("pr").resolve("labeler").resolve(pr.repository().name());
var seedPath = bot.seedStorage().orElse(scratchPath.resolve("seeds"));
var hostedRepositoryPool = new HostedRepositoryPool(seedPath);
var localRepo = hostedRepositoryPool.checkout(pr, path);
localRepo.fetch(pr.repository().url(), "+" + pr.targetRef() + ":labelerworkitem", false);
var localRepo = PullRequestUtils.materialize(hostedRepositoryPool, pr, path);
var newLabels = getLabels(localRepo);
var currentLabels = pr.labels().stream()
.filter(key -> bot.labelPatterns().containsKey(key))
@@ -27,8 +27,6 @@
import org.openjdk.skara.vcs.Hash;

import java.io.*;
import java.net.URLEncoder;
import java.nio.charset.StandardCharsets;
import java.nio.file.Path;
import java.util.*;
import java.util.logging.Logger;
@@ -73,12 +71,10 @@ public void handle(PullRequestBot bot, PullRequest pr, CensusInstance censusInst

// Execute merge
try {
var sanitizedUrl = URLEncoder.encode(pr.repository().webUrl().toString(), StandardCharsets.UTF_8);
var path = scratchPath.resolve("sponsor").resolve(sanitizedUrl);
var path = scratchPath.resolve("sponsor").resolve(pr.repository().name());
var seedPath = bot.seedStorage().orElse(scratchPath.resolve("seeds"));
var hostedRepositoryPool = new HostedRepositoryPool(seedPath);
var localRepo = hostedRepositoryPool.checkout(pr, path);
localRepo.fetch(pr.repository().url(), "+" + pr.targetRef() + ":sponsorcommand", false);
var localRepo = PullRequestUtils.materialize(hostedRepositoryPool, pr, path);
var checkablePr = new CheckablePullRequest(pr, localRepo, bot.ignoreStaleReviews());

// Validate the target hash if requested
@@ -171,8 +171,4 @@ public Repository checkout(HostedRepository hostedRepository, String ref, Path p
}
return localRepo;
}

public Repository checkout(PullRequest pr, Path path) throws IOException {
return checkout(pr.repository(), pr.headHash().hex(), path);
}
}
@@ -31,13 +31,7 @@
import java.util.regex.Pattern;

public class PullRequestUtils {
private final PullRequest pr;

public PullRequestUtils(PullRequest pr) {
this.pr = pr;
}

private Hash commitSquashed(Repository localRepo, Hash finalHead, Author author, Author committer, String commitMessage) throws IOException {
private static Hash commitSquashed(PullRequest pr, Repository localRepo, Hash finalHead, Author author, Author committer, String commitMessage) throws IOException {
return localRepo.commit(commitMessage, author.name(), author.email(), ZonedDateTime.now(),
committer.name(), committer.email(), ZonedDateTime.now(), List.of(pr.targetHash()), localRepo.tree(finalHead));
}
@@ -52,10 +46,10 @@ private MergeSource(String repositoryName, String branchName) {
}
}

private final Pattern mergeSourceFullPattern = Pattern.compile("^Merge ([-/\\w]+):([-\\w]+)$");
private final Pattern mergeSourceBranchOnlyPattern = Pattern.compile("^Merge ([-\\w]+)$");
private final static Pattern mergeSourceFullPattern = Pattern.compile("^Merge ([-/\\w]+):([-\\w]+)$");
private final static Pattern mergeSourceBranchOnlyPattern = Pattern.compile("^Merge ([-\\w]+)$");

private Optional<MergeSource> mergeSource(Repository localRepo) {
private static Optional<MergeSource> mergeSource(PullRequest pr, Repository localRepo) {
var repoMatcher = mergeSourceFullPattern.matcher(pr.title());
if (!repoMatcher.matches()) {
var branchMatcher = mergeSourceBranchOnlyPattern.matcher(pr.title());
@@ -64,9 +58,9 @@ private Optional<MergeSource> mergeSource(Repository localRepo) {
}

// Verify that the branch exists
var isValidBranch = remoteBranches(localRepo).stream()
.map(Reference::name)
.anyMatch(branch -> branch.equals(branchMatcher.group(1)));
var isValidBranch = remoteBranches(pr, localRepo).stream()
.map(Reference::name)
.anyMatch(branch -> branch.equals(branchMatcher.group(1)));
if (!isValidBranch) {
// Assume the name refers to a sibling repository
var repoName = Path.of(pr.repository().name()).resolveSibling(branchMatcher.group(1)).toString();
@@ -79,12 +73,12 @@ private Optional<MergeSource> mergeSource(Repository localRepo) {
return Optional.of(new MergeSource(repoMatcher.group(1), repoMatcher.group(2)));
}

private CommitMetadata findSourceMergeCommit(Repository localRepo, List<CommitMetadata> commits) throws IOException, CommitFailure {
private static CommitMetadata findSourceMergeCommit(PullRequest pr, Repository localRepo, List<CommitMetadata> commits) throws IOException, CommitFailure {
if (commits.size() < 2) {
throw new CommitFailure("A merge PR must contain at least two commits that are not already present in the target.");
}

var source = mergeSource(localRepo);
var source = mergeSource(pr, localRepo);
if (source.isEmpty()) {
throw new CommitFailure("Could not determine the source for this merge. A Merge PR title must be specified on the format: " +
"Merge `project`:`branch` to allow verification of the merge contents.");
@@ -131,9 +125,9 @@ private CommitMetadata findSourceMergeCommit(Repository localRepo, List<CommitMe
return commits.get(mergeCommitIndex);
}

private Hash commitMerge(Repository localRepo, Hash finalHead, Author author, Author committer, String commitMessage) throws IOException, CommitFailure {
var commits = localRepo.commitMetadata(baseHash(localRepo), finalHead);
var mergeCommit = findSourceMergeCommit(localRepo, commits);
private static Hash commitMerge(PullRequest pr, Repository localRepo, Hash finalHead, Author author, Author committer, String commitMessage) throws IOException, CommitFailure {
var commits = localRepo.commitMetadata(baseHash(pr, localRepo), finalHead);
var mergeCommit = findSourceMergeCommit(pr, localRepo, commits);

// Find the parent which is on the target branch - we will replace it with the target hash (if there were no merge conflicts)
Hash firstParent = null;
@@ -156,36 +150,42 @@ private Hash commitMerge(Repository localRepo, Hash finalHead, Author author, Au
committer.name(), committer.email(), ZonedDateTime.now(), finalParents, localRepo.tree(finalHead));
}

public boolean isMerge() {
public static Repository materialize(HostedRepositoryPool hostedRepositoryPool, PullRequest pr, Path path) throws IOException {
var localRepo = hostedRepositoryPool.checkout(pr.repository(), pr.headHash().hex(), path);
localRepo.fetch(pr.repository().url(), "+" + pr.targetRef() + ":prutils_targetref", false);
return localRepo;
}

public static boolean isMerge(PullRequest pr) {
return pr.title().startsWith("Merge");
}

public Hash createCommit(Repository localRepo, Hash finalHead, Author author, Author committer, String commitMessage) throws IOException, CommitFailure {
public static Hash createCommit(PullRequest pr, Repository localRepo, Hash finalHead, Author author, Author committer, String commitMessage) throws IOException, CommitFailure {
Hash commit;
if (!isMerge()) {
commit = commitSquashed(localRepo, finalHead, author, committer, commitMessage);
if (!isMerge(pr)) {
commit = commitSquashed(pr, localRepo, finalHead, author, committer, commitMessage);
} else {
commit = commitMerge(localRepo, finalHead, author, committer, commitMessage);
commit = commitMerge(pr, localRepo, finalHead, author, committer, commitMessage);
}
localRepo.checkout(commit, true);
return commit;
}

public Hash baseHash(Repository localRepo) throws IOException {
public static Hash baseHash(PullRequest pr, Repository localRepo) throws IOException {
return localRepo.mergeBase(pr.targetHash(), pr.headHash());
}

public Set<Path> changedFiles(Repository localRepo) throws IOException {
public static Set<Path> changedFiles(PullRequest pr, Repository localRepo) throws IOException {
var ret = new HashSet<Path>();
var changes = localRepo.diff(baseHash(localRepo), pr.headHash());
var changes = localRepo.diff(baseHash(pr, localRepo), pr.headHash());
for (var patch : changes.patches()) {
patch.target().path().ifPresent(ret::add);
patch.source().path().ifPresent(ret::add);
}
return ret;
}

private List<Reference> remoteBranches(Repository localRepo) {
private static List<Reference> remoteBranches(PullRequest pr, Repository localRepo) {
try {
return localRepo.remoteBranches(pr.repository().url().toString());
} catch (IOException e) {