Add a plugin to block merging PRs with `fixup!` commits

[nojnhuh]

I usually update my PRs with git commit --fixup to better enable more iterative review than rewriting history with git commit --amend. Then I git rebase --autosquash when the PR is LGTM'd and approved to remove all the intermediate changes.

I usually remember to /hold those PRs to remind myself to squash but sometimes end up merging PRs anyway with lots of unnecessary fixup! noise in the git log. It would be great if there was a plugin like mergecommitblocker that could look for commits whose messages start with fixup! or amend! and automatically add a do-not-merge/* label, and remove it when no such commits exist like after a git rebase --autosquash.

/kind feature
/area plugins

[petr-muller]

Sounds like this could go into the existing invalidcommitmsg plugin?

[petr-muller]

There may be some configuration / logic necessary to accommodate repos that merge with squash method, and possibly also the rebase method (not sure if GH rebase method applies the fixup! squash.

[valen-mascarenhas14]

There may be some configuration / logic necessary to accommodate repos that merge with squash method, and possibly also the rebase method (not sure if GH rebase method applies the fixup! squash.

@petr-muller I feel it may be better not to rely on the repository’s merge strategy for enforcement. GitHub’s Rebase & merge does not apply --autosquash, so fixup! commits would be preserved in the target branch unless cleaned up beforehand. Even with Squash and merge, while the individual commits wouldn’t appear separately, their messages could still end up in the final commit if it's not edited.

Since merge strategy is a repo-level setting & can change over time, it might be more robust to validate the PR commits directly & require fixup! commits to be cleaned up before merge, regardless of how the repository merges PRs.

[petr-muller]

What's acceptable to merge is mostly repository maintainers' decision, not Prow's (=ours). Blocking PRs from merge is a big hammer and Prow is used in different contexts. I think the check is useful enough for many repositories that I'd like the behavior to be on by default and ideally packed into the existing plugin - that way the value propagates to existing Prow instances without any config change needed.

But we need a way out for repos that don't want the check. I agree that trying to be smart about merge methods does not seem to be appropriate (it's even trickier because while default merge method is indeed a repo-level knob, individual PRs can be GH-merged using a different method; this behavior is even exposed by Tide...) and a simple opt-out config would be enough (we'd need that for org and repo level at least, but someone will always ask for branch level too 😓).

[valen-mascarenhas14]

What's acceptable to merge is mostly repository maintainers' decision, not Prow's (=ours). Blocking PRs from merge is a big hammer and Prow is used in different contexts. I think the check is useful enough for many repositories that I'd like the behavior to be on by default and ideally packed into the existing plugin - that way the value propagates to existing Prow instances without any config change needed.

But we need a way out for repos that don't want the check. I agree that trying to be smart about merge methods does not seem to be appropriate (it's even trickier because while default merge method is indeed a repo-level knob, individual PRs can be GH-merged using a different method; this behavior is even exposed by Tide...) and a simple opt-out config would be enough (we'd need that for org and repo level at least, but someone will always ask for branch level too 😓).

@petr-muller If I understand the proposal correctly, the check would be enabled by default, so existing Prow deployments benefit from it without requiring any config changes. At the same time, we will provide an opt-out configuration for repos that don't want this behavior.
The config will apply at an org & repo level, without trying to infer merge strategies or introduce branch-level granularity for now.

If this approach sounds reasonable, I’d be happy to pick this issue up and work on it.

[valen-mascarenhas14]

/assign valen-mascarenhas14