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

Add topological merge bot #105

Closed
wants to merge 7 commits into from
Closed

Conversation

JornVernee
Copy link
Member

@JornVernee JornVernee commented Aug 29, 2019

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.

Progress

  • Change must not contain extraneous whitespace
  • Change must be properly reviewed

Approvers

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Aug 29, 2019

👋 Welcome back jvernee! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request (refresh this page to view it).

@mlbridge
Copy link

@mlbridge mlbridge bot commented Aug 29, 2019

@edvbld
Copy link
Member

@edvbld edvbld commented Aug 29, 2019

Hi Jorn, thanks for the contribution! 👏

Just a quick suggestion after a brief look - how about moving Edge to its own file, Edge.java and also move tsort into its own class, TopologicalSort (in TopologicalSort.java)? That way you should have a fairly easy time to add a new test class, TopologicalSortTests, with some unit tests for the topological sort

Copy link

@mcimadamore mcimadamore left a comment

While the code looks generally ok, I'd like to understand a bit more what it does (e.g. what the various API method translate to when it comes to Git, and what the actual behavior is in case of merge conflicts). Also there are some improvements on the topo sort handling.

mcimadamore
Copy link

@mcimadamore mcimadamore commented on 4ffe0b0 Aug 29, 2019

Choose a reason for hiding this comment

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

from my understanding, git merge --abort is just fine here. This new command is the same - except for the --hard part?

mcimadamore
Copy link

@mcimadamore mcimadamore commented on 4ffe0b0 Aug 29, 2019

Choose a reason for hiding this comment

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

I see - you also wanna recover from a failed push? [in which case the changeset is there and there is no further changes in the working tree]

JornVernee
Copy link
Member Author

@JornVernee JornVernee commented on 4ffe0b0 Aug 29, 2019

Choose a reason for hiding this comment

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

Afaik merge --abort reverts any working tree changes, like conflicting file. reset --hard actually resets the branch to a certain commit. We need the hard reset when the push fails, but the merge commit has already succeeded (i.e. there is nothing to abort).

Copy link

@mcimadamore mcimadamore left a comment

looks ok

edvbld
edvbld approved these changes Aug 30, 2019
Copy link
Member

@edvbld edvbld left a comment

Looks good now, thanks for hanging in there! 👏

@openjdk openjdk bot removed the rfr label Aug 30, 2019
@openjdk
Copy link

@openjdk openjdk bot commented Aug 30, 2019

@JornVernee This change can now be integrated. The commit message will be:

Add topological merge bot

Reviewed-by: mcimadamore, ehelin
  • If you would like to add a summary, use the /summary command.
  • To list additional contributors, use the /contributor command.

Since the source branch of this PR was last updated there have been 4 commits pushed to the master branch:

  • c999004: Batch commit, add and files methods in HgRepository
  • 1f95176: 91: Avoid discarding pending items of different types
  • b46b8e5: jcheck "repository" property in [general] section not read from .jcheck/conf
  • 417c6f5: Avoid running PullRequestBot WorkItems for the same PR concurrently

Since there are no conflicts, your changes will automatically be rebased on top of the above commits when integrating. If you prefer to do this manually, please merge master into your branch first.

  • To integrate this PR with the above commit message, type /integrate in a new comment.

@openjdk openjdk bot added the ready label Aug 30, 2019
@JornVernee
Copy link
Member Author

@JornVernee JornVernee commented Aug 30, 2019

/integrate

@openjdk openjdk bot closed this Aug 30, 2019
@openjdk openjdk bot added the integrated label Aug 30, 2019
@openjdk
Copy link

@openjdk openjdk bot commented Aug 30, 2019

@JornVernee The following commits have been pushed to master since your change was applied:

Your commit was automatically rebased without conflicts.
Pushed as commit 8501faf.

@JornVernee JornVernee deleted the topo-rewrite branch Aug 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants