Repository: borgbackup/borg

Issue:
Consider making `--no-atime` the default
The [docs already mention](https://borgbackup.readthedocs.io/en/stable/usage/notes.html#noatime-noctime) the problem that storing `atime` can create:

> it might also affect metadata stream deduplication: if only this timestamp changes between backups and is stored into the metadata stream, the metadata stream chunks won’t deduplicate just because of that.

What the docs don't mention is that on systems that don't support `O_NOATIME` (e.g. macOS), every single file will have a modified `atime` by virtue of borg scanning it. This means that the metadata stream will get almost no deduplication at all, resulting in large archive sizes even when ~no data has changed.

Additionally, the stored `atime` values won't really be useful, since they'll just reflect the time of the backup.

So, I think it makes sense for `--noatime` to be the default (with a `--atime` flag to opt-in), at least on systems where `O_NOATIME` is not available. (But I'd lean towards making it the default everywhere, since it's unclear that the value of storing it outweighs the downside for the average user.)

There of course could be suitable compatibility measures, like keeping the old default for existing repos.


Discussion:

Patch:
diff --git a/docs/changes.rst b/docs/changes.rst
index dc18f892..f84a2c70 100644
--- a/docs/changes.rst
+++ b/docs/changes.rst
@@ -192,6 +192,8 @@ Compatibility notes:
   which does not need an external Python interpreter.
   Maybe this requirement will be raised to Python 3.6 later.
 - freeing repository space only happens when "borg compact" is invoked.
+- borg create --noatime is deprecated. Not storing atime is the default behaviour
+  now (use --atime if you want to store the atime).
 - list: corrected mix-up of "isomtime" and "mtime" formats. Previously,
   "isomtime" was the default but produced a verbose human format,
   while "mtime" produced a ISO-8601-like format.
@@ -245,6 +247,16 @@ New features:
 - enable placeholder usage in --comment, #4559
 - enable placeholder usage in --glob-archives, #4495
 - ability to use a system-provided version of "xxhash"
+- create:
+
+  - changed the default behaviour to not store the atime of fs items. atime is
+    often rather not interesting and fragile - it easily changes even if nothing
+    else has changed and, if stored into the archive, spoils deduplication of
+    the archive metadata stream.
+  - if you give the --noatime option, borg will output a deprecation warning
+    because it is currently ignored / does nothing.
+    Please remove the --noatime option when using borg 1.2.
+  - added a --atime option for storing files' atime into an archive
 
 Other changes:
 
diff --git a/src/borg/archive.py b/src/borg/archive.py
index e97b06b1..5a5f19d4 100644
--- a/src/borg/archive.py
+++ b/src/borg/archive.py
@@ -40,7 +40,6 @@
 from .helpers import ellipsis_truncate, ProgressIndicatorPercent, log_multi
 from .helpers import os_open, flags_normal
 from .helpers import msgpack
-from .helpers import sig_int
 from .patterns import PathPrefixPattern, FnmatchPattern, IECommand
 from .item import Item, ArchiveItem, ItemDiff
 from .platform import acl_get, acl_set, set_flags, get_flags, swidth, hostname
@@ -1097,19 +1096,6 @@ def write_part_file(self, item, from_chunk, number):
         self.write_checkpoint()
         return length, number
 
-    def maybe_checkpoint(self, item, from_chunk, part_number, forced=False):
-        sig_int_triggered = sig_int and sig_int.action_triggered()
-        if forced or sig_int_triggered or \
-            self.checkpoint_interval and time.monotonic() - self.last_checkpoint > self.checkpoint_interval:
-            if sig_int_triggered:
-                logger.info('checkpoint requested: starting checkpoint creation...')
-            from_chunk, part_number = self.write_part_file(item, from_chunk, part_number)
-            self.last_checkpoint = time.monotonic()
-            if sig_int_triggered:
-                sig_int.action_completed()
-                logger.info('checkpoint requested: finished checkpoint creation!')
-        return from_chunk, part_number
-
     def process_file_chunks(self, item, cache, stats, show_progress, chunk_iter, chunk_processor=None):
         if not chunk_processor:
             def chunk_processor(data):
@@ -1128,14 +1114,17 @@ def chunk_processor(data):
             item.chunks.append(chunk_processor(data))
             if show_progress:
                 stats.show_progress(item=item, dt=0.2)
-            from_chunk, part_number = self.maybe_checkpoint(item, from_chunk, part_number, forced=False)
+            if self.checkpoint_interval and time.monotonic() - self.last_checkpoint > self.checkpoint_interval:
+                from_chunk, part_number = self.write_part_file(item, from_chunk, part_number)
+                self.last_checkpoint = time.monotonic()
         else:
             if part_number > 1:
                 if item.chunks[from_chunk:]:
                     # if we already have created a part item inside this file, we want to put the final
                     # chunks (if any) into a part item also (so all parts can be concatenated to get
                     # the complete file):
-                    from_chunk, part_number = self.maybe_checkpoint(item, from_chunk, part_number, forced=True)
+                    from_chunk, part_number = self.write_part_file(item, from_chunk, part_number)
+                    self.last_checkpoint = time.monotonic()
 
                 # if we created part files, we have referenced all chunks from the part files,
                 # but we also will reference the same chunks also from the final, complete file:
diff --git a/src/borg/archiver.py b/src/borg/archiver.py
index 583dc2fb..23807f8d 100644
--- a/src/borg/archiver.py
+++ b/src/borg/archiver.py
@@ -71,7 +71,6 @@
     from .helpers import umount
     from .helpers import flags_root, flags_dir, flags_special_follow, flags_special
     from .helpers import msgpack
-    from .helpers import sig_int
     from .nanorst import rst_to_terminal
     from .patterns import ArgparsePatternAction, ArgparseExcludeFileAction, ArgparsePatternFileAction, parse_exclude_pattern
     from .patterns import PatternMatcher
@@ -532,12 +531,7 @@ def create_inner(archive, cache, fso):
                 if args.progress:
                     archive.stats.show_progress(final=True)
                 archive.stats += fso.stats
-                if sig_int:
-                    # do not save the archive if the user ctrl-c-ed - it is valid, but incomplete.
-                    # we already have a checkpoint archive in this case.
-                    self.print_error("Got Ctrl-C / SIGINT.")
-                else:
-                    archive.save(comment=args.comment, timestamp=args.timestamp, stats=archive.stats)
+                archive.save(comment=args.comment, timestamp=args.timestamp, stats=archive.stats)
                 args.stats |= args.json
                 if args.stats:
                     if args.json:
@@ -567,11 +561,11 @@ def create_inner(archive, cache, fso):
                        cache_mode=args.files_cache_mode) as cache:
                 archive = Archive(repository, key, manifest, args.location.archive, cache=cache,
                                   create=True, checkpoint_interval=args.checkpoint_interval,
-                                  numeric_owner=args.numeric_owner, noatime=args.noatime, noctime=args.noctime,
+                                  numeric_owner=args.numeric_owner, noatime=not args.atime, noctime=args.noctime,
                                   progress=args.progress,
                                   chunker_params=args.chunker_params, start=t0, start_monotonic=t0_monotonic,
                                   log_json=args.log_json)
