Repository: borgbackup/borg
Pull request: Add a --list switch to borg delete
Also add some documentation on this.

Fixes #5116.

Problem statement:
Inconsistent switch to display changes during 'dry-run' operation
There are two operations I use, so far, to destructively modify a repository:
_delete_ and _recreate_

They allow wildcards, patterns, etc.  Obviously this is a dangerous operation and therefore the '--dry-run' switch exists for both to check your work before execution.

I have found an inconsistency in the method of displaying the proposed (dry-run) changes in order to review them however.

When I use _recreate_, the switches are "--dry-run --list" which will output what is going to happen.

However when using _delete_  the same process of "--dry-run --list" results in error:
_borg: error: unrecognized arguments: --list_

When using _delete_ , instead (through trial and error) I found out that instead I have to use:
"--dry-run --verbose"

My issues are:
1) this is not consistent switch usage for the listing of changes among operations that are relatively similar/destructive/deleting/changing; I think '--list' should function for both; (addendum: '--list' is also used with _create_ to likewise show added files and directories)

2) '--verbose' is generally viewed in a debugging light and should not be used in this context to display the desired output of a '--dry-run'; (It can still do that, but I think the 'what will change with this command' should be primarily on the '--list' command. verbose might include other info as well);

3) if '--verbose' is kept as the command to review results of a _delete_ dry-run it is not listed on the documentation page, (whereas the _recreate_ documentation page has '--list' right next to the '--dry-run' switch so a user can immediately grasp what to do.)  I suggest adding it under --dry-run switch to clarify how to see proposed deletions:
"_--verbose_      output list of items deleted (or to delete with --dry-run)"

Hints from issue discussion:
There may be other operations that have --dry-run options.  I would suggest (asserting that nobody does a --dry-run without wanting to see what will happen) that '--list' is available consistently in all such circumstances. 
I'm using 1.1.11
maybe one should do a global review about consistency relating to this.
Also worth noting that https://borgbackup.readthedocs.io/en/stable/usage/delete.html makes no mention of `--verbose` or `-v` and without those options `borg delete --dry-run` produces no output whatsoever, which probably makes it rather useless to the average user who has not searched the GitHub issues.
`-v` or `--info` sets the log level to `INFO` and is a global borg option.

By default, the level is `WARNING` and you won't see info-level output.

So guess the global log level stuff is only explained at some other place in the docs, but maybe we could at least add an example showing correct usage of `--dry-run` with `-v` or `--info`.
Is there *any* situation where a person does a --dry-run but does not want to see the output?
Unlikely. :-) But the point is that borg needs to behave consistently and not do too much magic.

So, if you want to investigate, look at how similar use cases work (if their behaviour differs and is better).
Or, what else all needs to change IF we need a new mechanism for this and want consistent behaviour.

I think that:
 * It is necessary to add the --list switch to _delete_
 * Update the documentation

This way we have a consistent way of listing files in all commands.
I can work on this one.
"update docs" likely means to just update `archiver.py` strings, the docs are auto-generated from that.

except the examples, they need to be edited in the respective rst files.

Commit message: Add a --list switch to borg delete

Also add some documentation on this.

Fixes #5116

Patch to review:
diff --git a/docs/usage/delete.rst b/docs/usage/delete.rst
index 02fe0a04ae..8d566ef70e 100644
--- a/docs/usage/delete.rst
+++ b/docs/usage/delete.rst
@@ -12,11 +12,13 @@ Examples
     # delete all archives whose names begin with the machine's hostname followed by "-"
     $ borg delete --prefix '{hostname}-' /path/to/repo
 
+
     # delete all archives whose names contain "-2012-"
     $ borg delete --glob-archives '*-2012-*' /path/to/repo
 
     # see what would be deleted if delete was run without --dry-run
     $ borg delete -v --dry-run -a '*-May-*' /path/to/repo
+    $ borg delete --list --dry-run -a '*-May-*' /path/to/repo
 
     # delete the whole repository and the related local cache:
     $ borg delete /path/to/repo
