Fix context.(Global)IsSet to respect environment variables

This appeared to be the least messy approach to hack in support for
IsSet also checking environment variables to see if a particular
cli.Flag was set without making backwards incompatible changes to the
interface.

I intend to fix this more properly in v2, probably by adding another
method to the cli.Flag interface to push the responsibility down as it
occurred to me that it was really the `Flag`s themselves that offer
support for configuration via the environment as opposed to the
`context` or other supporting structures. This opens the door for the
anything implementing the `Flag` interface to have additional sources of
input while still supporting `context.IsSet`.

context.go

@@ -3,6 +3,8 @@ package cli
 import (
 	"errors"
 	"flag"
+	"os"
+	"reflect"
 	"strings"
 )
 
@@ -15,7 +17,6 @@ type Context struct {
 	Command       Command
 	flagSet       *flag.FlagSet
 	setFlags      map[string]bool
-	globalSetFlags map[string]bool
 	parentContext *Context
 }
 
@@ -43,28 +44,70 @@ func (c *Context) GlobalSet(name, value string) error {
 func (c *Context) IsSet(name string) bool {
 	if c.setFlags == nil {
 		c.setFlags = make(map[string]bool)
+
 		c.flagSet.Visit(func(f *flag.Flag) {
 			c.setFlags[f.Name] = true
 		})
+
+		c.flagSet.VisitAll(func(f *flag.Flag) {
+			if _, ok := c.setFlags[f.Name]; ok {
+				return
 			}
-	return c.setFlags[name] == true
+			c.setFlags[f.Name] = false
+		})
+
+		// XXX hack to support IsSet for flags with EnvVar
+		//
+		// There isn't an easy way to do this with the current implementation since
+		// whether a flag was set via an environment variable is very difficult to
+		// determine here. Instead, we intend to introduce a backwards incompatible
+		// change in version 2 to add `IsSet` to the Flag interface to push the
+		// responsibility closer to where the information required to determine
+		// whether a flag is set by non-standard means such as environment
+		// variables is avaliable.
+		//
+		// See https://github.com/urfave/cli/issues/294 for additional discussion
+		flags := c.Command.Flags
+		if c.Command.Name == "" { // cannot == Command{} since it contains slice types
+			if c.App != nil {
+				flags = c.App.Flags
+			}
+		}
+		for _, f := range flags {
+			eachName(f.GetName(), func(name string) {
+				if isSet, ok := c.setFlags[name]; isSet || !ok {
+					return
+				}
+
+				envVars := reflect.ValueOf(f).FieldByName("EnvVar").String()
+
+				eachName(envVars, func(envVar string) {
+					envVar = strings.TrimSpace(envVar)
+					if envVal := os.Getenv(envVar); envVal != "" {
+						c.setFlags[name] = true
+						return
+					}
+				})
+			})
+		}
+	}
+
+	return c.setFlags[name]
 }
 
 // GlobalIsSet determines if the global flag was actually set
 func (c *Context) GlobalIsSet(name string) bool {
-	if c.globalSetFlags == nil {
-		c.globalSetFlags = make(map[string]bool)
 	ctx := c
 	if ctx.parentContext != nil {
 		ctx = ctx.parentContext
 	}
-		for ; ctx != nil && c.globalSetFlags[name] == false; ctx = ctx.parentContext {
+
-			ctx.flagSet.Visit(func(f *flag.Flag) {
+	for ; ctx != nil; ctx = ctx.parentContext {
-				c.globalSetFlags[f.Name] = true
+		if ctx.IsSet(name) {
-			})
+			return true
 		}
 	}
-	return c.globalSetFlags[name]
+	return false
 }
 
 // FlagNames returns a slice of flag names used in this context.

context_test.go

@@ -2,6 +2,7 @@ package cli
 
 import (
 	"flag"
+	"os"
 	"testing"
 	"time"
 )
@@ -180,6 +181,33 @@ func TestContext_IsSet(t *testing.T) {
 	expect(t, c.IsSet("myflagGlobal"), false)
 }
 
+// XXX Corresponds to hack in context.IsSet for flags with EnvVar field
+// Should be moved to `flag_test` in v2
+func TestContext_IsSet_fromEnv(t *testing.T) {
+	var timeoutIsSet, tIsSet, noEnvVarIsSet, nIsSet bool
+
+	os.Clearenv()
+	os.Setenv("APP_TIMEOUT_SECONDS", "15.5")
+	a := App{
+		Flags: []Flag{
+			Float64Flag{Name: "timeout, t", EnvVar: "APP_TIMEOUT_SECONDS"},
+			Float64Flag{Name: "no-env-var, n"},
+		},
+		Action: func(ctx *Context) error {
+			timeoutIsSet = ctx.IsSet("timeout")
+			tIsSet = ctx.IsSet("t")
+			noEnvVarIsSet = ctx.IsSet("no-env-var")
+			nIsSet = ctx.IsSet("n")
+			return nil
+		},
+	}
+	a.Run([]string{"run"})
+	expect(t, timeoutIsSet, true)
+	expect(t, tIsSet, true)
+	expect(t, noEnvVarIsSet, false)
+	expect(t, nIsSet, false)
+}
+
 func TestContext_GlobalIsSet(t *testing.T) {
 	set := flag.NewFlagSet("test", 0)
 	set.Bool("myflag", false, "doc")
@@ -199,6 +227,38 @@ func TestContext_GlobalIsSet(t *testing.T) {
 	expect(t, c.GlobalIsSet("bogusGlobal"), false)
 }
 
+// XXX Corresponds to hack in context.IsSet for flags with EnvVar field
+// Should be moved to `flag_test` in v2
+func TestContext_GlobalIsSet_fromEnv(t *testing.T) {
+	var timeoutIsSet, tIsSet, noEnvVarIsSet, nIsSet bool
+
+	os.Clearenv()
+	os.Setenv("APP_TIMEOUT_SECONDS", "15.5")
+	a := App{
+		Flags: []Flag{
+			Float64Flag{Name: "timeout, t", EnvVar: "APP_TIMEOUT_SECONDS"},
+			Float64Flag{Name: "no-env-var, n"},
+		},
+		Commands: []Command{
+			{
+				Name: "hello",
+				Action: func(ctx *Context) error {
+					timeoutIsSet = ctx.GlobalIsSet("timeout")
+					tIsSet = ctx.GlobalIsSet("t")
+					noEnvVarIsSet = ctx.GlobalIsSet("no-env-var")
+					nIsSet = ctx.GlobalIsSet("n")
+					return nil
+				},
+			},
+		},
+	}
+	a.Run([]string{"run", "hello"})
+	expect(t, timeoutIsSet, true)
+	expect(t, tIsSet, true)
+	expect(t, noEnvVarIsSet, false)
+	expect(t, nIsSet, false)
+}
+
 func TestContext_NumFlags(t *testing.T) {
 	set := flag.NewFlagSet("test", 0)
 	set.Bool("myflag", false, "doc")