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

Comments

@bradfitz
Copy link
Member

@bradfitz 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

@hyangah

This comment has been minimized.

Copy link
Contributor

@hyangah hyangah commented Feb 1, 2018

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

@martisch

This comment has been minimized.

Copy link
Member

@martisch 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

This comment has been minimized.

Copy link
Member Author

@bradfitz 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

This comment has been minimized.

Copy link
Contributor

@josharian 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

This comment has been minimized.

Copy link
Member

@tklauser 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

This comment has been minimized.

Copy link
Member Author

@bradfitz 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

This comment has been minimized.

Copy link
Member

@andybons 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

This comment has been minimized.

Copy link
Member Author

@bradfitz 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
Projects
None yet
7 participants
You can’t perform that action at this time.