Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

gerrit: improve revert commit message template, if possible #30530

Open
dmitshur opened this issue Mar 2, 2019 · 3 comments
Open

gerrit: improve revert commit message template, if possible #30530

dmitshur opened this issue Mar 2, 2019 · 3 comments
Labels
Community DevExp anything around developer experience FeatureRequest help wanted NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone
Unplanned

Comments

@dmitshur
Copy link
Contributor

dmitshur commented Mar 2, 2019

When using Gerrit UI to create a revert of a CL, it populates the commit message with the following template:

Revert "{{.OriginalTitle}}"

This reverts commit {{.CommitHash}}.

Reason for revert: <INSERT REASONING HERE>

For example:

image

In most contexts of the Go project, we refer to changes by the CL number or a shortlink of the form golang.org/cl/nnn. This has some advantages compared to using the commit hash:

  • shorter and less visual noise
  • CL page contains code review discussion, in addition to the change; commit requires one extra click to get to discussion

If possible, we should consider changing the template to be:

Revert "{{.OriginalTitle}}"

This reverts https://golang.org/cl/{{.OriginalCL}}.

Reason for revert: <INSERT REASONING HERE>

Or perhaps the This reverts CL {{.OriginalCL}}. form.

Revert CLs are generally created in situations that involve more time pressure and stress compared to normal CLs, so it's less reliable for us to depend on people to remember to do this manually, or to reference documentation like at https://golang.org/wiki/CommitMessage. It's better to adjust the template.

Implementation

I'm not very familiar with the configurability of our Gerrit instance and whether this is possible. /cc @andybons @bradfitz From looking at its source at https://gerrit.googlesource.com/gerrit/+/v2.16.6/polygerrit-ui/app/elements/change/gr-confirm-revert-dialog/gr-confirm-revert-dialog.js#50, it may be the case that this is not a configurable Gerrit feature, and so this may be blocked on a feature request for Gerrit.

This issue is to investigate whether this is possible, and whether we agree it's a good idea. /cc @ianlancetaylor
@dmitshur dmitshur added NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. FeatureRequest DevExp anything around developer experience Community labels Mar 2, 2019
@dmitshur dmitshur added this to the Unplanned milestone Mar 2, 2019
@dmitshur
Copy link
Contributor Author

dmitshur commented Mar 2, 2019 •
edited

As an idea to consider, if we can't easily control the template Gerrit UI uses, another approach could be to use our own infrastructure (maintner/gopherbot/gerritbot) to detect when a revert CL with that template is created, and have it automatically update the commit message to our preferred template.

However, this approach may involve significantly more work, and may not be reliable or have unintended side-effects.

Update in 2022: As a bonus, if we're using our infrastructure for this, we can also check that the revert was clean, and add a Code-Review+1 with a comment confirming that to make revert CLs easier for humans to review (and easier to spot a mistake if it happens).

In cases where a revert happens quickly, the git tree would be identical and can be used to short-circuit TryBots as well.

For example, patch set 3 of revert CL 378586 had the same tree as https://go.googlesource.com/go/+/1302f93c4e1eae0e625b372f9bb6bb48780af30f, and patch set 3 of revert CL 378587 had the same tree as https://go.googlesource.com/go/+/e4a6b84962cc2fb4f4b8bb532a84bab5bfd68d99. Computers can be better (and quicker) at spotting this than humans.

One clear risk of updating the commit message automatically is that requires it pushing a new patch set, that may race with the CL author doing the same. Just leaving code-review votes/comments won't run into that problem.

@mvdan
Copy link
Member

mvdan commented Sep 2, 2019

I was about to file this exact same issue. I've created three revert CLs in the past week, and I'm getting tired of having to do this bit of extra work manually :)

@dmitshur
Copy link
Contributor Author

I took a look at the latest Gerrit source to see if anything changed since 2019 to make this easier by now, and initially spotted a promising comment:

"// This is to give plugins a chance to update message" (source)

It turned out that an API event callback for "revert" was added in Gerrit CL 80750 back in 2016. I found a plugin that appears to make use of it for reference.

I'm not closely familiar with plugins, but from a quick look at https://gerrit-review.googlesource.com/Documentation/dev-plugins-lifecycle.html, the overhead in creating and maintaining a plugin may be too high to make this a good viable next step for resolving this Go issue specifically. (It might be a better fit for a general solution of making revert messages template-able or otherwise customizable for any Gerrit host to use.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Community DevExp anything around developer experience FeatureRequest help wanted NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Milestone
Unplanned
Development

No branches or pull requests
2 participants
@dmitshur @mvdan