Skip to content
Permalink
Browse files

cmd/shfmt: atomically replace files with -w

First, add a test case where a script uses shfmt to format itself.
Before this change, we'd get a failure, since the file that the shell is
executing gets modified in-place:

	> exec sh input.sh
	[stdout]
	foo
	[stderr]
	input.sh: line 3: ho: command not found

The test requires a bit of magic to have the shfmt tool in $PATH without
installing it globally, nor building it every time. We reuse a method
form another tool which simply reuses the test binary.

Use renameio to instead replace the input file atomically. One open
question is file metadata; because the old file is thrown away, we would
normally lose its permissions and owner/group. Do our best to keep the
permission bits in place via a separate lstat call.

The separate lstat call isn't atomic, but that's okay. If something is
deleting or messing with the file while shfmt is running, other parts of
the tool could fail already, such as parsing it. Keeping the permissions
in-place is important, for example to preserve the original script's
executable bit, independently of whether it was set or unset.

For now, this feature isn't enabled on Windows, since renameio doesn't
support the operating system. On it, we fall back to the previous
ioutil.WriteFile, which isn't an atomic write, but still works.

Fixes #401.
  • Loading branch information
mvdan committed Feb 21, 2020
1 parent cc3ea66 commit dcba6fde1c6715e3a47fcd334d4d4902c964a4de
Showing with 59 additions and 4 deletions.
  1. +10 −4 cmd/shfmt/main.go
  2. +22 −0 cmd/shfmt/main_test.go
  3. +24 −0 cmd/shfmt/testdata/scripts/atomic.txt
  4. +1 −0 go.mod
  5. +2 −0 go.sum
@@ -13,7 +13,9 @@ import (
"os"
"path/filepath"
"regexp"
"runtime"

"github.com/google/renameio"
"github.com/pkg/diff"
"golang.org/x/term"
"mvdan.cc/editorconfig"
@@ -338,14 +340,18 @@ func formatBytes(src []byte, path string) error {
}
}
if *write {
f, err := os.OpenFile(path, os.O_WRONLY|os.O_TRUNC, 0)
info, err := os.Lstat(path)
if err != nil {
return err
}
if _, err := f.Write(res); err != nil {
return err
perm := info.Mode().Perm()
writeFile := renameio.WriteFile
// TODO: support atomic writes on Windows once renameio
// supports it
if runtime.GOOS == "windows" {
writeFile = ioutil.WriteFile
}
if err := f.Close(); err != nil {
if err := writeFile(path, res, perm); err != nil {
return err
}
}
@@ -6,10 +6,13 @@ package main
import (
"bufio"
"bytes"
"flag"
"fmt"
"io/ioutil"
"os"
"path/filepath"
"regexp"
"runtime"
"strings"
"testing"

@@ -29,10 +32,29 @@ func TestMain(m *testing.M) {
}))
}

var update = flag.Bool("u", false, "update testscript output files")

func TestScripts(t *testing.T) {
t.Parallel()
testscript.Run(t, testscript.Params{
Dir: filepath.Join("testdata", "scripts"),
Setup: func(env *testscript.Env) error {
bindir := filepath.Join(env.WorkDir, ".bin")
if err := os.Mkdir(bindir, 0777); err != nil {
return err
}
binfile := filepath.Join(bindir, "shfmt")
if runtime.GOOS == "windows" {
binfile += ".exe"
}
if err := os.Symlink(os.Args[0], binfile); err != nil {
return err
}
env.Vars = append(env.Vars, fmt.Sprintf("PATH=%s%c%s", bindir, filepath.ListSeparator, os.Getenv("PATH")))
env.Vars = append(env.Vars, "TESTSCRIPT_COMMAND=shfmt")
return nil
},
UpdateScripts: *update,
})
}

@@ -0,0 +1,24 @@
[windows] skip 'atomic writes aren''t supported on Windows'
[!exec:sh] skip 'sh is required to run this test'

# If we don't do atomic writes, most shells will error when shfmt overwrites the
# very script it's running from. This is because the shell doesn't read all of
# the input script upfront.
exec sh input.sh
cmp stdout stdout.golden
! stderr .

cmp input.sh input.sh.golden

-- input.sh --
echo foo
shfmt -l -w input.sh
echo bar
-- input.sh.golden --
echo foo
shfmt -l -w input.sh
echo bar
-- stdout.golden --
foo
input.sh
bar
1 go.mod
@@ -4,6 +4,7 @@ go 1.11

require (
github.com/creack/pty v1.1.9
github.com/google/renameio v0.1.0
github.com/kr/pretty v0.2.0
github.com/kr/text v0.2.0 // indirect
github.com/pkg/diff v0.0.0-20190930165518-531926345625
2 go.sum
@@ -2,6 +2,8 @@ github.com/creack/pty v1.1.9 h1:uDmaGzcdjhF4i/plgjmEsriH11Y0o7RKapEf/LDaM3w=
github.com/creack/pty v1.1.9/go.mod h1:oKZEueFk5CKHvIhNR5MUki03XCEU+Q6VDXinZuGJ33E=
github.com/davecgh/go-spew v1.1.0 h1:ZDRjVQ15GmhC3fiQ8ni8+OwkZQO4DARzQgrnXU1Liz8=
github.com/davecgh/go-spew v1.1.0/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38=
github.com/google/renameio v0.1.0 h1:GOZbcHa3HfsPKPlmyPyN2KEohoMXOhdMbHrvbpl2QaA=
github.com/google/renameio v0.1.0/go.mod h1:KWCgfxg9yswjAJkECMjeO8J8rahYeXnNhOm40UhjYkI=
github.com/kr/pretty v0.1.0 h1:L/CwN0zerZDmRFUapSPitk6f+Q3+0za1rQkzVuMiMFI=
github.com/kr/pretty v0.1.0/go.mod h1:dAy3ld7l9f0ibDNOQOHHMYYIIbhfbHSm3C4ZsoJORNo=
github.com/kr/pretty v0.2.0 h1:s5hAObm+yFO5uHYt5dYjxi2rXrsnmRpJx4OYvIWUaQs=

0 comments on commit dcba6fd

Please sign in to comment.
You can’t perform that action at this time.