search

golang / go

The Go programming language
https://go.dev
BSD 3-Clause "New" or "Revised" License
123.93k stars 17.65k forks source link

x/review/git-codereview: when working on the Go repo, use its bin/gofmt if present #26336

Open FiloSottile opened 6 years ago

FiloSottile commented 6 years ago

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.

gopherbot commented 6 years ago

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

josharian commented 6 years ago

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 https://github.com/golang/go/issues/27166 and then using the results of that.

agnivade commented 5 years ago

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

mvdan commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

Agreed, that was the poorly worded intention.

agnivade commented 5 years ago

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