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

linear-history should enforce rules on rebase too #8500

Closed
mhofman opened this issue Nov 6, 2023 · 3 comments
Closed

linear-history should enforce rules on rebase too #8500

mhofman opened this issue Nov 6, 2023 · 3 comments
Assignees
Labels
bug Something isn't working tooling repo-wide infrastructure

Comments

@mhofman
Copy link
Member

mhofman commented Nov 6, 2023

Describe the bug

#8253 added a linear-history check which enforce that no fixup or merge commits exists, to address #7768. The check is only enforced on merge PRs, assuming the rebase PRs would get autosquashed by mergify, or somehow always rebased. However if the fixup commit doesn't have a base to fixup, mergify will happily proceed forward (I believe that's the default git behavior). Also if the PR is already up to date from a merge commit with the destination branch, the rebase will not be performed.

To Reproduce

See #8481 and #8559

Expected behavior

No fixup or nested merge commits in history, regardless of automerge strategy

Additional context

We either need to get mergify to prevent merging in the rebase cases, or modify the linear-history check to run on rebase (likely has to simulate the interactive rebase first).

@mhofman mhofman added bug Something isn't working tooling repo-wide infrastructure labels Nov 6, 2023
@mhofman
Copy link
Member Author

mhofman commented Nov 7, 2023

Apparently mergify only runs on up to date PRs if allow_inplace_checks is false.

Edit: we don't want to use allow_inplace_checks: false as it would create a separate PR / merge not linked as well in GH.

Solution is likely to simulate a rebase, maybe only if the PR is up-to-date, before checking if there are any fixup commits left.

@mhofman
Copy link
Member Author

mhofman commented Jan 12, 2024

Related to this issue, we should also enforce that no merge commits exist on automerge:rebase, to prevent situations like #8559

@mhofman mhofman changed the title linear-history doesn't prevent failed interactive rebases linear-history should enforce rules on rebase too Jan 12, 2024
@frazarshad frazarshad self-assigned this Jul 29, 2024
mergify bot added a commit that referenced this issue Aug 9, 2024
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
@frazarshad
Copy link
Contributor

completed by #9794

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working tooling repo-wide infrastructure
Projects
None yet
Development

No branches or pull requests

2 participants