From 58c99427b3f827e2c9691ed6410d8163f9afe1b3 Mon Sep 17 00:00:00 2001
From: Ole Frost <82263101+olefrost@users.noreply.github.com>
Date: Thu, 20 May 2021 14:08:01 +0200
Subject: [PATCH] config: fixed issues with flags/options set by environment
 vars.
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Some environment variables didn’t behave like their corresponding
command line flags. The affected flags were --stats, --log-level,
--separator, --multi-tread-streams, --rc-addr, --rc-user and --rc-pass.
Example:

    RCLONE_STATS='10s'
    rclone check remote: remote: --progress
    # Expected: rclone check remote: remote: --progress –-stats=10s
    # Actual: rclone check remote: remote: --progress

Remote specific options set by environment variables was overruled by
less specific backend options set by environment variables. Example:

    RCLONE_DRIVE_USE_TRASH='false'
    RCLONE_CONFIG_MYDRIVE_USE_TRASH='true'
    rclone deletefile myDrive:my-test-file
    # Expected: my-test-file is recoverable in the trash folder
    # Actual: my-test-file is permanently deleted (not recoverable)

Backend specific options set by environment variables was overruled by
general backend options set by environment variables. Example:

    RCLONE_SKIP_LINKS='true'
    RCLONE_LOCAL_SKIP_LINKS='false'
    rclone lsd local:
    # Expected result: Warnings when symlinks are skipped
    # Actual result: No warnings when symlinks are skipped
    # That is RCLONE_SKIP_LINKS takes precedence

The above issues have been fixed.

The debug logging (-vv) has been enhanced to show when flags are set by
environment variables.

The documentation has been enhanced with details on the precedence of
configuration options.

See pull request #5341 for more information.
---
 cmd/cmd.go               |   3 +-
 docs/content/docs.md     |  22 ++++---
 fs/config.go             |  15 +++++
 fs/config/flags/flags.go | 124 +++++++++++++++++++++------------------
 fs/configmap.go          |  22 +++++--
 fs/rc/registry.go        |   4 +-
 6 files changed, 117 insertions(+), 73 deletions(-)

