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/git-codereview: support mailing multiple revisions with a single CL #16034

Closed
josharian opened this issue Jun 10, 2016 · 10 comments
Closed
Labels
Documentation FrozenDueToAge help wanted NeedsFix The path to resolution is known, but the work has not been done. Suggested Issues that may be good for new contributors looking for work to do.
Milestone

Comments

@josharian
Copy link
Contributor

$ git mail
git-codereview: cannot mail: multiple changes pending; must specify commit on command line:
    cacb069 git-codereview: pass args to installHook
    e3eeef8 git-codereview: use codereview.cfg from working tree
    1ac8f8d git-codereview: fix TestPendingNoneBranch
    1c26cef git-codereview: disable failing tests

I will now proceed to run git mail individually on every single commit, in order, manually copying and pasting in the SHA1s. This happens all the time, at least for me. I'd like to be able to tell git mail to mail all of them, re-using the other flags (reviewer, trybot) for every commit:

Maybe:

$ git mail -r adg -trybot --all

@adg, opinions?

@josharian josharian added this to the Unplanned milestone Jun 10, 2016
@josharian
Copy link
Contributor Author

Never mind. I'm an idiot. Can't believe it has taken me how long?? to figure out that I can just git mail the topmost commit. Sigh.

@timbunce
Copy link

You're not an idiot @josharian.
The error message is not helpful. This is a valid usability bug.
Adding something like "(use latest commit to submit them all)" would be simple and helpful.

@josharian
Copy link
Contributor Author

CL welcome!

@rasky rasky reopened this Feb 16, 2018
@rasky rasky added the NeedsFix The path to resolution is known, but the work has not been done. label Feb 16, 2018
@rasky
Copy link
Member

rasky commented Feb 16, 2018

Keep this open as it's a valid issue.

@josharian josharian added Suggested Issues that may be good for new contributors looking for work to do. help wanted Documentation labels Mar 28, 2018
@mvdan
Copy link
Member

mvdan commented Jun 1, 2018

I also completely failed to understand that this is how it's supposed to be used.

Here's a question - why shouldn't git-codereview mail (with no arguments) just mean git-codereview HEAD? It seems to me like that's what people would understand it to mean. For example, git push with no arguments also pushes the head of the current branch.

@josharian
Copy link
Contributor Author

That sounds good to me. Alternatively I’d be happy enough with a better error message and accepting any git ref (eg “head”).

@ysmolski
Copy link
Member

ysmolski commented Nov 7, 2018

Wait a minute, but mail HEAD works for me:

~/golang (ssa_cfg $%) % git mail HEAD
remote: Processing changes: refs: 1, updated: 2, done
remote:
remote: SUCCESS
remote:
remote: Updated Changes:
remote:   https://go-review.googlesource.com/c/go/+/142517 cmd/compile: add control flow graphs to ssa.html
remote:   https://go-review.googlesource.com/c/go/+/144557 cmd/compile: make ssa blocks collapsable in ssa.html
remote:
Updated tag 'ssa_cfg.mailed' (was 7794b8dadd)

Do we want to make mail command to mail the HEAD by default in case of the chain of changes?

@mvdan
Copy link
Member

mvdan commented Nov 7, 2018

@ysmolsky that's one of the suggestions made above. The other suggestion is for git-codereview mail to give a better error message, such as suggesting git-codereview mail HEAD if one wants to mail all the CLs in the current branch.

@gopherbot
Copy link

Change https://golang.org/cl/148137 mentions this issue: git-codereview: add a hint about mailing the whole chain of changes

@ysmolski ysmolski self-assigned this Nov 7, 2018
@ysmolski
Copy link
Member

ysmolski commented May 8, 2019

This was fixed by the CL.

@ysmolski ysmolski closed this as completed May 8, 2019
gopherbot pushed a commit to golang/review that referenced this issue May 8, 2019
"mail HEAD" will mail every commit between HEAD and branching point.

Fixes golang/go#16034

Change-Id: If95aece584f2a324f866c64770f7361ddb3e7636
Reviewed-on: https://go-review.googlesource.com/c/review/+/148137
Reviewed-by: Josh Bleecher Snyder <josharian@gmail.com>
@golang golang locked and limited conversation to collaborators May 7, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Documentation FrozenDueToAge help wanted NeedsFix The path to resolution is known, but the work has not been done. Suggested Issues that may be good for new contributors looking for work to do.
Projects
None yet
Development

No branches or pull requests

6 participants