How do you do post-commit review?
Maintainers: How do you ensure code in your area that is submitted directly is reviewed?
Do you distribute among contributors in any way, or take the task within a certain scope (source directory? some other scope definition?) alone? If you distribute it, how do you coordinate coverage amongst multiple reviewers?
My own story:
When I first joined the project, I opened every commit list email for LLVM and clang - and at least looked at the list of changed files (this is part of how I got a bit of a reputation as being pedantic about test coverage - it was easy to see from just the list of changed files if a patch didn’t include test coverage, and then go to see if that was suitable for the change) - but as the project has grown that started to be infeasible, though by that stage we’d changed the subject lines of commit emails to include the first line of the commit (way back in the day, the subject line just included the file paths that were changed, up to the length limit of the subject I guess… ) - so I started skimming subject lines. Not as effective because they aren’t forcibly structured in any way, someone could accidentally or intentionally include, say, an ADT change amongst the rest of their change and I might miss it, but it was best effort.
I never coordinated this with anyone, so it felt like I had to do the whole “set” (whatever I defined that set to be) myself.
Around middle of last year, as I moved to work more in Carbon, I stepped back from this level of involvement and I now only check the PRs autolabeled in the areas of my experience/ownership - mostly debug info and ADT.
I don’t know of any great way to either track that post-commit review is happening, or coordinate it so those engaging in it don’t each take it on as a solo task.
For LLVM-libc we prioritize pre-commit review, so we don’t really have much post-commit review to do. I also filter the llvm commits mailing list very aggressively to keep my inbox less of a mess, I only see ones that mention libc.
clang-frontend we generally do reviews independently and pull folks into specific reviews when individuals believe something requires extra attention or we don’t feel like we have the right knowledge.
I don’t think most of the others do post commit review but I definitely do it and I think more folks should be doing post-commit review. I very often find issues during post-commit review. If other areas are not doing it they are likely missing real issues.
I personally have been trying to focus on testing both pre and post commit review. Mainly b/c I have spent time lately looking at regressions and I came away with the impression most of the problems could have been caught with a little more attention to testing. My experience so far have been pushing for more testing is catching real bugs about 50% of the time so far. This requires more commitment of time to each PR but if it reduces regressions long-term that is well worth it.
The only post-commit reviewing I do is when a PR is merged in one of my focus areas, typically because another reviewer has looked at it before I got to it. I don’t have the time to go through the commit emails, personally.
This has the downside that I never see commits that were made without a PR. Personally, I’d love us get to a point where we could ban direct commits and only allow commits via PR. They don’t necessarily require reviewing, but it means that those who get auto-subscribed to PRs in a specific area will get to see the previously direct commits. It’s also the approach I’m following in my current (non-LLVM) work project, because my sole team mate is off right now, so I have no available reviewers.
I look at commits from PR emails, but basically don’t do post-commit review like we used to, only in the sense that I get too late on a PR review and I review the PR after it was merged, but not in a mailing-list / herald kind of way.
When a commit message catches my eye as something I hadn’t realized was changing, I open up the commit to do a spot-check. If that brings up anything suspicious, I open up the PR and do a full review.
It’s a bit of a challenge for me because I’m the “anything not covered by others” maintainer for Clang, which is a lot of area. So I do not ensure every review is getting a quality review, that’s not practical for me. However, I do try to add reviewers to any PR where I don’t see active review happening and the author is pinging for help. I mostly go off the maintainers list, but I also use git blame or historical knowledge to pick reviewers.
I try to distribute reviews amongst maintainers instead of overloading anyone. I also try to have at least two reviewers for everything so there’s extra coverage.
I do not assign reviewers post-commit unless I spot something significant and need to pull in additional help.
While I’m happy/glad for all the discussion so far, I should mention I’m /especially/ interested in post-commit review of patches submitted without pre-commit review, and moreso those without a PR at all.
Mostly I’m tilting towards @jh7370’s suggestion of requiring PRs for all commits, labeled if they are not intended for precommit review, then labeled to track the completion of post-commit review - so we could do post-commit review collaboratively (query for all PRs that have the “skip-precommit-review” label that haven’t been approved (whether or not they’ve been committed))
For my part, I cannot tell whether the commit did or did not have a PR associated with it very easily. So I don’t spot “committed without review” commits for post-commit review.
There’s broadly three scenarios:
Not having a PR because the changes are NFC (updating the website, adding a test case, etc). I think those are fine to expect a PR for and the labels would help get additional post-commit review for. Even if the PR doesn’t need any precommit testing, these kinds of changes are infrequent enough and usually nothing is on fire requiring a fast turn-around time.
Not having a PR because the person is a maintainer and feels like they’ve got the expertise in the area to not need a review. I think these should absolutely have a PR even if it was historically an acceptable practice.
Getting bots back to green (reverts, quick fixes, etc). Until precommit CI is significantly faster with turning around an answer, I do not think these should be required to have a PR in general, just to avoid community disruption from ToT being broken.
I would like to add that having a PR has the benefit that the buildbots have a place where they leave a comment in case something breaks. I personally feel that this is helpful in general. While, sure, a commit can be commented on, I feel like commenting on the PR leaves the message as easier to find.
It happens often enough that changes are marked NFC and still break the build for some configurations/environments.
I subscribe to llvm-commits, filter out all the PR related mails(*) and then look at commits that look both relevant to me and that I’m not already aware of. This serves dual purpose of keeping up with what happens in the wider project and doing post-commit review if necessary.
However, I’ll also add that it’s actually quite rare that I feel the need to comment on directly committed changes, and I think it’s becoming more rare over time. People are generally pretty reasonable about what they land without review.
There was one contributor who regularly abused the post-commit review system in the past, but since they stopped contributing a few years back, I’ve not encountered any issues worth mentioning with the current system.
(*) I really wish we did not send PR mails to llvm-commits, because the sheer quantity causes mail delivery bottlenecks.
Could you speak more to this ^ do you mean you review based on the actual commits to clang (using git log or similar/equivalent?) rather than PRs, so it doesn’t matter whether it’s a PR or not (presumably this is fairly cursory then - can’t review them all in detail, some are already reviewed by the time you look at them, etc - so changes for postcommit review might not be getting as much scrutiny as those with precommit review?) - or do you mean you only review PRs (both pre and post)?
To clarify, do you mean PR-without-precommit-review in (1) and (2) or PR-with-precommit review? (some of the wording confuses me a bit - creating a PR and submitting directly shouldn’t harm turnaround too much in (1) and the “absolutely have a PR even if it was historically an acceptable practice” makes it sound like maybe that’s PR-with-precommit-review - what I’m suggesting, at least at this moment, would be PR-without-precommit-review - more like “changing the process for how to make changes without precommit review”)
We don’t currently have blocking presubmits, do we? So that shouldn’t hold anyone up - we could have the presubmits even disabled when someone has the “skip-precommit-review” label applied (or make the presubmits less blocking, if they are blocking at all?)
Thanks! Good to know how folks are doing this, especially code owners/area leads.
That’s reassuring - though I’m still hoping we could have some more robust way to ensure things aren’t falling through the gaps between everyone’s “relevance” filter. (& perhaps allowing for some distribution of the work (doesn’t address folks own need to stay in touch with what’s happening in the project - they may still need to filter through some list of commits, etc))
Yeah, certainly open to other discussions about how better to manage the commits list (I still think maybe decomissioning it would be appropriate - git logs, discord channel, etc - but also pretty harmless to maintain it too, perhaps (it should be read-only these days, like the bugs list, at least - no one’s going to read custom email sent to the list these days))
The commits lists do have some maintenance, because github is not sending emails directly to the commit lists (the one exception to this being the all-commits list).
We have a python script running in Amazon’s Lambda service that listens on a webhook for commit events and then creates an email and sends it out to the correct list (e.g. flang commits to flang-commits list, clang commits to clang-commits list, etc.) Even though the ongoing maintenance is minimal and limited to updating the tokens once a month. We do have to make updates to the script when new projects are added or when someone uncovers a bug.
Just to note that Github’s UI allows discovering this, but it requires inspecting individual commits, so it’s not easily automated. I’m assuming that the information could be retrieved through the GH API as well, but checking every commit would require quite a few API calls/credits.
Compare the following[1] – if a commit comes from a (squash-)merge through the GH UI, you’ll see it marked as such.
It’s just about whether it has a number in braces at the end of the title, no? And shows in any list of commits? (this example shows 2 commits without corresponding PR and 5 with a PR)…
This gives an approximation that’s accurate as long as people do not change the default merge commit title suggested by GitHub, which is ${PR title} (${PR number}). However, it’s trivially possible to edit the title while merging, so that method is not 100% foolproof (but probably good enough for 95+%)
My team has adopted a policy among ourselves that all changes except reverts require PRs and pre-commit review, and we revert to green on breakage instead of fixing forward.
This has worked very well for us, and makes it easy to provide both pre and post-commit review via the PR.
Personally I’ve generally found that the only times I really do post-commit review anymore outside things brought to my attention by my team is when I stumble across something in the code that peaks my interest or seems out of place.
Thanks @dblaikie for bringing this up. The project has never had a way to track post-commit review so all evidence is anecdotal. I did used to skim the subject lines of everything on llvm-commits and cfe-commits, looking more deeply at things of interest, but mainly to make sure I had a clue what was going on in the project. This had dual goals of being able to mention things to colleagues of work that I’d seen fly by on the list, and keeping a handle on upstream changes that could affect Sony’s downstream work (of which there is less now than there used to be). I think a very small number of people do that sort of thing; it used to consume 1+ hours of my workday, every day.
Requiring a PR for all changes, even reverting breakages, seems like a quite reasonable policy.
