From d78da1a507a5ded833de17424688ba467b731bf8 Mon Sep 17 00:00:00 2001 From: Thomas Desveaux Date: Mon, 27 May 2024 17:31:44 +0000 Subject: [PATCH 1/7] fix: error and bail if multiple commands useStdin Since Stdin from Git is not intercepted and forwared to each commands/scripts, this could lead to hang. --- internal/git/lfs.go | 16 ++++++++++ internal/lefthook/runner/runner.go | 48 ++++++++++++++++++++++++------ 2 files changed, 55 insertions(+), 9 deletions(-) diff --git a/internal/git/lfs.go b/internal/git/lfs.go index 142e8bf8..6beb2e51 100644 --- a/internal/git/lfs.go +++ b/internal/git/lfs.go @@ -16,6 +16,10 @@ var lfsHooks = [...]string{ "pre-push", } +var lfsHookConsumeStdin = [...]string{ + "pre-push", +} + // IsLFSAvailable returns 'true' if git-lfs is installed. func IsLFSAvailable() bool { _, err := exec.LookPath("git-lfs") @@ -33,3 +37,15 @@ func IsLFSHook(hookName string) bool { return false } + +// DoesLFSHookConsumeStdin returns whether the LFS hookName will consume Stdin +// meaning it won't be available to following commands +func DoesLFSHookConsumeStdin(hookName string) bool { + for _, lfsHookName := range lfsHookConsumeStdin { + if lfsHookName == hookName { + return true + } + } + + return false +} diff --git a/internal/lefthook/runner/runner.go b/internal/lefthook/runner/runner.go index 80c1467c..a80b2d8c 100644 --- a/internal/lefthook/runner/runner.go +++ b/internal/lefthook/runner/runner.go @@ -82,7 +82,8 @@ type executable interface { func (r *Runner) RunAll(ctx context.Context, sourceDirs []string) []Result { results := make([]Result, 0, len(r.Hook.Commands)+len(r.Hook.Scripts)) - if err := r.runLFSHook(ctx); err != nil { + ranLFS, err := r.runLFSHook(ctx) + if err != nil { log.Error(err) } @@ -91,6 +92,34 @@ func (r *Runner) RunAll(ctx context.Context, sourceDirs []string) []Result { return results } + // Sanity check before running hooks + // More than one hook can't use Stdin + StdinConsumed := ranLFS && git.DoesLFSHookConsumeStdin(r.HookName) + for commandName, command := range r.Hook.Commands { + if command.UseStdin { + if StdinConsumed { + log.Errorf( + "command %s wants to use Stdin which was consumed by a previously.", + commandName, + ) + return results + } + StdinConsumed = true + } + } + for scriptName, script := range r.Hook.Scripts { + if script.UseStdin { + if StdinConsumed { + log.Errorf( + "script %s wants to use Stdin which was consumed by a previously.", + scriptName, + ) + return results + } + StdinConsumed = true + } + } + if !r.DisableTTY && !r.Hook.Follow { log.StartSpinner() defer log.StopSpinner() @@ -116,9 +145,10 @@ func (r *Runner) RunAll(ctx context.Context, sourceDirs []string) []Result { return results } -func (r *Runner) runLFSHook(ctx context.Context) error { +// returns whether it ran a LFS hook +func (r *Runner) runLFSHook(ctx context.Context) (bool, error) { if !git.IsLFSHook(r.HookName) { - return nil + return false, nil } lfsRequiredFile := filepath.Join(r.Repo.RootPath, git.LFSRequiredFile) @@ -126,11 +156,11 @@ func (r *Runner) runLFSHook(ctx context.Context) error { requiredExists, err := afero.Exists(r.Repo.Fs, lfsRequiredFile) if err != nil { - return err + return false, err } configExists, err := afero.Exists(r.Repo.Fs, lfsConfigFile) if err != nil { - return err + return false, err } if git.IsLFSAvailable() { @@ -161,10 +191,10 @@ func (r *Runner) runLFSHook(ctx context.Context) error { if err != nil && (requiredExists || configExists) { log.Warnf("git-lfs command failed: %s\n", output) - return err + return true, err } - return nil + return true, nil } if requiredExists || configExists { @@ -175,10 +205,10 @@ func (r *Runner) runLFSHook(ctx context.Context) error { " - %s\n", lfsRequiredFile, lfsConfigFile, ) - return errors.New("git-lfs is required") + return false, errors.New("git-lfs is required") } - return nil + return false, nil } func (r *Runner) preHook() { From d36d3a5113472fe8b041df4ee4ca73b2d6d9346d Mon Sep 17 00:00:00 2001 From: Thomas Desveaux Date: Mon, 27 May 2024 17:40:03 +0000 Subject: [PATCH 2/7] fix: CommandExecutor.RawExecute forward os.Stdin This is only used by LFS pre-push Hook which require Stdin. --- internal/git/lfs.go | 2 +- internal/lefthook/runner/exec/execute_unix.go | 6 +++++- internal/lefthook/runner/exec/execute_windows.go | 6 +++++- internal/lefthook/runner/exec/executor.go | 2 +- internal/lefthook/runner/runner.go | 3 ++- internal/lefthook/runner/runner_test.go | 2 +- 6 files changed, 15 insertions(+), 6 deletions(-) diff --git a/internal/git/lfs.go b/internal/git/lfs.go index 6beb2e51..d74109c2 100644 --- a/internal/git/lfs.go +++ b/internal/git/lfs.go @@ -39,7 +39,7 @@ func IsLFSHook(hookName string) bool { } // DoesLFSHookConsumeStdin returns whether the LFS hookName will consume Stdin -// meaning it won't be available to following commands +// meaning it won't be available to following commands. func DoesLFSHookConsumeStdin(hookName string) bool { for _, lfsHookName := range lfsHookConsumeStdin { if lfsHookName == hookName { diff --git a/internal/lefthook/runner/exec/execute_unix.go b/internal/lefthook/runner/exec/execute_unix.go index 9958e8b7..fd447329 100644 --- a/internal/lefthook/runner/exec/execute_unix.go +++ b/internal/lefthook/runner/exec/execute_unix.go @@ -72,12 +72,16 @@ func (e CommandExecutor) Execute(ctx context.Context, opts Options, out io.Write return nil } -func (e CommandExecutor) RawExecute(ctx context.Context, command []string, out io.Writer) error { +func (e CommandExecutor) RawExecute(ctx context.Context, command []string, out io.Writer, forwardStdin bool) error { cmd := exec.CommandContext(ctx, command[0], command[1:]...) cmd.Stdout = out cmd.Stderr = os.Stderr + if forwardStdin { + cmd.Stdin = os.Stdin + } + return cmd.Run() } diff --git a/internal/lefthook/runner/exec/execute_windows.go b/internal/lefthook/runner/exec/execute_windows.go index e911121b..5c2dabae 100644 --- a/internal/lefthook/runner/exec/execute_windows.go +++ b/internal/lefthook/runner/exec/execute_windows.go @@ -63,12 +63,16 @@ func (e CommandExecutor) Execute(ctx context.Context, opts Options, out io.Write return nil } -func (e CommandExecutor) RawExecute(ctx context.Context, command []string, out io.Writer) error { +func (e CommandExecutor) RawExecute(ctx context.Context, command []string, out io.Writer, forwardStdin bool) error { cmd := exec.Command(command[0], command[1:]...) cmd.Stdout = out cmd.Stderr = os.Stderr + if forwardStdin { + cmd.Stdin = os.Stdin + } + return cmd.Run() } diff --git a/internal/lefthook/runner/exec/executor.go b/internal/lefthook/runner/exec/executor.go index d4e67628..dff0f5d8 100644 --- a/internal/lefthook/runner/exec/executor.go +++ b/internal/lefthook/runner/exec/executor.go @@ -17,5 +17,5 @@ type Options struct { // It is used here for testing purpose mostly. type Executor interface { Execute(ctx context.Context, opts Options, out io.Writer) error - RawExecute(ctx context.Context, command []string, out io.Writer) error + RawExecute(ctx context.Context, command []string, out io.Writer, forwardStdin bool) error } diff --git a/internal/lefthook/runner/runner.go b/internal/lefthook/runner/runner.go index a80b2d8c..27192728 100644 --- a/internal/lefthook/runner/runner.go +++ b/internal/lefthook/runner/runner.go @@ -145,7 +145,7 @@ func (r *Runner) RunAll(ctx context.Context, sourceDirs []string) []Result { return results } -// returns whether it ran a LFS hook +// returns whether it ran a LFS hook. func (r *Runner) runLFSHook(ctx context.Context) (bool, error) { if !git.IsLFSHook(r.HookName) { return false, nil @@ -175,6 +175,7 @@ func (r *Runner) runLFSHook(ctx context.Context) (bool, error) { r.GitArgs..., ), out, + git.DoesLFSHookConsumeStdin(r.HookName), ) output := strings.Trim(out.String(), "\n") diff --git a/internal/lefthook/runner/runner_test.go b/internal/lefthook/runner/runner_test.go index 8536de55..780ad77a 100644 --- a/internal/lefthook/runner/runner_test.go +++ b/internal/lefthook/runner/runner_test.go @@ -31,7 +31,7 @@ func (e TestExecutor) Execute(_ctx context.Context, opts exec.Options, _out io.W return } -func (e TestExecutor) RawExecute(_ctx context.Context, _command []string, _out io.Writer) error { +func (e TestExecutor) RawExecute(_ctx context.Context, _command []string, _out io.Writer, forwardStdin bool) error { return nil } From f2e494b051581e28f99d0480335ea1b6c09b28b7 Mon Sep 17 00:00:00 2001 From: Valentin Kiselev Date: Wed, 29 May 2024 11:00:07 +0300 Subject: [PATCH 3/7] fix: use cached reader for stdin when use_stdin is specified --- internal/lefthook/runner/cached_reader.go | 49 ++++++++++++++ .../lefthook/runner/cached_reader_test.go | 24 +++++++ internal/lefthook/runner/exec/execute_unix.go | 13 +--- .../lefthook/runner/exec/execute_windows.go | 13 +--- internal/lefthook/runner/exec/executor.go | 4 +- internal/lefthook/runner/exec/nullReader.go | 10 --- internal/lefthook/runner/null_reader.go | 15 +++++ internal/lefthook/runner/null_reader_test.go | 20 ++++++ internal/lefthook/runner/runner.go | 67 +++++++------------ internal/lefthook/runner/runner_test.go | 4 +- 10 files changed, 144 insertions(+), 75 deletions(-) create mode 100644 internal/lefthook/runner/cached_reader.go create mode 100644 internal/lefthook/runner/cached_reader_test.go delete mode 100644 internal/lefthook/runner/exec/nullReader.go create mode 100644 internal/lefthook/runner/null_reader.go create mode 100644 internal/lefthook/runner/null_reader_test.go diff --git a/internal/lefthook/runner/cached_reader.go b/internal/lefthook/runner/cached_reader.go new file mode 100644 index 00000000..124fa042 --- /dev/null +++ b/internal/lefthook/runner/cached_reader.go @@ -0,0 +1,49 @@ +package runner + +import ( + "bytes" + "io" +) + +// cachedReader reads from the provided `io.Reader` until `io.EOF` and saves +// the read content into the inner buffer. +// +// After `io.EOF` it will provide the read data again and again. +type cachedReader struct { + in io.Reader + useBuffer bool + buf []byte + reader *bytes.Reader +} + +func NewCachedReader(in io.Reader) *cachedReader { + return &cachedReader{ + in: in, + buf: []byte{}, + reader: bytes.NewReader([]byte{}), + } +} + +func (r *cachedReader) Read(p []byte) (int, error) { + if r.useBuffer { + n, err := r.reader.Read(p) + if err == io.EOF { + _, seekErr := r.reader.Seek(0, io.SeekStart) + if seekErr != nil { + panic(seekErr) + } + + return n, err + } + + return n, err + } + + n, err := r.in.Read(p) + r.buf = append(r.buf, p[:n]...) + if err == io.EOF { + r.useBuffer = true + r.reader = bytes.NewReader(r.buf) + } + return n, err +} diff --git a/internal/lefthook/runner/cached_reader_test.go b/internal/lefthook/runner/cached_reader_test.go new file mode 100644 index 00000000..7c6d3056 --- /dev/null +++ b/internal/lefthook/runner/cached_reader_test.go @@ -0,0 +1,24 @@ +package runner + +import ( + "bytes" + "io" + "testing" +) + +func TestCachedReader(t *testing.T) { + testSlice := []byte("Some example string\nMultiline") + + cachedReader := NewCachedReader(bytes.NewReader(testSlice)) + + for range 5 { + res, err := io.ReadAll(cachedReader) + if err != nil { + t.Errorf("unexpected err: %s", err) + } + + if !bytes.Equal(res, testSlice) { + t.Errorf("expected %v to be equal to %v", res, testSlice) + } + } +} diff --git a/internal/lefthook/runner/exec/execute_unix.go b/internal/lefthook/runner/exec/execute_unix.go index fd447329..874aa169 100644 --- a/internal/lefthook/runner/exec/execute_unix.go +++ b/internal/lefthook/runner/exec/execute_unix.go @@ -28,11 +28,7 @@ type executeArgs struct { interactive, useStdin bool } -func (e CommandExecutor) Execute(ctx context.Context, opts Options, out io.Writer) error { - var in io.Reader = nullReader{} - if opts.UseStdin { - in = os.Stdin - } +func (e CommandExecutor) Execute(ctx context.Context, opts Options, in io.Reader, out io.Writer) error { if opts.Interactive && !isatty.IsTerminal(os.Stdin.Fd()) { tty, err := os.Open("/dev/tty") if err == nil { @@ -72,16 +68,13 @@ func (e CommandExecutor) Execute(ctx context.Context, opts Options, out io.Write return nil } -func (e CommandExecutor) RawExecute(ctx context.Context, command []string, out io.Writer, forwardStdin bool) error { +func (e CommandExecutor) RawExecute(ctx context.Context, command []string, in io.Reader, out io.Writer) error { cmd := exec.CommandContext(ctx, command[0], command[1:]...) + cmd.Stdin = in cmd.Stdout = out cmd.Stderr = os.Stderr - if forwardStdin { - cmd.Stdin = os.Stdin - } - return cmd.Run() } diff --git a/internal/lefthook/runner/exec/execute_windows.go b/internal/lefthook/runner/exec/execute_windows.go index 5c2dabae..9e34a21f 100644 --- a/internal/lefthook/runner/exec/execute_windows.go +++ b/internal/lefthook/runner/exec/execute_windows.go @@ -23,11 +23,7 @@ type executeArgs struct { root string } -func (e CommandExecutor) Execute(ctx context.Context, opts Options, out io.Writer) error { - var in io.Reader = nullReader{} - if opts.UseStdin { - in = os.Stdin - } +func (e CommandExecutor) Execute(ctx context.Context, opts Options, in io.Reader, out io.Writer) error { if opts.Interactive && !isatty.IsTerminal(os.Stdin.Fd()) { tty, err := tty.Open() if err == nil { @@ -63,16 +59,13 @@ func (e CommandExecutor) Execute(ctx context.Context, opts Options, out io.Write return nil } -func (e CommandExecutor) RawExecute(ctx context.Context, command []string, out io.Writer, forwardStdin bool) error { +func (e CommandExecutor) RawExecute(ctx context.Context, command []string, in io.Reader, out io.Writer) error { cmd := exec.Command(command[0], command[1:]...) + cmd.Stdin = in cmd.Stdout = out cmd.Stderr = os.Stderr - if forwardStdin { - cmd.Stdin = os.Stdin - } - return cmd.Run() } diff --git a/internal/lefthook/runner/exec/executor.go b/internal/lefthook/runner/exec/executor.go index dff0f5d8..12ec9a39 100644 --- a/internal/lefthook/runner/exec/executor.go +++ b/internal/lefthook/runner/exec/executor.go @@ -16,6 +16,6 @@ type Options struct { // Executor provides an interface for command execution. // It is used here for testing purpose mostly. type Executor interface { - Execute(ctx context.Context, opts Options, out io.Writer) error - RawExecute(ctx context.Context, command []string, out io.Writer, forwardStdin bool) error + Execute(ctx context.Context, opts Options, in io.Reader, out io.Writer) error + RawExecute(ctx context.Context, command []string, in io.Reader, out io.Writer) error } diff --git a/internal/lefthook/runner/exec/nullReader.go b/internal/lefthook/runner/exec/nullReader.go deleted file mode 100644 index dd774b5e..00000000 --- a/internal/lefthook/runner/exec/nullReader.go +++ /dev/null @@ -1,10 +0,0 @@ -package exec - -import "io" - -// nullReader always returns EOF. -type nullReader struct{} - -func (nullReader) Read(b []byte) (int, error) { - return 0, io.EOF -} diff --git a/internal/lefthook/runner/null_reader.go b/internal/lefthook/runner/null_reader.go new file mode 100644 index 00000000..caf5a568 --- /dev/null +++ b/internal/lefthook/runner/null_reader.go @@ -0,0 +1,15 @@ +package runner + +import "io" + +// nullReader always returns `io.EOF`. +type nullReader struct{} + +func NewNullReader() io.Reader { + return nullReader{} +} + +// Implements io.Reader interface. +func (nullReader) Read(b []byte) (int, error) { + return 0, io.EOF +} diff --git a/internal/lefthook/runner/null_reader_test.go b/internal/lefthook/runner/null_reader_test.go new file mode 100644 index 00000000..df4fd06e --- /dev/null +++ b/internal/lefthook/runner/null_reader_test.go @@ -0,0 +1,20 @@ +package runner + +import ( + "bytes" + "io" + "testing" +) + +func TestNullReader(t *testing.T) { + nullReader := NewNullReader() + + res, err := io.ReadAll(nullReader) + if err != nil { + t.Errorf("unexpected err: %s", err) + } + + if !bytes.Equal(res, []byte{}) { + t.Errorf("expected %v to be equal to %v", res, []byte{}) + } +} diff --git a/internal/lefthook/runner/runner.go b/internal/lefthook/runner/runner.go index 27192728..dd0d50e6 100644 --- a/internal/lefthook/runner/runner.go +++ b/internal/lefthook/runner/runner.go @@ -51,6 +51,7 @@ type Options struct { type Runner struct { Options + stdin io.Reader partiallyStagedFiles []string failed atomic.Bool executor exec.Executor @@ -58,7 +59,11 @@ type Runner struct { func New(opts Options) *Runner { return &Runner{ - Options: opts, + Options: opts, + + // Some hooks use STDIN for parsing data from Git. To allow multiple commands + // and scripts access the same Git data STDIN is cached via cachedReader. + stdin: NewCachedReader(os.Stdin), executor: exec.CommandExecutor{}, } } @@ -82,7 +87,7 @@ type executable interface { func (r *Runner) RunAll(ctx context.Context, sourceDirs []string) []Result { results := make([]Result, 0, len(r.Hook.Commands)+len(r.Hook.Scripts)) - ranLFS, err := r.runLFSHook(ctx) + err := r.runLFSHook(ctx) if err != nil { log.Error(err) } @@ -92,34 +97,6 @@ func (r *Runner) RunAll(ctx context.Context, sourceDirs []string) []Result { return results } - // Sanity check before running hooks - // More than one hook can't use Stdin - StdinConsumed := ranLFS && git.DoesLFSHookConsumeStdin(r.HookName) - for commandName, command := range r.Hook.Commands { - if command.UseStdin { - if StdinConsumed { - log.Errorf( - "command %s wants to use Stdin which was consumed by a previously.", - commandName, - ) - return results - } - StdinConsumed = true - } - } - for scriptName, script := range r.Hook.Scripts { - if script.UseStdin { - if StdinConsumed { - log.Errorf( - "script %s wants to use Stdin which was consumed by a previously.", - scriptName, - ) - return results - } - StdinConsumed = true - } - } - if !r.DisableTTY && !r.Hook.Follow { log.StartSpinner() defer log.StopSpinner() @@ -146,9 +123,9 @@ func (r *Runner) RunAll(ctx context.Context, sourceDirs []string) []Result { } // returns whether it ran a LFS hook. -func (r *Runner) runLFSHook(ctx context.Context) (bool, error) { +func (r *Runner) runLFSHook(ctx context.Context) error { if !git.IsLFSHook(r.HookName) { - return false, nil + return nil } lfsRequiredFile := filepath.Join(r.Repo.RootPath, git.LFSRequiredFile) @@ -156,11 +133,11 @@ func (r *Runner) runLFSHook(ctx context.Context) (bool, error) { requiredExists, err := afero.Exists(r.Repo.Fs, lfsRequiredFile) if err != nil { - return false, err + return err } configExists, err := afero.Exists(r.Repo.Fs, lfsConfigFile) if err != nil { - return false, err + return err } if git.IsLFSAvailable() { @@ -174,8 +151,8 @@ func (r *Runner) runLFSHook(ctx context.Context) (bool, error) { []string{"git", "lfs", r.HookName}, r.GitArgs..., ), + r.stdin, out, - git.DoesLFSHookConsumeStdin(r.HookName), ) output := strings.Trim(out.String(), "\n") @@ -192,10 +169,10 @@ func (r *Runner) runLFSHook(ctx context.Context) (bool, error) { if err != nil && (requiredExists || configExists) { log.Warnf("git-lfs command failed: %s\n", output) - return true, err + return err } - return true, nil + return nil } if requiredExists || configExists { @@ -206,10 +183,10 @@ func (r *Runner) runLFSHook(ctx context.Context) (bool, error) { " - %s\n", lfsRequiredFile, lfsConfigFile, ) - return false, errors.New("git-lfs is required") + return errors.New("git-lfs is required") } - return false, nil + return nil } func (r *Runner) preHook() { @@ -521,6 +498,12 @@ func (r *Runner) run(ctx context.Context, opts exec.Options, follow bool) bool { log.SetName(opts.Name) defer log.UnsetName(opts.Name) + // If the command does not explicitly `use_stdin` no input will be provided. + var in io.Reader = NewNullReader() + if opts.UseStdin { + in = r.stdin + } + if (follow || opts.Interactive) && r.LogSettings.LogExecution() { r.logExecute(opts.Name, nil, nil) @@ -531,12 +514,14 @@ func (r *Runner) run(ctx context.Context, opts exec.Options, follow bool) bool { out = io.Discard } - err := r.executor.Execute(ctx, opts, out) + err := r.executor.Execute(ctx, opts, r.stdin, out) + return err == nil } out := bytes.NewBuffer(make([]byte, 0)) - err := r.executor.Execute(ctx, opts, out) + + err := r.executor.Execute(ctx, opts, in, out) r.logExecute(opts.Name, err, out) diff --git a/internal/lefthook/runner/runner_test.go b/internal/lefthook/runner/runner_test.go index 780ad77a..034027d8 100644 --- a/internal/lefthook/runner/runner_test.go +++ b/internal/lefthook/runner/runner_test.go @@ -21,7 +21,7 @@ import ( type TestExecutor struct{} -func (e TestExecutor) Execute(_ctx context.Context, opts exec.Options, _out io.Writer) (err error) { +func (e TestExecutor) Execute(_ctx context.Context, opts exec.Options, _in io.Reader, _out io.Writer) (err error) { if strings.HasPrefix(opts.Commands[0], "success") { err = nil } else { @@ -31,7 +31,7 @@ func (e TestExecutor) Execute(_ctx context.Context, opts exec.Options, _out io.W return } -func (e TestExecutor) RawExecute(_ctx context.Context, _command []string, _out io.Writer, forwardStdin bool) error { +func (e TestExecutor) RawExecute(_ctx context.Context, _command []string, _in io.Reader, _out io.Writer) error { return nil } From 9969b2da0604cfbb54ab5040febc085b2644db92 Mon Sep 17 00:00:00 2001 From: Valentin Kiselev Date: Wed, 29 May 2024 11:10:05 +0300 Subject: [PATCH 4/7] chore: remove special checks for pre-push hook --- internal/git/lfs.go | 16 ---------------- 1 file changed, 16 deletions(-) diff --git a/internal/git/lfs.go b/internal/git/lfs.go index d74109c2..142e8bf8 100644 --- a/internal/git/lfs.go +++ b/internal/git/lfs.go @@ -16,10 +16,6 @@ var lfsHooks = [...]string{ "pre-push", } -var lfsHookConsumeStdin = [...]string{ - "pre-push", -} - // IsLFSAvailable returns 'true' if git-lfs is installed. func IsLFSAvailable() bool { _, err := exec.LookPath("git-lfs") @@ -37,15 +33,3 @@ func IsLFSHook(hookName string) bool { return false } - -// DoesLFSHookConsumeStdin returns whether the LFS hookName will consume Stdin -// meaning it won't be available to following commands. -func DoesLFSHookConsumeStdin(hookName string) bool { - for _, lfsHookName := range lfsHookConsumeStdin { - if lfsHookName == hookName { - return true - } - } - - return false -} From b1a5d7a50a95f93574f9215dd45792c3791cb282 Mon Sep 17 00:00:00 2001 From: Valentin Kiselev Date: Wed, 29 May 2024 11:21:06 +0300 Subject: [PATCH 5/7] chore: fix typos --- internal/lefthook/runner/runner.go | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/internal/lefthook/runner/runner.go b/internal/lefthook/runner/runner.go index dd0d50e6..8f3f48d9 100644 --- a/internal/lefthook/runner/runner.go +++ b/internal/lefthook/runner/runner.go @@ -87,8 +87,7 @@ type executable interface { func (r *Runner) RunAll(ctx context.Context, sourceDirs []string) []Result { results := make([]Result, 0, len(r.Hook.Commands)+len(r.Hook.Scripts)) - err := r.runLFSHook(ctx) - if err != nil { + if err := r.runLFSHook(ctx); err != nil { log.Error(err) } @@ -122,7 +121,6 @@ func (r *Runner) RunAll(ctx context.Context, sourceDirs []string) []Result { return results } -// returns whether it ran a LFS hook. func (r *Runner) runLFSHook(ctx context.Context) error { if !git.IsLFSHook(r.HookName) { return nil @@ -514,7 +512,7 @@ func (r *Runner) run(ctx context.Context, opts exec.Options, follow bool) bool { out = io.Discard } - err := r.executor.Execute(ctx, opts, r.stdin, out) + err := r.executor.Execute(ctx, opts, in, out) return err == nil } From 24872b49000a830467afab760033b5c2afa6c01a Mon Sep 17 00:00:00 2001 From: Valentin Kiselev Date: Wed, 29 May 2024 11:43:13 +0300 Subject: [PATCH 6/7] fix: fail the hook if git-lfs command failed --- internal/lefthook/run.go | 5 ++++- internal/lefthook/runner/runner.go | 8 ++++---- internal/lefthook/runner/runner_test.go | 5 ++++- 3 files changed, 12 insertions(+), 6 deletions(-) diff --git a/internal/lefthook/run.go b/internal/lefthook/run.go index 59b526fb..5c0ab50d 100644 --- a/internal/lefthook/run.go +++ b/internal/lefthook/run.go @@ -169,7 +169,10 @@ Run 'lefthook install' manually.`, ) startTime := time.Now() - results := r.RunAll(ctx, sourceDirs) + results, runErr := r.RunAll(ctx, sourceDirs) + if runErr != nil { + return runErr + } if ctx.Err() != nil { return errors.New("Interrupted") diff --git a/internal/lefthook/runner/runner.go b/internal/lefthook/runner/runner.go index 8f3f48d9..e1095f4e 100644 --- a/internal/lefthook/runner/runner.go +++ b/internal/lefthook/runner/runner.go @@ -84,16 +84,16 @@ type executable interface { // RunAll runs scripts and commands. // LFS hook is executed at first if needed. -func (r *Runner) RunAll(ctx context.Context, sourceDirs []string) []Result { +func (r *Runner) RunAll(ctx context.Context, sourceDirs []string) ([]Result, error) { results := make([]Result, 0, len(r.Hook.Commands)+len(r.Hook.Scripts)) if err := r.runLFSHook(ctx); err != nil { - log.Error(err) + return results, err } if r.Hook.DoSkip(r.Repo.State()) { r.logSkip(r.HookName, "hook setting") - return results + return results, nil } if !r.DisableTTY && !r.Hook.Follow { @@ -118,7 +118,7 @@ func (r *Runner) RunAll(ctx context.Context, sourceDirs []string) []Result { r.postHook() - return results + return results, nil } func (r *Runner) runLFSHook(ctx context.Context) error { diff --git a/internal/lefthook/runner/runner_test.go b/internal/lefthook/runner/runner_test.go index 034027d8..16888a81 100644 --- a/internal/lefthook/runner/runner_test.go +++ b/internal/lefthook/runner/runner_test.go @@ -766,7 +766,10 @@ func TestRunAll(t *testing.T) { } t.Run(fmt.Sprintf("%d: %s", i, tt.name), func(t *testing.T) { - results := runner.RunAll(context.Background(), tt.sourceDirs) + results, err := runner.RunAll(context.Background(), tt.sourceDirs) + if err != nil { + t.Errorf("unexpected error %s", err) + } var success, fail []Result for _, result := range results { From 59d86ff8a222707f9359d746d7e27bea178fb994 Mon Sep 17 00:00:00 2001 From: Valentin Kiselev Date: Thu, 30 May 2024 11:00:28 +0300 Subject: [PATCH 7/7] chore: add more verbose error --- internal/lefthook/run.go | 2 +- internal/lefthook/runner/cached_reader.go | 2 +- internal/lefthook/runner/exec/execute_windows.go | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/internal/lefthook/run.go b/internal/lefthook/run.go index 5c0ab50d..4d2b58fa 100644 --- a/internal/lefthook/run.go +++ b/internal/lefthook/run.go @@ -171,7 +171,7 @@ Run 'lefthook install' manually.`, startTime := time.Now() results, runErr := r.RunAll(ctx, sourceDirs) if runErr != nil { - return runErr + return fmt.Errorf("failed to run the hook: %w", runErr) } if ctx.Err() != nil { diff --git a/internal/lefthook/runner/cached_reader.go b/internal/lefthook/runner/cached_reader.go index 124fa042..b2bfbb65 100644 --- a/internal/lefthook/runner/cached_reader.go +++ b/internal/lefthook/runner/cached_reader.go @@ -8,7 +8,7 @@ import ( // cachedReader reads from the provided `io.Reader` until `io.EOF` and saves // the read content into the inner buffer. // -// After `io.EOF` it will provide the read data again and again. +// After `io.EOF` it will be providing the read data again and again. type cachedReader struct { in io.Reader useBuffer bool diff --git a/internal/lefthook/runner/exec/execute_windows.go b/internal/lefthook/runner/exec/execute_windows.go index 9e34a21f..752a264f 100644 --- a/internal/lefthook/runner/exec/execute_windows.go +++ b/internal/lefthook/runner/exec/execute_windows.go @@ -60,7 +60,7 @@ func (e CommandExecutor) Execute(ctx context.Context, opts Options, in io.Reader } func (e CommandExecutor) RawExecute(ctx context.Context, command []string, in io.Reader, out io.Writer) error { - cmd := exec.Command(command[0], command[1:]...) + cmd := exec.CommandContext(ctx, command[0], command[1:]...) cmd.Stdin = in cmd.Stdout = out