cmd/gerritbot/internal/rules: make user recommendations more explicit
In CL 513397, I added a small rules engine to GerritBot along with a set
of rules to flag common mistakes made by external contributors,
targeting PRs imported from GitHub. Part of the intent was to give
immediate feedback and get people to more quickly perform their first
concrete use of Gerrit, such as logging in for the first time or
tweaking the commit message. This might then help them be more ready
when a human reviewer shows up later.
Around 75% of GitHub PRs get flagged in practice, and the rate is even
higher for first-time contributors. It also seems to help experienced
contributors who are still using PRs when they might accidentally use
markdown or otherwise not quite follow the Go project conventions.
(Experienced contributors seem more likely to fully fix everything
flagged; newer contributors might only address a subset of problems
flagged, but that still accomplishes the goal of getting them to
successfully interact with Gerrit before a human reviewer engages. Some
people seem not to take any action until they are sometimes asked
multiple times by human reviewer, or the end result might be the human
reviewer decides something more minor is not worth a round of feedback.)
This CL adjusts some of the wording and user advice based on having now
observed how people react.
One of my concerns prior to initially going live was that some
percentage of people might be annoyed by a nitpicking bot, especially if
it was wrong.
In practice, the false positive rate has been extremely low, and I don't
think I've seen anyone express material annoyance. This CL therefore
reduces some of the "pre-apology" text and moves it later. This also
helps people see the listed problems more easily.
This CL also tries to make some of the instructions more explicit
especially in areas where the behavior of the end-to-end PR importing
system might be somewhat counter-intuitive to some people. This includes
bigger anchor text in the commit editing advice to better catch the eye
of people who only skim but might spot a useful link while scanning.
This CL does not change any of the detection logic in the rules -- it
just changes the wording (plus a small amount of plumbing to enable
that).
'go test -cover' currently reports 99.5% of statements covered in the
internal/rules package.
I have some other related WIP changes, but I am attempting to keep
this CL somewhat narrowly scoped.
Updates golang/go#61573
Change-Id: I4d3ac3c31779855e99d6e33125b8a81d505fd6f6
Reviewed-on: https://go-review.googlesource.com/c/build/+/691517
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
Reviewed-by: Mark Freeman <mark@golang.org>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
diff --git a/cmd/gerritbot/gerritbot.go b/cmd/gerritbot/gerritbot.go
index 7781e27..14be80d 100644
--- a/cmd/gerritbot/gerritbot.go
+++ b/cmd/gerritbot/gerritbot.go
@@ -812,18 +812,23 @@
}
problems := rules.Check(change)
if len(problems) > 0 {
- summary := rules.FormatResults(problems)
- // If needed, summary contains advice for how to edit the commit message.
- msg := fmt.Sprintf("I spotted some possible problems.\n\n"+
- "These findings are based on simple heuristics. If a finding appears wrong, briefly reply here saying so. "+
- "Otherwise, please address any problems and update the GitHub PR. "+
- "When complete, mark this comment as 'Done' and click the [blue 'Reply' button](https://go.dev/wiki/GerritBot#i-left-a-reply-to-a-comment-in-gerrit-but-no-one-but-me-can-see-it) above.\n\n"+
- "%s\n\n"+
+ findings, notes := rules.FormatResults(problems)
+ // When applicable, notes contains advice for how to edit the commit message.
+ if notes != "" {
+ notes += "\n"
+ }
+ // TODO(thepudds): it might be better to push this text down to the internal/rules package.
+ msg := fmt.Sprintf("I spotted some possible problems with your PR:\n\n"+
+ "%s\n"+ // markdown-formatted list of findings
+ "Please address any problems by updating the GitHub PR.\n\n"+
+ "When complete, mark this comment as 'Done' and click the [blue 'Reply' button](https://go.dev/wiki/GerritBot#i-left-a-reply-to-a-comment-in-gerrit-but-no-one-but-me-can-see-it) above. "+
+ "These findings are based on heuristics; if a finding does not apply, briefly reply here saying so.\n\n"+
+ "%s"+ // notes or empty string
"(In general for Gerrit code reviews, the change author is expected to [log in to Gerrit](https://go-review.googlesource.com/login/) "+
"with a Gmail or other Google account and then close out each piece of feedback by "+
"marking it as 'Done' if implemented as suggested or otherwise reply to each review comment. "+
"See the [Review](https://go.dev/doc/contribute#review) section of the Contributing Guide for details.)",
- summary)
+ findings, notes)
gcl, err := b.gerritChangeForPR(pr)
if err != nil {
diff --git a/cmd/gerritbot/internal/rules/rules.go b/cmd/gerritbot/internal/rules/rules.go
index 72234b9..2d8fc8e 100644
--- a/cmd/gerritbot/internal/rules/rules.go
+++ b/cmd/gerritbot/internal/rules/rules.go
@@ -201,9 +201,10 @@
f: func(change Change) (finding string, note string) {
finding = "Are you describing the change in complete sentences with correct punctuation in the commit message body, including ending sentences with periods?"
if !mightBeTrivial(change) && !match("[a-zA-Z0-9\"'`)][.:?!)]( |\\n|$)", change.Body) {
- // A complete English sentence usually ends with an alphanumeric immediately followed by a terminating punctuation.
- // (This is an approximate test, but currently has a very low false positive rate. Brief manual checking suggests
- // ~zero false positives in a corpus of 1000 CLs).
+ // A complete English sentence usually ends with an alphanumeric or quote immediately followed by a terminating punctuation.
+ // This is an approximate test, but currently has a very low false positive rate. Brief manual checking suggests
+ // ~zero false positives in a corpus of 1000 CLs.
+ // (Sorry, currently our ugliest regexp).
return finding, commitMessageAdvice
}
return "", ""
@@ -214,7 +215,10 @@
{
name: "body: long lines",
f: func(change Change) (finding string, note string) {
- finding = "Lines in the commit message should be wrapped at ~76 characters unless needed for things like URLs or tables. You have a %d character line."
+ finding = "You have a long %d character line in the commit message body. " +
+ "Please add line breaks to long lines that should be wrapped. " +
+ "Lines in the commit message body should be wrapped at ~76 characters unless needed for things like URLs or tables. " +
+ "(Note: GitHub might render long lines as soft-wrapped, so double-check in the Gerrit commit message shown above.)"
// Check if something smells like a table, ASCII art, or benchstat output.
if match("----|____|[\u2500-\u257F]", change.Body) || match(` ± [0-9.]+%`, change.Body) {
// This might be something that is allowed to be wide.
@@ -246,7 +250,10 @@
{
name: "body: might use markdown",
f: func(change Change) (finding string, note string) {
- finding = "Are you using markdown? Markdown should not be used to augment text in the commit message."
+ // In practice, this rule has had close to zero false positives, though it cannot be perfect.
+ finding = "It looks like you are using markdown in the commit message. If so, please remove it. " +
+ "Be sure to double-check the plain text shown in the Gerrit commit message above for any " +
+ "markdown backticks, markdown links, or other markdown formatting."
if match(`(?i)markdown`, change.Title) || match(`(?i)markdown`, change.Body) {
// Could be a markdown-related fix.
return "", ""
@@ -272,7 +279,7 @@
}
}
if match(`\[.+]\(https?://.+\)`, change.Body) {
- // Looks like a markdown link. (Sorry, currently our ugliest regexp).
+ // Looks like a markdown link.
return finding, commitMessageAdvice
}
return "", ""
@@ -385,9 +392,13 @@
}
// Auxiliary advice.
-var commitMessageAdvice = "The commit title and commit message body come from the GitHub PR title and description, and must be edited in the GitHub web interface (not via git). " +
- "For instructions, see [here](https://go.dev/wiki/GerritBot/#how-does-gerritbot-determine-the-final-commit-message). " +
- "For guidelines on commit messages for the Go project, see [here](https://go.dev/doc/contribute#commit_messages)."
+var commitMessageAdvice = "To update the commit title or commit message body shown here in Gerrit, " +
+ "you must edit the GitHub PR title and PR description (the first comment) in the " +
+ "GitHub web interface using the 'Edit' button or 'Edit' menu entry there. " +
+ "Note: pushing a new commit to the PR will not automatically update the commit message used by Gerrit.\n\n" +
+ "For more details, see:\n" +
+ "* [how to update commit messages](https://go.dev/wiki/GerritBot/#how-does-gerritbot-determine-the-final-commit-message) for PRs imported into Gerrit.\n" +
+ "* the Go project's [conventions for commit messages](https://go.dev/doc/contribute#commit_messages) that you should follow.\n"
// mightBeTrivial reports if a change looks like something
// where a commit body is often allowed to be skipped, such
diff --git a/cmd/gerritbot/internal/rules/rules_test.go b/cmd/gerritbot/internal/rules/rules_test.go
index 25af204..7bb3c9f 100644
--- a/cmd/gerritbot/internal/rules/rules_test.go
+++ b/cmd/gerritbot/internal/rules/rules_test.go
@@ -13,53 +13,57 @@
func TestFormatRuleResults(t *testing.T) {
tests := []struct {
- title string
- repo string // if empty string, treated as "go" repo
- body string
- want string
+ title string
+ repo string // if empty string, treated as "go" repo
+ body string
+ wantFindings string
+ wantNotes string
}{
{
- title: `fmt: improve some things`, // We consider this a good commit message title.
- body: goodCommitBody,
- want: ``,
+ title: `fmt: improve some things`, // We consider this a good commit message title.
+ body: goodCommitBody,
+ wantFindings: ``,
+ wantNotes: ``,
},
{
title: `a bad commit message title.`,
body: goodCommitBody,
- want: `Possible problems detected:
- 1. The commit title should start with the primary affected package name followed by a colon, like "net/http: improve [...]".
+ wantFindings: ` 1. The commit title should start with the primary affected package name followed by a colon, like "net/http: improve [...]".
2. The first word in the commit title after the package should be a lowercase English word (usually a verb).
3. The commit title should not end with a period.
-
-The commit title and commit message body come from the GitHub PR title and description, and must be edited in the GitHub web interface (not via git). For instructions, see [here](https://go.dev/wiki/GerritBot/#how-does-gerritbot-determine-the-final-commit-message). For guidelines on commit messages for the Go project, see [here](https://go.dev/doc/contribute#commit_messages).
`,
+ wantNotes: commitMessageAdvice,
},
{
title: `A bad vscode-go commit title`, // This verifies we complain about a "component" rather than "package".
repo: "vscode-go",
body: "This includes a bad bug format for vscode-go repo.\nFixes #1234",
- want: `Possible problems detected:
- 1. The commit title should start with the primary affected component name followed by a colon, like "src/goInstallTools: improve [...]".
+ wantFindings: ` 1. The commit title should start with the primary affected component name followed by a colon, like "src/goInstallTools: improve [...]".
2. The first word in the commit title after the component should be a lowercase English word (usually a verb).
3. Do you have the right bug reference format? For the vscode-go repo, the format is usually 'Fixes golang/vscode-go#1234' or 'Updates golang/vscode-go#1234' at the end of the commit message.
-
-The commit title and commit message body come from the GitHub PR title and description, and must be edited in the GitHub web interface (not via git). For instructions, see [here](https://go.dev/wiki/GerritBot/#how-does-gerritbot-determine-the-final-commit-message). For guidelines on commit messages for the Go project, see [here](https://go.dev/doc/contribute#commit_messages).
`,
+ wantNotes: commitMessageAdvice,
},
{
- title: `A bad wiki commit title we allow`, // We ignore the wiki repo.
- repo: "wiki",
- body: "A bad body we allow",
- want: "",
+ title: `A bad wiki commit title we allow`, // We ignore the wiki repo.
+ repo: "wiki",
+ body: "A bad body we allow",
+ wantFindings: "",
+ wantNotes: "",
+ },
+ {
+ title: goodCommitTitle,
+ body: "This commit body is missing a bug reference.",
+ wantFindings: " 1. You usually need to reference a bug number for all but trivial or cosmetic fixes. For this repo, the format is usually 'Fixes #12345' or 'Updates #12345' at the end of the commit message. Should you have a bug reference?\n",
+ wantNotes: commitMessageAdvice,
},
{
title: goodCommitTitle,
- body: "This commit body is missing a bug reference.",
- want: `Possible problems detected:
- 1. You usually need to reference a bug number for all but trivial or cosmetic fixes. For this repo, the format is usually 'Fixes #12345' or 'Updates #12345' at the end of the commit message. Should you have a bug reference?
-
-The commit title and commit message body come from the GitHub PR title and description, and must be edited in the GitHub web interface (not via git). For instructions, see [here](https://go.dev/wiki/GerritBot/#how-does-gerritbot-determine-the-final-commit-message). For guidelines on commit messages for the Go project, see [here](https://go.dev/doc/contribute#commit_messages).
+ body: "Some `backticks`\n" + strings.Repeat("long line", 20) + "\n" + goodCommitBody,
+ wantFindings: ` 1. You have a long 180 character line in the commit message body. Please add line breaks to long lines that should be wrapped. Lines in the commit message body should be wrapped at ~76 characters unless needed for things like URLs or tables. (Note: GitHub might render long lines as soft-wrapped, so double-check in the Gerrit commit message shown above.)
+ 2. It looks like you are using markdown in the commit message. If so, please remove it. Be sure to double-check the plain text shown in the Gerrit commit message above for any markdown backticks, markdown links, or other markdown formatting.
`,
+ wantNotes: commitMessageAdvice,
},
}
for _, tt := range tests {
@@ -74,10 +78,14 @@
t.Fatalf("ParseCommitMessage failed: %v", err)
}
results := Check(change)
- got := FormatResults(results)
- t.Log("FormatResults:\n" + got)
- if diff := cmp.Diff(tt.want, got); diff != "" {
- t.Errorf("checkRules() mismatch (-want +got):\n%s", diff)
+ gotFindings, gotNotes := FormatResults(results)
+ t.Log("FormatResults:\n" + gotFindings)
+
+ if diff := cmp.Diff(tt.wantFindings, gotFindings); diff != "" {
+ t.Errorf("checkRules() findings mismatch (-want +got):\n%s", diff)
+ }
+ if diff := cmp.Diff(tt.wantNotes, gotNotes); diff != "" {
+ t.Errorf("checkRules() notes mismatch (-want +got):\n%s", diff)
}
})
}
diff --git a/cmd/gerritbot/internal/rules/run.go b/cmd/gerritbot/internal/rules/run.go
index a5e4603..ee92bd0 100644
--- a/cmd/gerritbot/internal/rules/run.go
+++ b/cmd/gerritbot/internal/rules/run.go
@@ -103,24 +103,20 @@
return results
}
-// FormatResults returns a string ready to be placed in a CL comment,
+// FormatResults returns the findings and notes ready to be placed in a CL comment,
// formatted as simple markdown.
-func FormatResults(results []Result) string {
+func FormatResults(results []Result) (findings string, notes string) {
if len(results) == 0 {
- return ""
+ return "", ""
}
var b strings.Builder
- b.WriteString("Possible problems detected:\n")
cnt := 1
for _, r := range results {
fmt.Fprintf(&b, " %d. %s\n", cnt, r.Finding)
cnt++
}
advice := formatAdvice(results)
- if advice != "" {
- b.WriteString("\n" + advice + "\n")
- }
- return b.String()
+ return b.String(), advice
}
// formatAdvice returns a deduplicated string containing all the advice in results.
