Repository: rdiff-backup/rdiff-backup (language: python)

Issue:
[ENH] when removing increments, optionally show the size of each removed increment
## Is your feature request related to a problem? Please describe.

Tools like rdiff-backup make it very easy to create and manage backups.  The biggest problem I have is managing the storage for the backups.  I have a script that
1. displays the storage capacity and usage of the backup destination volume before the backup
2. removes increments before some time period (e.g. 1 year)
3. does the backup showing backup statistics (i.e. --print-statistics)
4. displays the storage capacity and usage of the backup destination volume after the backup
5. verifies the backup
6. lists the changes in the last backup (--list-changed-since 1B)
7. lists all the increments and their sizes

This provides me some information about how/why my backup storage usage changed each day, but there are two things that would be helpful.
1. when removing increments, optionally show the size of each removed increment
2. when listing changes since ..., optionally show the size of each new/deleted file and the change in size of each changed file

For me, item 2 would be nice-to-have, but item 1 is important.

## Describe the solution you'd like

In rdiff-backup 2.2 (syntax) added a --size option to the "remove increments" action that would display the sizes of each removed increment.



Discussion / hints:
I've split the issue into two different ones, see also #768 
Some comments on this enhancement request:

1. you should already have quite a lot of the code necessary as part of the "list increments" action, as it already has a `--size` option.
2. basically, using the function `self.repo.get_increments_sizes()` you should have already all the information you need and only need to output it accordingly.
3. important is that you don't create "holes" through the repository encapsulation. If you need information from the repository, create a function (consider that it might impact the API), but don't fiddle _in the action_ with the internas of the repository. This was done in the past, and this is what I'm currently trying to get rid of, so don't add to the mess, please!
4. else, if you have something somewhat running, feel free to create a draft PR and i can do an early review to make sure it goes in the right direction.
I've added code to add a --size option to the "remove increments" command, and to experiment I've added code to `RemoveAction._get_removal_time` that calls `self.repo.get_increments_sizes()` if the --size option is specified.  When I invoke `rdiff-backup remove increments --older-than 1D --size ...` I get

> AssertionError: Root path 'None' must be a real remote path.

at

