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/review: add an -m option #24912

Closed
jeanbza opened this issue Apr 18, 2018 · 15 comments
Closed

x/review: add an -m option #24912

jeanbza opened this issue Apr 18, 2018 · 15 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@jeanbza
Copy link
Member

jeanbza commented Apr 18, 2018

I'd love to be able to git codereview change -m "some message", the same way that I can git commit -m "some message".

@gopherbot gopherbot added this to the Unreleased milestone Apr 18, 2018
@gopherbot
Copy link

Change https://golang.org/cl/107625 mentions this issue: x/review: add -m option

@jeanbza
Copy link
Member Author

jeanbza commented Apr 18, 2018

@ysmolski
Copy link
Member

Since commit messages for the Go projects rarely fit one line, I found an editor as better tool.

I am not sure how many people would use that.

@ysmolski ysmolski added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Apr 18, 2018
@jeanbza
Copy link
Member Author

jeanbza commented Apr 18, 2018

We (and many others) use the codereview tool to submit changesets to projects other than the Go project. In these other projects, one-liners are fairly frequent - example here.

The main usecase for this is that we're automating our pipeline of producing and mailing out autogenerated code; it's a pain automating the git codereview change part of that since it defaults to using an editor. Being able to use git codereview change -m "internal: regen code" would simplify our lives greatly.

The secondary usecase is that trivial commits made by humans would be less hassle if we could skip the editor step (for example, the commit linked above; there are many more like that).

@agnivade
Copy link
Contributor

You can always use the underlying git command if you are automating git commits.

(In Git terms, git change runs git checkout -b branch, then git branch --set-upstream-to origin/master, then git commit.)

I have never used one-line commits personally, although I can imagine it can be useful in some cases. In any case, a tool should not limit one's workflow, whatever that may be. We might as well maintain parity with git.

@jeanbza
Copy link
Member Author

jeanbza commented Apr 18, 2018

@agnivade Pardon my ignorance - at which step is the change ID generated and inserted into the commit message? That's the part of my impl that's naive (I didn't make sure all the logic around changeID is in place properly).

@ysmolski
Copy link
Member

ysmolski commented Apr 18, 2018

@jadekler
for example i have following hooks installed in gerrit repo:

...golang.org/x/review (mail_prevent_backups>) % cat .git/hooks/pre-commit
#!/bin/sh
exec git-codereview hook-invoke pre-commit "$@"
...golang.org/x/review (mail_prevent_backups>) % cat .git/hooks/commit-msg
#!/bin/sh
exec git-codereview hook-invoke commit-msg "$@"

change-id happens in commit-msg hook:
https://github.com/golang/review/blob/master/git-codereview/hook.go#L181

@agnivade
Copy link
Contributor

/cc @josharian according to owners. Josh - can you check if we should add the -m flag ?

@jadekler - Have you had a chance to look into the commit-msg hook and see if that works for you ?

@josharian
Copy link
Contributor

git-codereview is intended to be generally useful, not just for the Go project (where, with @ysmolsky, I would usually expect longer commit messages). I am generally reluctant to add new flags, but this seems reasonable enough to me. I'll leave some comments on the outstanding CL.

@josharian josharian added the NeedsFix The path to resolution is known, but the work has not been done. label Aug 13, 2018
@gopherbot gopherbot removed the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Aug 13, 2018
@gopherbot
Copy link

Change https://golang.org/cl/130155 mentions this issue: git-codereview: add -m option

@ysmolski
Copy link
Member

ysmolski commented Nov 6, 2018

ping @jadekler. Could please fix the test in your CL?

@jeanbza
Copy link
Member Author

jeanbza commented Nov 6, 2018

[redacted, wrong issue]

@ianlancetaylor
Copy link
Contributor

@jadekler Are you sure you're on the right issue here?

@jeanbza
Copy link
Member Author

jeanbza commented Nov 6, 2018

Oh, wow. Whoops. Thanks @ianlancetaylor!

@jeanbza
Copy link
Member Author

jeanbza commented Nov 6, 2018

@ysmolsky Done!

@golang golang locked and limited conversation to collaborators Nov 8, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

6 participants