refactor yes(), cleanup env var semantics, fixes #355

refactorings:

- introduced concept of default answer:

if the answer string is in the defaultish sequence, the return value of yes() will be the default.
e.g. if just pressing <enter> when asked on the console or if an empty string or "default" is
in the environment variable for overriding.

if an environment var has an invalid value and no retries are enabled: return default
if retries are enabled, next retry won't use the env var again, but either ask via input().

- simplify:

only one default - this should be a SAFE default as it is used in some special conditions
like EOF or invalid input with retries disallowed.

no isatty() magic, the "yes" shell command exists, so we could receive input even if it is not from a tty.

- clean:

separate retry flag from retry_msg

borg/archiver.py

@@ -105,8 +105,8 @@ class Archiver:
             msg = ("'check --repair' is an experimental feature that might result in data loss." +
                    "\n" +
                    "Type 'YES' if you understand this and want to continue: ")
-            if not yes(msg, false_msg="Aborting.", default_notty=False,
-                       env_var_override='BORG_CHECK_I_KNOW_WHAT_I_AM_DOING', truish=('YES', )):
+            if not yes(msg, false_msg="Aborting.", truish=('YES', ),
+                       env_var_override='BORG_CHECK_I_KNOW_WHAT_I_AM_DOING'):
                 return EXIT_ERROR
         if not args.archives_only:
             if not repository.check(repair=args.repair, save_space=args.save_space):
@@ -374,8 +374,8 @@ class Archiver:
                         msg.append(format_archive(archive_info))
                     msg.append("Type 'YES' if you understand this and want to continue: ")
                     msg = '\n'.join(msg)
-                    if not yes(msg, false_msg="Aborting.", default_notty=False,
-                               env_var_override='BORG_DELETE_I_KNOW_WHAT_I_AM_DOING', truish=('YES', )):
+                    if not yes(msg, false_msg="Aborting.", truish=('YES', ),
+                               env_var_override='BORG_DELETE_I_KNOW_WHAT_I_AM_DOING'):
                         self.exit_code = EXIT_ERROR
                         return self.exit_code
                     repository.destroy()

borg/cache.py

@@ -54,8 +54,7 @@ class Cache:
                 msg = ("Warning: Attempting to access a previously unknown unencrypted repository!" +
                        "\n" +
                        "Do you want to continue? [yN] ")
-                if not yes(msg, false_msg="Aborting.", default_notty=False,
-                           env_var_override='BORG_UNKNOWN_UNENCRYPTED_REPO_ACCESS_IS_OK'):
+                if not yes(msg, false_msg="Aborting.", env_var_override='BORG_UNKNOWN_UNENCRYPTED_REPO_ACCESS_IS_OK'):
                     raise self.CacheInitAbortedError()
             self.create()
         self.open(lock_wait=lock_wait)
@@ -64,8 +63,7 @@ class Cache:
             msg = ("Warning: The repository at location {} was previously located at {}".format(repository._location.canonical_path(), self.previous_location) +
                    "\n" +
                    "Do you want to continue? [yN] ")
-            if not yes(msg, false_msg="Aborting.", default_notty=False,
-                       env_var_override='BORG_RELOCATED_REPO_ACCESS_IS_OK'):
+            if not yes(msg, false_msg="Aborting.", env_var_override='BORG_RELOCATED_REPO_ACCESS_IS_OK'):
                 raise self.RepositoryAccessAborted()
 
         if sync and self.manifest.id != self.manifest_id:

borg/helpers.py

@@ -842,71 +842,69 @@ def is_slow_msgpack():
     return msgpack.Packer is msgpack.fallback.Packer
 
 
