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

block autosquash/fixup commits #7768

Closed
turadg opened this issue May 17, 2023 · 5 comments · Fixed by #8253
Closed

block autosquash/fixup commits #7768

turadg opened this issue May 17, 2023 · 5 comments · Fixed by #8253
Assignees
Labels
devex developer experience enhancement New feature or request tooling repo-wide infrastructure

Comments

@turadg
Copy link
Member

turadg commented May 17, 2023

What is the Problem Being Solved?

We've taken to using fixup! commits in PRs so the reviewer can see the revision to the commits but they get squashed before master. Sometimes the author forgets to squash before merging to master, so fixups end up in master branch and make it harder to see meaningful changes (in history and in git blame).

Description of the Design

Set up CI for master that prevents such commits. E.g. https://github.com/xt0rted/block-autosquash-commits-action

Security Considerations

Scaling Considerations

Test Plan

@turadg turadg added enhancement New feature or request tooling repo-wide infrastructure devex developer experience labels May 17, 2023
@mhofman
Copy link
Member

mhofman commented May 25, 2023

Only a problem if we don't use mergify's rebase process.

I'd prefer to find a solution that handles these commits and automatically "does the right thing" with them.

@turadg
Copy link
Member Author

turadg commented May 25, 2023

Only a problem if we don't use mergify's rebase process.

Note that some of the fixup commits in master were while Mergify's rebase process was available. If you're proposing that we only ever allow merges to master using Mergify's rebase then, yes, we wouldn't need this. Barring that, fulfilling this issue is useful.

@mhofman
Copy link
Member

mhofman commented May 26, 2023

To be more specific, squash "merging" also does not expose that issue. It's only a problem for the pure merge commit merging method, which is currently implicit for GH based merge queues, and explicit for the automerge:merge mergify based label process. It's only for these cases we should enforce a no fixup commit rule.

If we keep a GH merge queue process, I'd like to replicate the squash / rebase+fixup+merge methods. Hence the comment about not blocking but automatically handle these commits.

@mhofman mhofman linked a pull request Aug 27, 2023 that will close this issue
@mhofman
Copy link
Member

mhofman commented Aug 27, 2023

Well this won't effectively be closed until we make the linear-history check required.

@mhofman
Copy link
Member

mhofman commented Aug 29, 2023

linear-history is now a required check that should prevent these commits from mistakenly making it into master

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
devex developer experience enhancement New feature or request tooling repo-wide infrastructure
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants