61182 starts with an enumeration of friction for the GitHub PR workflow.
We could reduce some friction by automatically flagging common PR problems to save maintainer time and give faster feedback to a potential contributor.
If a maintainer sees that someone who sent a GitHub PR has responded properly in Gerrit to automated feedback, that is a signal that more detailed feedback from a human reviewer is more likely to be acted upon, which might help a maintainer triage which CLs get their attention when. (One of the worst issues listed in #61182 is that for some percent of GitHub PRs, the author never responds to feedback in Gerrit).
I am about to send a WIP CL for discussion. It started as a way to flag improper word wrapping (#24832), but once we can flag one problem, it was a small-ish step to add a loop, define a simple rule func signature, and then define a set of small rules.
Current rule names
title: no package found
title: no lowercase word after a first colon
title: no colon then single space after package
title: ends with period
body: short
body: long lines
body: might use markdown
body: no sentence candidates found
body: no bug reference candidate found
body: bug format looks incorrect
body: bug reference candidate not at end
body: still contains PR instructions
body: contains Signed-off-by
Most of those rule definitions are small -- the median rule body is 5 lines of code (an if statement + 2 lines).
We could add more rules in the future, such as flagging raw benchmark output (suggesting benchstat), asking about the seeming absence of a test, flagging an incorrectly formatted CL reference, possibly checking gofmt, and so on.
Sample results
Running those rules against a corpus of 1000 recent CLs that started as GitHub PRs gives the following stats:
Findings per CL: 1.45 (avg.)
CLs with findings: 76.5%
CL hit % Finding name
-------- ------------------------------------------------
7.6% title: no package found
13.6% title: no lowercase word after a first colon
0.0% title: no colon then single space after package
2.0% title: ends with period
21.8% body: short
21.7% body: long lines
10.5% body: might use markdown
6.8% body: no sentence candidates found
46.0% body: no bug reference candidate found
10.3% body: bug format looks incorrect
1.6% body: bug reference candidate not at end
2.2% body: still contains PR instructions
0.9% body: contains Signed-off-by
That corpus is the 1000 most recent PRs that were ultimately merged. The results here are for the commit message in the first patch set (prior to any human feedback).
Philosophy
The rules mostly err on the side of reducing false positives, though some are unavoidable. We attempt to mitigate false positives by stating in the message we post to the CL that the findings are "potential problems" based on "simple heuristics", and we ask the contributor to reply on the CL if a finding appears wrong. Part of the intent is to engage a first time contributor and get them using Gerrit, but without annoying them too much.
We also attempt to point towards auxiliary info and advice specific to the rule violation. If a rule triggers for a potential problem related to a commit message, we also include links to the contributing guide Good commit messages section and instructions on how to edit the commit message. (A current problem is new contributors don't always know how to edit the commit message after a human asks them to change it, which leads to extra review iterations).
There is also some simple logic to be more lenient for PRs that appear to be trivial changes (e.g., don't complain about a missing bug reference if the title contains things like "typo" or "grammar"). Where we have less certainty (e.g., identifying markdown use), we attempt to express that uncertainty in the resulting message shown to the user. The rule definition also allows us to skip certain rules for some repos (e.g., don't complain in the proposal repo about a missing package).
CC @heschi, @cagedmantis
Comment From: thepudds
There is some potential overlap with another CL I sent (CL 509135 for #61316), but to start, I suggest we discuss the two issues separately. (Later, we can resolve whether/how to combine the message from CL 509135 with this message, vs. if it is better to have separate messages, but might be good to first establish if there is any interest in proceeding with this issue).
Also, there is some overlap with what could eventually be done with Tricium, but: * This seemed simple enough to try as an interim step. * This is in Go rather than Python. * Heschi recently mentioned here that it might be some time before even simple rules are added to Tricium, which seemingly does not have much out of the box. * There might be some benefit to building experience with automatically nit picking GitHub PRs (a workflow correlated with newer contributors in greater need of help) before we attempt to nit pick more seasoned contributors for all Gerrit CLs.
Comment From: gopherbot
Change https://go.dev/cl/513397 mentions this issue: cmd/gerritbot: automatically flag common issues in PRs
Comment From: thepudds
There is now an initial implementation in https://go.dev/cl/513397 (with a few small tweaks such that the CL hit % numbers are slightly different in the CL vs. the opening comment here).
Comment From: gopherbot
Change https://go.dev/cl/518679 mentions this issue: cmd/gerritbot: add corpus for CL rules
Comment From: gopherbot
Change https://go.dev/cl/519796 mentions this issue: cmd/gerritbot: generate change triple for posting comment
Comment From: gopherbot
Change https://go.dev/cl/520116 mentions this issue: cmd/gerritbot: greet with the GitHub username for the PR rule findings comment
Comment From: thepudds
It looks like https://go.dev/cl/520268 might have been the first real GitHub PR where the new rules were triggred.
The original commit message (on patch set 1) was:
path/filepath: add Localize
Fixes #57151.
The findings posted to the CL were:
Possible problems detected:
1. The commit message body is very brief. That can be OK if
the change is trivial like correcting spelling or fixing a broken link,
but usually the description should provide context for the change
and explain what it does in complete sentences.
2. Do you have the right bug reference format? For this repo, the
format is usually 'Fixes #12345' or 'Updates #12345' (without a period)
at the end of the commit message.
From the timestamps, it looks the the contributor fixed those two problems within ~3 minutes, and then within ~10 minutes GerritBot imported the update back into Gerrit.
The update commit message was:
path/filepath: add Localize
Implemented filepath.Localize using internal/safefilepath.FromFS.
Fixes #57151
...which addresses the two findings that were reported.
Comment From: dmitshur
Thanks for that analysis @thepudds.
I want to comment on the "without a period" contributing to being a possible problem. It seems better to me to consider the period or no period as fine. That is, both Fixes #57151. and Fixes #57151 should be interchangeable.
https://go.dev/wiki/CommitMessage includes:
Don't be too pedantic in code reviews: it's not worth asking people to change from Updates or something else to For, or vice versa.
I usually include periods after "For #nnn." because I consider them to be short sentences. A recent CL by Russ https://go.dev/cl/506415 is another example that included a period in the Fixes line.
Comment From: thepudds
Hi @dmitshur, that makes sense. (That rule already allows different aliases like Fixes vs. Closes vs. Resolves, but it was not top of mind for me that the trailing period is common for some people on the core Go team such as yourself, and the examples in the Contributing Guide and wiki don't seem to use the trailing period, which is why the rule as originally defined would complain about that).
I'll circle back to this shortly to stop it from complaining about a trailing period. (I might stack it behind the corpus CL, in part to serve as an example of how the summary stats and whatnot change for the corpus after a small change).
Comment From: thepudds
Also, before I did anything here, I skimmed a dozen or so random-ish CLs to see how reviews would stall, what seemed to be taking up human time, what types of review feedback could potentially be automated, and so on, and during that, I thought I saw ~1-2 times where someone on the core Go team asked someone to remove a trailing period from a Fixes or Updates line, but I might have been mistaken. (E.g., it might be that a reviewer just supplied an example without a period for a commit message that happened to use a period but also had some other bug format problems).
In any event, to help make sure everyone is on the same page, I just tweaked the CommitMessage wiki page to say that a period is acceptable. Given this is policy-ish, please let me know if that's wrong, or correct my edit if needed.
Comment From: gopherbot
Change https://go.dev/cl/525976 mentions this issue: cmd/gerritbot: remind again to log in to Gerrit
Comment From: gopherbot
Change https://go.dev/cl/525975 mentions this issue: cmd/gerritbot/internal/rules: stop complaining about trailing period after bug number
Comment From: gopherbot
Change https://go.dev/cl/530736 mentions this issue: cmd/gerritbot: avoid changing the content of comment sync messages
Comment From: gopherbot
Change https://go.dev/cl/591455 mentions this issue: cmd/gerritbot: flag common PR mistakes consider backport