RFR: 304: Squash additional commits into merge commits for merge PRs

Erik Helin ehelin at openjdk.java.net
Thu Mar 19 08:41:29 UTC 2020


On Thu, 19 Mar 2020 07:39:11 GMT, Robin Westberg <rwestberg at openjdk.org> wrote:

> Hi all,
> 
> Please review this change that squashes additional commits made on top of the first merge commit, to allow updating the
> PR.
> Best regards,
> Robin

Looks good in general, just a few inline comments!

bots/pr/src/main/java/org/openjdk/skara/bots/pr/CheckRun.java line 174:

> 173:                 }
> 174:                 if (mergeCommitIndex >= commits.size() - 1) {
> 175:                     ret.add("A Merge PR must contain a merge commit.");

What if you do a "Merge PR" with a single trivial merge just to merge to branches (that have identical commits). Too
narrow of a use case to think about?

bots/pr/src/main/java/org/openjdk/skara/bots/pr/CheckRun.java line 175:

> 174:                 if (mergeCommitIndex >= commits.size() - 1) {
> 175:                     ret.add("A Merge PR must contain a merge commit.");
> 176:                 }

Related to my previous comment, should the message be "A Merge PR must contain at least one merge commit and one
non-merge commit"?

bots/pr/src/main/java/org/openjdk/skara/bots/pr/PullRequestInstance.java line 153:

> 152:
> 153:         localRepo.checkout(commits.get(mergeCommitIndex).hash(), true);
> 154:         localRepo.squash(headHash);

Check that `mergeCommitIndex < commits.size()` before and otherwise throw an exception?

-------------

PR: https://git.openjdk.java.net/skara/pull/519


More information about the skara-dev mailing list