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 a rebase_and_merge strategy #115

Closed
Elv13 opened this issue Jul 22, 2018 · 17 comments
Closed

Add a rebase_and_merge strategy #115

Elv13 opened this issue Jul 22, 2018 · 17 comments
Labels
enhancement New feature or request

Comments

@Elv13
Copy link

Elv13 commented Jul 22, 2018

Hello, me again,

In our current quest to use Mergify (as @jd proposed) to standardize our merging workflow, we come across this:

screenshot_20180722_074613

As seen in this screenshot, the graph is a bit messy. This will happen a lot more often with Mergify because maintainers tend to review the pull requests in bunch. GitHub as a "rebase this pull request" button. Gerrit even make pressing the button mandatory.

Having Mergify.io take care of this would be greatly appreciated in the short term. I would consider this (from my point of view), an high priority feature.

The rebase_and_merge strategy

It would be greatly appreciated if Mergify could automatically rebase the branches before merging them.

Of course, in some case it can fail, so, just as the normal "rebase" strategy has a fallback, "rebase_and_merge" also need a fallback. Given it would create 2 fallback variables, maybe it is also better to make a more generic "chain of strategy priority" configuration table instead of having a single default merging strategy and a bunch of variables to configure what happens when it fails. Given the other possible strategies (see below and #112), you probably need a way to make configuration more future proof.

The autosquash_and_merge strategy

git rebase --autosquash exists to allow fixup commits to be squashed together. GitHub PR led to the bad habit of using force-push to update them because autosquashing isn't one of their official merge strategy. fixup commits make PR easier to review because you don't lose the previous code a comment is replaying upon.

Again, of course, this strategy can fail like the above. It can also fail because the PR submitter unchecked the (enabled by default) option to let project maintainers update the pull request content.

@sileht
Copy link
Member

sileht commented Jul 22, 2018

We already support a global option for these https://doc.mergify.io/configuration.html#merge-strategy

@Elv13
Copy link
Author

Elv13 commented Jul 22, 2018

We already support a global option for these https://doc.mergify.io/configuration.html#merge-strategy

Sorry if I missed something, but currently, this seem to be limited to merge, rebase and squash. rebase_and_merge and autosquash_and_merge are different strategies offered by tools such as Gerrit. The other issue I opened yesterday also has a potential single_commit_rebase strategy (if you decide to implement the feature as a strategy instead of a parallel option).

If you referred to the "global option" itself, my point was that a single fallback is not enough if those strategies are to be implemented. Currently, only rebase can fail, but if the extra strategies are implemented, many of them can fail, so a single fallback is not going to work.

@sileht
Copy link
Member

sileht commented Jul 22, 2018

Ok, I see the Gerrit one and the difference, I like them too, the graph is far cleaner. That's a good idea.

We currently use what Github provides, but it should be possible to implement our own method merge strategies.

But, I think we need first to do #116 to be able to do this one.

@sileht sileht added the enhancement New feature or request label Jul 22, 2018
@Elv13
Copy link
Author

Elv13 commented Jul 22, 2018

We currently use what Github provides

Since very recently, I think it does

screenshot_20180722_134453

They added this button when merging with the merge strategy. I never tried, but I think it does rebase branches on top of the branch its getting merged into. I havn't looked if there is an API for it. However, even if there isn't if the PR author left the "allow project maintainer to mess with this", you can force-push the rebase.

@sileht
Copy link
Member

sileht commented Jul 22, 2018

If you enable "Strict" workflow on your repo, when the pull request is ready, Mergify does exactly the same thing this button does, then wait for CIs one last time, and merge the PR with the configured strategy.

This button is not available in the API. But we do the same by running really git commands, because it's just a merge of the head of the base branch in the Pull Request.

One thing, we want to do is to be able to allow to do a rebase instead of this merge. But because of some restriction of Github API, we need #116, first.

@Elv13
Copy link
Author

Elv13 commented Jul 22, 2018

Cool, thanks for the interest, looking forward to test it when it becomes available!

@jd jd changed the title [Wish] Add a rebase_and_merge strategy Add a rebase_and_merge strategy Jul 23, 2018
@blueyed
Copy link
Contributor

blueyed commented Jul 24, 2018

@Elv13

GitHub as a "rebase this pull request" button. Gerrit even make pressing the button mandatory.

That is not a rebase-button, but merge-master-into-this-button.

I think a problem with rebasing would be that the mergify would have to force-push to the PR branch (messing with the submitters upstream branch), so that is not really an option I think.
AFAIK this is required to trigger all the status checks again, and there is no way of triggering them for another "merge commit" that would be the (possibly autosquashed) rebase, is there?

@Elv13
Copy link
Author

Elv13 commented Jul 24, 2018

I think a problem with rebasing would be that the mergify would have to force-push to the PR branch (messing with the submitters upstream branch),

The branch is about to be merged anyway, I say the benefit outweigh the drawbacks. Plus, as said in the first message, it makes our git log much cleaner. To me, that's a feature in itself. Other GitHub alternative force merged commits to be above the previous HEAD (from the merged commits PoV) for this reason. If they do it, I say we should enforce this too.

Of course Gerrit implementation of this uses magic IDs in each commit and magic branches. GitHub don't do that kind of Voodoo so there is some drawbacks (also with signed commits?), but overall it sovle both the "there is too many merge commits", "the git log is spagatti" and "we want strict mode checks" all at once.

@Profpatsch
Copy link

Profpatsch commented Sep 5, 2019

One thing, we want to do is to be able to allow to do a rebase instead of this merge. But because of some restriction of Github API, we need #116, first.

What’s the status of this? #116 was closed (because not applicable in v2).

We’d really like to see mergify doing rebases onto the current master instead of merging it back into the PR branch for the strict variant. The clutter is enormous, there’s usually two mergify commits for one-commit PRs.

@jd
Copy link
Member

jd commented Sep 5, 2019

@Profpatsch Do you want strict_method: rebase? This is implemented https://doc.mergify.io/actions.html#merge

@Profpatsch
Copy link

Ah, I see. I dismissed it because of the warnings in the documentation.

  • GitHub branch protection of your repository may dismiss approved reviews.
  • GitHub branch protection of the contributor repository may refuse Mergify to force push the rebased pull request.

Can you elaborate on the may? There have to be rules, right?

@jd
Copy link
Member

jd commented Sep 5, 2019

@Profpatsch When you fork a repository and send a PR, there's a little tickbox down right in the UI (enabled by default) that allows people and apps from the target repo to modify the branch of your PR. If you disable this, Mergify won't be able to rebase the branch since.

You'll see this error then: https://doc.mergify.io/faq.html#how-do-i-fix-the-pull-request-can-t-be-updated-with-latest-base-branch-changes-owner-doesn-t-allow-modification-error

@jd
Copy link
Member

jd commented Sep 5, 2019

Closing this issue as it's now solved with strict_method: rebase.

@jd jd closed this as completed Sep 5, 2019
@Profpatsch
Copy link

Ah yes, that makes sense. Can you add the link to the documentation? I assume other people have the same question about the docstring, too.

@jd
Copy link
Member

jd commented Sep 9, 2019

Profpatsch added a commit to tweag/rules_haskell that referenced this issue Sep 9, 2019
Rebasing the PR will remove the pull request number from history, so
we merge.

But we can remove the second merge, where mergify merges
the (conflict-free) master into the PR before doing CI. Instead,
mergify should rebase (`strict_method = rebase`).

The [mergify documentation](https://doc.mergify.io/actions.html#merge)
has two warnings about strict_method rebase:

* GitHub branch protection of your repository may dismiss approved
  reviews.
* GitHub branch protection of the contributor repository may refuse
  Mergify to force push the rebased pull request.

As clarified in
Mergifyio/mergify#115 (comment)
this only applies to contributors who manually untick the “allow
maintainers to modify my branch” (or similar) option. Since that is
not the default, we can require PR authors to either have it ticked or
rebase their PR manually when mergify requests it.
@Profpatsch
Copy link

It's already in the FAQ

Can you add a link to the FAQ entry then? :)

@jd
Copy link
Member

jd commented Sep 10, 2019

I already did in #115 (comment)

Profpatsch added a commit to tweag/rules_haskell that referenced this issue Sep 10, 2019
Rebasing the PR will remove the pull request number from history, so
we merge.

But we can remove the second merge, where mergify merges
the (conflict-free) master into the PR before doing CI. Instead,
mergify should rebase (`strict_method = rebase`).

The [mergify documentation](https://doc.mergify.io/actions.html#merge)
has two warnings about strict_method rebase:

* GitHub branch protection of your repository may dismiss approved
  reviews.
* GitHub branch protection of the contributor repository may refuse
  Mergify to force push the rebased pull request.

As clarified in
Mergifyio/mergify#115 (comment)
this only applies to contributors who manually untick the “allow
maintainers to modify my branch” (or similar) option. Since that is
not the default, we can require PR authors to either have it ticked or
rebase their PR manually when mergify requests it.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Development

No branches or pull requests

5 participants