diff --git a/cmd/cmd.go b/cmd/cmd.go
index 716889d5a..c8bc202c5 100644
--- a/cmd/cmd.go
+++ b/cmd/cmd.go
@@ -539,7 +539,8 @@ func AddBackendFlags() {
 				if opt.IsPassword {
 					help += " (obscured)"
 				}
-				flag := flags.VarPF(pflag.CommandLine, opt, name, opt.ShortOpt, help)
+				flag := pflag.CommandLine.VarPF(opt, name, opt.ShortOpt, help)
+				flags.SetDefaultFromEnv(pflag.CommandLine, name)
 				if _, isBool := opt.Default.(bool); isBool {
 					flag.NoOptDefVal = "true"
 				}
diff --git a/docs/content/docs.md b/docs/content/docs.md
index ceb24c6a1..ea547b7aa 100644
--- a/docs/content/docs.md
+++ b/docs/content/docs.md
@@ -2129,6 +2129,8 @@ Or to always use the trash in drive `--drive-use-trash`, set
 The same parser is used for the options and the environment variables
 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`.
+
 ### Config file ###
 
 You can set defaults for values in the config file on an individual
@@ -2165,16 +2167,20 @@ it is probably easier to use those instead which makes the above example
 The various different methods of backend configuration are read in
 this order and the first one with a value is used.
 
-- Flag values as supplied on the command line, e.g. `--drive-use-trash`.
-- Remote specific environment vars, e.g. `RCLONE_CONFIG_MYREMOTE_USE_TRASH` (see above).
-- Backend specific environment vars, e.g. `RCLONE_DRIVE_USE_TRASH`.
-- Config file, e.g. `use_trash = false`.
-- Default values, e.g. `true` - these can't be changed.
+- Parameters in connection strings, e.g. `myRemote,skip_links:`
+- Flag values as supplied on the command line, e.g. `--skip-links`
+- Remote specific environment vars, e.g. `RCLONE_CONFIG_MYREMOTE_SKIP_LINKS` (see above).
+- Backend specific environment vars, e.g. `RCLONE_LOCAL_SKIP_LINKS`.
+- Backend generic environment vars, e.g. `RCLONE_SKIP_LINKS`.
+- Config file, e.g. `skip_links = true`.
+- Default values, e.g. `false` - these can't be changed.
 
-So if both `--drive-use-trash` is supplied on the config line and an
-environment variable `RCLONE_DRIVE_USE_TRASH` is set, the command line
+So if both `--skip-links` is supplied on the command line and an
+environment variable `RCLONE_LOCAL_SKIP_LINKS` is set, the command line
 flag will take preference.
 
+The backend configurations set by environment variables can be seen with the `-vv` flag, e.g. `rclone about myRemote: -vv`.
+
 For non backend configuration the order is as follows:
 
 - Flag values as supplied on the command line, e.g. `--stats 5s`.
@@ -2189,3 +2195,5 @@ For non backend configuration the order is as follows:
     - The environment values may be either a complete URL or a "host[:port]" for, in which case the "http" scheme is assumed.
 - `USER` and `LOGNAME` values are used as fallbacks for current username. The primary method for looking up username is OS-specific: Windows API on Windows, real user ID in /etc/passwd on Unix systems. In the documentation the current username is simply referred to as `$USER`.
 - `RCLONE_CONFIG_DIR` - rclone **sets** this variable for use in config files and sub processes to point to the directory holding the config file.
+
+The options set by environment variables can be seen with the `-vv` and `--log-level=DEBUG` flags, e.g. `rclone version -vv`.
diff --git a/fs/config.go b/fs/config.go
index 8c31d5a73..1ed3000eb 100644
--- a/fs/config.go
+++ b/fs/config.go
@@ -3,6 +3,7 @@ package fs
 import (
 	"context"
 	"net"
+	"os"
 	"strings"
 	"time"
 
@@ -169,6 +170,20 @@ func NewConfig() *ConfigInfo {
 	c.FsCacheExpireDuration = 300 * time.Second
 	c.FsCacheExpireInterval = 60 * time.Second
 
+	// Perform a simple check for debug flags to enable debug logging during the flag initialization
+	for argIndex, arg := range os.Args {
+		if strings.HasPrefix(arg, "-vv") && strings.TrimRight(arg, "v") == "-" {
+			c.LogLevel = LogLevelDebug
+		}
+		if arg == "--log-level=DEBUG" || (arg == "--log-level" && len(os.Args) > argIndex+1 && os.Args[argIndex+1] == "DEBUG") {
+			c.LogLevel = LogLevelDebug
+		}
+	}
+	envValue, found := os.LookupEnv("RCLONE_LOG_LEVEL")
+	if found && envValue == "DEBUG" {
+		c.LogLevel = LogLevelDebug
+	}
+
 	return c
 }
 
diff --git a/fs/config/flags/flags.go b/fs/config/flags/flags.go
index d248249f4..31f59e457 100644
--- a/fs/config/flags/flags.go
+++ b/fs/config/flags/flags.go
@@ -11,194 +11,206 @@ import (
 	"github.com/spf13/pflag"
 )
 
-// setDefaultFromEnv constructs a name from the flag passed in and
-// sets the default from the environment if possible.
-func setDefaultFromEnv(flags *pflag.FlagSet, name string) {
-	key := fs.OptionToEnv(name)
-	newValue, found := os.LookupEnv(key)
+// setValueFromEnv constructs a name from the flag passed in and
+// sets the value and default from the environment if possible
+// the value may be overridden when the command line is parsed
+//
+// Used to create non-backend flags like --stats
+func setValueFromEnv(flags *pflag.FlagSet, name string) {
+	envKey := fs.OptionToEnv(name)
+	envValue, found := os.LookupEnv(envKey)
 	if found {
 		flag := flags.Lookup(name)
 		if flag == nil {
 			log.Fatalf("Couldn't find flag --%q", name)
 		}
-		err := flag.Value.Set(newValue)
+		err := flags.Set(name, envValue)
 		if err != nil {
-			log.Fatalf("Invalid value for environment variable %q when setting default for --%s: %v", key, name, err)
+			log.Fatalf("Invalid value when setting --%s from environment variable %s=%q: %v", name, envKey, envValue, err)
 		}
-		fs.Debugf(nil, "Set default for --%q from %q to %q (%v)", name, key, newValue, flag.Value)
-		flag.DefValue = newValue
+		fs.Debugf(nil, "Setting --%s %q from environment variable %s=%q", name, flag.Value, envKey, envValue)
+		flag.DefValue = envValue
 	}
 }
 
-// StringP defines a flag which can be overridden by an environment variable
+// SetDefaultFromEnv constructs a name from the flag passed in and
+// sets the default from the environment if possible
+//
+// Used to create backend flags like --skip-links
+func SetDefaultFromEnv(flags *pflag.FlagSet, name string) {
+	envKey := fs.OptionToEnv(name)
+	envValue, found := os.LookupEnv(envKey)
+	if found {
+		flag := flags.Lookup(name)
+		if flag == nil {
+			log.Fatalf("Couldn't find flag --%q", name)
+		}
+		fs.Debugf(nil, "Setting default for %s=%q from environment variable %s", name, envValue, envKey)
+		//err = tempValue.Set()
+		flag.DefValue = envValue
+	}
+}
+
+// StringP defines a flag which can be set by an environment variable
 //
 // It is a thin wrapper around pflag.StringP
 func StringP(name, shorthand string, value string, usage string) (out *string) {
 	out = pflag.StringP(name, shorthand, value, usage)
-	setDefaultFromEnv(pflag.CommandLine, name)
+	setValueFromEnv(pflag.CommandLine, name)
 	return out
 }
 
-// StringVarP defines a flag which can be overridden by an environment variable
+// StringVarP defines a flag which can be set by an environment variable
 //
 // It is a thin wrapper around pflag.StringVarP
 func StringVarP(flags *pflag.FlagSet, p *string, name, shorthand string, value string, usage string) {
 	flags.StringVarP(p, name, shorthand, value, usage)
-	setDefaultFromEnv(flags, name)
+	setValueFromEnv(flags, name)
 }
 
-// BoolP defines a flag which can be overridden by an environment variable
+// BoolP defines a flag which can be set by an environment variable
 //
 // It is a thin wrapper around pflag.BoolP
 func BoolP(name, shorthand string, value bool, usage string) (out *bool) {
 	out = pflag.BoolP(name, shorthand, value, usage)
-	setDefaultFromEnv(pflag.CommandLine, name)
+	setValueFromEnv(pflag.CommandLine, name)
 	return out
 }
 
-// BoolVarP defines a flag which can be overridden by an environment variable
+// BoolVarP defines a flag which can be set by an environment variable
 //
 // It is a thin wrapper around pflag.BoolVarP
 func BoolVarP(flags *pflag.FlagSet, p *bool, name, shorthand string, value bool, usage string) {
 	flags.BoolVarP(p, name, shorthand, value, usage)
-	setDefaultFromEnv(flags, name)
+	setValueFromEnv(flags, name)
 }
 
-// IntP defines a flag which can be overridden by an environment variable
+// IntP defines a flag which can be set by an environment variable
 //
 // It is a thin wrapper around pflag.IntP
 func IntP(name, shorthand string, value int, usage string) (out *int) {
 	out = pflag.IntP(name, shorthand, value, usage)
-	setDefaultFromEnv(pflag.CommandLine, name)
+	setValueFromEnv(pflag.CommandLine, name)
 	return out
 }
 
-// Int64P defines a flag which can be overridden by an environment variable
+// Int64P defines a flag which can be set by an environment variable
 //
 // It is a thin wrapper around pflag.IntP
 func Int64P(name, shorthand string, value int64, usage string) (out *int64) {
 	out = pflag.Int64P(name, shorthand, value, usage)
-	setDefaultFromEnv(pflag.CommandLine, name)
+	setValueFromEnv(pflag.CommandLine, name)
 	return out
 }
 
-// Int64VarP defines a flag which can be overridden by an environment variable
+// Int64VarP defines a flag which can be set by an environment variable
 //
 // It is a thin wrapper around pflag.Int64VarP
 func Int64VarP(flags *pflag.FlagSet, p *int64, name, shorthand string, value int64, usage string) {
 	flags.Int64VarP(p, name, shorthand, value, usage)
-	setDefaultFromEnv(flags, name)
+	setValueFromEnv(flags, name)
 }
 
-// IntVarP defines a flag which can be overridden by an environment variable
+// IntVarP defines a flag which can be set by an environment variable
 //
 // It is a thin wrapper around pflag.IntVarP
 func IntVarP(flags *pflag.FlagSet, p *int, name, shorthand string, value int, usage string) {
 	flags.IntVarP(p, name, shorthand, value, usage)
-	setDefaultFromEnv(flags, name)
+	setValueFromEnv(flags, name)
 }
 
-// Uint32VarP defines a flag which can be overridden by an environment variable
+// Uint32VarP defines a flag which can be set by an environment variable
 //
 // It is a thin wrapper around pflag.Uint32VarP
 func Uint32VarP(flags *pflag.FlagSet, p *uint32, name, shorthand string, value uint32, usage string) {
 	flags.Uint32VarP(p, name, shorthand, value, usage)
-	setDefaultFromEnv(flags, name)
+	setValueFromEnv(flags, name)
 }
 
-// Float64P defines a flag which can be overridden by an environment variable
+// Float64P defines a flag which can be set by an environment variable
 //
 // It is a thin wrapper around pflag.Float64P
 func Float64P(name, shorthand string, value float64, usage string) (out *float64) {
 	out = pflag.Float64P(name, shorthand, value, usage)
-	setDefaultFromEnv(pflag.CommandLine, name)
+	setValueFromEnv(pflag.CommandLine, name)
 	return out
 }
 
-// Float64VarP defines a flag which can be overridden by an environment variable
+// Float64VarP defines a flag which can be set by an environment variable
 //
 // It is a thin wrapper around pflag.Float64VarP
 func Float64VarP(flags *pflag.FlagSet, p *float64, name, shorthand string, value float64, usage string) {
 	flags.Float64VarP(p, name, shorthand, value, usage)
-	setDefaultFromEnv(flags, name)
+	setValueFromEnv(flags, name)
 }
 
-// DurationP defines a flag which can be overridden by an environment variable
+// DurationP defines a flag which can be set by an environment variable
 //
 // It is a thin wrapper around pflag.DurationP
 func DurationP(name, shorthand string, value time.Duration, usage string) (out *time.Duration) {
 	out = pflag.DurationP(name, shorthand, value, usage)
-	setDefaultFromEnv(pflag.CommandLine, name)
+	setValueFromEnv(pflag.CommandLine, name)
 	return out
 }
 
-// DurationVarP defines a flag which can be overridden by an environment variable
+// DurationVarP defines a flag which can be set by an environment variable
 //
 // It is a thin wrapper around pflag.DurationVarP
 func DurationVarP(flags *pflag.FlagSet, p *time.Duration, name, shorthand string, value time.Duration, usage string) {
 	flags.DurationVarP(p, name, shorthand, value, usage)
-	setDefaultFromEnv(flags, name)
+	setValueFromEnv(flags, name)
 }
 
-// VarP defines a flag which can be overridden by an environment variable
+// VarP defines a flag which can be set by an environment variable
 //
 // It is a thin wrapper around pflag.VarP
 func VarP(value pflag.Value, name, shorthand, usage string) {
 	pflag.VarP(value, name, shorthand, usage)
-	setDefaultFromEnv(pflag.CommandLine, name)
+	setValueFromEnv(pflag.CommandLine, name)
 }
 
-// FVarP defines a flag which can be overridden by an environment variable
+// FVarP defines a flag which can be set by an environment variable
 //
 // It is a thin wrapper around pflag.VarP
 func FVarP(flags *pflag.FlagSet, value pflag.Value, name, shorthand, usage string) {
 	flags.VarP(value, name, shorthand, usage)
-	setDefaultFromEnv(flags, name)
+	setValueFromEnv(flags, name)
 }
 
-// VarPF defines a flag which can be overridden by an environment variable
-//
-// It is a thin wrapper around pflag.VarPF
-func VarPF(flags *pflag.FlagSet, value pflag.Value, name, shorthand, usage string) *pflag.Flag {
-	flag := flags.VarPF(value, name, shorthand, usage)
-	setDefaultFromEnv(flags, name)
-	return flag
-}
-
-// StringArrayP defines a flag which can be overridden by an environment variable
+// StringArrayP defines a flag which can be set by an environment variable
 //
 // It sets one value only - command line flags can be used to set more.
 //
 // It is a thin wrapper around pflag.StringArrayP
 func StringArrayP(name, shorthand string, value []string, usage string) (out *[]string) {
 	out = pflag.StringArrayP(name, shorthand, value, usage)
-	setDefaultFromEnv(pflag.CommandLine, name)
+	setValueFromEnv(pflag.CommandLine, name)
 	return out
 }
 
-// StringArrayVarP defines a flag which can be overridden by an environment variable
+// StringArrayVarP defines a flag which can be set by an environment variable
 //
 // It sets one value only - command line flags can be used to set more.
 //
 // It is a thin wrapper around pflag.StringArrayVarP
 func StringArrayVarP(flags *pflag.FlagSet, p *[]string, name, shorthand string, value []string, usage string) {
 	flags.StringArrayVarP(p, name, shorthand, value, usage)
-	setDefaultFromEnv(flags, name)
+	setValueFromEnv(flags, name)
 }
 
-// CountP defines a flag which can be overridden by an environment variable
+// CountP defines a flag which can be set by an environment variable
 //
 // It is a thin wrapper around pflag.CountP
 func CountP(name, shorthand string, usage string) (out *int) {
 	out = pflag.CountP(name, shorthand, usage)
-	setDefaultFromEnv(pflag.CommandLine, name)
+	setValueFromEnv(pflag.CommandLine, name)
 	return out
 }
 
-// CountVarP defines a flag which can be overridden by an environment variable
+// CountVarP defines a flag which can be set by an environment variable
 //
 // It is a thin wrapper around pflag.CountVarP
 func CountVarP(flags *pflag.FlagSet, p *int, name, shorthand string, usage string) {
 	flags.CountVarP(p, name, shorthand, usage)
-	setDefaultFromEnv(flags, name)
+	setValueFromEnv(flags, name)
 }
diff --git a/fs/configmap.go b/fs/configmap.go
index db2ca6aff..e788771ec 100644
--- a/fs/configmap.go
+++ b/fs/configmap.go
@@ -14,7 +14,12 @@ type configEnvVars string
 
 // Get a config item from the environment variables if possible
 func (configName configEnvVars) Get(key string) (value string, ok bool) {
-	return os.LookupEnv(ConfigToEnv(string(configName), key))
+	envKey := ConfigToEnv(string(configName), key)
+	value, ok = os.LookupEnv(envKey)
+	if ok {
+		Debugf(nil, "Setting %s=%q for %q from environment variable %s", key, value, configName, envKey)
+	}
+	return value, ok
 }
 
 // A configmap.Getter to read from the environment RCLONE_option_name
@@ -28,14 +33,19 @@ func (oev optionEnvVars) Get(key string) (value string, ok bool) {
 	if opt == nil {
 		return "", false
 	}
-	// For options with NoPrefix set, check without prefix too
-	if opt.NoPrefix {
-		value, ok = os.LookupEnv(OptionToEnv(key))
+	envKey := OptionToEnv(oev.fsInfo.Prefix + "-" + key)
+	value, ok = os.LookupEnv(envKey)
+	if ok {
+		Debugf(nil, "Setting %s_%s=%q from environment variable %s", oev.fsInfo.Prefix, key, value, envKey)
+	} else if opt.NoPrefix {
+		// For options with NoPrefix set, check without prefix too
+		envKey := OptionToEnv(key)
+		value, ok = os.LookupEnv(envKey)
 		if ok {
-			return value, ok
+			Debugf(nil, "Setting %s=%q for %s from environment variable %s", key, value, oev.fsInfo.Prefix, envKey)
 		}
 	}
-	return os.LookupEnv(OptionToEnv(oev.fsInfo.Prefix + "-" + key))
+	return value, ok
 }
 
 // A configmap.Getter to read either the default value or the set
diff --git a/fs/rc/registry.go b/fs/rc/registry.go
index 4cc80541a..8cc11d526 100644
--- a/fs/rc/registry.go
+++ b/fs/rc/registry.go
@@ -7,8 +7,6 @@ import (
 	"sort"
 	"strings"
 	"sync"
-
-	"github.com/rclone/rclone/fs"
 )
 
 // Func defines a type for a remote control function
@@ -45,7 +43,7 @@ func (r *Registry) Add(call Call) {
 	defer r.mu.Unlock()
 	call.Path = strings.Trim(call.Path, "/")
 	call.Help = strings.TrimSpace(call.Help)
-	fs.Debugf(nil, "Adding path %q to remote control registry", call.Path)
+	// fs.Debugf(nil, "Adding path %q to remote control registry", call.Path) // disabled to make initialization less verbose
 	r.call[call.Path] = &call
 }