simplify getVariables() and improve nested variables support

/cc @smyrman

task_test.go

@@ -72,17 +72,16 @@ func TestVars(t *testing.T) {
 		Target:    "default",
 		TrimSpace: true,
 		Files: map[string]string{
-			// hello task:
 			"foo.txt":              "foo",
 			"bar.txt":              "bar",
 			"baz.txt":              "baz",
 			"tmpl_foo.txt":         "foo",
-			"tmpl_bar.txt":         "<no value>",
+			"tmpl_bar.txt":         "bar",
 			"tmpl_foo2.txt":        "foo2",
 			"tmpl_bar2.txt":        "bar2",
 			"shtmpl_foo.txt":       "foo",
 			"shtmpl_foo2.txt":      "foo2",
-			"nestedtmpl_foo.txt":   "{{.FOO}}",
+			"nestedtmpl_foo.txt":   "<no value>",
 			"nestedtmpl_foo2.txt":  "foo2",
 			"foo2.txt":             "foo2",
 			"bar2.txt":             "bar2",
@@ -90,10 +89,10 @@ func TestVars(t *testing.T) {
 			"tmpl2_foo.txt":        "<no value>",
 			"tmpl2_foo2.txt":       "foo2",
 			"tmpl2_bar.txt":        "<no value>",
-			"tmpl2_bar2.txt":       "<no value>",
+			"tmpl2_bar2.txt":       "bar2",
 			"shtmpl2_foo.txt":      "<no value>",
 			"shtmpl2_foo2.txt":     "foo2",
-			"nestedtmpl2_foo2.txt": "{{.FOO2}}",
+			"nestedtmpl2_foo2.txt": "<no value>",
 			"override.txt":         "bar",
 		},
 	}

variables.go

@@ -85,82 +85,49 @@ func (v *Var) UnmarshalYAML(unmarshal func(interface{}) error) error {
 }
 
 // getVariables returns fully resolved variables following the priority order:
-// 1. Call variables (should already have been resolved)
-// 2. Environment (should not need to be resolved)
-// 3. Task variables, resolved with access to:
-//    - call, taskvars and environment variables
-// 4. Taskvars variables, resolved with access to:
-//    - environment variables
+// 1. Task variables
+// 2. Call variables
+// 3. Taskvars file variables
+// 4. Environment variables
 func (e *Executor) getVariables(call Call) (Vars, error) {
 	t, ok := e.Tasks[call.Task]
 	if !ok {
-		return nil, &taskNotFoundError{call.Task}
+		return nil, &taskNotFoundError{taskName: call.Task}
 	}
+	vr := varResolver{e: e, vars: getEnvironmentVariables()}
+	vr.merge(e.taskvars)
+	vr.merge(e.taskvars)
+	vr.merge(call.Vars)
+	vr.merge(call.Vars)
+	vr.merge(t.Vars)
+	vr.merge(t.Vars)
+	return vr.vars, vr.err
+}
 
-	merge := func(dest Vars, srcs ...Vars) {
-		for _, src := range srcs {
-			for k, v := range src {
-				dest[k] = v
-			}
-		}
-	}
-	varsKeys := func(srcs ...Vars) []string {
-		m := make(map[string]struct{})
-		for _, src := range srcs {
-			for k := range src {
-				m[k] = struct{}{}
-			}
-		}
-		lst := make([]string, 0, len(m))
-		for k := range m {
-			lst = append(lst, k)
-		}
-		return lst
-	}
-	replaceVars := func(dest Vars, keys []string) error {
-		r := varReplacer{vars: dest}
-		for _, k := range keys {
-			v := dest[k]
-			dest[k] = Var{
-				Static: r.replace(v.Static),
-				Sh:     r.replace(v.Sh),
-			}
-		}
-		return r.err
-	}
-	resolveShell := func(dest Vars, keys []string) error {
-		for _, k := range keys {
-			v := dest[k]
-			static, err := e.handleShVar(v)
-			if err != nil {
-				return err
-			}
-			dest[k] = Var{Static: static}
-		}
-		return nil
-	}
-	update := func(dest Vars, srcs ...Vars) error {
-		merge(dest, srcs...)
-		// updatedKeys ensures template evaluation is run only once.
-		updatedKeys := varsKeys(srcs...)
-		if err := replaceVars(dest, updatedKeys); err != nil {
-			return err
-		}
-		return resolveShell(dest, updatedKeys)
-	}
+type varResolver struct {
+	e    *Executor
+	vars Vars
+	err  error
+}
 
-	// Resolve taskvars variables to "result" with environment override variables.
-	override := getEnvironmentVariables()
-	result := make(Vars, len(e.taskvars)+len(t.Vars)+len(override))
-	if err := update(result, e.taskvars, override); err != nil {
-		return nil, err
+func (vr *varResolver) merge(vars Vars) {
+	if vr.err != nil {
+		return
 	}
-	// Resolve task variables to "result" with environment and call override variables.
-	merge(override, call.Vars)
-	if err := update(result, t.Vars, override); err != nil {
-		return nil, err
+	r := varReplacer{vars: vr.vars}
+	for k, v := range vars {
+		v = Var{
+			Static: r.replace(v.Static),
+			Sh:     r.replace(v.Sh),
+		}
+		static, err := vr.e.handleShVar(v)
+		if err != nil {
+			vr.err = err
+			return
+		}
+		vr.vars[k] = Var{Static: static}
 	}
-	return result, nil
+	vr.err = r.err
 }
 
 func (e *Executor) handleShVar(v Var) (string, error) {