Skip to content

Commit

Permalink
Fix infinite loop case for envvar backed opts (#50)
Browse files Browse the repository at this point in the history
This is a regression introduced by the feature "Allow required options to be satisfied by an nev var".

The bug lies in `optsMatcher`, which iterates over its options, and keeps looping until no more options can match or there is no more input to feed them.

If one of the options can be satsified from an en var, it doesn't consume any input, and returns true, causing an inifinite loop.

The proposed fix is to exclude an option (in `optsMatcher`) whenever  it has been satisfied by an env var.
  • Loading branch information
jawher authored Jul 12, 2017
1 parent 8327d12 commit a608864
Show file tree
Hide file tree
Showing 3 changed files with 41 additions and 1 deletion.
8 changes: 7 additions & 1 deletion fsm.go
Original file line number Diff line number Diff line change
Expand Up @@ -142,11 +142,17 @@ func dot(s *state, visited map[*state]bool) []string {
type parseContext struct {
args map[*arg][]string
opts map[*opt][]string
excludedOpts map[*opt]struct{}
rejectOptions bool
}

func newParseContext() parseContext {
return parseContext{map[*arg][]string{}, map[*opt][]string{}, false}
return parseContext{
args: map[*arg][]string{},
opts: map[*opt][]string{},
excludedOpts: map[*opt]struct{}{},
rejectOptions: false,
}
}

func (pc parseContext) merge(o parseContext) {
Expand Down
7 changes: 7 additions & 0 deletions matchers.go
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,10 @@ func (o *optMatcher) match(args []string, c *parseContext) (bool, []string) {
idx += consumed

default:
if o.theOne.valueSetFromEnv {
// exclude opt
c.excludedOpts[o.theOne] = struct{}{}
}
return o.theOne.valueSetFromEnv, args
}
}
Expand Down Expand Up @@ -227,6 +231,9 @@ func (om optsMatcher) try(args []string, c *parseContext) (bool, []string) {
return false, args
}
for _, o := range om.options {
if _, exclude := c.excludedOpts[o]; exclude {
continue
}
if ok, nargs := (&optMatcher{theOne: o, optionsIdx: om.optionsIndex}).match(args, c); ok {
return ok, nargs
}
Expand Down
27 changes: 27 additions & 0 deletions spec_n_parse_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1254,6 +1254,33 @@ func TestEnvOverrideOk(t *testing.T) {
}
}

// Anti-regression test for infinite loop case with envvar backed opts
// https://github.com/jawher/mow.cli/pull/49
func TestEnvOptSeq(t *testing.T) {
defer os.Unsetenv("envopt")

var envopt *string
var arg *string

init := func(c *Cmd) {
defer os.Unsetenv("envopt")

envopt = c.String(StringOpt{
Name: "e envopt",
Value: "envdefault",
EnvVar: "envopt",
})

arg = c.StringArg("ARG", "", "")
}

os.Setenv("envopt", "envval")
okCmd(t, "", init, []string{"argval"})

assert.Equal(t, "envval", *envopt)
assert.Equal(t, "argval", *arg)
}

// Test that not setting an environment variable correctly causes
// required options to fail if no value is supplied in args.
func TestEnvOverrideFail(t *testing.T) {
Expand Down

0 comments on commit a608864

Please sign in to comment.