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

x/review/git-codereview: refuse to mail fixup commits? #20580

Closed
rsc opened this issue Jun 5, 2017 · 4 comments
Closed

x/review/git-codereview: refuse to mail fixup commits? #20580

rsc opened this issue Jun 5, 2017 · 4 comments
Labels
FrozenDueToAge
Milestone
Unreleased

Comments

@rsc
Copy link
Contributor

rsc commented Jun 5, 2017

I just accidentally mailed two fixup commits (e.g. https://go-review.googlesource.com/c/44857/1) instead of running git rw first. I wonder if mail should refuse to push a stack including fixup commits?

/cc @aclements
@gopherbot gopherbot added this to the Unreleased milestone Jun 5, 2017
@aclements
Copy link
Member

Seems reasonable to me, thought also not a big deal as long as they don't get submitted.

@mvdan
Copy link
Member

mvdan commented Jun 5, 2017

While at it, you should also look for a squash! prefix - that's the other form of commit that should be rebased before mailed.

@ysmolski
Copy link
Member

I think this was already fixed:

func TestDoNotMail(t *testing.T) {
	gt := newGitTest(t)
	defer gt.done()
	gt.work(t)
	trun(t, gt.client, "git", "commit", "--amend", "-m", "This is my commit.\n\nDO NOT MAIL\n")

	testMainDied(t, "mail")
	testPrintedStderr(t, "DO NOT MAIL")

	trun(t, gt.client, "git", "commit", "--amend", "-m", "fixup! This is my commit.")

	testMainDied(t, "mail")
	testPrintedStderr(t, "fixup! commit")

	trun(t, gt.client, "git", "commit", "--amend", "-m", "squash! This is my commit.")

	testMainDied(t, "mail")
	testPrintedStderr(t, "squash! commit")

	trun(t, gt.client, "git", "commit", "--amend", "-m", "This is my commit.\n\nDO NOT MAIL\n")

	// Do not mail even when the DO NOT MAIL is a parent of the thing we asked to mail.
	gt.work(t)
	testMainDied(t, "mail", "HEAD")
	testPrintedStderr(t, "DO NOT MAIL")
}

https://github.com/golang/review/blob/34696713d8ce9c6e709a50d8192fd973246a30e9/git-codereview/mail_test.go#L33-L58

@bradfitz, should we close it?

@aclements
Copy link
Member

Thanks @ysmolsky. Yes, this was fixed by commit golang/review@0d52c01

@golang golang locked and limited conversation to collaborators Mar 21, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge
Projects
None yet
Milestone
Unreleased
Development

No branches or pull requests
5 participants
@rsc @ysmolski @aclements @mvdan @gopherbot