fs: fix setting stringArray config values from environment variables

After the config re-organisation, the setting of stringArray config
values (eg `--exclude` set with `RCLONE_EXCLUDE`) was broken and gave
a message like this for `RCLONE_EXCLUDE=*.jpg`:

    Failed to load "filter" default values: failed to initialise "filter" options:
    couldn't parse config item "exclude" = "*.jpg" as []string: parsing "*.jpg" as []string failed:
    invalid character '/' looking for beginning of value

This was caused by the parser trying to parse the input string as a
JSON value.

When the config was re-organised it was thought that the internal
representation of stringArray values was not important as it was never
visible externally, however this turned out not to be true.

A defined representation was chosen - a comma separated string and
this was documented and tests were introduced in this patch.

This potentially introduces a very small backwards incompatibility. In
rclone v1.67.0

    RCLONE_EXCLUDE=a,b

Would be interpreted as

    --exclude "a,b"

Whereas this new code will interpret it as

    --exclude "a" --exclude "b"

The benefit of being able to set multiple values with an environment
variable was deemed to outweigh the very small backwards compatibility
risk.

If a value with a `,` is needed, then use CSV escaping, eg

    RCLONE_EXCLUDE="a,b"

(Note this needs to have the quotes in so at the unix shell that would be

    RCLONE_EXCLUDE='"a,b"'

Fixes rclone#8063

Author: ncw

cmdtest/environment_test.go

@@ -6,7 +6,9 @@ package cmdtest
 
 import (
 	"os"
+	"regexp"
 	"runtime"
+	"strings"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
@@ -344,4 +346,42 @@ func TestEnvironmentVariables(t *testing.T) {
 	env = ""
 	out, err = rcloneEnv(env, "version", "-vv", "--use-json-log")
 	jsonLogOK()
+
+	// Find all the File filter lines in out and return them
+	parseFileFilters := func(out string) (extensions []string) {
+		// Match: - (^|/)[^/]*\.jpg$
+		find := regexp.MustCompile(`^- \(\^\|\/\)\[\^\/\]\*\\\.(.*?)\$$`)
+		for _, line := range strings.Split(out, "\n") {
+			if m := find.FindStringSubmatch(line); m != nil {
+				extensions = append(extensions, m[1])
+			}
+		}
+		return extensions
+	}
+
+	// Make sure that multiple valued (stringArray) environment variables are handled properly
+	env = ``
+	out, err = rcloneEnv(env, "version", "-vv", "--dump", "filters", "--exclude", "*.gif", "--exclude", "*.tif")
+	require.NoError(t, err)
+	assert.Equal(t, []string{"gif", "tif"}, parseFileFilters(out))
+
+	env = `RCLONE_EXCLUDE=*.jpg`
+	out, err = rcloneEnv(env, "version", "-vv", "--dump", "filters", "--exclude", "*.gif")
+	require.NoError(t, err)
+	assert.Equal(t, []string{"jpg", "gif"}, parseFileFilters(out))
+
+	env = `RCLONE_EXCLUDE=*.jpg,*.png`
+	out, err = rcloneEnv(env, "version", "-vv", "--dump", "filters", "--exclude", "*.gif", "--exclude", "*.tif")
+	require.NoError(t, err)
+	assert.Equal(t, []string{"jpg", "png", "gif", "tif"}, parseFileFilters(out))
+
+	env = `RCLONE_EXCLUDE="*.jpg","*.png"`
+	out, err = rcloneEnv(env, "version", "-vv", "--dump", "filters")
+	require.NoError(t, err)
+	assert.Equal(t, []string{"jpg", "png"}, parseFileFilters(out))
+
+	env = `RCLONE_EXCLUDE="*.,,,","*.png"`
+	out, err = rcloneEnv(env, "version", "-vv", "--dump", "filters")
+	require.NoError(t, err)
+	assert.Equal(t, []string{",,,", "png"}, parseFileFilters(out))
 }

docs/content/docs.md

@@ -2912,6 +2912,22 @@ so they take exactly the same form.
 
 The options set by environment variables can be seen with the `-vv` flag, e.g. `rclone version -vv`.
 
+Options that can appear multiple times (type `stringArray`) are
+treated slighly differently as environment variables can only be
+defined once. In order to allow a simple mechanism for adding one or
+many items, the input is treated as a [CSV encoded](https://godoc.org/encoding/csv)
+string. For example
+
+| Environment Variable | Equivalent options |
+|----------------------|--------------------|
+| `RCLONE_EXCLUDE="*.jpg"` | `--exclude "*.jpg"` |
+| `RCLONE_EXCLUDE="*.jpg,*.png"` | `--exclude "*.jpg"` `--exclude "*.png"` |
+| `RCLONE_EXCLUDE='"*.jpg","*.png"'` | `--exclude "*.jpg"` `--exclude "*.png"` |
+| `RCLONE_EXCLUDE='"/directory with comma , in it /**"'` | `--exclude "/directory with comma , in it /**" |
+
+If `stringArray` options are defined as environment variables **and**
+options on the command line then all the values will be used.
+
 ### Config file ###
 
 You can set defaults for values in the config file on an individual

fs/config/configstruct/configstruct.go

@@ -2,7 +2,7 @@
 package configstruct
 
 import (
-	"encoding/json"
+	"encoding/csv"
 	"errors"
 	"fmt"
 	"reflect"
@@ -31,7 +31,7 @@ func camelToSnake(in string) string {
 //
 // Builtin types are expected to be encoding as their natural
 // stringificatons as produced by fmt.Sprint except for []string which
-// is expected to be encoded as JSON with empty array encoded as "".
+// is expected to be encoded a a CSV with empty array encoded as "".
 //
 // Any other types are expected to be encoded by their String()
 // methods and decoded by their `Set(s string) error` methods.
@@ -58,14 +58,18 @@ func StringToInterface(def interface{}, in string) (newValue interface{}, err er
 	case time.Duration:
 		newValue, err = time.ParseDuration(in)
 	case []string:
-		// JSON decode arrays of strings
-		if in != "" {
-			var out []string
-			err = json.Unmarshal([]byte(in), &out)
-			newValue = out
-		} else {
-			// Empty string we will treat as empty array
+		// CSV decode arrays of strings - ideally we would use
+		// fs.CommaSepList here but we can't as it would cause
+		// a circular import.
+		if len(in) == 0 {
 			newValue = []string{}
+		} else {
+			r := csv.NewReader(strings.NewReader(in))
+			newValue, err = r.Read()
+			switch _err := err.(type) {
+			case *csv.ParseError:
+				err = _err.Err // remove line numbers from the error message
+			}
 		}
 	default:
 		// Try using a Set method

fs/config/configstruct/configstruct_test.go

@@ -204,9 +204,11 @@ func TestStringToInterface(t *testing.T) {
 		{"1m1s", fs.Duration(0), fs.Duration(61 * time.Second), ""},
 		{"1potato", fs.Duration(0), nil, `parsing "1potato" as fs.Duration failed: parsing time "1potato" as "2006-01-02": cannot parse "1potato" as "2006"`},
 		{``, []string{}, []string{}, ""},
-		{`[]`, []string(nil), []string{}, ""},
-		{`["hello"]`, []string{}, []string{"hello"}, ""},
-		{`["hello","world!"]`, []string(nil), []string{"hello", "world!"}, ""},
+		{`""`, []string(nil), []string{""}, ""},
+		{`hello`, []string{}, []string{"hello"}, ""},
+		{`"hello"`, []string{}, []string{"hello"}, ""},
+		{`hello,world!`, []string(nil), []string{"hello", "world!"}, ""},
+		{`"hello","world!"`, []string(nil), []string{"hello", "world!"}, ""},
 		{"1s", time.Duration(0), time.Second, ""},
 		{"1m1s", time.Duration(0), 61 * time.Second, ""},
 		{"1potato", time.Duration(0), nil, `parsing "1potato" as time.Duration failed: time: unknown unit "potato" in duration "1potato"`},

fs/config/flags/flags.go

@@ -143,12 +143,32 @@ func installFlag(flags *pflag.FlagSet, name string, groupsString string) {
 	// Read default from environment if possible
 	envKey := fs.OptionToEnv(name)
 	if envValue, envFound := os.LookupEnv(envKey); envFound {
-		err := flags.Set(name, envValue)
-		if err != nil {
-			fs.Fatalf(nil, "Invalid value when setting --%s from environment variable %s=%q: %v", name, envKey, envValue, err)
+		isStringArray := false
+		opt, isOption := flag.Value.(*fs.Option)
+		if isOption {
+			_, isStringArray = opt.Default.([]string)
+		}
+		if isStringArray {
+			// Treat stringArray differently, treating the environment variable as a CSV array
+			var list fs.CommaSepList
+			err := list.Set(envValue)
+			if err != nil {
+				fs.Fatalf(nil, "Invalid value when setting stringArray --%s from environment variable %s=%q: %v", name, envKey, envValue, err)
+			}
+			// Set both the Value (so items on the command line get added) and DefValue so the help is correct
+			opt.Value = ([]string)(list)
+			flag.DefValue = list.String()
+			for _, v := range list {
+				fs.Debugf(nil, "Setting --%s %q from environment variable %s=%q", name, v, envKey, envValue)
+			}
+		} else {
+			err := flags.Set(name, envValue)
+			if err != nil {
+				fs.Fatalf(nil, "Invalid value when setting --%s from environment variable %s=%q: %v", name, envKey, envValue, err)
+			}
+			fs.Debugf(nil, "Setting --%s %q from environment variable %s=%q", name, flag.Value, envKey, envValue)
+			flag.DefValue = envValue
 		}
-		fs.Debugf(nil, "Setting --%s %q from environment variable %s=%q", name, flag.Value, envKey, envValue)
-		flag.DefValue = envValue
 	}
 
 	// Add flag to Group if it is a global flag

fs/registry.go

@@ -264,14 +264,9 @@ func (o *Option) String() string {
 		if len(stringArray) == 0 {
 			return ""
 		}
-		// Encode string arrays as JSON
+		// Encode string arrays as CSV
 		// The default Go encoding can't be decoded uniquely
-		buf, err := json.Marshal(stringArray)
-		if err != nil {
-			Errorf(nil, "Can't encode default value for %q key - ignoring: %v", o.Name, err)
-			return "[]"
-		}
-		return string(buf)
+		return CommaSepList(stringArray).String()
 	}
 	return fmt.Sprint(v)
 }