Provide more helpful error message and require input for every non-bool option.

internal/driver/commands.go

@@ -128,7 +128,7 @@ var pprofCommands = commands{
 // reported generated by pprof.
 var pprofVariables = variables{
 	// Filename for file-based output formats, stdout by default.
-	"output": &variable{stringKind, "", "", helpText("Output filename for file-based outputs")},
+	"output": &variable{stringKind, "", "", helpText("<s> is the output filename for file-based outputs")},
 
 	// Comparisons.
 	"drop_negative": &variable{boolKind, "f", "", helpText(
@@ -146,18 +146,18 @@ var pprofVariables = variables{
 		"If unset, percentages are relative to full graph before focusing",
 		"to facilitate comparison with original graph.")},
 	"unit": &variable{stringKind, "minimum", "", helpText(
-		"Measurement units to display",
+		"<s> is the measurement unit to display",
 		"Scale the sample values to this unit.",
 		"For time-based profiles, use seconds, milliseconds, nanoseconds, etc.",
 		"For memory profiles, use megabytes, kilobytes, bytes, etc.",
 		"Using auto will scale each value independently to the most natural unit.")},
 	"compact_labels": &variable{boolKind, "f", "", "Show minimal headers"},
-	"source_path":    &variable{stringKind, "", "", "Search path for source files"},
-	"trim_path":      &variable{stringKind, "", "", "Path to trim from source paths before search"},
+	"source_path":    &variable{stringKind, "", "", "Search path <s> for source files"},
+	"trim_path":      &variable{stringKind, "", "", "Path <s> to trim from source paths before search"},
 
 	// Filtering options
 	"nodecount": &variable{intKind, "-1", "", helpText(
-		"Max number of nodes to show",
+		"<n> is the max number of nodes to show",
 		"Uses heuristics to limit the number of locations to be displayed.",
 		"On graphs, dotted edges represent paths through nodes that have been removed.")},
 	"nodefraction": &variable{floatKind, "0.005", "", "Hide nodes below <f>*total"},
@@ -166,56 +166,56 @@ var pprofVariables = variables{
 		"Honor nodefraction/edgefraction/nodecount defaults",
 		"Set to false to get the full profile, without any trimming.")},
 	"focus": &variable{stringKind, "", "", helpText(
-		"Restricts to samples going through a node matching regexp",
+		"Restricts to samples going through a node matching regexp <s>",
 		"Discard samples that do not include a node matching this regexp.",
 		"Matching includes the function name, filename or object name.")},
 	"ignore": &variable{stringKind, "", "", helpText(
-		"Skips paths going through any nodes matching regexp",
+		"Skips paths going through any nodes matching regexp <s>",
 		"If set, discard samples that include a node matching this regexp.",
 		"Matching includes the function name, filename or object name.")},
 	"prune_from": &variable{stringKind, "", "", helpText(
-		"Drops any functions below the matched frame.",
+		"Drops any functions below the matched frame <s>.",
 		"If set, any frames matching the specified regexp and any frames",
 		"below it will be dropped from each sample.")},
 	"hide": &variable{stringKind, "", "", helpText(
-		"Skips nodes matching regexp",
+		"Skips nodes matching regexp <s>",
 		"Discard nodes that match this location.",
 		"Other nodes from samples that include this location will be shown.",
 		"Matching includes the function name, filename or object name.")},
 	"show": &variable{stringKind, "", "", helpText(
-		"Only show nodes matching regexp",
+		"Only show nodes matching regexp <s>",
 		"If set, only show nodes that match this location.",
 		"Matching includes the function name, filename or object name.")},
 	"show_from": &variable{stringKind, "", "", helpText(
-		"Drops functions above the highest matched frame.",
+		"Drops functions above the highest matched frame <s>.",
 		"If set, all frames above the highest match are dropped from every sample.",
 		"Matching includes the function name, filename or object name.")},
 	"tagfocus": &variable{stringKind, "", "", helpText(
-		"Restricts to samples with tags in range or matched by regexp",
+		"Restricts to samples with tags in range or matched by regexp <s>",
 		"Use name=value syntax to limit the matching to a specific tag.",
 		"Numeric tag filter examples: 1kb, 1kb:10kb, memory=32mb:",
 		"String tag filter examples: foo, foo.*bar, mytag=foo.*bar")},
 	"tagignore": &variable{stringKind, "", "", helpText(
-		"Discard samples with tags in range or matched by regexp",
+		"Discard samples with tags in range or matched by regexp <s>",
 		"Use name=value syntax to limit the matching to a specific tag.",
 		"Numeric tag filter examples: 1kb, 1kb:10kb, memory=32mb:",
 		"String tag filter examples: foo, foo.*bar, mytag=foo.*bar")},
 	"tagshow": &variable{stringKind, "", "", helpText(
-		"Only consider tags matching this regexp",
+		"Only consider tags matching this regexp <s>",
 		"Discard tags that do not match this regexp")},
 	"taghide": &variable{stringKind, "", "", helpText(
-		"Skip tags matching this regexp",
+		"Skip tags matching this regexp <s>",
 		"Discard tags that match this regexp")},
 	// Heap profile options
 	"divide_by": &variable{floatKind, "1", "", helpText(
-		"Ratio to divide all samples before visualization",
+		"<f> is the ratio to divide all samples before visualization",
 		"Divide all samples values by a constant, eg the number of processors or jobs.")},
 	"mean": &variable{boolKind, "f", "", helpText(
 		"Average sample value over first value (count)",
 		"For memory profiles, report average memory per allocation.",
 		"For time-based profiles, report average time per event.")},
 	"sample_index": &variable{stringKind, "", "", helpText(
-		"Sample value to report (0-based index or name)",
+		"Sample value to report (<x> is 0-based index or name)",
 		"Profiles contain multiple values per sample.",
 		"Use sample_index=i to select the ith value (starting at 0).")},
 	"normalize": &variable{boolKind, "f", "", helpText(
@@ -246,6 +246,22 @@ func helpText(s ...string) string {
 	return strings.Join(s, "\n") + "\n"
 }
 
+func variablesPostFix(name string, vr *variable) string {
+	var postfix string
+	switch vr.kind {
+	case intKind:
+		postfix = "=<n>"
+	case floatKind:
+		postfix = "=<f>"
+	case stringKind:
+		postfix = "=<s>"
+	}
+	if name == "sample_index" {
+		postfix = "=<x>"
+	}
+	return postfix
+}
+
 // usage returns a string describing the pprof commands and variables.
 // if commandLine is set, the output reflect cli usage.
 func usage(commandLine bool) string {
@@ -254,7 +270,7 @@ func usage(commandLine bool) string {
 		prefix = "-"
 	}
 	fmtHelp := func(c, d string) string {
-		return fmt.Sprintf("    %-16s %s", c, strings.SplitN(d, "\n", 2)[0])
+		return fmt.Sprintf("    %-22s %s", c, strings.SplitN(d, "\n", 2)[0])
 	}
 
 	var commands []string
@@ -273,7 +289,11 @@ func usage(commandLine bool) string {
 	}
 
 	help = help + strings.Join(commands, "\n") + "\n\n" +
-		"  Options:\n"
+		"  Options:\n" +
+		"    General format is <option>=<value>\n" +
+		"    <f> is a float, <n> is an integer, and <s> is a string\n" +
+		"    If the option is a boolean value, then you can simply type <option>\n" +
+		"    Type \"help <option>\" for detailed usage\n\n"
 
 	// Print help for variables after sorting them.
 	// Collect radio variables by their group name to print them together.
@@ -284,7 +304,9 @@ func usage(commandLine bool) string {
 			radioOptions[vr.group] = append(radioOptions[vr.group], name)
 			continue
 		}
-		variables = append(variables, fmtHelp(prefix+name, vr.help))
+		option := prefix + name + variablesPostFix(name, vr)
+
+		variables = append(variables, fmtHelp(option, vr.help))
 	}
 	sort.Strings(variables)
 

internal/driver/interactive.go

@@ -70,6 +70,12 @@ func interactive(p *profile.Profile, o *plugin.Options) error {
 					value = strings.TrimSpace(value)
 				}
 				if v := pprofVariables[name]; v != nil {
+					// All non-bool options require inputs
+					if v.kind != boolKind && value == "" {
+						example := name + variablesPostFix(name, v)
+						o.UI.PrintErr(fmt.Errorf("please input a value, e.g. %v", example))
+						continue
+					}
 					if name == "sample_index" {
 						// Error check sample_index=xxx to ensure xxx is a valid sample type.
 						index, err := p.SampleIndexByName(value)
@@ -267,6 +273,9 @@ func parseCommandLine(input []string) ([]string, variables, error) {
 		}
 	}
 	if c == nil {
+		if v := pprofVariables[name]; v != nil {
+			return nil, nil, fmt.Errorf("do you mean: \"%v=%v\"?", name, args[0])
+		}
 		return nil, nil, fmt.Errorf("unrecognized command: %q", name)
 	}
 

internal/driver/interactive_test.go

@@ -40,62 +40,53 @@ func TestShell(t *testing.T) {
 	savedVariables := pprofVariables
 	defer func() { pprofVariables = savedVariables }()
 
-	// Random interleave of independent scripts
-	pprofVariables = testVariables(savedVariables)
+	shortcuts1, scScript1 := makeShortcuts(interleave(script, 2), 1)
+	shortcuts2, scScript2 := makeShortcuts(interleave(script, 1), 2)
 
-	// pass in HTTPTransport when setting defaults, because otherwise default
-	// transport will try to add flags to the default flag set.
-	o := setDefaults(&plugin.Options{HTTPTransport: transport.New(nil)})
-	o.UI = newUI(t, interleave(script, 0))
-	if err := interactive(p, o); err != nil {
-		t.Error("first attempt:", err)
-	}
-	// Random interleave of independent scripts
-	pprofVariables = testVariables(savedVariables)
-	o.UI = newUI(t, interleave(script, 1))
-	if err := interactive(p, o); err != nil {
-		t.Error("second attempt:", err)
+	var testcases = []struct {
+		name              string
+		input             []string
+		shortcuts         shortcuts
+		allowRx           string
+		numAllowRxMatches int
+		propagateError    bool
+	}{
+		{"Random interleave of independent scripts 1", interleave(script, 0), pprofShortcuts, "", 0, false},
+		{"Random interleave of independent scripts 2", interleave(script, 1), pprofShortcuts, "", 0, false},
+		{"Random interleave of independent scripts with shortcuts 1", scScript1, shortcuts1, "", 0, false},
+		{"Random interleave of independent scripts with shortcuts 2", scScript2, shortcuts2, "", 0, false},
+		{"Group with invalid vale", []string{"cumulative=this"}, pprofShortcuts, `unrecognized value for cumulative: "this". Use one of cum, flat`, 1, false},
+		{"No special value provided for the option", []string{"sample_index"}, pprofShortcuts, `please input a value, e.g. sample_index=<x>`, 1, false},
+		{"No string value provided for the option", []string{"focus="}, pprofShortcuts, `please input a value, e.g. focus=<s>`, 1, false},
+		{"No float value provided for the option", []string{"divide_by"}, pprofShortcuts, `please input a value, e.g. divide_by=<f>`, 1, false},
+		{"Helpful input format reminder", []string{"sample_index 0"}, pprofShortcuts, `do you mean: "sample_index=0`, 1, false},
+		{"Verify propagation of IO errors", []string{"**error**"}, pprofShortcuts, "", 0, false},
 	}
 
-	// Random interleave of independent scripts with shortcuts
-	pprofVariables = testVariables(savedVariables)
-	var scScript []string
-	pprofShortcuts, scScript = makeShortcuts(interleave(script, 2), 1)
-	o.UI = newUI(t, scScript)
-	if err := interactive(p, o); err != nil {
-		t.Error("first shortcut attempt:", err)
-	}
+	o := setDefaults(&plugin.Options{HTTPTransport: transport.New(nil)})
+	for _, tc := range testcases {
+		t.Run(tc.name, func(t *testing.T) {
+			pprofVariables = testVariables(savedVariables)
+			pprofShortcuts = tc.shortcuts
+			ui := &proftest.TestUI{
+				T:       t,
+				Input:   tc.input,
+				AllowRx: tc.allowRx,
+			}
+			o.UI = ui
 
-	// Random interleave of independent scripts with shortcuts
-	pprofVariables = testVariables(savedVariables)
-	pprofShortcuts, scScript = makeShortcuts(interleave(script, 1), 2)
-	o.UI = newUI(t, scScript)
-	if err := interactive(p, o); err != nil {
-		t.Error("second shortcut attempt:", err)
-	}
+			err := interactive(p, o)
+			if (tc.propagateError && err == nil) || (tc.propagateError && err != nil) {
+				t.Errorf("%v: %v", tc.name, err)
+			}
 
-	// Group with invalid value
-	pprofVariables = testVariables(savedVariables)
-	ui := &proftest.TestUI{
-		T:       t,
-		Input:   []string{"cumulative=this"},
-		AllowRx: `unrecognized value for cumulative: "this". Use one of cum, flat`,
-	}
-	o.UI = ui
-	if err := interactive(p, o); err != nil {
-		t.Error("invalid group value:", err)
-	}
-	// Confirm error message written out once.
-	if ui.NumAllowRxMatches != 1 {
-		t.Errorf("want error message to be printed 1 time, got %v", ui.NumAllowRxMatches)
-	}
-	// Verify propagation of IO errors
-	pprofVariables = testVariables(savedVariables)
-	o.UI = newUI(t, []string{"**error**"})
-	if err := interactive(p, o); err == nil {
-		t.Error("expected IO error, got nil")
+			// Confirm error message written out once.
+			if tc.numAllowRxMatches != ui.NumAllowRxMatches {
+				t.Errorf("want error message to be printed %v time(s), got %v",
+					tc.numAllowRxMatches, ui.NumAllowRxMatches)
+			}
+		})
 	}
-
 }
 
 var testCommands = commands{
@@ -132,7 +123,7 @@ var script = []string{
 	"f=-1;f=-2.5;check f=-2.5;f=0.0001;check f=0.0001",
 	"check ff=0;ff=-1.01;check ff=-1.01;ff=100;check ff=100",
 	"s=one;s=two;check s=two",
-	"ss=tree;check ss=tree;ss=;check ss;ss=forest;check ss=forest",
+	"ss=tree;check ss=tree;ss=forest;check ss=forest",
 	"ta=true;check ta=true;check tb=false;check tc=false;tb=1;check tb=true;check ta=false;check tc=false;tc=yes;check tb=false;check ta=false;check tc=true",
 }
 
@@ -162,13 +153,6 @@ func makeShortcuts(input []string, seed int) (shortcuts, []string) {
 	return s, output
 }
 
-func newUI(t *testing.T, input []string) plugin.UI {
-	return &proftest.TestUI{
-		T:     t,
-		Input: input,
-	}
-}
-
 func checkValue(p *profile.Profile, cmd []string, vars variables, o *plugin.Options) error {
 	if len(cmd) != 2 {
 		return fmt.Errorf("expected len(cmd)==2, got %v", cmd)