fix(builtins): honor --help argv order and rewrite pflag errors to GNU style

builtins/builtins.go

@@ -13,6 +13,7 @@ import (
 	"io/fs"
 	"os"
 	"sort"
+	"strings"
 	"syscall"
 	"time"
 
@@ -96,14 +97,250 @@ func (c Command) Register() {
 		if normalize != nil {
 			args = normalize(args)
 		}
+		hasHelp := fs.Lookup("help") != nil
+		// Honor `--help` once it's reached in argv to match GNU coreutils:
+		// `cmd --no-arg-flag --help --bogus` should print help and exit 0.
+		// We trim args at the first `--help` only when every preceding
+		// token is a registered no-argument flag. Value-takers (`-n 5`),
+		// explicit-value forms (`--foo=bar`), positionals, and unknown
+		// flags all preclude the trim — those tokens may still fail
+		// later validation (e.g. `head -n nope`'s numeric check), and
+		// trimming would silently swallow that error by short-circuiting
+		// on `--help`. GNU's left-to-right semantics require the earlier
+		// failure to surface even when `--help` follows it.
+		if hasHelp {
+			if idx, ok := safeHelpTrimIndex(fs, args); ok {
+				args = args[:idx+1]
+			}
+		}
 		if err := fs.Parse(args); err != nil {
-			callCtx.Errf("%s: %v\n", name, err)
+			callCtx.Errf("%s: %s\n", name, rewritePflagError(err, args))
+			if hasHelp {
+				callCtx.Errf("Try '%s --help' for more information.\n", name)
+			}
 			return Result{Code: 1}
 		}
 		return handler(ctx, callCtx, fs.Args())
 	})
 }
 
