Split up interval parsing from filtering for --keep-within

Fixes #2610

Parse --keep-within argument early, via new validator method interval
passed to argparse type=, so that better error messages can be given.

Also swallows ValueError stacktrace per the comment in the old code that
including it wasn't desirable.

src/borg/archiver.py

@@ -49,7 +49,7 @@ from .helpers import PrefixSpec, SortBySpec, HUMAN_SORT_KEYS
 from .helpers import BaseFormatter, ItemFormatter, ArchiveFormatter
 from .helpers import format_timedelta, format_file_size, parse_file_size, format_archive
 from .helpers import safe_encode, remove_surrogates, bin_to_hex, prepare_dump_dict
-from .helpers import prune_within, prune_split
+from .helpers import interval, prune_within, prune_split
 from .helpers import timestamp
 from .helpers import get_cache_dir
 from .helpers import Manifest
@@ -3347,7 +3347,7 @@ class Archiver:
                                help='print statistics for the deleted archive')
         subparser.add_argument('--list', dest='output_list', action='store_true',
                                help='output verbose list of archives it keeps/prunes')
-        subparser.add_argument('--keep-within', dest='within', type=str, metavar='WITHIN',
+        subparser.add_argument('--keep-within', dest='within', type=interval, metavar='INTERVAL',
                                help='keep all archives within this time interval')
         subparser.add_argument('--keep-last', '--keep-secondly', dest='secondly', type=int, default=0,
                                help='number of secondly archives to keep')

src/borg/helpers.py

@@ -364,15 +364,32 @@ class Manifest:
         self.repository.put(self.MANIFEST_ID, self.key.encrypt(data))
 
 
-def prune_within(archives, within):
+def interval(s):
+    """Convert a string representing a valid interval to a number of hours."""
     multiplier = {'H': 1, 'd': 24, 'w': 24 * 7, 'm': 24 * 31, 'y': 24 * 365}
+
+    if s.endswith(tuple(multiplier.keys())):
+        number = s[:-1]
+        suffix = s[-1]
+    else:
+        # range suffixes in ascending multiplier order
+        ranges = [k for k, v in sorted(multiplier.items(), key=lambda t: t[1])]
+        raise argparse.ArgumentTypeError(
+            'Unexpected interval time unit "%s": expected one of %r' % (s[-1], ranges))
+
     try:
-        hours = int(within[:-1]) * multiplier[within[-1]]
+        hours = int(number) * multiplier[suffix]
-    except (KeyError, ValueError):
+    except ValueError:
-        # I don't like how this displays the original exception too:
+        hours = -1
-        raise argparse.ArgumentTypeError('Unable to parse --keep-within option: "%s"' % within)
+
     if hours <= 0:
-        raise argparse.ArgumentTypeError('Number specified using --keep-within option must be positive')
+        raise argparse.ArgumentTypeError(
+            'Unexpected interval number "%s": expected an integer greater than 0' % number)
+
+    return hours
+
+
+def prune_within(archives, hours):
     target = datetime.now(timezone.utc) - timedelta(seconds=hours * 3600)
     return [a for a in archives if a.ts > target]
 

src/borg/testsuite/helpers.py

@@ -1,9 +1,9 @@
-import argparse
 import hashlib
 import io
 import os
 import shutil
 import sys
+from argparse import ArgumentTypeError
 from datetime import datetime, timezone, timedelta
 from time import mktime, strptime, sleep
 
@@ -17,7 +17,7 @@ from ..helpers import Location
 from ..helpers import Buffer
 from ..helpers import partial_format, format_file_size, parse_file_size, format_timedelta, format_line, PlaceholderError, replace_placeholders
 from ..helpers import make_path_safe, clean_lines
-from ..helpers import prune_within, prune_split
+from ..helpers import interval, prune_within, prune_split
 from ..helpers import get_cache_dir, get_keys_dir, get_security_dir
 from ..helpers import is_slow_msgpack
 from ..helpers import yes, TRUISH, FALSISH, DEFAULTISH
@@ -368,16 +368,48 @@ class PruneSplitTestCase(BaseTestCase):
         dotest(test_archives, 0, [], [])
 
 
-class PruneWithinTestCase(BaseTestCase):
+class IntervalTestCase(BaseTestCase):
+    def test_interval(self):
+        self.assert_equal(interval('1H'), 1)
+        self.assert_equal(interval('1d'), 24)
+        self.assert_equal(interval('1w'), 168)
+        self.assert_equal(interval('1m'), 744)
+        self.assert_equal(interval('1y'), 8760)
 
-    def test(self):
+    def test_interval_time_unit(self):
+        with pytest.raises(ArgumentTypeError) as exc:
+            interval('H')
+        self.assert_equal(
+            exc.value.args,
+            ('Unexpected interval number "": expected an integer greater than 0',))
+        with pytest.raises(ArgumentTypeError) as exc:
+            interval('-1d')
+        self.assert_equal(
+            exc.value.args,
+            ('Unexpected interval number "-1": expected an integer greater than 0',))
+        with pytest.raises(ArgumentTypeError) as exc:
+            interval('food')
+        self.assert_equal(
+            exc.value.args,
+            ('Unexpected interval number "foo": expected an integer greater than 0',))
+
+    def test_interval_number(self):
+        with pytest.raises(ArgumentTypeError) as exc:
+            interval('5')
+        self.assert_equal(
+            exc.value.args,
+            ("Unexpected interval time unit \"5\": expected one of ['H', 'd', 'w', 'm', 'y']",))
+
+
+class PruneWithinTestCase(BaseTestCase):
+    def test_prune_within(self):
 
         def subset(lst, indices):
             return {lst[i] for i in indices}
 
         def dotest(test_archives, within, indices):
             for ta in test_archives, reversed(test_archives):
-                self.assert_equal(set(prune_within(ta, within)),
+                self.assert_equal(set(prune_within(ta, interval(within))),
                                   subset(test_archives, indices))
 
         # 1 minute, 1.5 hours, 2.5 hours, 3.5 hours, 25 hours, 49 hours