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