diff --git a/docs/usage/delete.rst.inc b/docs/usage/delete.rst.inc
index 060f5e76d7..fa443d75f5 100644
--- a/docs/usage/delete.rst.inc
+++ b/docs/usage/delete.rst.inc
@@ -68,6 +68,7 @@ borg delete
 
     optional arguments
         -n, --dry-run    do not change repository
+        --list          list the archives/files which would be deleted
         -s, --stats     print statistics for the deleted archive
         --cache-only    delete only the local cache for the given repository
         --force         force deletion of corrupted archives, use ``--force --force`` in case ``--force`` does not work.
@@ -112,4 +113,5 @@ two options are mutually exclusive.
 
 To avoid accidentally deleting archives, especially when using glob patterns,
 it might be helpful to use the ``--dry-run`` to test out the command without
-actually making any changes to the repository.
\ No newline at end of file
+actually making any changes to the repository.
+
diff --git a/src/borg/archiver.py b/src/borg/archiver.py
index a5bc3bff96..f354c97d70 100644
--- a/src/borg/archiver.py
+++ b/src/borg/archiver.py
@@ -1144,6 +1144,7 @@ def do_delete(self, args, repository):
         """Delete an existing repository or archives"""
         archive_filter_specified = any((args.first, args.last, args.prefix is not None, args.glob_archives))
         explicit_archives_specified = args.location.archive or args.archives
+        self.output_list = args.output_list
         if archive_filter_specified and explicit_archives_specified:
             self.print_error('Mixing archive filters and explicitly named archives is not supported.')
             return self.exit_code
@@ -1180,7 +1181,10 @@ def _delete_archives(self, args, repository):
                 else:
                     deleted = True
                     msg = 'Would delete: {} ({}/{})' if dry_run else 'Deleted archive: {} ({}/{})'
-                    logger.info(msg.format(format_archive(current_archive), i, len(archive_names)))
+                    if self.output_list:
+                        logging.getLogger('borg.output.list').info(msg.format(format_archive(current_archive), i, len(archive_names)))
+                    else:
+                        logger.info(msg.format(format_archive(current_archive), i, len(archive_names)))
             if dry_run:
                 logger.info('Finished dry-run.')
             elif deleted:
@@ -1202,7 +1206,10 @@ def _delete_archives(self, args, repository):
                 except KeyError:
                     logger.warning(msg_not_found.format(archive_name, i, len(archive_names)))
                 else:
-                    logger.info(msg_delete.format(format_archive(archive_info), i, len(archive_names)))
+                    if self.output_list:
+                        logging.getLogger('borg.output.list').info(msg_delete.format(format_archive(archive_info), i, len(archive_names)))
+                    else:
+                        logger.info(msg_delete.format(format_archive(archive_info), i, len(archive_names)))
                     if not dry_run:
                         archive = Archive(repository, key, manifest, archive_name, cache=cache,
                                           consider_part_files=args.consider_part_files)
@@ -1252,8 +1259,12 @@ def _delete_repository(self, args, repository):
                 if not keep_security_info:
                     SecurityManager.destroy(repository)
             else:
-                logger.info("Would delete repository.")
-                logger.info("Would %s security info." % ("keep" if keep_security_info else "delete"))
+                if self.output_list:
+                    logging.getLogger('borg.output.list').info("Would delete repository.")
+                    logging.getLogger('borg.output.list').info("Would %s security info." % ("keep" if keep_security_info else "delete"))
+                else:
+                    logger.info("Would delete repository.")
+                    logger.info("Would %s security info." % ("keep" if keep_security_info else "delete"))
         if not dry_run:
             Cache.destroy(repository)
             logger.info("Cache deleted.")
@@ -3486,6 +3497,9 @@ def define_borg_mount(parser):
         with the ``--cache-only`` option, or keep the security info with the
         ``--keep-security-info`` option.
 
+        When in doubt, use ``--dry-run --verbose --list`` to see how patterns/PATHS are
+        interpreted. See :ref:`list_item_flags` in ``borg create`` for details.
+
         When using ``--stats``, you will get some statistics about how much data was
         deleted - the "Deleted data" deduplicated size there is most interesting as
         that is how much your repository will shrink.
