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

x/build/devapp: support R= annotations in commit messages #23656

Open
bradfitz opened this issue Feb 1, 2018 · 8 comments
Open

x/build/devapp: support R= annotations in commit messages #23656

bradfitz opened this issue Feb 1, 2018 · 8 comments
Labels
Builders x/build issues (builders, bots, dashboards) NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Milestone

Comments

@bradfitz
Copy link
Contributor

bradfitz commented Feb 1, 2018

I see people using R= in commit messages, instead of in comments:

https://go-review.googlesource.com/c/go/+/91435
https://go-review.googlesource.com/c/go/+/63274

Do we want to support R= in commit messages too?

Easy technical change. But do we care about commit message prettiness? One CL was R=go1.11 and we approved it for Go 1.10. Fortunately @tklauser updated the commit message before submitting, but that won't always be the case if we make our tooling support R=go1.N and promote its use.

/cc @andybons @ianlancetaylor

@bradfitz bradfitz added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Feb 1, 2018
@gopherbot gopherbot added this to the Unreleased milestone Feb 1, 2018
@gopherbot gopherbot added the Builders x/build issues (builders, bots, dashboards) label Feb 1, 2018
@hyangah
Copy link
Contributor

hyangah commented Feb 1, 2018

Also, I want this (and others like DNR, etc) is documented in the contribution guideline.

@martisch
Copy link
Contributor

martisch commented Feb 1, 2018

Used R= in the commit message based on other CLs i had seen. e.g.
https://go-review.googlesource.com/c/go/+/86035

My intention is to remove them from my commit messages once the tree is open and before commit but I too see the problem that it is easy to forget.

@bradfitz
Copy link
Contributor Author

bradfitz commented Feb 1, 2018

I suppose we could also make the bot mark them as Do-Not-Submit (or just -2 them) until the R=go1.N tree is open too.

@josharian
Copy link
Contributor

josharian commented Feb 1, 2018

But do we care about commit message prettiness?

I do.

For those who are heavy git mail users, we could add a secret flag (like -trybot) to auto-add an R=go1.N comment.

A similar question exists for RELNOTE=yes. I would prefer to see them in gerrit but not commit messages, personally.

Does gerrit support pre-submit hooks? We could have gerrit rewrite away such lines in the commit message. Then out bot would just need to replicate the commit message annotations in gerrit comments.

@tklauser
Copy link
Member

tklauser commented Feb 2, 2018

Used R= in the commit message based on other CLs i had seen. [...]

My intention is to remove them from my commit messages once the tree is open and before commit but I too see the problem that it is easy to forget.

Same for me. Also, my idea was to save reviewer's time (even though https://golang.org/cl/91055 was then approved for go1.10) and for myself to keep track of my branches/CLs locally. Also being a git mail user, it would be nice to have a way to mark a CL targeting a specific R=go1.N locally as well as in gerrit (and have the two in sync).

@bradfitz
Copy link
Contributor Author

bradfitz commented Feb 2, 2018

I actually propose we stop using magic comments and start using CL-level tags in Gerrit. Gerrit calls the feature "hashtags", and it's now supported by the PolyGerrit UI. We're using them in Perkeep and they're great. It means we can search on them, too.

On their silly name: https://groups.google.com/forum/#!topic/repo-discuss/jZ0raMyuiG0

https://gerrit-review.googlesource.com/Documentation/rest-api-changes.html#get-hashtags
https://gerrit-review.googlesource.com/Documentation/rest-api-changes.html#set-hashtags

Example Gerrit search: https://camlistore-review.googlesource.com/q/is:open+-owner:mathieu+-hashtag:op (we use the "op" tag in Perkeep to mean that it's the CL author's turn to do something).

/cc @andybons

@andybons
Copy link
Member

andybons commented Feb 2, 2018

(we use the "op" tag in Perkeep to mean that it's the CL author's turn to do something).

This can also be achieved by setting the “assignee” field, right?

@bradfitz
Copy link
Contributor Author

bradfitz commented Feb 2, 2018

This can also be achieved by setting the “assignee” field, right?

Maybe. It was just an example.

@andybons andybons removed their assignment Sep 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Builders x/build issues (builders, bots, dashboards) NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Projects
None yet
Development

No branches or pull requests

7 participants