`owners-label` should ignore merge commits

[danwinship]

Often, new users will mistakenly push a merge commit to a PR, causing prow to then apply a large number of sig/ and area/ labels based on what files were changed in the merge, and then mergecommitblocker tags it with its label and adds a comment explaining what the user did wrong. But since owners-label only ever adds labels, and never removes them, the erroneously-added labels stay there even after the user force-pushes over the bad merge PR, screwing up everyone's searches/filters/etc.

It would be cool if owners-label could be smart and not add labels based on commits that will need to be reverted.

It looks like owners-label always just adds all labels based on every file currently in the PR, rather than diffing against the last push. So it can just ignore any push containing a merge commit, and then it will re-sync correctly after the user force-pushes over the merge commit.

[BenTheElder]

It would be cool if owners-label could be smart and not add labels based on commits that will need to be reverted.

+1

It might need to be opt-in (since some prow users may not have the functionality to ban merge commits enabled?) but we would likely enable this in prow.k8s.io

/kind feature

It looks like owners-label always just adds all labels based on every file currently in the PR, rather than diffing against the last push. So it can just ignore any push containing a merge commit, and then it will re-sync correctly after the user force-pushes over the merge commit.

This is because you can add labels via other means than owners-label (e.g. /sig foo comment), so it shouldn't be removing them.

[danwinship]

Er, yeah, I was just clarifying that the problem I had previously identified in my head didn't actually exist. 🙂 (If it only did "add labels for every file that has changed since the last push", then it could end up missing some files if it ignored pushes that contained a mix of good and bad commits.)

[danwinship]

It might need to be opt-in (since some prow users may not have the functionality to ban merge commits enabled?) but we would likely enable this in prow.k8s.io

handlePullRequest gets a plugins.Agent, which contains a config.Config, so I assume owners-label ought to be able to tell if mergecommitblocker is also enabled (though I couldn't figure out how)

[danwinship]

It might need to be opt-in (since some prow users may not have the functionality to ban merge commits enabled?) but we would likely enable this in prow.k8s.io

Hm, but even if you allow merge commits, you wouldn't want to add labels based on the files in the merges... It seems more likely that owners-label is currently essentially incompatible with repos where people merge rather than rebasing.

In which case, the way to fix it would be to switch the behavior from:

• for each file modified by this PR
  • add labels for that file

to

• for each commit in this PR
  • if it's a merge from the branch that this PR will merge to, then continue
  • else, for each file modified by the commit
    • add labels for that file

[BenTheElder]

I was thinking the opposite, if you don't allow merge commits anyhow then no point applying the labels from them, just the label to block merging it?

If you do allow merge commits, then it might still make sense to ignore them for labeling purposes. I'm not sure because we don't use that workflow anywhere.

[danwinship]

I think you missed the continue in the pseudocode? It always skips applying labels for merge commits.

I'm not sure because we don't use that workflow anywhere.

ISTM that there is no workflow where it would make sense to apply labels based on what changes were picked up by a merge commit. That would mean that if you merge master into your PR, you would get labels reflecting every other PR that had merged into master since you originally pushed your PR, but if you never merged master back into your PR, or if you rebased instead of merging, or if you closed the original PR and opened a new one against HEAD, you wouldn't get all of those labels.

Basically, adding labels because a merge occurred could only make sense if you were suspicious of merges and thought they needed extra review. But if you think that, then just block merge commits! 🙂