handle unknown flags in reverse (#290)

While at it, expand the tests for build and test too.

main.go

@@ -358,7 +358,7 @@ func toolexecCmd(command string, args []string) (*exec.Cmd, error) {
 
 	// Note that we also need to pass build flags to 'go list', such
 	// as -tags.
-	cache.BuildFlags = filterBuildFlags(flags)
+	cache.BuildFlags, _ = filterBuildFlags(flags)
 	if command == "test" {
 		cache.BuildFlags = append(cache.BuildFlags, "-test")
 	}
@@ -1337,7 +1337,7 @@ var booleanFlags = map[string]bool{
 	"-benchmem": true,
 }
 
-func filterBuildFlags(flags []string) (filtered []string) {
+func filterBuildFlags(flags []string) (filtered []string, firstUnknown string) {
 	for i := 0; i < len(flags); i++ {
 		arg := flags[i]
 		name := arg
@@ -1348,6 +1348,8 @@ func filterBuildFlags(flags []string) (filtered []string) {
 		buildFlag := buildFlags[name]
 		if buildFlag {
 			filtered = append(filtered, arg)
+		} else {
+			firstUnknown = name
 		}
 		if booleanFlags[arg] || strings.Contains(arg, "=") {
 			// Either "-bool" or "-name=value".
@@ -1358,7 +1360,7 @@ func filterBuildFlags(flags []string) (filtered []string) {
 			filtered = append(filtered, flags[i])
 		}
 	}
-	return filtered
+	return filtered, firstUnknown
 }
 
 // splitFlagsFromFiles splits args into a list of flag and file arguments. Since

main_test.go

@@ -298,7 +298,7 @@ func TestFilterBuildFlags(t *testing.T) {
 		test := test
 		t.Run(test.name, func(t *testing.T) {
 			t.Parallel()
-			got := filterBuildFlags(test.flags)
+			got, _ := filterBuildFlags(test.flags)
 
 			if diff := cmp.Diff(test.want, got); diff != "" {
 				t.Fatalf("filterBuildFlags(%q) mismatch (-want +got):\n%s", test.flags, diff)

reverse.go

@@ -5,6 +5,7 @@ package main
 
 import (
 	"bufio"
+	"flag"
 	"fmt"
 	"go/ast"
 	"go/parser"
@@ -37,6 +38,15 @@ func commandReverse(args []string) error {
 		return err
 	}
 
+	// We don't actually run a main Go command with all flags,
+	// so if the user gave a non-build flag,
+	// we need this check to not silently ignore it.
+	if _, firstUnknown := filterBuildFlags(flags); firstUnknown != "" {
+		// A bit of a hack to get a normal flag.Parse error.
+		// Longer term, "reverse" might have its own FlagSet.
+		return flag.NewFlagSet("", flag.ContinueOnError).Parse([]string{firstUnknown})
+	}
+
 	// A package's names are generally hashed with the action ID of its
 	// obfuscated build. We recorded those action IDs above.
 	// Note that we parse Go files directly to obtain the names, since the

testdata/scripts/help.txt

@@ -18,12 +18,17 @@ stderr 'garble \[flags\] build'
 ! stdout .
 
 ! garble -badflag
+stderr 'flag provided but not defined'
 stderr 'garble \[flags\] build'
 ! stdout .
 
 ! garble badcmd
 stderr 'unknown command'
 
+! garble build -badflag
+stderr 'usage: go build' # TODO: is this confusing?
+! stdout .
+
 [!windows] ! garble /does/not/exist/compile
 [windows] ! garble C:\does\not\exist\compile
 stderr 'not running "garble \[command\]"'
@@ -36,3 +41,9 @@ stderr 'does not take arguments'
 
 ! garble version arg
 stderr 'does not take arguments'
+
+# We need a dummy module for "garble build -badflag".
+-- go.mod --
+module dummy
+-- dummy.go --
+package dummy

testdata/scripts/reverse.txt

@@ -1,9 +1,8 @@
 env GOPRIVATE=test/main
 
 # Unknown build flags should result in errors.
-# TODO: reenable and fix
-# ! garble reverse -badflag
-# stderr 'flag provided but not defined'
+! garble reverse -badflag
+stderr 'flag provided but not defined'
 
 garble build
 exec ./main

testdata/scripts/test.txt

@@ -1,5 +1,6 @@
 # build the test binary
 garble test -c -a
+! stdout 'PASS'
 binsubstr bar.test$exe 'TestFoo' 'TestSeparateFoo'
 ! binsubstr bar.test$exe 'LocalFoo|ImportedVar|OriginalFuncName'
 
@@ -19,12 +20,17 @@ garble test -v . ./somemain ./sometest
 stdout 'ok\s+test/bar\s'
 stdout 'PASS.*TestFoo'
 stdout 'PASS.*TestSeparateFoo'
+stdout 'SKIP.*TestWithFlag'
 stdout 'package bar, func name:'
 stdout 'package bar_test, func name:'
 ! stdout 'OriginalFuncName'
 stdout '\?\s+test/bar/somemain\s'
 stdout 'ok\s+test/bar/sometest\s'
 
+# verify that non-build flags are kept
+garble test -withflag -v
+stdout 'PASS.*TestWithFlag'
+
 # verify with regular cmd/go; OriginalFuncName should appear
 go test -v
 stdout 'PASS.*TestFoo'
@@ -67,11 +73,14 @@ func TestFoo(t *testing.T) {
 package bar_test
 
 import (
+	"flag"
 	"testing"
 
 	"test/bar"
 )
 
+var withFlag = flag.Bool("withflag", false, "")
+
 func TestSeparateFoo(t *testing.T) {
 	t.Log(bar.ImportedVar)
 	if bar.LocalFoo() != "Foo" {
@@ -79,6 +88,12 @@ func TestSeparateFoo(t *testing.T) {
 	}
 	t.Logf("package bar_test, func name: %s", bar.OriginalFuncName())
 }
+
+func TestWithFlag(t *testing.T) {
+	if !*withFlag {
+		t.Skip()
+	}
+}
 -- main_test.go --
 package bar