@@ -3509,6 +3523,8 @@ def define_borg_mount(parser):
         subparser.set_defaults(func=self.do_delete)
         subparser.add_argument('-n', '--dry-run', dest='dry_run', action='store_true',
                                help='do not change repository')
+        subparser.add_argument('--list', dest='output_list', action='store_true',
+                               help='output verbose list of items (files, dirs, ...)')
         subparser.add_argument('-s', '--stats', dest='stats', action='store_true',
                                help='print statistics for the deleted archive')
         subparser.add_argument('--cache-only', dest='cache_only', action='store_true',


Code review comments:
diff_hunk: @@ -1252,8 +1259,12 @@ def _delete_repository(self, args, repository):
                 if not keep_security_info:
                     SecurityManager.destroy(repository)
             else:
-                logger.info("Would delete repository.")
-                logger.info("Would %s security info." % ("keep" if keep_security_info else "delete"))
+                if self.output_list:
+                    logging.getLogger('borg.output.list').info("Would delete repository.")
+                    logging.getLogger('borg.output.list').info("Would %s security info." % ("keep" if keep_security_info else "delete"))
+                else:
+                    logger.info("Would delete repository.")
+                    logger.info("Would %s security info." % ("keep" if keep_security_info else "delete"))
path: src/borg/archiver.py
text: @user1:
guess this is a bit questionable regarding whether this counts as "list".

as borg always operates on 1 repo, generic hints about this repo and the related cache are not "listing" anything.

only when it outputs all archives in that repo, that would be "listing".

@author:
Hum, then I don't know which logger to use.
If I use the info level, there is no difference between the --list and nothing for the repositories (which can be a consistent way of acting). To be sure that something is written I could use the warn level.


@author:
@user1 What's your idea on this?
Should I use the warn level to be sure to print something?

@user1:
this is not a list, so no change is needed. giving --list or not should not make a difference here.

1244-1247 is the "listy" stuff here:
- with --list: like now
- without --list: just tell that "N archives are going to get deleted (use --list for details)"

diff_hunk: @@ -1180,7 +1181,10 @@ def _delete_archives(self, args, repository):
                 else:
                     deleted = True
                     msg = 'Would delete: {} ({}/{})' if dry_run else 'Deleted archive: {} ({}/{})'
-                    logger.info(msg.format(format_archive(current_archive), i, len(archive_names)))
+                    if self.output_list:
+                        logging.getLogger('borg.output.list').info(msg.format(format_archive(current_archive), i, len(archive_names)))
+                    else:
+                        logger.info(msg.format(format_archive(current_archive), i, len(archive_names)))
path: src/borg/archiver.py
text: @user1:
if only the logger is different, how about:
```
_logger = logging.getLogger('borg.output.list') if self.output_list else logger
_logger.info(msg.format(format_archive(current_archive), i, len(archive_names)))
```

diff_hunk: @@ -68,6 +68,7 @@ borg delete
 
     optional arguments
         -n, --dry-run    do not change repository
+        --list          list the archives/files which would be deleted
path: docs/usage/delete.rst.inc
text: @user1:
read line 1 of this file - no need to commit this and make the PR review more work, all these files will be auto-generated before a release.

diff_hunk: @@ -3486,6 +3497,9 @@ def define_borg_mount(parser):
         with the ``--cache-only`` option, or keep the security info with the
         ``--keep-security-info`` option.
 
+        When in doubt, use ``--dry-run --verbose --list`` to see how patterns/PATHS are
+        interpreted. See :ref:`list_item_flags` in ``borg create`` for details.
path: src/borg/archiver.py
text: @user1:
when --list is used, does it really also require --verbose?

please check for delete and also for other borg subcommands and do it in a consistent way.

@user1:
also, the item_flags are not relevant for borg delete (ARCHIVE|REPO).

@author:
No I don't think it requires --verbose, however it is present by default in all subparsers due to `define_common_options` but I can silently forget about it if --list is present.
On `borg create`, `--verbose` does nothing interesting, so this might be what we want.