From 54b9a1242385c1cde3ce742b32527ab83f5b2a31 Mon Sep 17 00:00:00 2001 From: Oliver Newman Date: Mon, 28 Aug 2023 22:28:52 +0000 Subject: [PATCH] [engine] Support analyzing only specified files Users can now run `shac {check,fix,fmt} file1 file2 file3 ...` to run checks only on the specified list of files, regardless of which files are affected according to the SCM. Checks that explicitly call `all_files()` will still see all files. Change-Id: Ife998392327438a1fae443e477e0b20b5f8490a8 Reviewed-on: https://fuchsia-review.googlesource.com/c/shac-project/shac/+/904638 Commit-Queue: Marc-Antoine Ruel Reviewed-by: Marc-Antoine Ruel Fuchsia-Auto-Submit: Oliver Newman --- internal/cli/base.go | 10 ++- internal/cli/check.go | 12 ++- internal/cli/fix.go | 9 +-- internal/cli/fmt.go | 9 +-- internal/engine/run.go | 3 + internal/engine/run_test.go | 118 ++++++++++++++++++++++------- internal/engine/runtime_ctx_scm.go | 9 +++ 7 files changed, 123 insertions(+), 47 deletions(-) diff --git a/internal/cli/base.go b/internal/cli/base.go index 3ad981c..929e908 100644 --- a/internal/cli/base.go +++ b/internal/cli/base.go @@ -15,6 +15,8 @@ package cli import ( + "errors" + flag "github.com/spf13/pflag" "go.fuchsia.dev/shac-project/shac/internal/engine" ) @@ -31,10 +33,14 @@ func (c *commandBase) SetFlags(f *flag.FlagSet) { f.BoolVar(&c.noRecurse, "no-recurse", false, "do not look for shac.star files recursively") } -func (c *commandBase) options() engine.Options { +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") + } return engine.Options{ Root: c.root, AllFiles: c.allFiles, + Files: files, Recurse: !c.noRecurse, - } + }, nil } diff --git a/internal/cli/check.go b/internal/cli/check.go index d82f1f9..b88e5c7 100644 --- a/internal/cli/check.go +++ b/internal/cli/check.go @@ -17,7 +17,6 @@ package cli import ( "bytes" "context" - "errors" "os" flag "github.com/spf13/pflag" @@ -43,11 +42,7 @@ func (c *checkCmd) SetFlags(f *flag.FlagSet) { f.StringVar(&c.jsonOutput, "json-output", "", "path to write SARIF output to") } -func (c *checkCmd) Execute(ctx context.Context, args []string) error { - if len(args) != 0 { - return errors.New("unsupported arguments") - } - +func (c *checkCmd) Execute(ctx context.Context, files []string) error { var buf bytes.Buffer r, err := reporting.Get(ctx) @@ -56,7 +51,10 @@ func (c *checkCmd) Execute(ctx context.Context, args []string) error { } r.Reporters = append(r.Reporters, &reporting.SarifReport{Out: &buf}) - o := c.options() + o, err := c.options(files) + if err != nil { + return err + } o.Report = r err = engine.Run(ctx, &o) diff --git a/internal/cli/fix.go b/internal/cli/fix.go index 064b48a..5ceb9e9 100644 --- a/internal/cli/fix.go +++ b/internal/cli/fix.go @@ -16,7 +16,6 @@ package cli import ( "context" - "errors" flag "github.com/spf13/pflag" "go.fuchsia.dev/shac-project/shac/internal/engine" @@ -38,11 +37,11 @@ func (c *fixCmd) SetFlags(f *flag.FlagSet) { c.commandBase.SetFlags(f) } -func (c *fixCmd) Execute(ctx context.Context, args []string) error { - if len(args) != 0 { - return errors.New("unsupported arguments") +func (c *fixCmd) Execute(ctx context.Context, files []string) error { + o, err := c.options(files) + if err != nil { + return err } - o := c.options() o.Filter = engine.OnlyNonFormatters return engine.Fix(ctx, &o) } diff --git a/internal/cli/fmt.go b/internal/cli/fmt.go index 019249b..37fefaa 100644 --- a/internal/cli/fmt.go +++ b/internal/cli/fmt.go @@ -16,7 +16,6 @@ package cli import ( "context" - "errors" flag "github.com/spf13/pflag" "go.fuchsia.dev/shac-project/shac/internal/engine" @@ -38,11 +37,11 @@ func (c *fmtCmd) SetFlags(f *flag.FlagSet) { c.commandBase.SetFlags(f) } -func (c *fmtCmd) Execute(ctx context.Context, args []string) error { - if len(args) != 0 { - return errors.New("unsupported arguments") +func (c *fmtCmd) Execute(ctx context.Context, files []string) error { + o, err := c.options(files) + if err != nil { + return err } - o := c.options() o.Filter = engine.OnlyFormatters return engine.Fix(ctx, &o) } diff --git a/internal/engine/run.go b/internal/engine/run.go index 91f4de2..66a71ff 100644 --- a/internal/engine/run.go +++ b/internal/engine/run.go @@ -143,6 +143,8 @@ type Options struct { Report Report // Root directory. Defaults to the current working directory. Root string + // Files lists specific files to analyze. + Files []string // AllFiles tells to consider all files as affected. AllFiles bool // Recurse tells the engine to run all Main files found in subdirectories. @@ -204,6 +206,7 @@ func Run(ctx context.Context, o *Options) error { } scm = &cachingSCM{ scm: &filteredSCM{ + files: o.Files, matcher: gitignore.NewMatcher(patterns), scm: scm, }, diff --git a/internal/engine/run_test.go b/internal/engine/run_test.go index 3c187b6..7c37ef0 100644 --- a/internal/engine/run_test.go +++ b/internal/engine/run_test.go @@ -116,6 +116,73 @@ func TestRun_Fail(t *testing.T) { } } +func TestRun_SpecificFiles(t *testing.T) { + t.Parallel() + root := t.TempDir() + writeFile(t, root, "shac.textproto", prototext.Format(&Document{ + Ignore: []string{ + "*.py", + }, + })) + + writeFile(t, root, "python.py", "a python file") + writeFile(t, root, "rust.rs", "a rust file") + + copySCM(t, root) + + data := []struct { + name string + want string + files []string + }{ + { + "ctx-scm-affected_files.star", + "[//ctx-scm-affected_files.star:19] \n" + + scmStarlarkFiles("") + + "rust.rs: \n" + + "shac.textproto: \n" + + "\n", + nil, + }, + { + "ctx-scm-all_files.star", + "[//ctx-scm-all_files.star:19] \n" + + scmStarlarkFiles("") + + "rust.rs: \n" + + "shac.textproto: \n" + + "\n", + nil, + }, + } + for i := range data { + i := i + t.Run(data[i].name, func(t *testing.T) { + t.Parallel() + testStarlarkPrint(t, root, data[i].name, false, data[i].want) + }) + } + + t.Run("empty ignore field", func(t *testing.T) { + t.Parallel() + root := t.TempDir() + writeFile(t, root, "shac.textproto", prototext.Format(&Document{ + Ignore: []string{ + "*.foo", + "", + }, + })) + + r := reportPrint{reportNoPrint: reportNoPrint{t: t}} + o := Options{Report: &r, Root: root, AllFiles: false, main: "shac.star"} + err := Run(context.Background(), &o) + if err == nil { + t.Fatal("Expected empty ignore field to be rejected") + } else if !errors.Is(err, errEmptyIgnore) { + t.Fatalf("Expected error %q, got %q", errEmptyIgnore, err) + } + }) +} + func TestRun_Ignore(t *testing.T) { t.Parallel() root := t.TempDir() @@ -207,11 +274,7 @@ func TestRun_SCM_Raw(t *testing.T) { t.Parallel() want := "[//ctx-scm-affected_files.star:19] \n" + "a.txt: \n" + - "ctx-scm-affected_files-include_deleted.star: \n" + - "ctx-scm-affected_files-new_lines.star: \n" + - "ctx-scm-affected_files.star: \n" + - "ctx-scm-all_files-include_deleted.star: \n" + - "ctx-scm-all_files.star: \n" + + scmStarlarkFiles("") + "\n" testStarlarkPrint(t, root, "ctx-scm-affected_files.star", false, want) }) @@ -225,22 +288,21 @@ func TestRun_SCM_Raw(t *testing.T) { t.Parallel() want := "[//ctx-scm-all_files.star:19] \n" + "a.txt: \n" + - "ctx-scm-affected_files-include_deleted.star: \n" + - "ctx-scm-affected_files-new_lines.star: \n" + - "ctx-scm-affected_files.star: \n" + - "ctx-scm-all_files-include_deleted.star: \n" + - "ctx-scm-all_files.star: \n" + + scmStarlarkFiles("") + "\n" testStarlarkPrint(t, root, "ctx-scm-all_files.star", false, want) }) } -const scmStarlarkAddedFiles = "" + - "ctx-scm-affected_files-include_deleted.star: A\n" + - "ctx-scm-affected_files-new_lines.star: A\n" + - "ctx-scm-affected_files.star: A\n" + - "ctx-scm-all_files-include_deleted.star: A\n" + - "ctx-scm-all_files.star: A\n" +func scmStarlarkFiles(action string) string { + return fmt.Sprintf("" + + "ctx-scm-affected_files-include_deleted.star: " + action + "\n" + + "ctx-scm-affected_files-new_lines.star: " + action + "\n" + + "ctx-scm-affected_files.star: " + action + "\n" + + "ctx-scm-all_files-include_deleted.star: " + action + "\n" + + "ctx-scm-all_files.star: " + action + "\n", + ) +} func TestRun_SCM_Git_NoUpstream_Pristine(t *testing.T) { // No upstream branch set, pristine checkout. @@ -259,7 +321,7 @@ func TestRun_SCM_Git_NoUpstream_Pristine(t *testing.T) { "ctx-scm-affected_files.star", false, "[//ctx-scm-affected_files.star:19] \n" + - scmStarlarkAddedFiles + + scmStarlarkFiles("A") + "\n", }, { @@ -267,7 +329,7 @@ func TestRun_SCM_Git_NoUpstream_Pristine(t *testing.T) { true, "[//ctx-scm-affected_files.star:19] \n" + "a.txt: A\n" + - scmStarlarkAddedFiles + + scmStarlarkFiles("A") + "z.txt: A\n" + "\n", }, @@ -276,7 +338,7 @@ func TestRun_SCM_Git_NoUpstream_Pristine(t *testing.T) { false, "[//ctx-scm-all_files.star:19] \n" + "a.txt: A\n" + - scmStarlarkAddedFiles + + scmStarlarkFiles("A") + "z.txt: A\n" + "\n", }, @@ -310,7 +372,7 @@ func TestRun_SCM_Git_NoUpstream_Staged(t *testing.T) { "ctx-scm-affected_files.star", false, "[//ctx-scm-affected_files.star:19] \n" + - scmStarlarkAddedFiles + + scmStarlarkFiles("A") + "\n", }, { @@ -330,7 +392,7 @@ func TestRun_SCM_Git_NoUpstream_Staged(t *testing.T) { false, "[//ctx-scm-all_files.star:19] \n" + "a.txt: A\n" + - scmStarlarkAddedFiles + + scmStarlarkFiles("A") + "z.txt: A\n" + "\n", }, @@ -366,7 +428,7 @@ func TestRun_SCM_Git_Upstream_Staged(t *testing.T) { "ctx-scm-affected_files.star", "[//ctx-scm-affected_files.star:19] \n" + "a.txt: R\n" + - scmStarlarkAddedFiles + + scmStarlarkFiles("A") + "z.txt: A\n" + "\n", }, @@ -374,7 +436,7 @@ func TestRun_SCM_Git_Upstream_Staged(t *testing.T) { "ctx-scm-all_files.star", "[//ctx-scm-all_files.star:19] \n" + "a.txt: A\n" + - scmStarlarkAddedFiles + + scmStarlarkFiles("A") + "z.txt: A\n" + "\n", }, @@ -484,7 +546,7 @@ func TestRun_SCM_Git_Submodule(t *testing.T) { "ctx-scm-affected_files.star", "[//ctx-scm-affected_files.star:19] \n" + ".gitmodules: A\n" + - scmStarlarkAddedFiles + + scmStarlarkFiles("A") + "\n", }, { @@ -492,7 +554,7 @@ func TestRun_SCM_Git_Submodule(t *testing.T) { "[//ctx-scm-all_files.star:19] \n" + ".gitmodules: A\n" + "a.txt: A\n" + - scmStarlarkAddedFiles + + scmStarlarkFiles("A") + "z.txt: A\n" + "\n", }, @@ -542,7 +604,7 @@ func TestRun_SCM_DeletedFile(t *testing.T) { "ctx-scm-all_files.star", "[//ctx-scm-all_files.star:19] \n" + "a.txt: A\n" + - scmStarlarkAddedFiles + + scmStarlarkFiles("A") + "z.txt: A\n" + "\n", }, @@ -551,13 +613,13 @@ func TestRun_SCM_DeletedFile(t *testing.T) { "[//ctx-scm-all_files-include_deleted.star:26] \n" + "With deleted:\n" + "a.txt: A\n" + - scmStarlarkAddedFiles + + scmStarlarkFiles("A") + "file-to-delete.txt: D\n" + "z.txt: A\n" + "\n" + "Without deleted:\n" + "a.txt: A\n" + - scmStarlarkAddedFiles + + scmStarlarkFiles("A") + "z.txt: A\n" + "\n"}, } diff --git a/internal/engine/runtime_ctx_scm.go b/internal/engine/runtime_ctx_scm.go index 7f637e0..58edb9a 100644 --- a/internal/engine/runtime_ctx_scm.go +++ b/internal/engine/runtime_ctx_scm.go @@ -129,11 +129,20 @@ type scmCheckout interface { } type filteredSCM struct { + // files lists specific files that should be force-included. + files []string matcher gitignore.Matcher scm scmCheckout } func (f *filteredSCM) affectedFiles(ctx context.Context, includeDeleted bool) ([]file, error) { + if len(f.files) > 0 { + var res []file + for _, path := range f.files { + res = append(res, &fileImpl{path: filepath.ToSlash(path)}) + } + return res, nil + } files, err := f.scm.affectedFiles(ctx, includeDeleted) return f.filter(files), err }