Skip to content

Commit

Permalink
2190: Setting maximum size limit for PR diff in generating Webrev
Browse files Browse the repository at this point in the history
Reviewed-by: erikj
  • Loading branch information
zhaosongzs committed Mar 8, 2024
1 parent 7b937ab commit af682b4
Show file tree
Hide file tree
Showing 9 changed files with 235 additions and 59 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@

class ArchiveMessages {

private static final String WEBREV_UNAVAILABLE_COMMENT = "Webrev is not available because diff is too large";
private static String filterCommentsAndCommands(String body) {
var parsedBody = PullRequestBody.parse(body);
body = parsedBody.bodyText();
Expand Down Expand Up @@ -325,7 +326,9 @@ static String composeConversationFooter(PullRequest pr, URI issueProject, String
"Commit messages:\n" +
formatCommitMessagesBrief(commits, commitsLink).orElse("") + "\n\n" +
"Changes: " + pr.changeUrl() + "\n" +
(webrev.uri() == null ? "" : " Webrev: " + webrev.uri().toString() + "\n") +
(webrev.diffTooLarge() ?
" Webrev: " + WEBREV_UNAVAILABLE_COMMENT + "\n" :
(webrev.uri() == null ? "" : " Webrev: " + webrev.uri().toString() + "\n")) +
issueString +
" Stats: " + stats(localRepo, base, head) + "\n" +
" Patch: " + pr.diffUrl().toString() + "\n" +
Expand All @@ -338,7 +341,7 @@ static String composeMergeConversationFooter(PullRequest pr, Repository localRep
var commitsLink = commitsLink(pr, base, head);
String webrevLinks;
if (webrevs.size() > 0) {
if (webrevs.stream().noneMatch(w -> w.uri() != null)) {
if (webrevs.stream().noneMatch(w -> w.uri() != null || w.diffTooLarge())) {
webrevLinks = "";
} else {
var containsConflicts = webrevs.stream().anyMatch(w -> w.type().equals(WebrevDescription.Type.MERGE_CONFLICT));
Expand All @@ -351,7 +354,9 @@ static String composeMergeConversationFooter(PullRequest pr, Repository localRep
(containsMergeDiffs ? "the adjustments done while merging with regards to each parent branch" : "")
+ ":\n" +
webrevs.stream()
.map(d -> String.format(" - %s: %s", d.shortLabel(), d.uri()))
.map(d -> d.diffTooLarge() ?
String.format(" - %s: %s", d.shortLabel(), WEBREV_UNAVAILABLE_COMMENT) :
String.format(" - %s: %s", d.shortLabel(), d.uri()))
.collect(Collectors.joining("\n")) + "\n\n";
}
} else {
Expand All @@ -369,7 +374,9 @@ static String composeMergeConversationFooter(PullRequest pr, Repository localRep

static String composeRebasedFooter(PullRequest pr, Repository localRepo, WebrevDescription fullWebrev, Hash base, Hash head) {
return "Changes: " + pr.changeUrl() + "\n" +
(fullWebrev.uri() == null ? "" : " Webrev: " + fullWebrev.uri().toString() + "\n") +
(fullWebrev.diffTooLarge() ?
" Webrev: " + WEBREV_UNAVAILABLE_COMMENT + "\n" :
(fullWebrev.uri() == null ? "" : " Webrev: " + fullWebrev.uri().toString() + "\n")) +
" Stats: " + stats(localRepo, base, head) + "\n" +
" Patch: " + pr.diffUrl().toString() + "\n" +
" Fetch: " + fetchCommand(pr) + "\n\n" +
Expand All @@ -380,9 +387,11 @@ static String composeIncrementalFooter(PullRequest pr, Repository localRepo, Web
return "Changes:\n" +
" - all: " + pr.changeUrl() + "\n" +
" - new: " + pr.changeUrl(lastHead) + "\n\n" +
(fullWebrev.uri() == null ? "" : "Webrevs:\n") +
(fullWebrev.uri() == null ? "" : " - full: " + fullWebrev.uri().toString() + "\n") +
(incrementalWebrev.uri() == null ? "" : " - incr: " + incrementalWebrev.uri().toString() + "\n\n") +
(fullWebrev.diffTooLarge() ? "Webrevs:\n" : fullWebrev.uri() == null ? "" : "Webrevs:\n") +
(fullWebrev.diffTooLarge() ? " - full: " + WEBREV_UNAVAILABLE_COMMENT + "\n" :
fullWebrev.uri() == null ? "" : " - full: " + fullWebrev.uri().toString() + "\n") +
(incrementalWebrev.diffTooLarge() ? " - incr: " + WEBREV_UNAVAILABLE_COMMENT + "\n\n" :
incrementalWebrev.uri() == null ? "" : " - incr: " + incrementalWebrev.uri().toString() + "\n\n") +
" Stats: " + stats(localRepo, lastHead, head) + "\n" +
" Patch: " + pr.diffUrl().toString() + "\n" +
" Fetch: " + fetchCommand(pr) + "\n\n" +
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2019, 2023, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2019, 2024, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
Expand Down Expand Up @@ -176,16 +176,18 @@ private boolean ignoreComment(HostUser author, String body, ZonedDateTime create
private static final String webrevListMarker = "<!-- mlbridge webrev list -->";

private void updateWebrevComment(List<Comment> comments, int index, List<WebrevDescription> webrevs) {
if (webrevs.stream().noneMatch(w -> w.uri() != null)) {
if (webrevs.stream().noneMatch(w -> (w.uri() != null || w.diffTooLarge()))) {
return;
}
var existing = comments.stream()
.filter(comment -> comment.author().equals(pr.repository().forge().currentUser()))
.filter(comment -> comment.body().contains(WEBREV_COMMENT_MARKER))
.findAny();
var webrevDescriptions = webrevs.stream()
.map(d -> String.format("[%s](%s)", d.label(), d.uri()))
.collect(Collectors.joining(" - "));
.map(d -> d.diffTooLarge() ?
String.format("[%s](%s)", d.label(), "Webrev is not available because diff is too large") :
String.format("[%s](%s)", d.label(), d.uri()))
.collect(Collectors.joining(" - "));
var comment = WEBREV_COMMENT_MARKER + "\n";
comment += webrevHeaderMarker + "\n";
comment += "### Webrevs" + "\n";
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2020, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2020, 2024, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
Expand Down Expand Up @@ -36,17 +36,20 @@ public enum Type {
private final URI uri;
private final Type type;
private final String description;
private final boolean diffTooLarge;

public WebrevDescription(URI uri, Type type, String description) {
public WebrevDescription(URI uri, Type type, String description, boolean diffTooLarge) {
this.uri = uri;
this.type = type;
this.description = description;
this.diffTooLarge = diffTooLarge;
}

public WebrevDescription(URI uri, Type type) {
public WebrevDescription(URI uri, Type type, boolean diffTooLarge) {
this.uri = uri;
this.type = type;
this.description = null;
this.diffTooLarge = diffTooLarge;
}

public Type type() {
Expand All @@ -57,6 +60,9 @@ public URI uri() {
return uri;
}

public boolean diffTooLarge() {
return diffTooLarge;
}
public String label() {
switch (type) {
case FULL:
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2019, 2022, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2019, 2024, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
Expand Down Expand Up @@ -30,6 +30,7 @@
import org.openjdk.skara.vcs.*;
import org.openjdk.skara.vcs.openjdk.Issue;
import org.openjdk.skara.version.Version;
import org.openjdk.skara.webrev.DiffTooLargeException;
import org.openjdk.skara.webrev.Webrev;

import java.io.*;
Expand Down Expand Up @@ -89,7 +90,7 @@ class WebrevStorage {
this.generateJSON = generateJSON;
}

private void generateHTML(PullRequest pr, ReadOnlyRepository localRepository, Path folder, Diff diff, Hash base, Hash head) throws IOException {
private void generateHTML(PullRequest pr, ReadOnlyRepository localRepository, Path folder, Diff diff, Hash base, Hash head) throws IOException, DiffTooLargeException {
Files.createDirectories(folder);
var fullName = pr.author().fullName();
var builder = Webrev.repository(localRepository)
Expand Down Expand Up @@ -127,7 +128,7 @@ private void generateHTML(PullRequest pr, ReadOnlyRepository localRepository, Pa
}
}

private void generateJSON(PullRequest pr, ReadOnlyRepository localRepository, Path folder, Diff diff, Hash base, Hash head) throws IOException {
private void generateJSON(PullRequest pr, ReadOnlyRepository localRepository, Path folder, Diff diff, Hash base, Hash head) throws IOException, DiffTooLargeException {
Files.createDirectories(folder);
var builder = Webrev.repository(localRepository)
.output(folder)
Expand Down Expand Up @@ -286,7 +287,7 @@ private void awaitPublication(URI uri, Duration timeout) throws IOException {

private URI createAndArchive(PullRequest pr, Repository localRepository, Path jsonScratchPath, Path htmlScratchPath,
Diff diff, Hash base, Hash head, String identifier,
Repository jsonLocalStorage, Repository htmlLocalStorage) {
Repository jsonLocalStorage, Repository htmlLocalStorage) throws DiffTooLargeException {
try {
if (!generateHTML && !generateJSON) {
return null;
Expand Down Expand Up @@ -353,15 +354,23 @@ private void initializeLocalStorage() {
@Override
public WebrevDescription generate(Hash base, Hash head, String identifier, WebrevDescription.Type type) {
initializeLocalStorage();
var uri = createAndArchive(pr, localRepository, jsonScratchPath, htmlScratchPath, null, base, head, identifier, jsonLocalStorage, htmlLocalStorage);
return new WebrevDescription(uri, type);
try {
var uri = createAndArchive(pr, localRepository, jsonScratchPath, htmlScratchPath, null, base, head, identifier, jsonLocalStorage, htmlLocalStorage);
return new WebrevDescription(uri, type, false);
} catch (DiffTooLargeException e) {
return new WebrevDescription(null, type, true);
}
}

@Override
public WebrevDescription generate(Diff diff, String identifier, WebrevDescription.Type type, String description) {
initializeLocalStorage();
var uri = createAndArchive(pr, localRepository, jsonScratchPath, htmlScratchPath, diff, diff.from(), diff.to(), identifier, jsonLocalStorage, htmlLocalStorage);
return new WebrevDescription(uri, type, description);
try {
var uri = createAndArchive(pr, localRepository, jsonScratchPath, htmlScratchPath, diff, diff.from(), diff.to(), identifier, jsonLocalStorage, htmlLocalStorage);
return new WebrevDescription(uri, type, description, false);
} catch (DiffTooLargeException e) {
return new WebrevDescription(null, type, description, true);
}
}
};
}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2019, 2023, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2019, 2024, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
Expand Down Expand Up @@ -3985,4 +3985,100 @@ void archiveLongBody(TestInfo testInfo) throws IOException {
assertTrue(mail.body().contains(pr.store().body()));
}
}

@Test
void largeDiffArchive(TestInfo testInfo) throws IOException {
try (var credentials = new HostCredentials(testInfo);
var tempFolder = new TemporaryDirectory();
var archiveFolder = new TemporaryDirectory();
var webrevFolder = new TemporaryDirectory();
var listServer = new TestMailmanServer();
var webrevServer = new TestWebrevServer()) {
var author = credentials.getHostedRepository();
var archive = credentials.getHostedRepository();
var ignored = credentials.getHostedRepository();
var listAddress = EmailAddress.parse(listServer.createList("test"));
var censusBuilder = credentials.getCensusBuilder()
.addAuthor(author.forge().currentUser().id());
var from = EmailAddress.from("test", "test@test.mail");
var mlBot = MailingListBridgeBot.newBuilder()
.from(from)
.repo(author)
.archive(archive)
.censusRepo(censusBuilder.build())
.lists(List.of(new MailingListConfiguration(listAddress, Set.of())))
.ignoredUsers(Set.of(ignored.forge().currentUser().username()))
.ignoredComments(Set.of())
.listArchive(listServer.getArchive())
.smtpServer(listServer.getSMTP())
.webrevStorageHTMLRepository(archive)
.webrevStorageRef("webrev")
.webrevStorageBase(Path.of("test"))
.webrevStorageBaseUri(webrevServer.uri())
.readyLabels(Set.of("rfr"))
.readyComments(Map.of(ignored.forge().currentUser().username(), Pattern.compile("ready")))
.issueTracker(URIBuilder.base("http://issues.test/browse/").build())
.headers(Map.of("Extra1", "val1", "Extra2", "val2"))
.sendInterval(Duration.ZERO)
.build();

// Populate the projects repository
var localRepo = CheckableRepository.init(tempFolder.path(), author.repositoryType());
var masterHash = localRepo.resolve("master").orElseThrow();
localRepo.push(masterHash, author.authenticatedUrl(), "master", true);
localRepo.push(masterHash, archive.authenticatedUrl(), "webrev", true);

// Make a very big change with a corresponding PR
var editHash = CheckableRepository.appendAndCommit(localRepo, "A simple change\n".repeat(300001),
"Change msg\n\nWith several lines");
localRepo.push(editHash, author.authenticatedUrl(), "edit", true);
var pr = credentials.createPullRequest(archive, "master", "edit", "1234: This is a pull request");
pr.setBody("This should not be ready");

// Flag it as ready for review
pr.setBody("This should now be ready");
pr.addLabel("rfr");

// Post a ready comment
var ignoredPr = ignored.pullRequest(pr.id());
ignoredPr.addComment("ready");

// Run an archive pass
TestBotRunner.runPeriodicItems(mlBot);

// The archive should now contain an entry
Repository.materialize(archiveFolder.path(), archive.authenticatedUrl(), "master");
assertTrue(archiveContains(archiveFolder.path(), "Webrev: Webrev is not available because diff is too large"));

assertTrue(pr.store().comments().get(1).body().contains("[Full](Webrev is not available because diff is too large)"));
// The mailing list as well
listServer.processIncoming();
var mailmanServer = MailingListServerFactory.createMailmanServer(listServer.getArchive(), listServer.getSMTP(), Duration.ZERO);
var mailmanList = mailmanServer.getListReader(listAddress.address());
var conversations = mailmanList.conversations(Duration.ofDays(1));
assertEquals(1, conversations.size());
var mail = conversations.get(0).first();
assertTrue(mail.body().contains("Webrev: Webrev is not available because diff is too large"));

// And there shouldn't be a webrev
Repository.materialize(webrevFolder.path(), archive.authenticatedUrl(), "webrev");
assertFalse(Files.exists(webrevFolder.path().resolve("test")));

// Make a small change
editHash = CheckableRepository.appendAndCommit(localRepo, "A simple change 2",
"Change msg\n\nWith several lines");
localRepo.push(editHash, author.authenticatedUrl(), "edit", true);

TestBotRunner.runPeriodicItems(mlBot);
assertTrue(pr.store().comments().get(1).body().contains("webrevs/test/1/00-01)"));
listServer.processIncoming();
conversations = mailmanList.conversations(Duration.ofDays(1));
mail = conversations.get(0).allMessages().get(1);
assertTrue(mail.body().contains("webrevs/test/1/00-01"));

// And there should be a webrev
Repository.materialize(webrevFolder.path(), archive.authenticatedUrl(), "webrev");
assertTrue(webrevContains(webrevFolder.path(), "1 lines changed"));
}
}
}
Loading

1 comment on commit af682b4

@openjdk-notifier
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please sign in to comment.