Remove --reverse parameter

The --reverse parameter, used for transforming a "backup" operation into a "restore" by simply adding a parameter was a good idea but makes the code unnecessarily complex. Also, it is hard to test sufficiently as the number of parameter combinations that would require testing explodes with more parameters being added. This change removes the --reverse parameter and the associated code in an attempt to simplify the internals slightly.

Author: d-e-s-o

btrfs-backup/README.md

@@ -73,18 +73,6 @@ the following command can be used:
 
 ``$ btrfs-backup restore --subvolume=subvolume/ backup/ snapshots/``
 
-Alternatively, you can use the --reverse option to keep the order of the
-source and destination repository that was used during backup (as
-opposed to restoration). This option exists for convenience only, so
-that not the entire command line has to be amended but only one word
-replaced and an option appended to convert a backup operation into a
-restore operation.
-
-```
-$ btrfs-backup restore --reverse
- --subvolume=subvolume/ snapshots/ backup/
-```
-
 The above step for restoration assumes that the subvolume you initially
 backed up got deleted. However, under certain circumstances it might be
 the case that you only want to restore the snapshots (the read-only
@@ -171,8 +159,7 @@ stream might not result in much savings of bandwidth since it is
 reasonable to believe that the btrfs program already creates a
 sufficiently compressed stream of data.
 Note furthermore that since filters are sensitive to ordering, they have
-to be reordered for the restore case unless the --reverse option
-(explained above) is used.
+to be reordered for the restore case.
 Lastly, note that filtering can be combined with remote command
 execution as one would naively expect.
 

btrfs-backup/src/deso/btrfs/main.py

@@ -129,25 +129,6 @@ def checkSnapshotExtension(string):
  return "%s%s" % (extsep, string)
 
 
-class ReverseAction(Action):
- """Action to reverse some source/destination arguments."""
- def __init__(self, option_strings, dest, const=None, default=None,
- required=False, help=None, metavar=None):
- """Create a new ReverseAction object."""
- super().__init__(option_strings=option_strings, dest=dest, nargs=0,
- const=const, default=default, required=required,
- help=help)
-
-
- def __call__(self, parser, namespace, values, option_string=None):
- """Helper function to reverse arguments during parsing."""
- # In case the reverse option was given we swap the repositories and
- # the filters.
- with alias(namespace) as ns:
- ns.src, ns.dst = ns.dst, ns.src
- ns.send_filters, ns.recv_filters = ns.recv_filters, ns.send_filters
-
-
 class CheckSnapshotExtensionAction(Action):
  """Action to check correct usage of the --snapshot-ext parameter."""
  def __init__(self, option_strings, dest, nargs=None, const=None,
@@ -241,11 +222,6 @@ def addOptionalArgs(parser, backup):
  "to include the full path to the binary or script, e.g., "
  "\"/usr/bin/ssh server\".",
  )
- parser.add_argument(
- "--reverse", action=ReverseAction, dest="reverse",
- help="Reverse (i.e., swap) the source and destination repositories "
- "as well as the send and receive filters.",
- )
  parser.add_argument(
  "--send-filter", action="append", default=None, dest="send_filters",
  metavar="command", nargs=1,
@@ -427,14 +403,12 @@ def prepare(filters):
  # not be a valid command.
  remote_cmd = remote_cmd.split()
 
- # Note that intuitively only the send filters can contain a None
- # element in case the --join option is supplied. However, in case we
- # have a --reverse option as well the filters could get swapped (which
- # always happens after the insertion of the empty element).
+ # The send filters might contain a None element in case the --join
+ # option is supplied. Handle this case correctly.
  if send_filters:
  send_filters = prepare(send_filters)
  if recv_filters:
- recv_filters = prepare(recv_filters)
+ recv_filters = split(recv_filters)
 
  ns.recv_filters = recv_filters
  ns.send_filters = send_filters
@@ -448,7 +422,6 @@ def prepare(filters):
  del ns.src
  del ns.dst
  del ns.command
- del ns.reverse
 
  return command, subvolumes, src_repo, dst_repo
 
@@ -477,16 +450,7 @@ def main(argv):
  # that end, we move it to the end of the options because the
  # ArgumentParser evaluates arguments in the order in which they are
  # provided in the argument vector.
- # In order to have less branching in various cases, we want to
- # evaluate the --reverse option on the fly. This option has a special
- # action that reverses the source and destination repositories as well
- # as the send and receive filters. However, in order for it to work
- # correctly, it needs to be evaluated just before the --snapshot-ext
- # option (because the latter requires the reordered arguments) but
- # after all other options. Hence, we move it to the end right before
- # we do the same with the --snapshot-ext option.
  args = argv[1:].copy()
- args = reorderArg(args, "--reverse", has_arg=False)
  args = reorderArg(args, "--snapshot-ext", has_arg=True)
  namespace = parser.parse_args(args)
 

btrfs-backup/src/deso/btrfs/test/testMain.py

@@ -1,7 +1,7 @@
 # testMain.py
 
 #/***************************************************************************
-# * Copyright (C) 2015 Daniel Mueller (deso@posteo.net)  *
+# * Copyright (C) 2015-2016 Daniel Mueller (deso@posteo.net) *
 # * *
 # * This program is free software: you can redistribute it and/or modify *
 # * it under the terms of the GNU General Public License as published by *
@@ -438,20 +438,7 @@ def performTest(self, backup, restore, src, dst):
  self.assertContains(m.path(user, "data", "movie.mp4"), "abcdefgh")
  self.assertContains(m.path(root, ".ssh", "key.pub"), "1234567890")
 
- # Case 3) Once again delete all snapshots but this time
- # restore them in conjunction with the --reverse
- # option.
- self.wipeSubvolumes(self._snapshots)
-
- # This time we use the '--reverse' option.
- restore("--reverse", "--snapshots-only", src, dst, reverse=True)
-
- user, root = glob(m.path("snapshots", "*"))
-
- self.assertContains(m.path(user, "data", "movie.mp4"), "abcdefgh")
- self.assertContains(m.path(root, ".ssh", "key.pub"), "1234567890")
-
- # Case 4) Also delete the original subvolumes we snapshotted
+ # Case 3) Also delete the original subvolumes we snapshotted
  # and verify that they can be restored as well.
  self.wipeSubvolumes(m.path("home"))
  self.wipeSubvolumes(m.path(), pattern="root")
@@ -514,9 +501,8 @@ def backup(src, dst, *options):
  ]
  self.backup(src, dst, *options)
 
- def restore(src, dst, *options, reverse=False):
+ def restore(src, dst, *options):
  """Invoke the program to restore snapshots/subvolumes from an encrypted source."""
- filt = "recv" if reverse else "send"
  gpg_options = "--decrypt --no-default-keyring "\
  "--keyring={pubkey} --secret-keyring={privkey} "\
  "--trust-model=always --batch {{file}}"
@@ -525,10 +511,10 @@ def restore(src, dst, *options, reverse=False):
  options = list(options)
  options += [
  "--snapshot-ext=gpg",
- "--%s-filter=%s %s" % (filt, GPG, gpg_options),
+ "--send-filter=%s %s" % (GPG, gpg_options),
  "--join",
  ]
- self.restore(src, dst, *options, reverse=reverse)
+ self.restore(src, dst, *options)
  try:
  GPG = findCommand("gpg")
  except FileNotFoundError:
@@ -591,17 +577,16 @@ def backup(*options):
  ]
  self.backup(*options)
 
- def restore(src, dst, *options, reverse=False):
+ def restore(src, dst, *options):
  """Invoke the program to restore snapshots/subvolumes from a remote host."""
- filt = "recv" if reverse else "send"
  options = list(options)
  options += [
  "--remote-cmd=/usr/bin/ssh %s" % host,
  "--snapshot-ext=bin",
- "--%s-filter=/bin/dd if={file}" % filt,
+ "--send-filter=/bin/dd if={file}",
  "--no-read-stderr",
  ]
- self.restore(src, dst, *options, reverse=reverse)
+ self.restore(src, dst, *options)
 
  try:
  SSH = findCommand("ssh")