Re: [PATCH] command line options to use external command to compress

Hi.

Thank you for your feedback
I fixed the patch.

[[[
   add two command line options, --bzip2-cmd and --gzip-cmd
   to svn-backup-dumps.py

   if --bzip2-cmd option is specified, the script uses bzip2 command to
   compress the dump stream instead of python bzip2 module.

   if --gzip-cmd option is specified, the script uses gzip command to
   compress the dump stream instead of python gzip module.

   * tools/server-side/svn-backup-dumps.py
     (SvnBackupOutputCommand): add new class to launch external command to
                               compress.
     (SvnBackup.__init__): add two member variables, __bzip2_path and
                           __gzip_path to hold command line parameters
     (SvnBackup.__init__): add mutually exclusive checks for compress options.
     (SvnBackup.create_dump): create instance of SvnBackupOutputCommand
                              when __bzip2_path or __gzip_path is valid
     (): fix command option help for "-b" and "-z" to distinguish from
         --bzip2-cmd and --gzip-cmd option.
     (): change parser.add_option parameters for -b and -z to
         do mutually exclusive checks
   ]]]

> I reviewed your patch which looks good. But before committing I want to have
> some changes,
>
> 1) In the usage message we have
> -b compress the dump using bzip2.
> --bzip2-path=BZIP2_PATH
> compress the dump using bzip2 command
>
> which is confusing. May be we can say 'compress the dump using python bzip2
> library' for the former and 'compress the dump using bzip2 custom command'
> for the later?
> 2) Same thing applies for gzip command too.
>

added comments.

> 3) May be we can detect usage of -b and --bzip2-path together and say they are
> mutually exclusive? This is because, with the current patch if we give both,
> only --bzip2-path works and -b is silently ignored, which may confuse users.

added mutually exclusive check for compress options.

Regards.
Masaru.

------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2408997