Re: [PATCH] v2 Add archive support to hot-backup

On Tue, 21 Feb 2006, Chris Foote wrote:

> Thanks Dan for the review.
>
> Here's an updated version of the patch and log.

Chris, again this looks good. If you're up for it, I'd like a last
few changes made before committing (related to UI and error handling).
Comments inline below...

> Add option to archive the backup using gz, bz2 or zip.
>
> * tools/backup/hot-backup.py.in
> Update copyright year.
> (archive_map): New. Supported archive extentions.
> (usage): New. Print out a usage message.
> Add optional 3rd arg to the command line to specify the archive type.
> (comparator): If archiving, add the archive extenstion to the regexp to
> find archives as well.
> Step 2, also find archives.
> Step 4, archive backup.
> Step 5, look for archives to remove as well.

Indent a couple more space for all those "Step N" lines.

> ----- Original Message -----
> From: "Daniel Rall" <dlr@collab.net>
> To: "Chris Foote" <cfoote@v21.me.uk>
> Cc: <dev@subversion.tigris.org>
> Sent: Monday, February 20, 2006 2:49 AM
> Subject: Re: [PATCH] Add archive support to hot-backup
>
>
> > This patch seems pretty reasonable. A couple comments:
> >
> > 1) Changes to arguments processing aren't mentioned in the log
> > message.
> >
> > 2) Description of the changes to comparator in the log message could
> > be a little more clear.
> >
> > 3) Error handling for a failed import vs. failed execution should be
> > more verbose, with the error string differentiating between the two
> > cases (instead of using a single, generic "failed" message for both).
...
> Index: tools/backup/hot-backup.py.in
> ===================================================================
> --- tools/backup/hot-backup.py.in (revision 18545)
> +++ tools/backup/hot-backup.py.in (working copy)
...
> @@ -36,12 +36,34 @@
> # Number of backups to keep around (0 for "keep them all")
> num_backups = 64
>
> +# Archive types/extentions
> +archive_map = {
> + 'gz' : ".tar.gz",
> + 'bz2' : ".tar.bz2",
> + 'zip' : ".zip"
> + }
> +
> ######################################################################
> # Command line arguments
>
> +def usage():
> + scriptname = os.path.basename(sys.argv[0])
> + sys.stderr.write(
> +"""USAGE: %s REPOS_PATH BACKUP_PATH [ARCHIVE_TYPE]

An optional argument like ARCHIVE_TYPE would be better off as a flag,
rather than a positional arg, because it gives us the flexibility to
add more options in the future without worrying about command-line
interface changes. I suggest something like:

  hot-backup.py [--archive-type=ARCHIVE_TYPE] REPOS_PATH BACKUP_PATH

> -if len(sys.argv) != 3:
> - print "Usage: ", os.path.basename(sys.argv[0]), " <repos_path> <backup_path>"
> +Create a backup of the repository at REPOS_PATH in a subdirectory of
> +the BACKUP_PATH location, named after the youngest revision.
> +
> +If specified, ARCHIVE_TYPE creates an archive of the backup instead;
> +can be one of:
> + bz2 : Creates a bzip2 compressed tar file.
> + gz : Creates a gzip compressed tar file.
> + zip : Creates a compressed zip file.
> +""" % (scriptname,))
> +
> +
> +if len(sys.argv) < 3 or len(sys.argv) > 4:
> + usage()
> sys.exit(1)
>
> # Path to repository
> @@ -53,12 +75,28 @@
> # revision.
> backup_dir = sys.argv[2]
>
> +# Do we want to create an archive of the backup
> +if len(sys.argv) < 4:
> + archive = None
> + ext_re = ""
> +elif not archive_map.has_key(sys.argv[3]):
> + sys.stderr.write(
> +"""Unknown archive type '%s'.
> +
> +""" % (sys.argv[3],))
> + usage()
> + sys.exit(1)
> +else:
> + archive = sys.argv[3]
> + ext_re = "(" + re.escape(archive_map[archive]) + ")?"

You could manage to stick with this positional argument parsing, but
more easily extensionable would be to switch to use of getopt.
(There's some good example of its usage all over our tools and contrib
directories.)

...
> -### Step 4: finally, remove all repository backups other than the last
> +### Step 4: Make an archive of the backup if required.
> +if archive:
> + archive_path = backup_subdir + archive_map[archive]
> + err_msg = ""
> +
> + old_cwd = os.getcwd()
> + try:
> + os.chdir(backup_dir)
> + if os.path.exists(archive_path):
> + os.remove(archive_path)
> +
> + print "Archiving backup to '" + archive_path + "'..."
> + if archive == 'gz' or archive == 'bz2':
> + try:
> + import tarfile
> + tar = tarfile.open(archive_path, 'w:' + archive)
> + tar.add(backup_subdir, os.path.basename(backup_subdir))
> + tar.close()
> + except ImportError, e:
> + err_msg = "Import failed: " + str(e)
> + err_code = -2
> + except tarfile.TarError, e:
> + err_msg = "Tar failed: " + str(e)
> + err_code = -3

There should be two separate try/except blocks here, the first around
the "import tarfile" statement, and the second around the remainder of
the existing "try" block.

TarError is a polymorphic base class, in this case probably
representing read, compress, or stream problems. (It can also
represent extraction, but there's none here.) Handle CompressionError
separately, so you can report a "Compression failed: " message.

> +
> + elif archive == 'zip':
> + try:
> + import zipfile
> +
> + def add_to_zip(baton, dirname, names):
> + zp = baton[0]
> + root = os.path.join(baton[1], '')
> +
> + for file in names:
> + path = os.path.join(dirname, file)
> + if os.path.isfile(path):
> + zp.write(path, path[len(root):])
> + elif os.path.isdir(path) and os.path.islink(path):
> + os.path.walk(path, add_to_zip, (zp, path))
> +
> + zp = zipfile.ZipFile(archive_path, 'w', zipfile.ZIP_DEFLATED)
> + os.path.walk(backup_subdir, add_to_zip, (zp, backup_subdir))
> + zp.close()
> + except ImportError, e:
> + err_msg = "Import failed: " + str(e)
> + err_code = -4
> + except zipfile.error, e:
> + err_msg = "Zip failed: " + str(e)
> + err_code = -5

Handle ImportError as above.

> + finally:
> + os.chdir(old_cwd)
> +
> + if err_code != 0:
> + print "Unable to archive the backup."

Error should go to stderr:

       print >> sys.stderr, ...

For the message itself, how about something a little more verbose to
remove any ambiguity (e.g. "Unable to create an archive for the
backup")?

> + print err_msg
> + sys.exit(err_code)
> + else:
> + print "Archive created, removing backup '" + backup_subdir + "'..."
> + shutil.rmtree(backup_subdir)
> +
...