+// rewritePflagError translates pflag's default error messages to the
+// GNU-getopt-style wording that matches GNU coreutils byte-for-byte.
+// It returns the rewritten message without a trailing newline or any
+// "cmd:" prefix; callers prepend the builtin name themselves.
+//
+// args is the argv that was handed to fs.Parse. It is consulted only
+// for the unknown-long-flag case, where pflag strips the `=value`
+// suffix from its error string but GNU getopt reports the full token
+// (`--no-such=value`, not just `--no-such`).
+//
+// Patterns covered:
+//
+//	pflag                                       → GNU
+//	"unknown flag: --foo"                       → "unrecognized option '--foo'"
+//	                                              ("--foo=value" if the original argv used =value)
+//	"unknown shorthand flag: 'X' in -..."       → "invalid option -- 'X'"
+//	"flag needs an argument: --foo"             → "option '--foo' requires an argument"
+//	"flag needs an argument: 'X' in -Y"         → "option requires an argument -- 'X'"
+//	`invalid argument "..." for "DESC" flag:    → "option '--LONG' doesn't allow
+//	   flag does not allow an argument`              an argument"
+//
+// Unknown messages are returned unchanged.
+func rewritePflagError(err error, args []string) string {
+	msg := err.Error()
+
+	// pflag wraps errors returned by a Var's Set(value) as
+	//   `invalid argument "VALUE" for "DESC" flag: INNER`
+	// where DESC is e.g. `-h, --human-readable` or `--total`. df's
+	// noArgBool / unitFlag.Set returns the literal "flag does not
+	// allow an argument" so users (and tests) see GNU's "doesn't"
+	// instead of the wrapped pflag verbosity.
+	if strings.HasPrefix(msg, "invalid argument ") &&
+		strings.HasSuffix(msg, "flag does not allow an argument") {
+		if d, ok := extractFlagDescriptor(msg); ok {
+			// `-X=value` for a no-arg shorthand: GNU getopt iterates
+			// shorthand chars and treats `=` as an unknown shorthand,
+			// emitting `invalid option -- '='`. pflag instead routes
+			// the value through Set, hitting our no-arg guard. Match
+			// GNU when the original argv used the shorthand=value form.
+			if shortFlagEqualsValueIn(d, args) {
+				return "invalid option -- '='"
+			}
+			return "option '" + longFlagName(d) + "' doesn't allow an argument"
+		}
+	}
+
+	if rest, ok := strings.CutPrefix(msg, "unknown flag: "); ok {
+		// pflag's error carries only the flag name; GNU's reports the
+		// full token, so we recover `--foo=value` from argv when the
+		// user wrote it that way.
+		return "unrecognized option '" + recoverLongFlagToken(rest, args) + "'"
+	}
+
+	const shortPrefix = "unknown shorthand flag: '"
+	if rest, ok := strings.CutPrefix(msg, shortPrefix); ok {
+		if char, _, ok := strings.Cut(rest, "'"); ok {
+			return "invalid option -- '" + char + "'"
+		}
+	}
+
+	if rest, ok := strings.CutPrefix(msg, "flag needs an argument: "); ok {
+		// pflag emits two distinct payloads depending on the form:
+		//   long form:  `--foo`
+		//   short form: `'X' in -Y` (X is the char, Y is the run
+		//                            of shorthand letters)
+		// GNU getopt uses different wording for each: long flags get
+		// `option '--foo' requires an argument`, short flags get
+		// `option requires an argument -- 'X'`.
+		if char, found := shortMissingArg(rest); found {
+			return "option requires an argument -- '" + char + "'"
+		}
+		return "option '" + rest + "' requires an argument"
+	}
+
+	return msg
+}
+
+// safeHelpTrimIndex returns the index of the first `--help` in args if
+// trimming at it is provably safe — i.e. every preceding token is a
+// registered no-argument flag and the scan does not cross a `--`
+// end-of-flags marker. Returns false otherwise, signalling the caller
+// to leave args alone and let pflag report any earlier validation
+// error.
+func safeHelpTrimIndex(fs *pflag.FlagSet, args []string) (int, bool) {
+	for i, a := range args {
+		if a == "--" {
+			return 0, false
+		}
+		if a == "--help" {
+			return i, true
+		}
+		if !isNoArgFlagToken(fs, a) {
+			return 0, false
+		}
+	}
+	return 0, false
+}
+
+// isNoArgFlagToken reports whether a is a registered flag (long or
+// shorthand cluster) whose every component takes no value. Tokens
+// containing `=` (explicit-value form) or matching value-taking flags
+// return false — both can fail at parse time. Non-ASCII bytes in a
+// shorthand cluster also return false: pflag's ShorthandLookup panics
+// on inputs longer than one byte and `string(byte)` for any byte ≥
+// 0x80 produces a 2-byte UTF-8 encoding.
+func isNoArgFlagToken(fs *pflag.FlagSet, a string) bool {
+	if len(a) < 2 || a[0] != '-' || a == "-" {
+		return false
+	}
+	if strings.Contains(a, "=") {
+		return false
+	}
+	if strings.HasPrefix(a, "--") {
+		f := fs.Lookup(a[2:])
+		return f != nil && f.NoOptDefVal != ""
+	}
+	for i := 1; i < len(a); i++ {
+		c := a[i]
+		if c > 0x7E || c <= ' ' {
+			return false
+		}
+		f := fs.ShorthandLookup(string(c))
+		if f == nil || f.NoOptDefVal == "" {
+			return false
+		}
+	}
+	return true
+}
+
+// recoverLongFlagToken returns the original argv token for an unknown
+// long flag whose stripped name (e.g. `--no-such`) appears in pflag's
+// error. If the user wrote `--no-such=value`, the full token is
+// returned; otherwise the bare flag is returned. Stops at `--` so a
+// later positional like `-- --no-such=foo` is never misclassified.
+func recoverLongFlagToken(flag string, args []string) string {
+	prefix := flag + "="
+	for _, a := range args {
+		if a == "--" {
+			break
+		}
+		if a == flag || strings.HasPrefix(a, prefix) {
+			return a
+		}
+	}
+	return flag
+}
+
+// shortFlagEqualsValueIn reports whether args contains a token of the
+// form `-X=...` whose shorthand char X matches the shorthand encoded
+// in descriptor (e.g. `-h, --human-readable` → X=`h`). Used to detect
+// the GNU-getopt shorthand=value error class.
+func shortFlagEqualsValueIn(descriptor string, args []string) bool {
+	short, ok := shortFlagFromDescriptor(descriptor)
+	if !ok {
+		return false
+	}
+	for _, a := range args {
+		if a == "--" {
+			break
+		}
+		if len(a) >= 3 && a[0] == '-' && a[1] != '-' && a[1] == short && a[2] == '=' {
+			return true
+		}
+	}
+	return false
+}
+
+// shortFlagFromDescriptor extracts the single shorthand char from a
+// pflag descriptor like `-h, --human-readable`. Returns false for
+// long-only descriptors (`--total`).
+func shortFlagFromDescriptor(d string) (byte, bool) {
+	if len(d) >= 2 && d[0] == '-' && d[1] != '-' {
+		return d[1], true
+	}
+	return 0, false
+}
+
+// shortMissingArg parses pflag's short-form payload for
+// `flag needs an argument: 'X' in -Y` and returns the bare char
+// (`X`). The second return is false when payload is in the long-form
+// (`--foo`), letting the caller fall through.
+func shortMissingArg(payload string) (string, bool) {
+	if !strings.HasPrefix(payload, "'") {
+		return "", false
+	}
+	char, _, found := strings.Cut(payload[1:], "'")
+	if !found {
+		return "", false
+	}
+	return char, true
+}
+
+// extractFlagDescriptor parses pflag's `invalid argument "..." for
+// "DESC" flag: ...` wrapper and returns the DESC segment.
+func extractFlagDescriptor(msg string) (string, bool) {
+	_, after, found := strings.Cut(msg, ` for "`)
+	if !found {
+		return "", false
+	}
+	desc, _, found := strings.Cut(after, `" flag:`)
+	if !found {
+		return "", false
+	}
+	return desc, true
+}
+
+// longFlagName returns the long-form (--name) flag name from a pflag
+// descriptor like `-X, --LONG` or `--LONG`. For shorthand-only flags
+// (rare; e.g. df's hidden `-k`) the descriptor is just `-X` and we
+// return it unchanged.
+func longFlagName(descriptor string) string {
+	if i := strings.LastIndex(descriptor, ", "); i >= 0 {
+		return descriptor[i+2:]
+	}
+	return descriptor
+}
+
 // CallContext provides the capabilities available to builtin commands.
 // It is created by the Runner for each builtin invocation.
 type CallContext struct {

builtins/du/du_test.go

@@ -273,7 +273,7 @@ func TestDuUnknownFlag(t *testing.T) {
 	_, stderr, code := cmdRun(t, "du --no-such-flag .", dir)
 	assert.Equal(t, 1, code)
 	assert.Contains(t, stderr, "du:")
-	assert.Contains(t, stderr, "unknown flag")
+	assert.Contains(t, stderr, "unrecognized option")
 }
 
 // --- Security-sensitive flags must be rejected ---