Re: [PATCH] Add a --config-dir option

Shlomi Fish <shlomif@vipe.stud.technion.ac.il> writes:

> This patch adds a --config-dir option which overrides the configuration
> directories. For instance:

Great! However it's a little out of date since r6525.

> svn checkout --config-dir $HOME/myconf/svn/ http://localhost/svn/repos/
>
> Will read the configuration out of $HOME/myconf/svn instead of
> $HOME/.subversion and the global config file.
>
> Here's the log:
>
> <<<
> Added a --config-dir option to svn to override the configuration directory
> with a new per-session parameter.
>
> * subversion/svnadmin/main.c
> Added the svnadmin__config_dir option and configured it for all the
> options.

I think you mean "sub-commands" rather than "options", but why all the
sub-commands? I am confident that "svnadmin help" doesn't need it, I
suspect that other commands such as "svnadmin dump" don't need it
either. I don't like commands having options that have no effect.

> (main): Added the processing of the config_dir option.
> (subcommand_create): added the config_dir param to svn_config_get_config
>
> * svn_config.h
> Modified the declaration and documentation of svn_config_get_config
> to accept an overriding config_dir parameter.
>
> * subversion/libsvn_wc/log.c
> Removed the call to svn_config_get_config which seems like a leftover
> from ancient times.
>
> * subversion/libsvn_subr/config.c
> (get_category_config): added the config_dir param and made it override
> the other directories in case it is set.
> (svn_config_get_config): added the config_dir param and passed it to
> get_category_config
>
> * subversion/clients/cmdline/cl.h
> Added the svn_cl__config_dir_opt option.
> Added the config_dir parameter to the opt_state structure.
>
> * subversion/clients/cmdline/main.c
> Added the config-dir option to the command line processing, and enabled
> it in all sub-commands.

Again, I don't think it is appropriate for all the sub-commands.

> (main): Added processing of the svn_cl__config_dir_opt, and passed
> config_dir to svn_config_get_config.

One thing that appears to be missing is support for --config-dir in
svn_config_ensure(). It doesn't make sense for svn_config_ensure to
use $HOME/.subversion when "--config-dir foo" is specified.

> Index: subversion/svnadmin/main.c
> ===================================================================
> --- subversion/svnadmin/main.c (revision 6516)
> +++ subversion/svnadmin/main.c (working copy)
> @@ -77,7 +77,8 @@
> svnadmin__ignore_uuid,
> svnadmin__force_uuid,
> svnadmin__parent_dir,
> - svnadmin__bdb_txn_nosync
> + svnadmin__bdb_txn_nosync,
> + svnadmin__config_dir,
> };
>
> /* Option codes and descriptions.
> @@ -128,9 +129,13 @@
> {SVN_FS_CONFIG_BDB_TXN_NOSYNC, svnadmin__bdb_txn_nosync, 0,
> "disable fsync at database transaction commit [Berkeley DB]."},
>
> + {"config-dir", svnadmin__config_dir, 1,
> + "override the configuration directory to look for configuration files"},

I'd prefer something shorter, say

         "use configuration files from directory ARG"

> +
> {NULL}
> };
>
> +#define SVNADMIN_OPTS_GLOBAL svnadmin__config_dir

I don't think the #define is appropriate since not all the commands
need --config-dir.

> /* Array of available subcommands.
> * The entire list must be terminated with an entry of nulls.
> @@ -140,18 +145,18 @@
> {"archive", subcommand_archive, {0},
> "usage: svnadmin archive REPOS_PATH\n\n"
> "Ask Berkeley DB which logfiles can be safely deleted.\n\n",
> - {0} },
> + {SVNADMIN_OPTS_GLOBAL} },
>
> {"create", subcommand_create, {0},
> "usage: svnadmin create REPOS_PATH\n\n"
> "Create a new, empty repository at REPOS_PATH.\n",
> {svnadmin__on_disk_template, svnadmin__in_repos_template,
> - svnadmin__bdb_txn_nosync} },
> + svnadmin__bdb_txn_nosync, SVNADMIN_OPTS_GLOBAL} },
>
> {"createtxn", subcommand_createtxn, {0},
> "usage: svnadmin createtxn REPOS_PATH -r REVISION\n\n"
> "Create a new transaction based on REVISION.\n",
> - {'r'} },
> + {'r', SVNADMIN_OPTS_GLOBAL} },
>
> {"dump", subcommand_dump, {0},
> "usage: svnadmin dump REPOS_PATH [-r LOWER[:UPPER]] [--incremental]\n\n"
> @@ -161,12 +166,12 @@
> "revision trees. If only LOWER is given, dump that one revision tree.\n"
> "If --incremental is passed, then the first revision dumped will be\n"
> "a diff against the previous revision, instead of the usual fulltext.\n",
> - {'r', svnadmin__incremental, 'q'} },
> + {'r', svnadmin__incremental, 'q', SVNADMIN_OPTS_GLOBAL} },
>
> {"help", subcommand_help, {"?", "h"},
> "usage: svn help [SUBCOMMAND1 [SUBCOMMAND2] ...]\n\n"
> "Display this usage message.\n",
> - {svnadmin__version} },
> + {svnadmin__version, SVNADMIN_OPTS_GLOBAL} },
>
> {"load", subcommand_load, {0},
> "usage: svnadmin load REPOS_PATH\n\n"
> @@ -174,7 +179,7 @@
> "new revisions into the repository's filesystem. If the repository\n"
> "was previously empty, its UUID will, by default, be changed to the\n"
> "one specified in the stream. Progress feedback is sent to stdout.\n",
> - {svnadmin__ignore_uuid, svnadmin__force_uuid, svnadmin__parent_dir} },
> + {svnadmin__ignore_uuid, svnadmin__force_uuid, svnadmin__parent_dir, SVNADMIN_OPTS_GLOBAL} },

Please split lines that are longer than 80 characters.

-- 
Philip Martin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org