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 IgnoreUpdateMerges option #39

Merged
merged 5 commits into from
Dec 13, 2018
Merged

Conversation

bluekeyes
Copy link
Member

When this option is true, a commit on the PR branch is ignored if it meets these conditions:

  1. Has two parents (i.e. is a simple merge commit)
  2. Was created via the UI or the API
  3. Has one parent in the last 100 commits on the target branch

An ignored commit does not invalidate approvals and the author/committer do not count as contributors.

Closes #12. See that issue for background on the problem and this solution.

This PR also refactors some of the test code to avoid destructively modifying the default test data.

@bluekeyes
Copy link
Member Author

This also needs to be updated after #37 merges.

@bmoylan
Copy link

bmoylan commented Dec 11, 2018

Are we able to check that no "merge conflict resolution" changes were added to the merge commit?

@bluekeyes
Copy link
Member Author

I haven't found a way to distinguish between an automatically resolved conflict in a file and a manually resolved conflict short of having policy-bot simulate the merge commit. If the merge is no a fast-forward, it will reference a new tree, and if a given file had changes in both parents, the tree will reference a new blob for the file that exists in neither parent. That blob is the final state of the file, so we don't have any information about how it was created.

By performing a tree comparison, I could skip any merges where both branches modify the same file, but that (1) might block some common safe cases, leading to confusion and (2) is expensive in terms of API calls, requiring up to one call for each directory in the repository for each parent for each merge.

I don't actually know how GitHub implements this for GitHub Reviews, but I suspect they either have extra information that isn't exposed via the API or this is easier to solve if you have all the state about the PR history (e.g. if the merge state was conflict, and after a merge commit is pushed the state is mergeable, the merge commit must have resolved the conflict). We don't track any kind of state at the moment, so supporting that approach would require more work to set up and maintain a database or something.

When this option is true, a commit on the PR branch is ignored if it
meets these conditions:

  1. Has two parents (i.e. is a simple merge commit)
  2. Was created via the UI or the API
  3. Has one parent in the last 100 commits on the target branch

An ignored commit does not invalidate approvals and the author/committer
do not count as contributors.
Refactor the approval test code to avoid destructively modifying the
default pull.Context data.
@bluekeyes bluekeyes changed the base branch from bkeyes/v4-timeline to develop December 11, 2018 23:58
Copy link
Member

@jmcampanini jmcampanini left a comment

Choose a reason for hiding this comment

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

lgtm

merges on long-running branches or merges created with the API may not be
ignored. If this happens, you will need to reapprove the pull request.

Note that `policy-bot` cannot detect if an update merge contains any merge
Copy link
Member

Choose a reason for hiding this comment

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

that last sentence sounds a bit dangerous. i wonder how many of the use cases motivating this feature could be addressed with just requiring 2 approvals and allowing authors/contributors to be approvers

Copy link
Member Author

Choose a reason for hiding this comment

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

I think the main use case is when you get approval, and then someone else merges another branch first, so now you have to update your branch and get approval again. That doesn't change based on the approval conditions.

For the case where you want to ignore other contributors who click update, then the double approval approach could work.

@bluekeyes
Copy link
Member Author

I'm going to merge this with the current limitations so we can start testing it with real usage. If it turns out that it needs to be stricter to be useful, we can revisit how to detect manual conflict resolution.

@bluekeyes bluekeyes merged commit c137b46 into develop Dec 13, 2018
@bluekeyes bluekeyes deleted the bkeyes/ignore-update-merges branch December 13, 2018 19:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants