ci: force rebase and autosquash before merging

[frazarshad]

refs: #8500

Description

This PR does the following:

• adds a new rule to mergify which forces it to rebase and autosquash the branch.
• adds a condition of linear-history to the queuing rule for rebasing, so that only rebased PRs can be queued.
• adds a new fixup commit ci check which is a required condition for merging

Note: autosquash here refers to --autosquash flag used by the git rebase command. it is used to squash and combine fixup! commits. it does not squash all commits into a single commit before merging

It fixes the following problems in #8500

• rebases the PR to remove all merge commits and fixup! commits
• forces rebase on all PRs even if they are up to date with the master branch
• enforce linear-history check on automerge:rebase enqueue to always enforce a linear-history before merging
• the rebase and autosquash rule in mergify also fails if the PR cannot be rebased without conflicts. The failure indicates to the user that the PR must be rebased manually

Examples of use:

Testing rebase on PR that was up to date: frazarshad/mergify-experiements#2
Testing if a fixup with incorrect title stops the PR from merging: frazarshad#6

Fallback:

In case the new action does not work as expected and does not auto-rebase, the user can still manually rebase in order to use the automerge:rebase action. All other automerge actions work as originally intended.

In order to bypass the no-fixup-commit check, simply add a bypass:linear-history label to the PR

Comparison report:

This is a comparison made of the previous method for merging and the new method: https://drive.google.com/file/d/1kfIW-pVOORuYwBOQlxt-2cus0k9ynAfb/view?usp=drive_link

Security Considerations

Scaling Considerations

Documentation Considerations

Testing Considerations

Upgrade Considerations

[mhofman]

I'm confused, does this force a rebase on every PR that is labelled automerge:rebase? That seems like that may be costly on PRs that don't have any fixup or merge commits.

I also don't quite understand how the added condition for linear history helps triggering this 2, step merge process.

[warner]

My favorite history shape is a series of either non-overlapping linear branches with a single merge commit at the end:

or single commits:

A policy which always rebases, but then always uses a merge commit, makes each of those single commits into a one-commit branch (like the first case, but with one commit per PR instead of more than one), which is kind of verbose but tolerable:

We've been pretty good about maintaining these shapes.. I had to dig pretty far back to find examples of anything else. The shape that bugs me a little bit is when we get a lot of overlap between branches, like when this kind of railroad-diagram becomes more than two columns wide. That can happen when branches aren't rebased before merging:

