config: demote Fetcher, introduce Environment

Previously, to fetch data out of the `*config.Configuration` type, a reference
to a `Fetcher` was used, a-la:

```
cfg.Env.Get(...)
```

This is quite convenient, however, it forces the LFS client to implement
several methods more than once. Consider the interface:

```
type Fetcher interface {
        Get(key string) (val string)

        Bool(key string, def bool) (val bool)
        // et. al.
}
```

In order to return typed information from a configuration instance, _each_
`Fetcher` must implement its own `N` methods for `Int`, `Bool`, etc.

To remedy this, the `Environment` type was introduced. It instead _has_ a
`Fetcher`, and defines its own type conversions, like so:

```
type Environment struct {
        f Fetcher
}

func (e *Environment) Bool(key string, def bool) (val bool) { }
func (e *Environment) Int(key string, def int)   (val int) { }

// et. al.
```

Now, the `config.Configuration` type holds a reference to an `Environment`, and
all type conversion methods are defined only once, saving time, and enforcing
consistency across multiple sources.

config/config.go

@@ -47,10 +47,10 @@ type FetchPruneConfig struct {
 }
 
 type Configuration struct {
-	// Env provides a Fetcher implementation used to access to the system's
+	// Env provides a `*Environment` used to access to the system's
 	// environment through os.Getenv. It is the point of entry for all
 	// system environment configuration.
-	Env Fetcher
+	Env *Environment
 
 	CurrentRemote   string
 	NtlmSession     ntlm.ClientSession
@@ -74,7 +74,7 @@ type Configuration struct {
 
 func New() *Configuration {
 	c := &Configuration{
-		Env: NewEnvFetcher(),
+		Env: EnvironmentOf(NewEnvFetcher()),
 
 		CurrentRemote: defaultRemote,
 		envVars:       make(map[string]string),
@@ -101,7 +101,7 @@ type Values struct {
 // This method should only be used during testing.
 func NewFrom(v Values) *Configuration {
 	config := &Configuration{
-		Env: mapFetcher(v.Env),
+		Env: EnvironmentOf(mapFetcher(v.Env)),
 
 		gitConfig: make(map[string]string, 0),
 		envVars:   make(map[string]string, 0),

config/env_fetcher.go

@@ -2,7 +2,6 @@ package config
 
 import (
 	"os"
-	"strings"
 	"sync"
 )
 
@@ -47,29 +46,3 @@ func (e *EnvFetcher) Get(key string) (val string) {
 
 	return v
 }
-
-// Bool returns the boolean state assosicated with a given key, or the value
-// "def", if no value was assosicated.
-//
-// The "boolean state assosicated with a given key" is defined as the
-// case-insensitive string comparsion with the following:
-//
-// 1) true if...
-//   "true", "1", "on", "yes", or "t"
-// 2) false if...
-//   "false", "0", "off", "no", "f", or otherwise.
-func (e *EnvFetcher) Bool(key string, def bool) (val bool) {
-	s := e.Get(key)
-	if len(s) == 0 {
-		return def
-	}
-
-	switch strings.ToLower(s) {
-	case "true", "1", "on", "yes", "t":
-		return true
-	case "false", "0", "off", "no", "f":
-		return false
-	default:
-		return false
-	}
-}

config/environment.go

@@ -0,0 +1,77 @@
+package config
+
+import (
+	"strconv"
+	"strings"
+)
+
+// An Environment adds additional behavior to a Fetcher, such a type conversion,
+// and default values.
+//
+// `Environment`s are the primary way to communicate with various configuration
+// sources, such as the OS environment variables, the `.gitconfig`, and even
+// `map[string]string`s.
+type Environment struct {
+	// Fetcher is the `Environment`'s source of data.
+	Fetcher Fetcher
+}
+
+// EnvironmentOf creates a new `*Environment` initialized with the givne
+// `Fetcher`, "f".
+func EnvironmentOf(f Fetcher) *Environment {
+	return &Environment{f}
+}
+
+// Get is shorthand for calling `e.Fetcher.Get(key)`.
+func (e *Environment) Get(key string) (val string) {
+	return e.Fetcher.Get(key)
+}
+
+// Bool returns the boolean state associated with a given key, or the value
+// "def", if no value was associated.
+//
+// The "boolean state associated with a given key" is defined as the
+// case-insensitive string comparison with the following:
+//
+// 1) true if...
+//   "true", "1", "on", "yes", or "t"
+// 2) false if...
+//   "false", "0", "off", "no", "f", or otherwise.
+func (e *Environment) Bool(key string, def bool) (val bool) {
+	s := e.Fetcher.Get(key)
+	if len(s) == 0 {
+		return def
+	}
+
+	switch strings.ToLower(s) {
+	case "true", "1", "on", "yes", "t":
+		return true
+	case "false", "0", "off", "no", "f":
+		return false
+	default:
+		return false
+	}
+}
+
+// Int returns the int value associated with a given key, or the value "def",
+// if no value was associated.
+//
+// To convert from a the string value attached to a given key,
+// `strconv.Atoi(val)` is called. If `Atoi` returned a non-nil error, then the
+// value "def" will be returned instead.
+//
+// Otherwise, if the value was converted `string -> int` successfully, then it
+// will be returned wholesale.
+func (e *Environment) Int(key string, def int) (val int) {
+	s := e.Fetcher.Get(key)
+	if len(s) == 0 {
+		return def
+	}
+
+	i, err := strconv.Atoi(s)
+	if err != nil {
+		return def
+	}
+
+	return i
+}

config/environment_test.go

@@ -0,0 +1,95 @@
+package config_test
+
+import (
+	"testing"
+
+	. "github.com/github/git-lfs/config"
+	"github.com/ttaylorr/assert"
+)
+
+func TestEnvironmentOfReturnsCorrectlyInitializedEnvironment(t *testing.T) {
+	fetcher := new(MockFetcher)
+
+	env := EnvironmentOf(fetcher)
+
+	assert.Equal(t, fetcher, env.Fetcher)
+}
+
+func TestEnvironmentGetDelegatesToFetcher(t *testing.T) {
+	var fetcher MockFetcher
+	fetcher.On("Get", "foo").Return("bar").Once()
+
+	env := EnvironmentOf(&fetcher)
+	val := env.Get("foo")
+
+	fetcher.AssertExpectations(t)
+	assert.Equal(t, "bar", val)
+}
+
+func TestEnvironmentBoolTruthyConversion(t *testing.T) {
+	for _, c := range []EnvironmentConversionTestCase{
+		{"", true, GetBoolDefault(true)},
+		{"", false, GetBoolDefault(false)},
+
+		{"true", true, GetBoolDefault(false)},
+		{"1", true, GetBoolDefault(false)},
+		{"on", true, GetBoolDefault(false)},
+		{"yes", true, GetBoolDefault(false)},
+		{"t", true, GetBoolDefault(false)},
+
+		{"false", false, GetBoolDefault(true)},
+		{"0", false, GetBoolDefault(true)},
+		{"off", false, GetBoolDefault(true)},
+		{"no", false, GetBoolDefault(true)},
+		{"f", false, GetBoolDefault(true)},
+	} {
+		c.Assert(t)
+	}
+}
+
+func TestEnvironmentIntTestCases(t *testing.T) {
+	for _, c := range []EnvironmentConversionTestCase{
+		{"", 1, GetIntDefault(1)},
+
+		{"1", 1, GetIntDefault(0)},
+		{"3", 3, GetIntDefault(0)},
+
+		{"malformed", 7, GetIntDefault(7)},
+	} {
+		c.Assert(t)
+	}
+}
+
+type EnvironmentConversionTestCase struct {
+	Val      string
+	Expected interface{}
+
+	GotFn func(env *Environment, key string) interface{}
+}
+
+var (
+	GetBoolDefault = func(def bool) func(e *Environment, key string) interface{} {
+		return func(e *Environment, key string) interface{} {
+			return e.Bool(key, def)
+		}
+	}
+
+	GetIntDefault = func(def int) func(e *Environment, key string) interface{} {
+		return func(e *Environment, key string) interface{} {
+			return e.Int(key, def)
+		}
+	}
+)
+
+func (c *EnvironmentConversionTestCase) Assert(t *testing.T) {
+	var fetcher MockFetcher
+	fetcher.On("Get", c.Val).Return(c.Val).Once()
+
+	env := EnvironmentOf(&fetcher)
+	got := c.GotFn(env, c.Val)
+
+	if c.Expected != got {
+		t.Errorf("lfs/config: expected val=%q to be %q (got: %q)", c.Val, c.Expected, got)
+	}
+	fetcher.AssertExpectations(t)
+}

config/fetcher.go

@@ -7,8 +7,4 @@ type Fetcher interface {
 	// Get returns the string value assosicated with a given key, or an
 	// empty string if none exists.
 	Get(key string) (val string)
-
-	// Bool returns the boolean value assosicated with a given key, or an
-	// empty string if none exists.
-	Bool(key string, def bool) (val bool)
 }

config/fetcher_test.go

@@ -0,0 +1,11 @@
+package config_test
+
+import "github.com/stretchr/testify/mock"
+
+type MockFetcher struct {
+	mock.Mock
+}
+
+func (m *MockFetcher) Get(key string) (val string) {
+	return m.Called(key).String(0)
+}

config/map_fetcher.go

@@ -1,28 +1,8 @@
 package config
 
-import "strconv"
-
 // mapFetcher provides an implementation of the Fetcher interface by wrapping
 // the `map[string]string` type.
 type mapFetcher map[string]string
 
 // Get implements the func `Fetcher.Get`.
 func (m mapFetcher) Get(key string) (val string) { return m[key] }
-
-// Bool implements the function Fetcher.Bool.
-//
-// NOTE: It exists as a temporary measure before the Environment type is
-// abstracted (see github/git-lfs#1415).
-func (m mapFetcher) Bool(key string, def bool) (val bool) {
-	s := m.Get(key)
-	if len(s) == 0 {
-		return def
-	}
-
-	b, err := strconv.ParseBool(m.Get(key))
-	if err != nil {
-		return def
-	}
-
-	return b
-}