-def yes(msg=None, retry_msg=None, false_msg=None, true_msg=None,
-        default=False, default_notty=None, default_eof=None,
-        falsish=('No', 'no', 'N', 'n'), truish=('Yes', 'yes', 'Y', 'y'),
-        env_var_override=None, ifile=None, ofile=None, input=input):
+FALSISH = ('No', 'NO', 'no', 'N', 'n', '0', )
+TRUISH = ('Yes', 'YES', 'yes', 'Y', 'y', '1', )
+DEFAULTISH = ('Default', 'DEFAULT', 'default', 'D', 'd', '', )
+
+def yes(msg=None, false_msg=None, true_msg=None, default_msg=None,
+        retry_msg=None, invalid_msg=None, env_msg=None,
+        falsish=FALSISH, truish=TRUISH, defaultish=DEFAULTISH,
+        default=False, retry=True, env_var_override=None, ofile=None, input=input):
     """
     Output <msg> (usually a question) and let user input an answer.
-    Qualifies the answer according to falsish and truish as True or False.
+    Qualifies the answer according to falsish, truish and defaultish as True, False or <default>.
     If it didn't qualify and retry_msg is None (no retries wanted),
     return the default [which defaults to False]. Otherwise let user retry
     answering until answer is qualified.
 
-    If env_var_override is given and it is non-empty, counts as truish answer
-    and won't ask user for an answer.
-    If we don't have a tty as input and default_notty is not None, return its value.
-    Otherwise read input from non-tty and proceed as normal.
-    If EOF is received instead an input, return default_eof [or default, if not given].
+    If env_var_override is given and this var is present in the environment, do not ask
+    the user, but just use the env var contents as answer as if it was typed in.
+    Otherwise read input from stdin and proceed as normal.
+    If EOF is received instead an input or an invalid input without retry possibility,
+    return default.
 
     :param msg: introducing message to output on ofile, no \n is added [None]
     :param retry_msg: retry message to output on ofile, no \n is added [None]
-           (also enforces retries instead of returning default)
     :param false_msg: message to output before returning False [None]
     :param true_msg: message to output before returning True [None]
-    :param default: default return value (empty answer is given) [False]
-    :param default_notty: if not None, return its value if no tty is connected [None]
-    :param default_eof: return value if EOF was read as answer [same as default]
+    :param default_msg: message to output before returning a <default> [None]
+    :param invalid_msg: message to output after a invalid answer was given [None]
+    :param env_msg: message to output when using input from env_var_override [None],
+           needs to have 2 placeholders for answer and env var name, e.g.: "{} (from {})"
     :param falsish: sequence of answers qualifying as False
     :param truish: sequence of answers qualifying as True
+    :param defaultish: sequence of answers qualifying as <default>
+    :param default: default return value (defaultish answer was given or no-answer condition) [False]
+    :param retry: if True and input is incorrect, retry. Otherwise return default. [True]
     :param env_var_override: environment variable name [None]
-    :param ifile: input stream [sys.stdin] (only for testing!)
     :param ofile: output stream [sys.stderr]
     :param input: input function [input from builtins]
     :return: boolean answer value, True or False
     """
-    # note: we do not assign sys.stdin/stderr as defaults above, so they are
+    # note: we do not assign sys.stderr as default above, so it is
     # really evaluated NOW,  not at function definition time.
-    if ifile is None:
-        ifile = sys.stdin
     if ofile is None:
         ofile = sys.stderr
     if default not in (True, False):
         raise ValueError("invalid default value, must be True or False")
-    if default_notty not in (None, True, False):
-        raise ValueError("invalid default_notty value, must be None, True or False")
-    if default_eof not in (None, True, False):
-        raise ValueError("invalid default_eof value, must be None, True or False")
     if msg:
-        print(msg, file=ofile, end='')
-        ofile.flush()
-    if env_var_override:
-        value = os.environ.get(env_var_override)
-        # currently, any non-empty value counts as truish
-        # TODO: change this so one can give y/n there?
-        if value:
-            value = bool(value)
-            value_str = truish[0] if value else falsish[0]
-            print("{} (from {})".format(value_str, env_var_override), file=ofile)
-            return value
-    if default_notty is not None and not ifile.isatty():
-        # looks like ifile is not a terminal (but e.g. a pipe)
-        return default_notty
+        print(msg, file=ofile, end='', flush=True)
     while True:
-        try:
-            answer = input()  # XXX how can we use ifile?
-        except EOFError:
-            return default_eof if default_eof is not None else default
+        answer = None
+        if env_var_override:
+            answer = os.environ.get(env_var_override)
+            if answer is not None and env_msg:
+                print(env_msg.format(answer, env_var_override), file=ofile)
+        if answer is None:
+            try:
+                answer = input()
+            except EOFError:
+                # avoid defaultish[0], defaultish could be empty
+                answer = truish[0] if default else falsish[0]
+        if answer in defaultish:
+            if default_msg:
+                print(default_msg, file=ofile)
+            return default
         if answer in truish:
             if true_msg:
                 print(true_msg, file=ofile)
@@ -915,11 +913,15 @@ def yes(msg=None, retry_msg=None, false_msg=None, true_msg=None,
             if false_msg:
                 print(false_msg, file=ofile)
             return False
-        if retry_msg is None:
-            # no retries wanted, we just return the default
+        # if we get here, the answer was invalid
+        if invalid_msg:
+            print(invalid_msg, file=ofile)
+        if not retry:
             return default
         if retry_msg:
             print(retry_msg, file=ofile, end='', flush=True)
+        # in case we used an environment variable and it gave an invalid answer, do not use it again:
+        env_var_override = None
 
 
 class ProgressIndicatorPercent:

borg/testsuite/archiver.py

@@ -136,7 +136,7 @@ def test_disk_full(cmd):
                 data = os.urandom(size)
                 f.write(data)
 
-    with environment_variable(BORG_CHECK_I_KNOW_WHAT_I_AM_DOING='1'):
+    with environment_variable(BORG_CHECK_I_KNOW_WHAT_I_AM_DOING='YES'):
         mount = DF_MOUNT
         assert os.path.exists(mount)
         repo = os.path.join(mount, 'repo')
@@ -190,8 +190,8 @@ class ArchiverTestCaseBase(BaseTestCase):
     prefix = ''
 
     def setUp(self):
-        os.environ['BORG_CHECK_I_KNOW_WHAT_I_AM_DOING'] = '1'
-        os.environ['BORG_DELETE_I_KNOW_WHAT_I_AM_DOING'] = '1'
+        os.environ['BORG_CHECK_I_KNOW_WHAT_I_AM_DOING'] = 'YES'
+        os.environ['BORG_DELETE_I_KNOW_WHAT_I_AM_DOING'] = 'YES'
         os.environ['BORG_PASSPHRASE'] = 'waytooeasyonlyfortests'
         self.archiver = not self.FORK_DEFAULT and Archiver() or None
         self.tmpdir = tempfile.mkdtemp()
@@ -330,7 +330,7 @@ class ArchiverTestCase(ArchiverTestCaseBase):
         item_count = 3 if has_lchflags else 4  # one file is UF_NODUMP
         self.assert_in('Number of files: %d' % item_count, info_output)
         shutil.rmtree(self.cache_path)
-        with environment_variable(BORG_UNKNOWN_UNENCRYPTED_REPO_ACCESS_IS_OK='1'):
+        with environment_variable(BORG_UNKNOWN_UNENCRYPTED_REPO_ACCESS_IS_OK='yes'):
             info_output2 = self.cmd('info', self.repository_location + '::test')
 
         def filter(output):

borg/testsuite/benchmark.py

@@ -16,9 +16,9 @@ from .archiver import changedir, cmd
 @pytest.yield_fixture
 def repo_url(request, tmpdir):
     os.environ['BORG_PASSPHRASE'] = '123456'
-    os.environ['BORG_CHECK_I_KNOW_WHAT_I_AM_DOING'] = '1'
-    os.environ['BORG_DELETE_I_KNOW_WHAT_I_AM_DOING'] = '1'
-    os.environ['BORG_UNKNOWN_UNENCRYPTED_REPO_ACCESS_IS_OK'] = '1'
+    os.environ['BORG_CHECK_I_KNOW_WHAT_I_AM_DOING'] = 'YES'
+    os.environ['BORG_DELETE_I_KNOW_WHAT_I_AM_DOING'] = 'YES'
+    os.environ['BORG_UNKNOWN_UNENCRYPTED_REPO_ACCESS_IS_OK'] = 'yes'
     os.environ['BORG_KEYS_DIR'] = str(tmpdir.join('keys'))
     os.environ['BORG_CACHE_DIR'] = str(tmpdir.join('cache'))
     yield str(tmpdir.join('repository'))

borg/testsuite/helpers.py

@@ -9,11 +9,11 @@ import sys
 import msgpack
 import msgpack.fallback
 
-from ..helpers import Location, format_file_size, format_timedelta, PathPrefixPattern, FnmatchPattern, make_path_safe, \
-    prune_within, prune_split, get_cache_dir, Statistics, is_slow_msgpack, yes, RegexPattern, \
+from ..helpers import Location, format_file_size, format_timedelta, make_path_safe, \
+    prune_within, prune_split, get_cache_dir, Statistics, is_slow_msgpack, yes, TRUISH, FALSISH, DEFAULTISH, \
     StableDict, int_to_bigint, bigint_to_int, parse_timestamp, CompressionSpec, ChunkerParams, \
-    ProgressIndicatorPercent, ProgressIndicatorEndless, load_excludes, parse_pattern, PatternMatcher, \
-    ShellPattern
+    ProgressIndicatorPercent, ProgressIndicatorEndless, load_excludes, parse_pattern, \
+    PatternMatcher, RegexPattern, PathPrefixPattern, FnmatchPattern, ShellPattern
 from . import BaseTestCase, environment_variable, FakeInputs
 
 
@@ -691,20 +691,28 @@ def test_is_slow_msgpack():
     assert not is_slow_msgpack()
 
 
-def test_yes_simple():
-    input = FakeInputs(['y', 'Y', 'yes', 'Yes', ])
-    assert yes(input=input)
-    assert yes(input=input)
-    assert yes(input=input)
-    assert yes(input=input)
-    input = FakeInputs(['n', 'N', 'no', 'No', ])
-    assert not yes(input=input)
-    assert not yes(input=input)
-    assert not yes(input=input)
-    assert not yes(input=input)
+def test_yes_input():
+    inputs = list(TRUISH)
+    input = FakeInputs(inputs)
+    for i in inputs:
+        assert yes(input=input)
+    inputs = list(FALSISH)
+    input = FakeInputs(inputs)
+    for i in inputs:
+        assert not yes(input=input)
 
 
-def test_yes_custom():
+def test_yes_input_defaults():
+    inputs = list(DEFAULTISH)
+    input = FakeInputs(inputs)
+    for i in inputs:
+        assert yes(default=True, input=input)
+    input = FakeInputs(inputs)
+    for i in inputs:
+        assert not yes(default=False, input=input)
+
+
+def test_yes_input_custom():
     input = FakeInputs(['YES', 'SURE', 'NOPE', ])
     assert yes(truish=('YES', ), input=input)
     assert yes(truish=('SURE', ), input=input)
@@ -712,11 +720,20 @@ def test_yes_custom():
 
 
 def test_yes_env():
-    input = FakeInputs(['n', 'n'])
-    with environment_variable(OVERRIDE_THIS='nonempty'):
-        assert yes(env_var_override='OVERRIDE_THIS', input=input)
-    with environment_variable(OVERRIDE_THIS=None):  # env not set
-        assert not yes(env_var_override='OVERRIDE_THIS', input=input)
+    for value in TRUISH:
+        with environment_variable(OVERRIDE_THIS=value):
+            assert yes(env_var_override='OVERRIDE_THIS')
+    for value in FALSISH:
+        with environment_variable(OVERRIDE_THIS=value):
+            assert not yes(env_var_override='OVERRIDE_THIS')
+
+
+def test_yes_env_default():
+    for value in DEFAULTISH:
+        with environment_variable(OVERRIDE_THIS=value):
+            assert yes(env_var_override='OVERRIDE_THIS', default=True)
+        with environment_variable(OVERRIDE_THIS=value):
+            assert not yes(env_var_override='OVERRIDE_THIS', default=False)
 
 
 def test_yes_defaults():
@@ -728,27 +745,27 @@ def test_yes_defaults():
     assert yes(default=True, input=input)
     assert yes(default=True, input=input)
     assert yes(default=True, input=input)
-    ifile = StringIO()
-    assert yes(default_notty=True, ifile=ifile)
-    assert not yes(default_notty=False, ifile=ifile)
     input = FakeInputs([])
-    assert yes(default_eof=True, input=input)
-    assert not yes(default_eof=False, input=input)
+    assert yes(default=True, input=input)
+    assert not yes(default=False, input=input)
     with pytest.raises(ValueError):
         yes(default=None)
-    with pytest.raises(ValueError):
-        yes(default_notty='invalid')
-    with pytest.raises(ValueError):
-        yes(default_eof='invalid')
 
 
 def test_yes_retry():
-    input = FakeInputs(['foo', 'bar', 'y', ])
+    input = FakeInputs(['foo', 'bar', TRUISH[0], ])
     assert yes(retry_msg='Retry: ', input=input)
-    input = FakeInputs(['foo', 'bar', 'N', ])
+    input = FakeInputs(['foo', 'bar', FALSISH[0], ])
     assert not yes(retry_msg='Retry: ', input=input)
 
 
+def test_yes_no_retry():
+    input = FakeInputs(['foo', 'bar', TRUISH[0], ])
+    assert not yes(retry=False, default=False, input=input)
+    input = FakeInputs(['foo', 'bar', FALSISH[0], ])
+    assert yes(retry=False, default=True, input=input)
+
+
 def test_yes_output(capfd):
     input = FakeInputs(['invalid', 'y', 'n'])
     assert yes(msg='intro-msg', false_msg='false-msg', true_msg='true-msg', retry_msg='retry-msg', input=input)

docs/usage.rst

@@ -69,15 +69,19 @@ General:
     TMPDIR
         where temporary files are stored (might need a lot of temporary space for some operations)
 
-Some "yes" sayers (if set, they automatically confirm that you really want to do X even if there is that warning):
-    BORG_UNKNOWN_UNENCRYPTED_REPO_ACCESS_IS_OK
+Some automatic "answerers" (if set, they automatically answer confirmation questions):
+    BORG_UNKNOWN_UNENCRYPTED_REPO_ACCESS_IS_OK=no (or =yes)
         For "Warning: Attempting to access a previously unknown unencrypted repository"
-    BORG_RELOCATED_REPO_ACCESS_IS_OK
+    BORG_RELOCATED_REPO_ACCESS_IS_OK=no (or =yes)
         For "Warning: The repository at location ... was previously located at ..."
-    BORG_CHECK_I_KNOW_WHAT_I_AM_DOING
-        For "Warning: '``check --repair``' is an experimental feature that might result in data loss."
-    BORG_DELETE_I_KNOW_WHAT_I_AM_DOING
-        For "You requested to completely DELETE the repository *including* all archives it contains:  "
+    BORG_CHECK_I_KNOW_WHAT_I_AM_DOING=NO (or =YES)
+        For "Warning: 'check --repair' is an experimental feature that might result in data loss."
+    BORG_DELETE_I_KNOW_WHAT_I_AM_DOING=NO (or =YES)
+        For "You requested to completely DELETE the repository *including* all archives it contains:"
+
+    Note: answers are case sensitive. setting an invalid answer value might either give the default
+    answer or ask you interactively, depending on whether retries are allowed (they by default are
+    allowed). So please test your scripts interactively before making them a non-interactive script.
 
 Directories:
     BORG_KEYS_DIR