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

cmd/gofmt: reconsider making -d exit with a status of 1 if a non-empty diff is printed #46289

Open
mvdan opened this issue May 20, 2021 · 10 comments

Comments

@mvdan
Copy link
Member

@mvdan mvdan commented May 20, 2021

First, I know that this has been asked many times before (#24230 #24427 #38551), and I was even on the other side of this argument for a long time.

The argument usually goes that, if you want to know if any files aren't gofmt-ed, you check that gofmt -l is empty, for example:

test -z $(gofmt -l .)

And this works, if one can assume a POSIX-like shell.

However, in light of #42119, I think we should make "verify that many files comply with gofmt" a trivial command, without extra machinery like checking for non-empty output.

In that issue, we suggest a number of commands that we'd encourage people to run on CI, from go test -race ./... to go mod verify and go vet ./.... You'll note that none of the commands require being wrapped with shell or extra commands to do their job, with two exceptions:

  • go mod tidy, where one needs to check if files like go.mod and go.sum were modified on disk. #27005 will fix that.
  • gofmt, which needs testing for empty output, hence this issue.

Once go mod tidy -check is implemented, gofmt will stand alone in that list as being the only command that most people should run on their CI, but which is non-portable or just not easy to use directly.

--

So, as others have proposed before: I suggest a very simple fix, which is to make gofmt -d exit with a status code of 1 if any of the input files have a non-empty diff. Then, CI could simply run gofmt -d . and get a good result: CI would fail if any files don't comply, and the diff will show exactly which ones and why.

It's also arguably more consistent to behave this way, as the diff tool usually behaves this way too. For example, from GNU diffutils 3.7:

Exit status is 0 if inputs are the same, 1 if different, 2 if trouble.

If we think this change is too invasive, or could break too many users, we could also consider a separate flag like -exitcode, simialr to git diff --exit-code. It's really not a big deal if the command run on CI gets a bit longer.

I also don't think a solution should come from go fmt. I think gofmt is simply superior for use in CI; it allows formatting files which are not part of Go packages, formatting many modules at once, and printing diffs.

It's also worth noting I plan to expose this feature in gofumpt: mvdan/gofumpt#114

cc @griesemer
cc @bcmills @jayconrod @stevetraut for "CI best practices"
cc @ryanm101 @stevenmatthewt @belm0 as previous posters

@seebs
Copy link
Contributor

@seebs seebs commented May 20, 2021

I could live with an alternative of a different flag to set the exit code, to avoid breaking existing things. i am sure there's someone out there who has gofmt in a script with set -e. But yes, please make it possible to use gofmt as a checker in a sane way.

Loading

@griesemer
Copy link
Contributor

@griesemer griesemer commented May 20, 2021

@griesemer for notifications on this issue.

Loading

@mvdan mvdan closed this May 21, 2021
@mvdan mvdan reopened this May 21, 2021
@bcmills
Copy link
Member

@bcmills bcmills commented Jun 16, 2021

If we guard the exit-status change with a flag, It would be nice if we could make the flag's syntax and semantics match whatever flag we add for #27005.

Loading

@bcmills
Copy link
Member

@bcmills bcmills commented Jun 16, 2021

If we think this change is too invasive, or could break too many users,

I think changing the exit status of gofmt -d probably would break some fraction of users, especially for Playground-like servers, and non-gopls integrations, and perhaps CI systems. I have a mild preference for guarding it with a flag.

Loading

@ryanm101
Copy link

@ryanm101 ryanm101 commented Jun 17, 2021

You could add the flag for now and put in a deprecation warning that in X releases the flag will be inverted and the default will be exit codes.

In terms of breakage of CI systems, that is the behaviour I want, If i am enforcing gofmt on my code and you don't do it I want the CI to fall over. IMHO not doing that is the same as a compiler exiting 0 when the code failed to compile.

(i do realise my feelings may be a tad towards the hardline side of this :) ) in all honesty though I'll take whichever implementation we can get. :)

Loading

@mvdan
Copy link
Member Author

@mvdan mvdan commented Jun 17, 2021

I don't think we can ever make the entire tool's default be using exit codes for "was the formating changed". It's entirely reasonable to use some-generator | gofmt to easily generate gofmt-formatted Go code, for example, and all those use cases where one wants to format Go code instead of checking for valid format can't be broken.

I'm also not sure that adding a deprecation or transition period will really help. Breaking a user's script in six months or in two years isn't much different, and I imagine most users will not even see the deprecation notices and remember all the scripts they need to update.

I think changing the exit status of gofmt -d probably would break some fraction of users, especially for Playground-like servers, and non-gopls integrations, and perhaps CI systems. I have a mild preference for guarding it with a flag.

At least from what I've seen, nearly all uses of gofmt -d fall under "check that Go code is correctly formatted", so I think nearly all users would want the exit status behavior. We could have them move to something like gofmt -d -check, but it would be a bit unfortunate to nearly always use both options at the same time.

My intuition is that most scripts using gofmt -d won't break, since what I've most commonly seen is in the form of:

DIFF="$(gofmt -d .)"
if [[ -n $DIFF ]]; then
    echo "$DIFF"
    echo "please run gofmt"
    exit 1
fi

In that kind of scenario, the exit code is by default discarded via the subshell. That's how I've seen many people use gofmt -l, too. The new form, actually using the exit code, could be as follows, or in shorter form via gofmt -d . || exit 1:

if ! gofmt -d .; then
    echo "please run gofmt"
    exit 1
fi

And for the cases where a script would break, I imagine it should be fairly easy to get the old behavior back, such as replacing gofmt -d . with gofmt -d . || true.

All that said, I don't have a strong opinion on new flag versus changing -d. I slightly lean towards not adding a flag, and seeing how many users report breakages during the beta and RC cycle.

Loading

@bcmills
Copy link
Member

@bcmills bcmills commented Jun 17, 2021

In that kind of scenario, the exit code is by default discarded via the subshell.

By default, sure — but many shell style guides require explicit checks, and we probably don't want to break scripts that conform to that style either.

Loading

@bcmills
Copy link
Member

@bcmills bcmills commented Jun 17, 2021

I slightly lean towards not adding a flag, and seeing how many users report breakages during the beta and RC cycle.

FWIW, in my experience changes outside the runtime, compiler, and standard library receive comparatively little testing during the beta and RC. (I would expect most breakage to be reported after the release.)

Loading

@cespare
Copy link
Contributor

@cespare cespare commented Jun 17, 2021

I prefer an explicit flag that's orthogonal to -d for a different reason than compatibility: in CI we don't use -d, we use -l. We don't care about the diffs; only reporting which files aren't formatted.

Loading

@ryanm101
Copy link

@ryanm101 ryanm101 commented Jun 17, 2021

I would agree with @cespare diffs are only really used for when a human is looking, in CI I would want ideally two returns 1) the file that is failing and 2) the line number of the fail would be nice in the case of a few lines but once you get about 10 lines or so this matters less as at that point it's likely the whole files that is wrong.

Still currently any implementation is preferable to none, so additional flag or not I'd just be happy for a non zero exit code.

Loading

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
6 participants