RFR: Add topological merge bot

Erik Duveblad via github.com duke at openjdk.java.net
Thu Aug 29 14:24:06 UTC 2019


On Thu, 29 Aug 2019 12:28:41 GMT, Jorn Vernee via github.com <duke at openjdk.java.net> wrote:

> This PR adds a bot that does a topological merge of the branches in a repo.
> 
> The branches can declare a dependencies file, which lists the branches that they depend on. This bot will crawl the branches, collect the dependencies for each branch, and topologically sort them based on their dependencies. Following that it will attempt to merge each dependency into the dependent in this order (this is mainly done so that we get less merges/failures if one of the root merges fails).
> 
> Branches that do not declare a dependency file implicitly depend on the master branch. Therefore the list of branches that the bot considers is passed in during configuration.
> 
> ---
> 
> Aside from that, it also fixes a minor problem with `Repository::clone` on Windows.
> 
> ----------------
> 
> Commits:
>  - 4a8f3610:	Added top bot module
> 
> Pull request:
> https://git.openjdk.java.net/skara/pull/105
> 
> Webrev:
> https://webrevs.openjdk.java.net/skara/105/webrev.00
> 
> Patch:
> https://git.openjdk.java.net/skara/pull/105.diff
> 
> Fetch command:
> git fetch https://git.openjdk.java.net/skara pull/105/head:pull/105

This PR has been reviewed by Erik Duveblad via github.com - a comment has been added. Review comment:

This is starting to look very nice, just a few more comments!

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

bots/topological/src/main/java/org/openjdk/skara/bots/topological/TopologicalBot.java line 147:

> 146:         for (var branch : branches) {
> 147:             repo.checkout(branch);
> 148:             dependencies(depsFile).forEach(dep -> deps.add(new Edge(dep, branch)));

There is no need to check out the branch just to read the file. You can replace this with for example:
```java
var hash = repo.resolve(branch).orElseThrow();
dependencies(repo.lines(hash, depsFile)).forEach(dep -> deps.add(new Edge(dep, branch)));
```
```

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

bots/topological/src/main/java/org/openjdk/skara/bots/topological/TopologicalBot.java line 151:

> 150:         return TopologicalSort.tsort(deps).stream()
> 151:             .filter(branch -> !branch.name().equals("master"))
> 152:             .collect(Collectors.toList());

If you want you can use `.filter(branch -> !branch.equals(repo.defaultBranch()))`

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

bots/topological/src/main/java/org/openjdk/skara/bots/topological/TopologicalBot.java line 181:

> 180:         var process = new ProcessBuilder()
> 181:             .command("git", "reset", "--hard", oldHead.hex())
> 182:             .directory(repo.root().toFile())

Please add `Repository::reset(boolean hard)` instead

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

bots/topological/src/main/java/org/openjdk/skara/bots/topological/TopologicalBot.java line 168:

> 167:             log.info("merge with " + parent + " succeeded. The following commits will be pushed:\n"
> 168:                     + log(repo, "origin/" + branch.name(), branch.name()).collect(Collectors.joining("\n", "\n", "\n")));
> 169:             try {

You could just do `repo.commits("origin/" + branch.name() + ".." + branch.name())` instead of rolling your own

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

bots/topological/src/main/java/org/openjdk/skara/bots/topological/TopologicalBot.java line 173:

> 172:                 log.severe("Pusing failed! Aborting...");
> 173:                 hardReset(repo, oldHead);
> 174:                 throw e;

So this would become `repo.reset(oldHead, true)` (see my comment further down for details)

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

bots/topological/src/main/java/org/openjdk/skara/bots/topological/TopologicalBot.java line 197:

> 196: 
> 197:     private static void await(Process process) throws IOException {
> 198:         try {

I don't think you need this after you have removed `log` and `hardReset`

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

bots/topological/src/main/java/org/openjdk/skara/bots/topological/TopologicalSort.java line 34:

> 33: 
> 34:     static List<Branch> tsort(List<Edge> edges) {
> 35:         List<Edge> eCopy = new ArrayList<>(edges);

I think naming the method `sort` is enough given that the class is named `TopologicalSort` :smiley_cat:

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


More information about the skara-dev mailing list