From 5203ef2fa303ccadb250a86602bb7e37cb1e5874 Mon Sep 17 00:00:00 2001
From: "Ronald G. Minnich" <rminnich@gmail.com>
Date: Thu, 23 Mar 2023 17:59:16 +0000
Subject: [PATCH] u-root command: add checkArgs function

There are errors which have confusing diagnostics.

A typical one is this
u-root -files `which ethtool` -files `which bash` all

if ethtool is not installed, this expands to
u-root -files -files `which bash` all
and a warning that is very confusing:
Skipping /usr/bin/bash because it is not a directory

This is not even remotely related to the problem.

Still worse, this command will create a badly broken initrd!

This problem has been known to hold up interns for weeks at a time :-)

Add checkArgs, which looks for common problems in os.Args,
and is easily expanded. Add a test.

Signed-off-by: Ronald G. Minnich <rminnich@gmail.com>
---
 u-root.go     | 39 +++++++++++++++++++++++++++++++++++++++
 uroot_test.go | 22 ++++++++++++++++++++++
 2 files changed, 61 insertions(+)

diff --git a/u-root.go b/u-root.go
index 36bde00dc3..9a4ab37917 100644
--- a/u-root.go
+++ b/u-root.go
@@ -6,6 +6,7 @@ package main
 
 import (
 	"encoding/json"
+	"errors"
 	"flag"
 	"fmt"
 	"log"
@@ -37,6 +38,11 @@ func (m *multiFlag) Set(value string) error {
 	return nil
 }
 
+// errors from the u-root command
+var (
+	ErrEmptyFilesArg = errors.New("Empty argument to -files")
+)
+
 // Flags for u-root builder.
 var (
 	build, format, tmpDir, base, outputPath *string
@@ -146,7 +152,40 @@ func generateLabel(env gbbgolang.Environ) string {
 	return fmt.Sprintf("%s-%s-%s-%s", *build, env.GOOS, env.GOARCH, strings.Join(baseCmds, "_"))
 }
 
+// checkArgs checks for common mistakes that cause confusion.
+//  1. -files as the last argument
+//  2. -files followed by any switch, indicating a shell expansion problem
+//     This is usually caused by Makfiles structured as follows
+//     u-root -files `which ethtool` -files `which bash`
+//     if ethtool is not installed, the expansion yields
+//     u-root -files -files `which bash`
+//     and the rather confusing error message
+//     16:14:51 Skipping /usr/bin/bash because it is not a directory
+//     which, in practice, nobody understands
+func checkArgs(args ...string) error {
+	if len(args) == 0 {
+		return nil
+	}
+
+	if args[len(args)-1] == "-files" {
+		return fmt.Errorf("last argument is -files:%w", ErrEmptyFilesArg)
+	}
+
+	// We know the last arg is not -files; scan the arguments for -files
+	// followed by a switch.
+	for i := 0; i < len(args)-1; i++ {
+		if args[i] == "-files" && args[i+1][0] == '-' {
+			return fmt.Errorf("-files argument %d is followed by a switch: %w", i, ErrEmptyFilesArg)
+		}
+	}
+
+	return nil
+}
+
 func main() {
+	if err := checkArgs(os.Args...); err != nil {
+		log.Fatal(err)
+	}
 	gbbOpts := &gbbgolang.BuildOpts{}
 	gbbOpts.RegisterFlags(flag.CommandLine)
 
diff --git a/uroot_test.go b/uroot_test.go
index 5c98395d3a..32536337c0 100644
--- a/uroot_test.go
+++ b/uroot_test.go
@@ -8,6 +8,7 @@ import (
 	"bufio"
 	"bytes"
 	"crypto/sha256"
+	"errors"
 	"fmt"
 	"io"
 	"os"
@@ -357,3 +358,24 @@ func buildIt(t *testing.T, args, env []string, want error) (*os.File, []byte) {
 func TestMain(m *testing.M) {
 	testutil.Run(m, main)
 }
+
+func TestCheckArgs(t *testing.T) {
+	for _, tt := range []struct {
+		name string
+		args []string
+		err  error
+	}{
+		{"-files is only arg", []string{"-files"}, ErrEmptyFilesArg},
+		{"-files followed by -files", []string{"-files", "-files"}, ErrEmptyFilesArg},
+		{"-files followed by any other switch", []string{"-files", "-abc"}, ErrEmptyFilesArg},
+		{"no args", []string{}, nil},
+		{"u-root alone", []string{"u-root"}, nil},
+		{"u-root with -files and other args", []string{"u-root", "-files", "/bin/bash", "core"}, nil},
+	} {
+		t.Run(tt.name, func(t *testing.T) {
+			if err := checkArgs(tt.args...); !errors.Is(err, tt.err) {
+				t.Errorf("%q: got %v, want %v", tt.args, err, tt.err)
+			}
+		})
+	}
+}