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

344: PR should warn when source branch is an upstream branch #555

Closed
wants to merge 2 commits 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
@@ -51,6 +51,7 @@ class CheckRun {
private final String progressMarker = "<!-- Anything below this marker will be automatically updated, please do not edit manually! -->";
private final String mergeReadyMarker = "<!-- PullRequestBot merge is ready comment -->";
private final String outdatedHelpMarker = "<!-- PullRequestBot outdated help comment -->";
private final String sourceBranchWarningMarker = "<!-- PullRequestBot source branch warning comment -->";
private final Pattern mergeSourceFullPattern = Pattern.compile("^Merge ([-/\\w]+):([-\\w]+)$");
private final Pattern mergeSourceBranchOnlyPattern = Pattern.compile("^Merge ([-\\w]+)$");
private final Set<String> newLabels;
@@ -83,13 +84,14 @@ private boolean isTargetBranchAllowed() {
}

private List<String> allowedTargetBranches() {
var remoteBranches = prInstance.remoteBranches().stream()
.map(Reference::name)
.map(name -> workItem.bot.allowedTargetBranches().matcher(name))
.filter(Matcher::matches)
.map(Matcher::group)
.collect(Collectors.toList());
return remoteBranches;
return pr.repository()
.branches()
.stream()
.map(HostedBranch::name)
.map(name -> workItem.bot.allowedTargetBranches().matcher(name))
.filter(Matcher::matches)
.map(Matcher::group)
.collect(Collectors.toList());
}

// For unknown contributors, check that all commits have the same name and email
@@ -560,6 +562,37 @@ private void updateMergeReadyComment(boolean isReady, String commitMessage, List
}
}

private void addSourceBranchWarningComment(List<Comment> comments) {
var existing = findComment(comments, sourceBranchWarningMarker);
if (existing.isPresent()) {
// Only add the comment once per PR
return;
}
var branch = pr.sourceRef();
var message = ":warning: @" + pr.author().userName() + " " +
"a branch with the same name as the source branch for this pull request (`" + branch + "`) " +
"is present in the [target repository](" + pr.repository().nonTransformedWebUrl() + "). " +
"If you eventually integrate this pull request then the branch `" + branch + "` " +
"in your [personal fork](" + pr.sourceRepository().nonTransformedWebUrl() + ") will diverge once you sync " +
"your personal fork with the upstream repository.\n" +
"\n" +
"To avoid this situation, create a new branch for your changes and reset the `" + branch + "` branch. " +
"You can do this by running the following commands in a local repository for your personal fork. " +
"_Note_: you do *not* have to name the new branch `NEW-BRANCH-NAME`." +
"\n" +
"```" +
"$ git checkout " + branch + "\n" +
"$ git checkout -b NEW-BRANCH-NAME\n" +
"$ git branch -f " + branch + " " + prInstance.baseHash().hex() + "\n" +
"$ git push -f origin " + branch + "\n" +
"```\n" +
"\n" +
"Then proceed to create a new pull request with `NEW-BRANCH-NAME` as the source branch and " +
"close this one.\n" +
sourceBranchWarningMarker;
pr.addComment(message);
}

private void addOutdatedComment(List<Comment> comments) {
var existing = findComment(comments, outdatedHelpMarker);
if (existing.isPresent()) {
@@ -647,6 +680,11 @@ private void checkStatus() {
newLabels.remove("merge-conflict");
}

var branchNames = pr.repository().branches().stream().map(HostedBranch::name).collect(Collectors.toSet());
if (!pr.repository().url().equals(pr.sourceRepository().url()) && branchNames.contains(pr.sourceRef())) {
addSourceBranchWarningComment(comments);
}

// Ensure that the ready for sponsor label is up to date
newLabels.remove("sponsor");
var readyHash = ReadyForSponsorTracker.latestReadyForSponsor(pr.repository().forge().currentUser(), comments);
@@ -140,4 +140,9 @@ public Hash branchHash(String ref) {
public List<PullRequest> findPullRequestsWithComment(String author, String body) {
return null;
}

@Override
public List<HostedBranch> branches() {
return List.of();
}
}
@@ -0,0 +1,65 @@
/*
* Copyright (c) 2020, 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
* under the terms of the GNU General Public License version 2 only, as
* published by the Free Software Foundation.
*
* This code is distributed in the hope that it will be useful, but WITHOUT
* ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
* FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
* version 2 for more details (a copy is included in the LICENSE file that
* accompanied this code).
*
* You should have received a copy of the GNU General Public License version
* 2 along with this work; if not, write to the Free Software Foundation,
* Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
*
* Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
* or visit www.oracle.com if you need additional information or have any
* questions.
*/
package org.openjdk.skara.forge;

import org.openjdk.skara.vcs.Hash;

import java.util.Objects;

public class HostedBranch {
private final String name;
private final Hash hash;

public HostedBranch(String name, Hash hash) {
this.name = name;
this.hash = hash;
}

public String name() {
return name;
}

public Hash hash() {
return hash;
}

@Override
public String toString() {
return name + "@" + hash.hex();
}

@Override
public int hashCode() {
return Objects.hash(name, hash);
}

@Override
public boolean equals(Object other) {
if (!(other instanceof HostedBranch)) {
return false;
}

var o = (HostedBranch) other;
return Objects.equals(name, o.name) && Objects.equals(hash, o.hash);
}
}
@@ -65,6 +65,7 @@ PullRequest createPullRequest(HostedRepository target,
HostedRepository fork();
long id();
Hash branchHash(String ref);
List<HostedBranch> branches();

default PullRequest createPullRequest(HostedRepository target,
String targetRef,
@@ -242,4 +242,13 @@ public Hash branchHash(String ref) {
var branch = request.get("branches/" + ref).execute();
return new Hash(branch.get("commit").get("sha").asString());
}

@Override
public List<HostedBranch> branches() {
var branches = request.get("branches").execute();
return branches.stream()
.map(b -> new HostedBranch(b.get("name").asString(),
new Hash(b.get("commit").get("sha").asString())))
.collect(Collectors.toList());
}
}
@@ -264,4 +264,13 @@ public Hash branchHash(String ref) {
var branch = request.get("repository/branches/" + ref).execute();
return new Hash(branch.get("commit").get("id").asString());
}

@Override
public List<HostedBranch> branches() {
var branches = request.get("branches").execute();
return branches.stream()
.map(b -> new HostedBranch(b.get("name").asString(),
new Hash(b.get("commit").get("id").asString())))
.collect(Collectors.toList());
}
}
@@ -185,6 +185,19 @@ public Hash branchHash(String ref) {
}
}

@Override
public List<HostedBranch> branches() {
try {
var result = new ArrayList<HostedBranch>();
for (var b : localRepository.branches()) {
result.add(new HostedBranch(b.name(), localRepository.resolve(b).orElseThrow()));
}
return result;
} catch (IOException e) {
throw new RuntimeException(e);
}
}

Repository localRepository() {
return localRepository;
}