-                metadata_collector = MetadataCollector(noatime=args.noatime, noctime=args.noctime,
+                metadata_collector = MetadataCollector(noatime=not args.atime, noctime=args.noctime,
                     nobsdflags=args.nobsdflags, numeric_owner=args.numeric_owner, nobirthtime=args.nobirthtime)
                 cp = ChunksProcessor(cache=cache, key=key,
                     add_item=archive.add_item, write_checkpoint=archive.write_checkpoint,
@@ -593,10 +587,6 @@ def _process(self, *, path, parent_fd=None, name=None,
 
         This should only raise on critical errors. Per-item errors must be handled within this method.
         """
-        if sig_int and sig_int.action_done():
-            # the user says "get out of here!" and we have already completed the desired action.
-            return
-
         try:
             recurse_excluded_dir = False
             if matcher.match(path):
@@ -2342,6 +2332,7 @@ def do_subcommand_help(self, parser, args):
     def preprocess_args(self, args):
         deprecations = [
             # ('--old', '--new' or None, 'Warning: "--old" has been deprecated. Use "--new" instead.'),
+            ('--noatime', None, 'Warning: "--noatime" has been deprecated because it is the default now.')
         ]
         for i, arg in enumerate(args[:]):
             for old_name, new_name, warning in deprecations:
@@ -3024,8 +3015,12 @@ def define_archive_filters_group(subparser, *, sort_by=True, first_last=True):
                               help='stay in the same file system and do not store mount points of other file systems')
         fs_group.add_argument('--numeric-owner', dest='numeric_owner', action='store_true',
                               help='only store numeric user and group identifiers')
+        # --noatime is the default now and the flag is deprecated. args.noatime is not used any more.
+        # use --atime if you want to store the atime (default behaviour before borg 1.2.0a7)..
         fs_group.add_argument('--noatime', dest='noatime', action='store_true',
                               help='do not store atime into archive')
+        fs_group.add_argument('--atime', dest='atime', action='store_true',
+                              help='do store atime into archive')
         fs_group.add_argument('--noctime', dest='noctime', action='store_true',
                               help='do not store ctime into archive')
         fs_group.add_argument('--nobirthtime', dest='nobirthtime', action='store_true',
@@ -4441,8 +4436,7 @@ def main():  # pragma: no cover
                 print(tb, file=sys.stderr)
             sys.exit(e.exit_code)
         try:
-            with sig_int:
-                exit_code = archiver.run(args)
+            exit_code = archiver.run(args)
         except Error as e:
             msg = e.get_message()
             msgid = type(e).__qualname__
diff --git a/src/borg/helpers/process.py b/src/borg/helpers/process.py
index 763a8c68..44b49892 100644
--- a/src/borg/helpers/process.py
+++ b/src/borg/helpers/process.py
@@ -86,52 +86,6 @@ def handler(sig_no, frame):
     return handler
 
 
-class SigIntManager:
-    def __init__(self):
-        self._sig_int_triggered = False
-        self._action_triggered = False
-        self._action_done = False
-        self.ctx = signal_handler('SIGINT', self.handler)
-
-    def __bool__(self):
-        # this will be True (and stay True) after the first Ctrl-C/SIGINT
-        return self._sig_int_triggered
-
-    def action_triggered(self):
-        # this is True to indicate that the action shall be done
-        return self._action_triggered
-
-    def action_done(self):
-        # this will be True after the action has completed
-        return self._action_done
-
-    def action_completed(self):
-        # this must be called when the action triggered is completed,
-        # to avoid that the action is repeatedly triggered.
-        self._action_triggered = False
-        self._action_done = True
-
-    def handler(self, sig_no, stack):
-        # handle the first ctrl-c / SIGINT.
-        self.__exit__(None, None, None)
-        self._sig_int_triggered = True
-        self._action_triggered = True
-
-    def __enter__(self):
-        self.ctx.__enter__()
-
-    def __exit__(self, exception_type, exception_value, traceback):
-        # restore the original ctrl-c handler, so the next ctrl-c / SIGINT does the normal thing:
-        if self.ctx:
-            self.ctx.__exit__(exception_type, exception_value, traceback)
-            self.ctx = None
-
-
-# global flag which might trigger some special behaviour on first ctrl-c / SIGINT,
-# e.g. if this is interrupting "borg create", it shall try to create a checkpoint.
-sig_int = SigIntManager()
-
-
 def popen_with_error_handling(cmd_line: str, log_prefix='', **kwargs):
     """
     Handle typical errors raised by subprocess.Popen. Return None if an error occurred,


Test patch:
diff --git a/src/borg/testsuite/archiver.py b/src/borg/testsuite/archiver.py
index 1ef802c1..42843e67 100644
--- a/src/borg/testsuite/archiver.py
+++ b/src/borg/testsuite/archiver.py
@@ -501,7 +501,7 @@ def has_noatime(some_file):
         have_noatime = has_noatime('input/file1')
         os.utime('input/file1', (atime, mtime))
         self.cmd('init', '--encryption=repokey', self.repository_location)
-        self.cmd('create', self.repository_location + '::test', 'input')
+        self.cmd('create', '--atime', self.repository_location + '::test', 'input')
         with changedir('output'):
             self.cmd('extract', self.repository_location + '::test')
         sti = os.stat('input/file1')
@@ -2172,8 +2172,8 @@ def has_noatime(some_file):
         self.cmd('init', '--encryption=repokey', self.repository_location)
         self.create_test_files()
         have_noatime = has_noatime('input/file1')
-        self.cmd('create', '--exclude-nodump', self.repository_location + '::archive', 'input')
-        self.cmd('create', '--exclude-nodump', self.repository_location + '::archive2', 'input')
+        self.cmd('create', '--exclude-nodump', '--atime', self.repository_location + '::archive', 'input')
+        self.cmd('create', '--exclude-nodump', '--atime', self.repository_location + '::archive2', 'input')
         if has_lchflags:
             # remove the file we did not backup, so input and output become equal
             os.remove(os.path.join('input', 'flagfile'))