[engine] Resolve root directory using git
Instead of assuming the current working directory (overridden by
`--root`) contains a shac.star file, locate the root of the current git
repository (if the current directory is in a git repository) and use the
shac.star file there, also analyzing all files in the git directory
instead of just files in the current working directory.
The `--root` flag is replaced by `-C` since it's not necessarily any
sort of root, instead being more analogous to the `-C` flag to git.
We have to keep around the `--root` flag for backwards compatibility
until references in other repos can be updated.
TODO: update the <files> positional argument to accept directories, to
make it possible to analyze just a single subdirectory (e.g. `shac check
subdir`).
Change-Id: Ic9aef678e2cb518a81ff2f36f3bec6fda969675d
Reviewed-on: https://fuchsia-review.googlesource.com/c/shac-project/shac/+/914336
Commit-Queue: Oliver Newman <olivernewman@google.com>
Reviewed-by: Marc-Antoine Ruel <maruel@google.com>
diff --git a/internal/cli/base.go b/internal/cli/base.go
index 929e908..4d0108c 100644
--- a/internal/cli/base.go
+++ b/internal/cli/base.go
@@ -23,22 +23,36 @@
type commandBase struct {
root string
+ cwd string
allFiles bool
noRecurse bool
}
func (c *commandBase) SetFlags(f *flag.FlagSet) {
- f.StringVar(&c.root, "root", ".", "path to the root of the tree to analyse")
+ f.StringVarP(&c.cwd, "cwd", "C", ".", "directory in which to run shac")
f.BoolVar(&c.allFiles, "all", false, "checks all the files instead of guess the upstream to diff against")
f.BoolVar(&c.noRecurse, "no-recurse", false, "do not look for shac.star files recursively")
+
+ // TODO(olivernewman): Delete this flag after it's no longer used.
+ f.StringVar(&c.root, "root", ".", "path to the root of the tree to analyse")
+ // MarkHidden instead of MarkDeprecated to prevent warnings from being
+ // emitted.
+ f.MarkHidden("root")
}
func (c *commandBase) options(files []string) (engine.Options, error) {
if c.allFiles && len(files) > 0 {
return engine.Options{}, errors.New("--all cannot be set together with positional file arguments")
}
+ cwd := c.cwd
+ if c.root != "." {
+ if cwd != "." {
+ return engine.Options{}, errors.New("--root and --cwd cannot both be set")
+ }
+ cwd = c.root
+ }
return engine.Options{
- Root: c.root,
+ Root: cwd,
AllFiles: c.allFiles,
Files: files,
Recurse: !c.noRecurse,
diff --git a/internal/engine/bench_test.go b/internal/engine/bench_test.go
index 0e8e731..88d0f60 100644
--- a/internal/engine/bench_test.go
+++ b/internal/engine/bench_test.go
@@ -32,8 +32,10 @@
}
func BenchmarkPrint_Git(b *testing.B) {
+ root := makeGit(b)
+ copyFile(b, root, "testdata/bench/print.star")
want := "[//print.star:16] running\n"
- benchStarlarkPrint(b, "testdata/bench", "print.star", true, want)
+ benchStarlarkPrint(b, root, "print.star", true, want)
}
func BenchmarkPrint100_Raw(b *testing.B) {
@@ -44,8 +46,10 @@
}
func BenchmarkPrint100_Git(b *testing.B) {
+ root := makeGit(b)
+ copyFile(b, root, "testdata/bench/print100.star")
want := strings.Repeat("[//print100.star:16] running\n", 100)
- benchStarlarkPrint(b, "testdata/bench", "print100.star", true, want)
+ benchStarlarkPrint(b, root, "print100.star", true, want)
}
func BenchmarkCtxEmitFinding(b *testing.B) {
diff --git a/internal/engine/env.go b/internal/engine/env.go
index a3d4cd3..f9d93c0 100644
--- a/internal/engine/env.go
+++ b/internal/engine/env.go
@@ -231,7 +231,7 @@
}
} else if errors.Is(err, fs.ErrNotExist) {
// Hide the underlying error for determinism.
- source.err = errors.New("file not found")
+ source.err = fmt.Errorf("%s not found", sk.relpath)
} else {
source.err = err
}
diff --git a/internal/engine/fix.go b/internal/engine/fix.go
index b8a0a0d..e513c7c 100644
--- a/internal/engine/fix.go
+++ b/internal/engine/fix.go
@@ -51,9 +51,8 @@
// Sort for determinism.
sort.Strings(orderedFiles)
- for _, f := range orderedFiles {
- findings := findingsByFile[f]
- path := filepath.Join(o.Root, f)
+ for _, path := range orderedFiles {
+ findings := findingsByFile[path]
numFixed, err := fixFindings(path, findings)
if err != nil {
return err
@@ -63,7 +62,7 @@
noun += "s"
}
if !quiet {
- fmt.Fprintf(os.Stderr, "Fixed %d %s in %s\n", numFixed, noun, f)
+ fmt.Fprintf(os.Stderr, "Fixed %d %s in %s\n", numFixed, noun, path)
}
}
return nil
@@ -185,7 +184,7 @@
c.mu.Lock()
defer c.mu.Unlock()
c.findings = append(c.findings, findingToFix{
- file: file,
+ file: filepath.Join(root, filepath.FromSlash(file)),
span: s,
replacement: replacements[0],
})
diff --git a/internal/engine/git.go b/internal/engine/git.go
new file mode 100644
index 0000000..7128e5b
--- /dev/null
+++ b/internal/engine/git.go
@@ -0,0 +1,103 @@
+// Copyright 2023 The Shac Authors
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package engine
+
+import (
+ "context"
+ "errors"
+ "fmt"
+ "os"
+ "os/exec"
+ "sort"
+ "strings"
+ "sync"
+
+ "go.fuchsia.dev/shac-project/shac/internal/execsupport"
+)
+
+// cachedGitEnv should never be accessed directly, only by calling `gitEnv()`.
+var cachedGitEnv []string
+var populateGitEnvOnce sync.Once
+
+func gitEnv() []string {
+ populateGitEnvOnce.Do(func() {
+ // First is for git version before 2.32, the rest are to skip the user and system config.
+ cachedGitEnv = append(os.Environ(),
+ "GIT_CONFIG_NOGLOBAL=true",
+ "GIT_CONFIG_GLOBAL=",
+ "GIT_CONFIG_SYSTEM=",
+ "LANG=C",
+ "GIT_EXTERNAL_DIFF=",
+ "GIT_DIFF_OPTS=",
+ )
+ gitConfig := map[string]string{
+ // Prevents automatic unicode decomposition of filenames. Only has
+ // an effect on macOS.
+ "core.precomposeUnicode": "true",
+ }
+ cachedGitEnv = append(cachedGitEnv, gitConfigEnv(gitConfig)...)
+ })
+ return cachedGitEnv
+}
+
+func runGitCmd(ctx context.Context, dir string, args ...string) (string, error) {
+ args = append([]string{
+ // Don't update the git index during read operations.
+ "--no-optional-locks",
+ }, args...)
+ cmd := exec.CommandContext(ctx, "git", args...)
+ cmd.Dir = dir
+ cmd.Env = gitEnv()
+ b := buffers.get()
+ cmd.Stdout = b
+ cmd.Stderr = b
+ err := execsupport.Run(cmd)
+ // Always make a copy of the output, since it could be persisted. Only reuse
+ // the temporary buffer.
+ out := b.String()
+ buffers.push(b)
+ if err != nil {
+ if errExit := (&exec.ExitError{}); errors.As(err, &errExit) {
+ return "", fmt.Errorf("error running git %s: %w\n%s", strings.Join(args, " "), err, out)
+ }
+ return "", err
+ }
+ return strings.TrimSpace(out), nil
+}
+
+// gitConfigEnv converts a map of key-value git config pairs into corresponding
+// environment variables.
+//
+// See https://git-scm.com/docs/git-config#ENVIRONMENT for details on how git
+// configs are set via environment variables.
+func gitConfigEnv(gitConfig map[string]string) []string {
+ // GIT_CONFIG_COUNT specifies how many key/value env var pairs to look for.
+ res := []string{fmt.Sprintf("GIT_CONFIG_COUNT=%d", len(gitConfig))}
+
+ keys := make([]string, 0, len(gitConfig))
+ for k := range gitConfig {
+ keys = append(keys, k)
+ }
+ sort.Strings(keys)
+
+ for i, k := range keys {
+ // Each config setting is specified by setting a pair of
+ // GIT_CONFIG_KEY_<N> and GIT_CONFIG_VALUE_<N> variables.
+ res = append(res,
+ fmt.Sprintf("GIT_CONFIG_KEY_%d=%s", i, k),
+ fmt.Sprintf("GIT_CONFIG_VALUE_%d=%s", i, gitConfig[k]))
+ }
+ return res
+}
diff --git a/internal/engine/pkg_test.go b/internal/engine/pkg_test.go
index 8124fbe..bc3db80 100644
--- a/internal/engine/pkg_test.go
+++ b/internal/engine/pkg_test.go
@@ -97,6 +97,10 @@
rel = newName
}
dest := filepath.Join(dstDir, rel)
+ fi, err := os.Stat(path)
+ if err != nil {
+ return err
+ }
b, err := os.ReadFile(path)
if err != nil {
return err
@@ -104,7 +108,7 @@
if err := os.MkdirAll(filepath.Dir(dest), 0o700); err != nil {
return err
}
- return os.WriteFile(dest, b, 0o600)
+ return os.WriteFile(dest, b, fi.Mode())
})
if err != nil {
t.Fatal(err)
diff --git a/internal/engine/run.go b/internal/engine/run.go
index bf785a4..400fc5e 100644
--- a/internal/engine/run.go
+++ b/internal/engine/run.go
@@ -22,7 +22,9 @@
"errors"
"fmt"
"io/fs"
+ "log"
"os"
+ "os/exec"
"path"
"path/filepath"
"runtime"
@@ -142,7 +144,9 @@
// reporting.Get() which returns the right implementation based on the
// environment (CI, interactive, etc).
Report Report
- // Root directory. Defaults to the current working directory.
+ // Root overrides the current working directory, making shac behave as if it
+ // was run in the specified directory. It defaults to the current working
+ // directory.
Root string
// Files lists specific files to analyze.
Files []string
@@ -162,11 +166,7 @@
// Run loads a main shac.star file from a root directory and runs it.
func Run(ctx context.Context, o *Options) error {
- root := o.Root
- if root == "" {
- root = "."
- }
- root, err := filepath.Abs(root)
+ root, err := resolveRoot(ctx, o.Root)
if err != nil {
return err
}
@@ -424,6 +424,44 @@
return nil
}
+// resolveRoot resolves an appropriate root directory from which to load shac
+// checks and analyze files.
+func resolveRoot(ctx context.Context, dir string) (string, error) {
+ if dir == "" {
+ dir = "."
+ }
+
+ fi, err := os.Stat(dir)
+ if errors.Is(err, fs.ErrNotExist) {
+ return "", fmt.Errorf("no such directory: %s", dir)
+ } else if err != nil {
+ return "", err
+ } else if !fi.IsDir() {
+ return "", fmt.Errorf("not a directory: %s", dir)
+ }
+
+ dir, err = filepath.Abs(dir)
+ if err != nil {
+ return "", err
+ }
+ root, err := runGitCmd(ctx, dir, "rev-parse", "--show-toplevel")
+ if err != nil {
+ if errors.Is(err, exec.ErrNotFound) {
+ log.Printf("git not detected on $PATH")
+ return dir, nil
+ } else if strings.Contains(err.Error(), "not a git repository") {
+ log.Printf("current working directory is not a git repository")
+ return dir, nil
+ }
+ // Any other error is fatal.
+ return "", err
+ }
+ // root will have normal Windows path but git returns a POSIX style path
+ // that may be incorrect. Clean it up.
+ root = strings.ReplaceAll(filepath.Clean(root), string(os.PathSeparator), "/")
+ return root, nil
+}
+
// shacState represents a parsing state of one shac.star.
type shacState struct {
env *starlarkEnv
diff --git a/internal/engine/run_test.go b/internal/engine/run_test.go
index 0fe522c..d0663e2 100644
--- a/internal/engine/run_test.go
+++ b/internal/engine/run_test.go
@@ -40,6 +40,7 @@
func TestRun_Fail(t *testing.T) {
t.Parallel()
+ scratchDir := t.TempDir()
data := []struct {
name string
o Options
@@ -71,9 +72,14 @@
},
{
"malformed config file",
- Options{
- config: "testdata/config/syntax.textproto",
- },
+ func() Options {
+ root := t.TempDir()
+ writeFile(t, root, "shac.star", ``)
+ writeFile(t, root, "shac.textproto", "bad")
+ return Options{
+ Root: root,
+ }
+ }(),
// The encoding is not deterministic.
"...: unknown field: bad",
},
@@ -84,7 +90,7 @@
},
// TODO(olivernewman): This error should be more specific, like "no
// shac.star file found".
- "file not found",
+ "shac.star not found",
},
{
"no shac.star files (recursive)",
@@ -94,6 +100,23 @@
},
"no shac.star files found",
},
+ {
+ "nonexistent directory",
+ Options{
+ Root: "!!!this-is-a-file-that-does-not-exist!!!",
+ },
+ "no such directory: !!!this-is-a-file-that-does-not-exist!!!",
+ },
+ {
+ "not a directory",
+ Options{
+ Root: func() string {
+ writeFile(t, scratchDir, "foo.txt", "")
+ return filepath.Join(scratchDir, "foo.txt")
+ }(),
+ },
+ fmt.Sprintf("not a directory: %s", filepath.Join(scratchDir, "foo.txt")),
+ },
}
for i := range data {
i := i
@@ -117,6 +140,41 @@
}
}
+func TestRun_DirOverridden(t *testing.T) {
+ t.Parallel()
+
+ data := []struct {
+ name string
+ // The full error message often includes the directory, hence a function
+ // so it's easier to construct the values.
+ dir string
+ want string
+ }{
+ {
+ "git-aware",
+ func() string {
+ root := makeGit(t)
+ writeFile(t, root, "shac.star", `print("hello")`)
+
+ // If shac is pointed at a subdirectory of a git repo, it should
+ // discover and run checks defined anywhere in the repo.
+ subdir := filepath.Join(root, "a", "b")
+ mkdirAll(t, subdir)
+ return subdir
+ }(),
+ "[//shac.star:1] hello\n",
+ },
+ }
+
+ for i := range data {
+ i := i
+ t.Run(data[i].name, func(t *testing.T) {
+ t.Parallel()
+ testStarlarkPrint(t, data[i].dir, "", false, data[i].want)
+ })
+ }
+}
+
func TestRun_SpecificFiles(t *testing.T) {
// Not parallelized because it calls os.Chdir.
@@ -282,9 +340,7 @@
root := t.TempDir()
subdir := filepath.Join(root, "dir")
- if err := os.Mkdir(subdir, 0o700); err != nil {
- t.Fatal(err)
- }
+ mkdirAll(t, subdir)
dirOutsideRoot := t.TempDir()
data := []struct {
@@ -1402,7 +1458,7 @@
},
{
"load-inexistant.star",
- "cannot load ./inexistant.star: file not found",
+ "cannot load ./inexistant.star: inexistant.star not found",
" //load-inexistant.star:15:1: in <toplevel>\n",
},
{
@@ -1945,7 +2001,10 @@
}
func TestRun_Vendored(t *testing.T) {
- testStarlarkPrint(t, "testdata/vendored", "shac.star", false, "[//shac.star:17] True\n")
+ t.Parallel()
+ dir := t.TempDir()
+ copyTree(t, dir, "testdata/vendored", nil)
+ testStarlarkPrint(t, dir, "shac.star", false, "[//shac.star:17] True\n")
}
// Utilities
@@ -1980,10 +2039,13 @@
out = append(out, n)
}
}
- return p, out
+ dest := t.TempDir()
+ copyTree(t, dest, p, nil)
+ return dest, out
}
func makeGit(t testing.TB) string {
+ t.Helper()
// scm.go requires two commits. Not really worth fixing yet, it's only
// annoying in unit tests.
root := t.TempDir()
@@ -2003,6 +2065,7 @@
}
func initGit(t testing.TB, dir string) {
+ t.Helper()
runGit(t, dir, "init")
runGit(t, dir, "config", "user.email", "test@example.com")
runGit(t, dir, "config", "user.name", "engine test")
@@ -2032,20 +2095,28 @@
}
func writeFile(t testing.TB, root, path, content string) {
+ t.Helper()
writeFileBytes(t, root, path, []byte(content), 0o600)
}
func writeFileBytes(t testing.TB, root, path string, content []byte, perm os.FileMode) {
+ t.Helper()
abs := filepath.Join(root, path)
- if err := os.MkdirAll(filepath.Dir(abs), 0o700); err != nil {
- t.Fatal(err)
- }
+ mkdirAll(t, filepath.Dir(abs))
if err := os.WriteFile(abs, content, perm); err != nil {
t.Fatal(err)
}
}
+func mkdirAll(t testing.TB, dir string) {
+ t.Helper()
+ if err := os.MkdirAll(dir, 0o700); err != nil {
+ t.Fatal(err)
+ }
+}
+
func readFile(t *testing.T, path string) string {
+ t.Helper()
b, err := os.ReadFile(path)
if err != nil {
t.Fatal(err)
diff --git a/internal/engine/runtime_ctx_scm.go b/internal/engine/runtime_ctx_scm.go
index 9c88c00..e2ca780 100644
--- a/internal/engine/runtime_ctx_scm.go
+++ b/internal/engine/runtime_ctx_scm.go
@@ -32,7 +32,6 @@
"unsafe"
"github.com/go-git/go-git/plumbing/format/gitignore"
- "go.fuchsia.dev/shac-project/shac/internal/execsupport"
"go.starlark.net/starlark"
)
@@ -285,8 +284,8 @@
func getSCM(ctx context.Context, root string, allFiles bool) (scmCheckout, error) {
// Flip to POSIX style path.
root = strings.ReplaceAll(root, string(os.PathSeparator), "/")
- g := &gitCheckout{returnAll: allFiles}
- err := g.init(ctx, root)
+ g := &gitCheckout{returnAll: allFiles, checkoutRoot: root}
+ err := g.init(ctx)
if err == nil {
if g.checkoutRoot != root {
if !strings.HasPrefix(root, g.checkoutRoot) {
@@ -365,7 +364,6 @@
// gitCheckout represents a git checkout.
type gitCheckout struct {
// Configuration.
- env []string
returnAll bool
// Detected environment at initialization.
@@ -378,13 +376,7 @@
err error // save error.
}
-func (g *gitCheckout) init(ctx context.Context, root string) error {
- // Find root.
- g.checkoutRoot = root
- g.checkoutRoot = g.run(ctx, "rev-parse", "--show-toplevel")
- // root will have normal Windows path but git returns a POSIX style path
- // that may be incorrect. Clean it up.
- g.checkoutRoot = strings.ReplaceAll(filepath.Clean(g.checkoutRoot), string(os.PathSeparator), "/")
+func (g *gitCheckout) init(ctx context.Context) error {
g.head.hash = g.run(ctx, "rev-parse", "HEAD")
g.head.ref = g.run(ctx, "rev-parse", "--abbrev-ref=strict", "--symbolic-full-name", "HEAD")
if g.err != nil {
@@ -421,69 +413,9 @@
if g.err != nil {
return ""
}
- args = append([]string{
- // Don't update the git index during read operations.
- "--no-optional-locks",
- }, args...)
- cmd := exec.CommandContext(ctx, "git", args...)
- cmd.Dir = g.checkoutRoot
- if g.env == nil {
- // First is for git version before 2.32, the rest are to skip the user and system config.
- g.env = append(os.Environ(),
- "GIT_CONFIG_NOGLOBAL=true",
- "GIT_CONFIG_GLOBAL=",
- "GIT_CONFIG_SYSTEM=",
- "LANG=C",
- "GIT_EXTERNAL_DIFF=",
- "GIT_DIFF_OPTS=",
- )
- gitConfig := map[string]string{
- // Prevents automatic unicode decomposition of filenames. Only has
- // an effect on macOS.
- "core.precomposeUnicode": "true",
- }
- g.env = append(g.env, gitConfigEnv(gitConfig)...)
- }
- cmd.Env = g.env
- b := buffers.get()
- cmd.Stdout = b
- cmd.Stderr = b
- err := execsupport.Run(cmd)
- // Always make a copy of the output, since it could be persisted. Only reuse
- // the temporary buffer.
- out := b.String()
- buffers.push(b)
+ res, err := runGitCmd(ctx, g.checkoutRoot, args...)
if err != nil {
- if errExit := (&exec.ExitError{}); errors.As(err, &errExit) {
- g.err = fmt.Errorf("error running git %s: %w\n%s", strings.Join(args, " "), err, out)
- } else {
- g.err = err
- }
- }
- return strings.TrimSpace(out)
-}
-
-// gitConfigEnv converts a map of key-value git config pairs into corresponding
-// environment variables.
-//
-// See https://git-scm.com/docs/git-config#ENVIRONMENT for details on how git
-// configs are set via environment variables.
-func gitConfigEnv(gitConfig map[string]string) []string {
- // GIT_CONFIG_COUNT specifies how many key/value env var pairs to look for.
- res := []string{fmt.Sprintf("GIT_CONFIG_COUNT=%d", len(gitConfig))}
-
- keys := make([]string, 0, len(gitConfig))
- for k := range gitConfig {
- keys = append(keys, k)
- }
- sort.Strings(keys)
-
- for i, k := range keys {
- // Each config setting is specified by setting a pair of
- // GIT_CONFIG_KEY_<N> and GIT_CONFIG_VALUE_<N> variables.
- res = append(res,
- fmt.Sprintf("GIT_CONFIG_KEY_%d=%s", i, k),
- fmt.Sprintf("GIT_CONFIG_VALUE_%d=%s", i, gitConfig[k]))
+ g.err = err
}
return res
}
diff --git a/internal/engine/testdata/config/syntax.textproto b/internal/engine/testdata/config/syntax.textproto
deleted file mode 100644
index bf97a57..0000000
--- a/internal/engine/testdata/config/syntax.textproto
+++ /dev/null
@@ -1,15 +0,0 @@
-# Copyright 2023 The Shac Authors
-#
-# Licensed under the Apache License, Version 2.0 (the "License");
-# you may not use this file except in compliance with the License.
-# You may obtain a copy of the License at
-#
-# http://www.apache.org/licenses/LICENSE-2.0
-#
-# Unless required by applicable law or agreed to in writing, software
-# distributed under the License is distributed on an "AS IS" BASIS,
-# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-# See the License for the specific language governing permissions and
-# limitations under the License.
-
-bad