> File "/tmp/rdiff-backup-env/lib/python3.10/site-packages/rdiff_backup-2.1.3b4.dev12+gfb1bd92.d20221109-py3.10-linux-x86_64.egg/rdiff_backup/selection.py", line 93, in __init__
>     assert isinstance(rootrp, rpath.RPath), (
> 

It appears this is caused by `self.repo.ref_path` being `None`.  Looking at how the `Repo` is created and initialized in `ListAction`, it appears `ref_path` gets initialized (to non-None) when `can_be_sub_path=True` is passed when creating the `Repo`.  I suspect, however, that passing `can_be_sub_path=True` when creating the `Repo` in RemoveAction would be incorrect (I haven't tried it through).

Any advise on how to proceed?
It wouldn't be correct to set `can_be_sub_path=True` because you can't remove a partial increment, hence it would be incorrect indeed. Because the function(s) `get_increments...` were meant initially for the `list increments` actions, which can use a sub_path, that's why these functions are using `ref_path`.

I see two possible courses of action:

1. set ref_path to base_dir in the cases where can_be_sub_path is False (and accordingly ref_inc), it's a more generic approach, which can have impact on other parts of the code, also positively because other functions might become usable in a more generic context, like `get_increments_sizes`.
2. use something like `self.ref_path or self.base_dir` where you need a "real" path, it's more tactical and less intrusive, but doesn't make the code nicer, and might have shortcomings

After a quick look at the code, you not only need ref_path but also ref_inc, so that the 1st course of action seems more promising, if also more intrusive. But I guess, it was already clear that your changes would only be accepted in the 2.3/2.4 development phase, so I'm fine with the "intrusion".

Fix patch:
diff --git a/docs/rdiff-backup.1.adoc b/docs/rdiff-backup.1.adoc
index 9c3208981..8e8fe00b8 100644
--- a/docs/rdiff-backup.1.adoc
+++ b/docs/rdiff-backup.1.adoc
@@ -195,7 +195,7 @@ If an rdiff-backup session fails, this action will undo the failed directory.
 This happens automatically if you attempt to back-up to a directory and the last backup failed.
 You can use the *--force* option to undo the last backup even if it wasn't failed.
 
-remove *increments* *--older-than* _time_ _repository_:: Remove the incremental backup information in the destination directory that has been around longer than the given time, or the oldest one if no time is provided.
+remove *increments* *--older-than* _time_ [*--size*] _repository_:: Remove the incremental backup information in the destination directory that has been around longer than the given time, or the oldest one if no time is provided.
 +
 By default, rdiff-backup will only delete information from one session at a time.
 To remove two or more sessions at the same time, supply the *--force* option (rdiff-backup will tell you if it is required).
@@ -207,6 +207,10 @@ Thus if you deleted a file two weeks ago, backed up immediately afterwards, and
 all the increments older than the given time will be deleted.
 See <<_time_formats,TIME FORMATS>> for details.
 
+--size;;
+Show the size of each increment being removed.
+The default is to _not_ show sizes.
+
 restore [<<_creation_options,CREATION OPTIONS>>] [<<_compression_options,COMPRESSION OPTIONS>>] [<<_selection_options,SELECTION OPTIONS>>] [<<_filesystem_options,FILESYSTEM OPTIONS>>] [<<_user_group_options,USER GROUP OPTIONS>>] [*--at* _time_|*--increment*] _source_ _targetdir_::
 restore a source backup repository at a specific time or a specific     source increment to a target directory.
 See <<_restoring,RESTORING>> for details.
diff --git a/src/rdiffbackup/actions/remove.py b/src/rdiffbackup/actions/remove.py
index 4d89ae46e..eb2a29334 100644
--- a/src/rdiffbackup/actions/remove.py
+++ b/src/rdiffbackup/actions/remove.py
@@ -22,9 +22,10 @@
 repository.
 """
 
-from rdiff_backup import (Globals, log, manage, Time)
+from rdiff_backup import (Globals, log, manage, statistics, Time)
 from rdiffbackup import actions
 from rdiffbackup.locations import repository
+from rdiffbackup.utils.argopts import BooleanOptionalAction
 
 
 class RemoveAction(actions.BaseAction):
@@ -42,6 +43,9 @@ def add_action_subparser(cls, sub_handler):
         entity_parsers["increments"].add_argument(
             "--older-than", metavar="TIME", required=True,
             help="remove increments older than given time")
+        entity_parsers["increments"].add_argument(
+            "--size", action=BooleanOptionalAction, default=False,
+            help="also output size of each increment (might take longer)")
         entity_parsers["increments"].add_argument(
             "locations", metavar="[[USER@]SERVER::]PATH", nargs=1,
             help="location of repository to remove increments from")
@@ -94,7 +98,8 @@ def setup(self):
                 self.repo.data_dir, 0)  # read_only=False
             self.repo.setup_quoting()
 
-        self.action_time = self._get_removal_time(self.values.older_than)
+        self.action_time = self._get_removal_time(self.values.older_than,
+                                                  self.values.size)
         if self.action_time is None:
             return ret_code | Globals.RET_CODE_ERR
 
@@ -120,7 +125,7 @@ def run(self):
 
         return ret_code
 
-    def _get_removal_time(self, time_string):
+    def _get_removal_time(self, time_string, show_increment_sizes):
         """
         Check remove older than time_string, return time in seconds
 
@@ -132,9 +137,14 @@ def _get_removal_time(self, time_string):
         if action_time is None:
             return None
 
-        times_in_secs = [
-            inc.getinctime() for inc in self.repo.incs_dir.get_incfiles_list()
-        ]
+        if self.values.size:
+            triples = self.repo.get_increments_sizes()[:-1]
+            times_in_secs = [triple["time"] for triple in triples]
+        else:
+            times_in_secs = [
+                inc.getinctime() for inc
+                in self.repo.incs_dir.get_incfiles_list()
+            ]
         times_in_secs = [t for t in times_in_secs if t < action_time]
         if not times_in_secs:
             log.Log("No increments older than {at} found, exiting.".format(
@@ -142,7 +152,20 @@ def _get_removal_time(self, time_string):
             return -1
 
         times_in_secs.sort()
-        pretty_times = "\n".join(map(Time.timetopretty, times_in_secs))
+        if self.values.size:
+            sizes = [triple["size"] for triple in triples
+                     if triple["time"] in times_in_secs]
+            stat_obj = statistics.StatsObj()  # used for byte summary string
+
+            def format_time_and_size(time, size):
+                return "{: <24} {: >17}".format(
+                    Time.timetopretty(time),
+                    stat_obj.get_byte_summary_string(size))
+
+            pretty_times_map = map(format_time_and_size, times_in_secs, sizes)
+            pretty_times = "\n".join(pretty_times_map)
+        else:
+            pretty_times = "\n".join(map(Time.timetopretty, times_in_secs))
         if len(times_in_secs) > 1:
             if not self.values.force:
                 log.Log(
diff --git a/src/rdiffbackup/arguments.py b/src/rdiffbackup/arguments.py
index 0d821a158..ebecae3df 100644
--- a/src/rdiffbackup/arguments.py
+++ b/src/rdiffbackup/arguments.py
@@ -260,6 +260,7 @@ def _make_values_like_new_compat200(values):  # noqa C901 "too complex"
             values.action = "remove"
             values.entity = "increments"
             values.older_than = values.remove_older_than
+            values.size = False
         elif values.restore_as_of:
             values.action = "restore"
             values.at = values.restore_as_of
diff --git a/src/rdiffbackup/locations/repository.py b/src/rdiffbackup/locations/repository.py
index b37d0ebec..20e16b53b 100644
--- a/src/rdiffbackup/locations/repository.py
+++ b/src/rdiffbackup/locations/repository.py
@@ -58,7 +58,8 @@ def __init__(self, base_dir, force, must_be_writable, must_exist,
             # we delay the error handling until the check step,
             # and as well the definition of ref_path and ref_inc
         else:
-            self.ref_type = self.ref_index = None
+            self.ref_index = ()
+            self.ref_type = None
 
         # Finish the initialization with the identified base_dir
         super().__init__(base_dir, force)
@@ -81,7 +82,9 @@ def check(self):
                 self.ref_inc = self.data_dir.append_path(b'increments',
                                                          self.ref_index)
         else:
-            self.ref_path = self.ref_inc = None
+            self.ref_path = self.base_dir
+            self.ref_inc = self.data_dir.append_path(b'increments',
+                                                     self.ref_index)
             log.Log("Using repository '{re}'".format(re=self.base_dir),
                     log.INFO)
         ret_code = Globals.RET_CODE_OK


Test patch:
diff --git a/testing/action_remove_test.py b/testing/action_remove_test.py
index 644454996..c656299a8 100644
--- a/testing/action_remove_test.py
+++ b/testing/action_remove_test.py
@@ -126,7 +126,7 @@ def test_action_removeincsolderthan(self):
         self.assertEqual(comtst.rdiff_backup_action(
             False, None, self.bak_path, None,
             ("--api-version", "201", ),
-            b"remove", ("increments", "--older-than", "30001")),
+            b"remove", ("increments", "--older-than", "30001", "--size")),
             Globals.RET_CODE_OK)
         # and check that only the mirror is left
         self.assertEqual(comtst.rdiff_backup_action(