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: when working on the Go repo, use its bin/gofmt if present #26336

Open
FiloSottile opened this issue Jul 11, 2018 · 8 comments
Labels
help wanted NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@FiloSottile
Copy link
Contributor

gofmt changed in Go 1.11, and git-codereview uses the gofmt from $PATH, so when that's the Go 1.10 version, it complains as git-codereview: gofmt needs to format these files (run 'git gofmt').

It should instead use the gofmt in GOROOT.

@FiloSottile FiloSottile added the NeedsFix The path to resolution is known, but the work has not been done. label Jul 11, 2018
@gopherbot gopherbot added this to the Unreleased milestone Jul 11, 2018
@gopherbot
Copy link

Change https://golang.org/cl/130695 mentions this issue: git-codereview: add -g flag to the gomft command

@josharian
Copy link
Contributor

It should instead use the gofmt in GOROOT.

This is not obvious to me. Why is GOROOT better than PATH in this context?

We go to pains to tell people not to set the GOROOT env var, as it is a source of significant confusion. (This is one of @davecheney's favorites.) And tools that memorize the toolchain that they were built with are really frustrating.

It seems to me that using the gofmt in your PATH is the best available solution. Or at most fixing #27166 and then using the results of that.

@agnivade
Copy link
Contributor

agnivade commented Jan 7, 2019

Seems like this needs further discussion. Re-labelled appropriately.

@agnivade agnivade added NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. and removed NeedsFix The path to resolution is known, but the work has not been done. labels Jan 7, 2019
@mvdan
Copy link
Member

mvdan commented Jan 7, 2019

We go to pains to tell people not to set the GOROOT env var

I presume @FiloSottile meant something like go env GOROOT or runtime.GOROOT(), both of which fall back to automatically detecting the directory if unset.

Though I generally agree with Josh. We tell people that to "select" which Go version should be used, they should modify PATH so that go executes the version they want. Why should gofmt be different?

@FiloSottile
Copy link
Contributor Author

git-codereview implies you are submitting code to the repository you are working on. If that repository is the go tree, you should use the gofmt that come with it.

@mvdan
Copy link
Member

mvdan commented Jan 7, 2019

Right, so this would only apply to the Go repo itself, using that clone's bin/gofmt. If special-casing that in git-codereview is fine, that seems reasonable. I think the original issue title is a bit confusing when it talks about GOROOT.

@FiloSottile
Copy link
Contributor Author

Agreed, that was the poorly worded intention.

@FiloSottile FiloSottile added help wanted NeedsFix The path to resolution is known, but the work has not been done. labels Jan 7, 2019
@gopherbot gopherbot removed the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Jan 7, 2019
@mvdan mvdan changed the title x/review/git-codereview: use the gofmt from $GOROOT/bin/gofmt x/review/git-codereview: when working on the Go repo, use its bin/gofmt if present Jan 7, 2019
@agnivade
Copy link
Contributor

agnivade commented Jan 7, 2019

@ekalinin - Could you please update the CL according to the logic discussed above ? Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

5 participants