Automation to retire committers does not recognize merge queue merges

[wolfgangwalther]

It seems like the automation to retire committers can't see merge queue merges.

Example: ryantm. PR was opened in #44, which was fine I guess. He commented to commit to reviewing more PRs in #44 (comment).

Looking at PRs manually, I can see (among others):

• stripe-cli: 1.30.0 -> 1.31.0, maintenance nixpkgs#448032 merged yesterday
• shen-sbcl: 39.1 -> 39.2 nixpkgs#442944 merged 4 days ago
• router: add updateScript and testVersion nixpkgs#336943 merged two weeks ago

None of these show up here: https://github.com/NixOS/nixpkgs/activity?actor=ryantm&time_period=year

(and not on the same API endpoint either)

[wolfgangwalther]

I believe we should replace the "activity" endpoint check with a plain check of "merge commits into master". The edge-cases of "hey, I recently merged a single commit into staging, that is not on master, yet" or "I only do backports, nothing else" are unlikely enough, so that we can just handle them individually - these people would clearly be active and could comment so on their PR.

[infinisil]

Hmm damn, yeah all merge queue actions show up as done by github-merge-queue[bot]: https://github.com/NixOS/nixpkgs/activity?time_period=day. I'll disable the retirement check for now, as I don't want people to be retired inadvertently.

The original https://github.com/nix-community/rfc55 used https://docs.github.com/en/rest/commits/commits?apiVersion=2022-11-28#list-commits, though it used authorship not committership, and didn't verify the user, see nix-community/rfc55#27. However I believe this could work with the right parameters and filtering to only count verified merges.

[wolfgangwalther]

If we were to switch to only look at merge commits, not activity, I believe the bot would also have retired garbas and grahamc - they both have zero merges and just some branch creation activity, which seems entirely accidental and surely not requiring a commit bit.

[emilazy]

The original https://github.com/nix-community/rfc55 used https://docs.github.com/en/rest/commits/commits?apiVersion=2022-11-28#list-commits, though it used authorship not committership, and didn't verify the user, see nix-community/rfc55#27. However I believe this could work with the right parameters and filtering to only count verified merges.

I think this shouldn’t be an issue – the incentive to forge seems very low here (faking activity by a committer to stop them being proposed for automatic removal?), and I’m not sure it’s even possible to forge the metadata for git log --first-parent master now that we require things to go through the merge queue. OTOH, I’m not sure how we’d easily detect merges specifically if we’re going exclusively through the API, since it doesn’t have an equivalent of --first-parent. I guess any commit with more than one parent is probably a good enough heuristic – or even just ones with a subject line in the right format…

[wolfgangwalther]

You wouldn't want to use --first-parent on master anyway, because then you don't count merges into staging and such. Counting "merge commits appearing on master" seems better.

[infinisil]

What I think should watched out for is that not everybody who ever gets a commit into Nixpkgs (thousands of people) can fake activity for any committer.

Also we have to only count activities that actually require write access.

[wolfgangwalther]

What I think should watched out for is that not everybody who ever gets a commit into Nixpkgs (thousands of people) can fake activity for any committer.

I don't think there is any incentive to do this - and if somebody tried, somebody else who have to commit this as well. This is a non-issue in practice, because the result is just an inactive committer keeping their commit bit. The whole thing already not exact anyway, but just a first approximate.

As long as "no commits found" reasonably sure means "there was no activity", we're good.

Also, keep in mind that when you're using the /commits endpoint you are going to filter by username, not the git email address / name. This data is returned by GitHub based on the actual merge. So it's actually impossible to fake this.

Also we have to only count activities that actually require write access.

Again, if we miscount non-write activity as write-activity, that's not a problem. We might miss some removals, but it won't be a major problem.

The status-quo is much worse, where we clearly have activity in the merge queue, but don't count it.

The approximation of "search for commits with multiple parents by that user present on master" will be more than enough.

[mdaniels5757]

The approximation of "search for commits with multiple parents by that user present on master" will be more than enough.

Long term, I agree. But rebase and squash-merges were enabled until fairly recently (until merge queues), right? So I suspect some sort of logic to capture those (probably ~identical to the existing code) needs to be in place until 1y after the queue was enabled.

[wolfgangwalther]

Good point. Going via both the commits and the activity endpoint should resolve that.

[emilazy]

The use of rebase and squash merges wasn’t that common. I think we could manually triage any PRs that get opened for people who were using them exclusively and then went inactive before the merge queue was enabled, to save the code complexity. As long as there’s a single merge done through the merge commit method it’d be fine.

Also, keep in mind that when you're using the /commits endpoint you are going to filter by username, not the git email address / name. This data is returned by GitHub based on the actual merge. So it's actually impossible to fake this.

You can easily fake a merge commit by anyone and put it in a PR; I’m pretty sure the endpoint just looks at the committer email. Of course it’d be silly to do so, but I’m guessing nobody would really notice if someone forged a master → staging-next merge, say. (Although few enough people do those in practice that it might look odd for most inactive committers…)

I agree that this is not a problem though – a simpler way to conspire to keep a committer active would be to just… get them to merge the PR you’d otherwise be sneaking the fake merge into. And of course if anyone actually tried forging commits and we noticed we’d have serious questions about it.

[MattSturgeon]

Measuring committer activity by "commits in development branches" SGTM. I agree the "exploit" vector of mis-attributed commits should be a non-issue in practice.

---

I’m pretty sure the endpoint just looks at the committer email.

That'd tally up with other GitHub endpoints that use commit-data.

E.g. the "first time contributor" status is based on the user having one of their account's emails listed as the committer-email of a merged commit.¹

It also aligns with:

Verified emails can be used as the author or committer addresses for web-based Git operations, e.g. edits and merges.
-- settings/emails

Thinking deductively: in most cases GitHub can't know who actually committed something, because commits are usually created locally and only pushed later.² They can know who pushed something, but that's reflected in the "activity" endpoint rather than the "commits" endpoint - so the conversation goes full circle. 😁

TL;DR, I'm fairly confident all commit<->user associations are done using the commit's committer-email and the user's verified email addresses list.

Footnotes

1. The CI Team noticed this when looking at weird edge-cases of the Nixpkgs "12.first-time contribution" PR label. ↩

2. Of course, sometimes GitHub themselves create the commit, as is the case for the Merge Queue's merge commits. But my point is that in the general case they don't, so it's unlikely they'd track user-based creation metadata. ↩

[emilazy]

Hopefully #73 fixes this.