From 0ea9433e021f66e64e938f47ad9ae11329c7c556 Mon Sep 17 00:00:00 2001 From: Valentin Kiselev Date: Tue, 9 Apr 2024 10:50:56 +0300 Subject: [PATCH] fix: add batching to implicit commands (#695) * fix: add batching to implicit commands * chore: move limits to a separate file * chore: apply the similar struct-interface model for skip setting command executor as in git --- internal/config/command.go | 5 +- internal/config/command_executor.go | 33 +++++++ internal/config/exec.go | 35 -------- internal/config/hook.go | 5 +- internal/config/script.go | 5 +- internal/config/skip_checker.go | 25 +++--- internal/config/skip_checker_test.go | 11 ++- internal/git/command_executor.go | 99 +++++++++++++++++++++ internal/git/exec.go | 69 -------------- internal/git/repository.go | 19 ++-- internal/git/repository_test.go | 21 ++--- internal/lefthook/lefthook.go | 3 +- internal/lefthook/run_test.go | 10 +-- internal/lefthook/runner/prepare_command.go | 20 +---- internal/lefthook/runner/runner_test.go | 26 ++---- internal/system/limits.go | 23 +++++ internal/system/system.go | 33 +++++++ 17 files changed, 244 insertions(+), 198 deletions(-) create mode 100644 internal/config/command_executor.go delete mode 100644 internal/config/exec.go create mode 100644 internal/git/command_executor.go delete mode 100644 internal/git/exec.go create mode 100644 internal/system/limits.go create mode 100644 internal/system/system.go diff --git a/internal/config/command.go b/internal/config/command.go index dfdc6c07..9caf44a0 100644 --- a/internal/config/command.go +++ b/internal/config/command.go @@ -7,6 +7,7 @@ import ( "github.com/spf13/viper" "github.com/evilmartians/lefthook/internal/git" + "github.com/evilmartians/lefthook/internal/system" ) var errFilesIncompatible = errors.New("One of your runners contains incompatible file types") @@ -44,8 +45,8 @@ func (c Command) Validate() error { } func (c Command) DoSkip(gitState git.State) bool { - skipChecker := NewSkipChecker(NewOsExec()) - return skipChecker.Check(gitState, c.Skip, c.Only) + skipChecker := NewSkipChecker(system.Executor{}) + return skipChecker.check(gitState, c.Skip, c.Only) } func (c Command) ExecutionPriority() int { diff --git a/internal/config/command_executor.go b/internal/config/command_executor.go new file mode 100644 index 00000000..859c539d --- /dev/null +++ b/internal/config/command_executor.go @@ -0,0 +1,33 @@ +package config + +import ( + "runtime" +) + +// Executor is a general execution interface for implicit commands. +type Executor interface { + Execute(args []string, root string) (string, error) +} + +// commandExecutor implements execution of a skip checks passed in a `run` option. +type commandExecutor struct { + exec Executor +} + +// cmd runs plain string command in a subshell returning the success of it. +func (c *commandExecutor) cmd(commandLine string) bool { + if commandLine == "" { + return false + } + + var args []string + if runtime.GOOS == "windows" { + args = []string{"powershell", "-Command", commandLine} + } else { + args = []string{"sh", "-c", commandLine} + } + + _, err := c.exec.Execute(args, "") + + return err == nil +} diff --git a/internal/config/exec.go b/internal/config/exec.go deleted file mode 100644 index 87971979..00000000 --- a/internal/config/exec.go +++ /dev/null @@ -1,35 +0,0 @@ -package config - -import ( - "os/exec" - "runtime" -) - -type Exec interface { - Cmd(commandLine string) bool -} - -type osExec struct{} - -// NewOsExec returns an object that executes given commands in the OS. -func NewOsExec() Exec { - return &osExec{} -} - -// Cmd runs plain string command. It checks only exit code and returns bool value. -func (o *osExec) Cmd(commandLine string) bool { - if commandLine == "" { - return false - } - - var cmd *exec.Cmd - if runtime.GOOS == "windows" { - cmd = exec.Command("powershell", "-Command", commandLine) - } else { - cmd = exec.Command("sh", "-c", commandLine) - } - - err := cmd.Run() - - return err == nil -} diff --git a/internal/config/hook.go b/internal/config/hook.go index 8103add4..64f065e9 100644 --- a/internal/config/hook.go +++ b/internal/config/hook.go @@ -8,6 +8,7 @@ import ( "github.com/spf13/viper" "github.com/evilmartians/lefthook/internal/git" + "github.com/evilmartians/lefthook/internal/system" ) const CMD = "{cmd}" @@ -43,8 +44,8 @@ func (h *Hook) Validate() error { } func (h *Hook) DoSkip(gitState git.State) bool { - skipChecker := NewSkipChecker(NewOsExec()) - return skipChecker.Check(gitState, h.Skip, h.Only) + skipChecker := NewSkipChecker(system.Executor{}) + return skipChecker.check(gitState, h.Skip, h.Only) } func unmarshalHooks(base, extra *viper.Viper) (*Hook, error) { diff --git a/internal/config/script.go b/internal/config/script.go index fe443ecd..7b2b700e 100644 --- a/internal/config/script.go +++ b/internal/config/script.go @@ -7,6 +7,7 @@ import ( "github.com/spf13/viper" "github.com/evilmartians/lefthook/internal/git" + "github.com/evilmartians/lefthook/internal/system" ) type Script struct { @@ -29,8 +30,8 @@ type scriptRunnerReplace struct { } func (s Script) DoSkip(gitState git.State) bool { - skipChecker := NewSkipChecker(NewOsExec()) - return skipChecker.Check(gitState, s.Skip, s.Only) + skipChecker := NewSkipChecker(system.Executor{}) + return skipChecker.check(gitState, s.Skip, s.Only) } func (s Script) ExecutionPriority() int { diff --git a/internal/config/skip_checker.go b/internal/config/skip_checker.go index 581610f7..009584fe 100644 --- a/internal/config/skip_checker.go +++ b/internal/config/skip_checker.go @@ -7,19 +7,16 @@ import ( "github.com/evilmartians/lefthook/internal/log" ) -type SkipChecker struct { - Executor Exec +type skipChecker struct { + exec *commandExecutor } -func NewSkipChecker(executor Exec) *SkipChecker { - if executor == nil { - executor = NewOsExec() - } - - return &SkipChecker{Executor: executor} +func NewSkipChecker(executor Executor) *skipChecker { + return &skipChecker{&commandExecutor{executor}} } -func (sc *SkipChecker) Check(gitState git.State, skip interface{}, only interface{}) bool { +// check returns the result of applying a skip/only setting which can be a branch, git state, shell command, etc. +func (sc *skipChecker) check(gitState git.State, skip interface{}, only interface{}) bool { if skip != nil { if sc.matches(gitState, skip) { return true @@ -33,7 +30,7 @@ func (sc *SkipChecker) Check(gitState git.State, skip interface{}, only interfac return false } -func (sc *SkipChecker) matches(gitState git.State, value interface{}) bool { +func (sc *skipChecker) matches(gitState git.State, value interface{}) bool { switch typedValue := value.(type) { case bool: return typedValue @@ -45,7 +42,7 @@ func (sc *SkipChecker) matches(gitState git.State, value interface{}) bool { return false } -func (sc *SkipChecker) matchesSlices(gitState git.State, slice []interface{}) bool { +func (sc *skipChecker) matchesSlices(gitState git.State, slice []interface{}) bool { for _, state := range slice { switch typedState := state.(type) { case string: @@ -66,7 +63,7 @@ func (sc *SkipChecker) matchesSlices(gitState git.State, slice []interface{}) bo return false } -func (sc *SkipChecker) matchesRef(gitState git.State, typedState map[string]interface{}) bool { +func (sc *skipChecker) matchesRef(gitState git.State, typedState map[string]interface{}) bool { ref, ok := typedState["ref"].(string) if !ok { return false @@ -81,13 +78,13 @@ func (sc *SkipChecker) matchesRef(gitState git.State, typedState map[string]inte return g.Match(gitState.Branch) } -func (sc *SkipChecker) matchesCommands(typedState map[string]interface{}) bool { +func (sc *skipChecker) matchesCommands(typedState map[string]interface{}) bool { commandLine, ok := typedState["run"].(string) if !ok { return false } - result := sc.Executor.Cmd(commandLine) + result := sc.exec.cmd(commandLine) log.Debugf("[lefthook] skip/only cmd: %s, result: %t", commandLine, result) diff --git a/internal/config/skip_checker_test.go b/internal/config/skip_checker_test.go index 4956859f..addac901 100644 --- a/internal/config/skip_checker_test.go +++ b/internal/config/skip_checker_test.go @@ -1,6 +1,7 @@ package config import ( + "errors" "testing" "github.com/evilmartians/lefthook/internal/git" @@ -8,8 +9,12 @@ import ( type mockExecutor struct{} -func (mc mockExecutor) Cmd(cmd string) bool { - return cmd == "success" +func (mc mockExecutor) Execute(args []string, _root string) (string, error) { + if len(args) == 3 && args[2] == "success" { + return "", nil + } else { + return "", errors.New("failure") + } } func TestDoSkip(t *testing.T) { @@ -139,7 +144,7 @@ func TestDoSkip(t *testing.T) { }, } { t.Run(tt.name, func(t *testing.T) { - if skipChecker.Check(tt.state, tt.skip, tt.only) != tt.skipped { + if skipChecker.check(tt.state, tt.skip, tt.only) != tt.skipped { t.Errorf("Expected: %v, Was %v", tt.skipped, !tt.skipped) } }) diff --git a/internal/git/command_executor.go b/internal/git/command_executor.go new file mode 100644 index 00000000..e71c3d70 --- /dev/null +++ b/internal/git/command_executor.go @@ -0,0 +1,99 @@ +package git + +import ( + "fmt" + "path/filepath" + "strings" + + "github.com/evilmartians/lefthook/internal/system" +) + +// Executor is a general execution interface for implicit commands. +// Added here mostly for mockable tests. +type Executor interface { + Execute(args []string, root string) (string, error) +} + +// CommandExecutor provides some methods that take some effect on execution and/or result data. +type CommandExecutor struct { + exec Executor + root string +} + +// NewExecutor returns an object that executes given commands in the OS. +func NewExecutor(exec Executor) *CommandExecutor { + return &CommandExecutor{exec: exec} +} + +// Cmd runs plain string command. Trims spaces around output. +func (c CommandExecutor) Cmd(args []string) (string, error) { + out, err := c.exec.Execute(args, c.root) + if err != nil { + return "", err + } + + return strings.TrimSpace(out), nil +} + +// BatchedCmd runs the command with any number of appended arguments batched in chunks to match the OS limits. +func (c CommandExecutor) BatchedCmd(cmd []string, args []string) (string, error) { + maxlen := system.MaxCmdLen() + result := strings.Builder{} + + argsBatched := batchByLength(args, maxlen-len(cmd)) + for i, batch := range argsBatched { + out, err := c.Cmd(append(cmd, batch...)) + if err != nil { + return "", fmt.Errorf("error in batch %d: %w", i, err) + } + result.WriteString(out) + result.WriteString("\n") + } + + return result.String(), nil +} + +// CmdLines runs plain string command, returns its output split by newline. +func (c CommandExecutor) CmdLines(cmd []string) ([]string, error) { + out, err := c.exec.Execute(cmd, c.root) + if err != nil { + return nil, err + } + + return strings.Split(strings.TrimSpace(out), "\n"), nil +} + +// CmdLines runs plain string command, returns its output split by newline. +func (c CommandExecutor) CmdLinesWithinFolder(cmd []string, folder string) ([]string, error) { + root := filepath.Join(c.root, folder) + out, err := c.exec.Execute(cmd, root) + if err != nil { + return nil, err + } + + return strings.Split(strings.TrimSpace(out), "\n"), nil +} + +func batchByLength(s []string, length int) [][]string { + batches := make([][]string, 0) + + var acc, prev int + for i := range s { + acc += len(s[i]) + if acc > length { + if i == prev { + batches = append(batches, s[prev:i+1]) + prev = i + 1 + } else { + batches = append(batches, s[prev:i]) + prev = i + } + acc = len(s[i]) + } + } + if acc > 0 { + batches = append(batches, s[prev:]) + } + + return batches +} diff --git a/internal/git/exec.go b/internal/git/exec.go deleted file mode 100644 index 91ae864e..00000000 --- a/internal/git/exec.go +++ /dev/null @@ -1,69 +0,0 @@ -package git - -import ( - "os" - "os/exec" - "strings" - - "github.com/evilmartians/lefthook/internal/log" -) - -type Exec interface { - SetRootPath(root string) - Cmd(cmd []string) (string, error) - CmdLines(cmd []string) ([]string, error) -} - -type OsExec struct { - root string -} - -// NewOsExec returns an object that executes given commands -// in the OS. -func NewOsExec() *OsExec { - return &OsExec{} -} - -func (o *OsExec) SetRootPath(root string) { - o.root = root -} - -// Cmd runs plain string command. Trims spaces around output. -func (o *OsExec) Cmd(cmd []string) (string, error) { - out, err := o.rawExecArgs(cmd) - if err != nil { - return "", err - } - - return strings.TrimSpace(out), nil -} - -// CmdLines runs plain string command, returns its output split by newline. -func (o *OsExec) CmdLines(cmd []string) ([]string, error) { - out, err := o.rawExecArgs(cmd) - if err != nil { - return nil, err - } - - return strings.Split(strings.TrimSpace(out), "\n"), nil -} - -// rawExecArgs executes git command with LEFTHOOK=0 in order -// to prevent calling subsequent lefthook hooks. -func (o *OsExec) rawExecArgs(args []string) (string, error) { - log.Debug("[lefthook] cmd: ", args) - - cmd := exec.Command(args[0], args[1:]...) - cmd.Dir = o.root - cmd.Env = append(os.Environ(), "LEFTHOOK=0") - - out, err := cmd.CombinedOutput() - log.Debug("[lefthook] dir: ", o.root) - log.Debug("[lefthook] err: ", err) - log.Debug("[lefthook] out: ", string(out)) - if err != nil { - return "", err - } - - return string(out), nil -} diff --git a/internal/git/repository.go b/internal/git/repository.go index aa92adac..cde27ae1 100644 --- a/internal/git/repository.go +++ b/internal/git/repository.go @@ -41,7 +41,7 @@ var ( // Repository represents a git repository. type Repository struct { Fs afero.Fs - Git Exec + Git *CommandExecutor HooksPath string RootPath string GitPath string @@ -52,7 +52,7 @@ type Repository struct { } // NewRepository returns a Repository or an error, if git repository it not initialized. -func NewRepository(fs afero.Fs, git Exec) (*Repository, error) { +func NewRepository(fs afero.Fs, git *CommandExecutor) (*Repository, error) { rootPath, err := git.Cmd(cmdRootPath) if err != nil { return nil, err @@ -91,7 +91,7 @@ func NewRepository(fs afero.Fs, git Exec) (*Repository, error) { log.Debug("Couldn't get empty tree SHA value, not critical") } - git.SetRootPath(rootPath) + git.root = rootPath return &Repository{ Fs: fs, @@ -184,8 +184,8 @@ func (r *Repository) PartiallyStagedFiles() ([]string, error) { } func (r *Repository) SaveUnstaged(files []string) error { - _, err := r.Git.Cmd( - append([]string{ + _, err := r.Git.BatchedCmd( + []string{ "git", "diff", "--binary", // support binary files @@ -199,14 +199,13 @@ func (r *Repository) SaveUnstaged(files []string) error { "--output", r.unstagedPatchPath, "--", - }, files...), - ) + }, files) return err } func (r *Repository) HideUnstaged(files []string) error { - _, err := r.Git.Cmd(append(cmdHideUnstaged, files...)) + _, err := r.Git.BatchedCmd(cmdHideUnstaged, files) return err } @@ -311,9 +310,7 @@ func (r *Repository) AddFiles(files []string) error { return nil } - _, err := r.Git.Cmd( - append(cmdStageFiles, files...), - ) + _, err := r.Git.BatchedCmd(cmdStageFiles, files) return err } diff --git a/internal/git/repository_test.go b/internal/git/repository_test.go index ea2be416..44980422 100644 --- a/internal/git/repository_test.go +++ b/internal/git/repository_test.go @@ -11,9 +11,7 @@ type GitMock struct { cases map[string]string } -func (g GitMock) SetRootPath(_root string) {} - -func (g GitMock) Cmd(cmd []string) (string, error) { +func (g GitMock) Execute(cmd []string, _root string) (string, error) { res, ok := g.cases[(strings.Join(cmd, " "))] if !ok { return "", errors.New("doesn't exist") @@ -22,15 +20,6 @@ func (g GitMock) Cmd(cmd []string) (string, error) { return strings.TrimSpace(res), nil } -func (g GitMock) CmdLines(cmd []string) ([]string, error) { - res, err := g.Cmd(cmd) - if err != nil { - return nil, err - } - - return strings.Split(res, "\n"), nil -} - func TestPartiallyStagedFiles(t *testing.T) { for i, tt := range [...]struct { name, gitOut string @@ -47,9 +36,11 @@ MM staged but changed } { t.Run(fmt.Sprintf("%d: %s", i, tt.name), func(t *testing.T) { repository := &Repository{ - Git: GitMock{ - cases: map[string]string{ - "git status --short": tt.gitOut, + Git: &CommandExecutor{ + exec: GitMock{ + cases: map[string]string{ + "git status --short": tt.gitOut, + }, }, }, } diff --git a/internal/lefthook/lefthook.go b/internal/lefthook/lefthook.go index ae1e1563..e688231a 100644 --- a/internal/lefthook/lefthook.go +++ b/internal/lefthook/lefthook.go @@ -11,6 +11,7 @@ import ( "github.com/evilmartians/lefthook/internal/git" "github.com/evilmartians/lefthook/internal/log" + "github.com/evilmartians/lefthook/internal/system" "github.com/evilmartians/lefthook/internal/templates" ) @@ -50,7 +51,7 @@ func initialize(opts *Options) (*Lefthook, error) { log.SetColors(!opts.NoColors) - repo, err := git.NewRepository(opts.Fs, git.NewOsExec()) + repo, err := git.NewRepository(opts.Fs, git.NewExecutor(system.Executor{})) if err != nil { return nil, err } diff --git a/internal/lefthook/run_test.go b/internal/lefthook/run_test.go index ac6b7f1b..b605cefc 100644 --- a/internal/lefthook/run_test.go +++ b/internal/lefthook/run_test.go @@ -13,16 +13,10 @@ import ( type GitMock struct{} -func (g GitMock) SetRootPath(_root string) {} - -func (g GitMock) Cmd(_cmd []string) (string, error) { +func (g GitMock) Execute(_cmd []string, root string) (string, error) { return "", nil } -func (g GitMock) CmdLines(_cmd []string) ([]string, error) { - return nil, nil -} - func TestRun(t *testing.T) { root, err := filepath.Abs("src") if err != nil { @@ -163,7 +157,7 @@ post-commit: Options: &Options{Fs: fs}, repo: &git.Repository{ Fs: fs, - Git: GitMock{}, + Git: git.NewExecutor(GitMock{}), HooksPath: hooksPath, RootPath: root, GitPath: gitPath, diff --git a/internal/lefthook/runner/prepare_command.go b/internal/lefthook/runner/prepare_command.go index f021a37a..288b07bb 100644 --- a/internal/lefthook/runner/prepare_command.go +++ b/internal/lefthook/runner/prepare_command.go @@ -10,6 +10,7 @@ import ( "github.com/evilmartians/lefthook/internal/config" "github.com/evilmartians/lefthook/internal/lefthook/runner/filter" "github.com/evilmartians/lefthook/internal/log" + "github.com/evilmartians/lefthook/internal/system" ) // An object that describes the single command's run option. @@ -24,15 +25,6 @@ type template struct { cnt int } -const ( - // https://serverfault.com/questions/69430/what-is-the-maximum-length-of-a-command-line-in-mac-os-x - // https://support.microsoft.com/en-us/help/830473/command-prompt-cmd-exe-command-line-string-limitation - // https://unix.stackexchange.com/a/120652 - maxCommandLengthDarwin = 260000 // 262144 - maxCommandLengthWindows = 7000 // 8191, but see issues#655 - maxCommandLengthLinux = 130000 // 131072 -) - func (r *Runner) prepareCommand(name string, command *config.Command) (*run, error) { if command.DoSkip(r.Repo.State()) { return nil, &skipError{"settings"} @@ -142,15 +134,7 @@ func (r *Runner) buildRun(command *config.Command) (*run, error) { runString := command.Run runString = replacePositionalArguments(runString, r.GitArgs) - var maxlen int - switch runtime.GOOS { - case "windows": - maxlen = maxCommandLengthWindows - case "darwin": - maxlen = maxCommandLengthDarwin - default: - maxlen = maxCommandLengthLinux - } + maxlen := system.MaxCmdLen() result := replaceInChunks(runString, templates, maxlen) if r.Force || len(result.files) != 0 { diff --git a/internal/lefthook/runner/runner_test.go b/internal/lefthook/runner/runner_test.go index 0c427754..8536de55 100644 --- a/internal/lefthook/runner/runner_test.go +++ b/internal/lefthook/runner/runner_test.go @@ -40,32 +40,22 @@ type GitMock struct { commands []string } -func (g *GitMock) SetRootPath(_root string) {} - -func (g *GitMock) Cmd(cmd []string) (string, error) { +func (g *GitMock) Execute(cmd []string, _root string) (string, error) { g.mux.Lock() g.commands = append(g.commands, strings.Join(cmd, " ")) g.mux.Unlock() - return "", nil -} - -func (g *GitMock) CmdLines(args []string) ([]string, error) { - cmd := strings.Join(args, " ") - g.mux.Lock() - g.commands = append(g.commands, cmd) - g.mux.Unlock() - - if cmd == "git diff --name-only --cached --diff-filter=ACMR" || - cmd == "git diff --name-only HEAD @{push}" { + cmdLine := strings.Join(cmd, " ") + if cmdLine == "git diff --name-only --cached --diff-filter=ACMR" || + cmdLine == "git diff --name-only HEAD @{push}" { root, _ := filepath.Abs("src") - return []string{ + return strings.Join([]string{ filepath.Join(root, "scripts", "script.sh"), filepath.Join(root, "README.md"), - }, nil + }, "\n"), nil } - return nil, nil + return "", nil } func (g *GitMock) reset() { @@ -83,7 +73,7 @@ func TestRunAll(t *testing.T) { gitExec := &GitMock{} gitPath := filepath.Join(root, ".git") repo := &git.Repository{ - Git: gitExec, + Git: git.NewExecutor(gitExec), HooksPath: filepath.Join(gitPath, "hooks"), RootPath: root, GitPath: gitPath, diff --git a/internal/system/limits.go b/internal/system/limits.go new file mode 100644 index 00000000..ae60f0c1 --- /dev/null +++ b/internal/system/limits.go @@ -0,0 +1,23 @@ +package system + +import "runtime" + +const ( + // https://serverfault.com/questions/69430/what-is-the-maximum-length-of-a-command-line-in-mac-os-x + // https://support.microsoft.com/en-us/help/830473/command-prompt-cmd-exe-command-line-string-limitation + // https://unix.stackexchange.com/a/120652 + maxCommandLengthDarwin = 260000 // 262144 + maxCommandLengthWindows = 7000 // 8191, but see issues#655 + maxCommandLengthLinux = 130000 // 131072 +) + +func MaxCmdLen() int { + switch runtime.GOOS { + case "windows": + return maxCommandLengthWindows + case "darwin": + return maxCommandLengthDarwin + default: + return maxCommandLengthLinux + } +} diff --git a/internal/system/system.go b/internal/system/system.go new file mode 100644 index 00000000..892ecaed --- /dev/null +++ b/internal/system/system.go @@ -0,0 +1,33 @@ +// system package contains OS-specific implementations. +package system + +import ( + "os" + "os/exec" + + "github.com/evilmartians/lefthook/internal/log" +) + +type Executor struct{} + +// Execute executes git command with LEFTHOOK=0 in order +// to prevent calling subsequent lefthook hooks. +func (e Executor) Execute(args []string, root string) (string, error) { + log.Debug("[lefthook] cmd: ", args) + + cmd := exec.Command(args[0], args[1:]...) + cmd.Env = append(os.Environ(), "LEFTHOOK=0") + if len(root) > 0 { + cmd.Dir = root + } + + out, err := cmd.CombinedOutput() + log.Debug("[lefthook] dir: ", root) + log.Debug("[lefthook] err: ", err) + log.Debug("[lefthook] out: ", string(out)) + if err != nil { + return "", err + } + + return string(out), nil +}