and then the kind that I find really annoying is when we need to merge trunk into the feature branch one or more times, and/or merging secondary fixup branches (like when author A makes a PR, and author B makes a second PR to merge into A's branch before A can merge their branch into trunk). This kind of tangle is hard to pick apart later, when trying to bisect or extract a subset of commits:

Forcing an entirely linear history (rebase and always squash) would avoid the tangled mess of that last case, but it would also lose frequently-useful semantic information: sometimes a PR really wants to be implemented in two or three or six commits, each of which makes a separate logical step of changes, and squashing all those together into a single commit would make life harder for future developers that want to understand the original programmer's thinking.

In some cases, force-rebase will untangle the cases I'm worried about, but we'd need to experiment to see which cases would remain. (I have a vague memory of using GitHub's "Rebase" button on a branch that had tangles but which was otherwise up-to-date, and I think it didn't feel the need to do anything, whereas using it when the branch was even one commit behind ended up cleaning things correctly).

Also, I know git has some tools to rewrite a branch and automatically act upon commits with fixup! or amend!, as a more-automated form of git rebase --interactive. I'd be totally happy with a force-rebase which executes those instructions (and I've seen plenty of tiny fixup! commits land on trunk when someone forgot to run that step before pushing. Is that what "autosquash" means in this context?

[frazarshad]

@warner the PR does not squash all the commits into a single commit. so all individual commits should be present in the commit history.
Just to make sure i ran it once again by merging this test PR and here is the resulting commit history. It is the same as our ideal scenario

[frazarshad]

I'm confused, does this force a rebase on every PR that is labelled automerge:rebase? That seems like that may be costly on PRs that don't have any fixup or merge commits.

@mhofman yes it forces a rebase on all PRs. this was done to ensure that the rebase action runs in every case scenario without fail.
something we can do to avoid this is to add the condtion-linear-history to the rebase action. With this, it will only run when there is a merge commit in the history, in all other cases, we will be depending on our normal rebase (the one that occurs when the PR is enqueued in the merge queue) to fix the commit history.

I also don't quite understand how the added condition for linear history helps triggering this 2, step merge process.

This condition was added to avoid any PRs from going into the merge queue before being rebased by the rebase action

[mhofman]

Oh I didn't realize linear-history was a new mergify condition, not the linear-history check that I wrote. While it's great to avoid merge commits, it doesn't address the concern of up to date PRs with fixup! commits, which our check enforces.

However, the linear-history check I wrote currently does not run at all on automerge:rebase PRs.

Unless we ask mergify to implement an auto-squashable commit check, I think what we need is something like the following:

• extend out own linear-history check to run on automerge:rebase
• output a variable from that job if a rebase is needed
• have a dependent job that does nothing, but only executes if the above variable said a rebase was needed
• change the rebase merge queue condition to proceed if the above job is "skipped"
• trigger the new rebase action if that job has a "success" status

The main risk is an infinite rebase loop where git rebase --autosquash does not correctly fixup the commits (I have seen this happen, e.g. when the fixup commit references the wrong message). I am not sure how to guard against that.

[frazarshad]

it doesn't address the concern of up to date PRs with fixup! commits, which our check enforces.

@mhofman This is already handled by the when our rebase updates then merge to master rule rebases the PR before enqueueing it. I tested this out in this PR.

The rebase updates then merge to master rule handles rebasing when history is linear and rebase and autosquash handles the rebasing when the history is non-linear.

While this seperation of responsibilities between two rules is less than ideal, I think its more reliable than the alternative of writing our own CI job to handle this.

[mhofman]

@mhofman This is already handled by the when our rebase updates then merge to master rule rebases the PR before enqueueing it.

To clarify, it does so because it forces a rebase of all automerge:rebase PRs, even if the PR already has an up to date linear history with no fixup commits?

Unless the rebase done by mergify does not change the commit in these cases (which would be wonderful), I think we can't afford a force rebase as it would force a re-run of all CI jobs when not necessary.

If the commit does not change, there may still be the case of erroneous fixup! commits that don't apply, or more generally failed rebase. I would like to know what the behavior of mergify is in these cases.

I tested this out in this PR.

Thanks for that video. FYI, the integration-test-result check should not be GH required, so that the PR can embark the merge train early. I think you need to reconfigure your test repo. The merge-strategy-chosen however should be a required check.

[cloudflare-workers-and-pages]

Deploying agoric-sdk with    Cloudflare Pages

Latest commit:	2dfc192
Status:	✅  Deploy successful!
Preview URL:	https://9849151f.agoric-sdk.pages.dev
Branch Preview URL:	https://fraz-force-rebase-before-que.agoric-sdk.pages.dev

View logs

[frazarshad]

To clarify, it does so because it forces a rebase of all automerge:rebase PRs, even if the PR already has an up to date linear history with no fixup commits?

@mhofman no. fixup commits are already handled by the mergify config in master. this PR only adds the ability to handle merge commits on up to date PRs

Unless the rebase done by mergify does not change the commit in these cases (which would be wonderful), I think we can't afford a force rebase as it would force a re-run of all CI jobs when not necessary.

As far as i have observed other than the case where there are no merge or fixup commits in a PR and the PR is up to date, the commit hashes are changed in all cases. This is true of our old way of doing things as well as our new method. In cases where the old method would not rebase a PR, the new method would not rebase a PR as well, hence the CI would not be re run

To make things more clear and understandable, i've tested every possible scenario i can come up with and compiled them in this document. Let me know if theres anything ive missed in this.
Overall i think this two step rebasing is the best case scenario.

[mhofman]

Some minor tweaks requested

[mhofman]

I realized we can avoid the separate rebase action in more cases.

[mhofman]

Thinking about this more, I think can get away with allowing non linear history PRs to enter the merge queue if they're not up to date

Suggested change

	- linear-history
	- or:
	- '#commits-behind>0'
	- linear-history

[mhofman]

similarly we should be able to restrict how often this runs to cases not covered by the above

Suggested change

	- -linear-history
	- '#commits-behind=0'
	- or:
	- -linear-history
	- check-failed=no-fixup-commits
	- -draft

[mhofman]

I know this is the default, but should we specify autosquash: true here too to make things more obvious?

[frazarshad]

that field is deprecated and will probably wont be supported in a few months time like the other deprecated fields

[mhofman]

I just realized we're only checking fixup! commits here, and we should also check for squash! and amend! commits as well. As a drive-by, we should fix the linear-history job above to include amend! too

[mhofman]

To summarize the requested change I think we'll end up with the following 3 cases when a non-draft PR has the automerge:rebase label:

• PR is up to date and linear, the PR will enter the merge queue
  • if the PR contains fixup commits, the no-fixup-commit check will fail, exit the queue, and trigger the rebase action
  • if the PR contains no fixup commits, it will simply merge
• PR is up to date and not linear, it will not enter the queue, but instead will trigger the rebase action.
• PR is not up to date (regardless of linear), it will enter the merge queue and immediately get rebased
  • after the rebase, see "PR is up to date and linear" (because a rebase makes a PR linear)

If I had time I'd make a diagram out of this.

[mhofman]

Thanks for addressing all these! Were you be able to redo a round of validation on mergify-experiments with the newest changes? I'm only 95% confident in my suggestions ;)

[frazarshad]

Thanks for addressing all these! Were you be able to redo a round of validation on mergify-experiments with the newest changes? I'm only 95% confident in my suggestions ;)

i validated all the main cases. especially verifying the new suggested parts