Re: [PATCH v2 02/11] test-svn-fe: use parse-options
- From
Jonathan Nieder <jrnieder@gmail.com>
- Date
- Jul 24, 2011, 13:04 UTC
- Message-ID
- <20110724130443.GB2934@elie.dc0b.debconf.org>
- In-Reply-To
- <1310559673-5026-3-git-send-email-divanorama@gmail.com>
Hi,
Sorry for the slow response.
Dmitry Ivankov wrote:
> There was custom options parsing. As more options arise it will > be easier to add and document new options with parse-options api.
In particular this gives us a "test-svn-fe -h" command --- sounds good. Might make sense to combine this with the patch that parsoptifies contrib/svn-fe/svn-fe.c.
Show 18 quoted lines
> --- a/test-svn-fe.c
> +++ b/test-svn-fe.c
> @@ -3,28 +3,38 @@
> */
>
> #include "git-compat-util.h"
> +#include "parse-options.h"
> #include "vcs-svn/svndump.h"
> #include "vcs-svn/svndiff.h"
> #include "vcs-svn/sliding_window.h"
> #include "vcs-svn/line_buffer.h"
>
> -static const char test_svnfe_usage[] =
> - "test-svn-fe (<dumpfile> | [-d] <preimage> <delta> <len>)";
> +static const char * const test_svnfe_usage[] = {
> + "test-svn-fe (<dumpfile> | -d <preimage> <delta> <len>)",
> + NULL
> +};With this API, we're allowed to print multiple usage strings. Might as well take advantage of that for clarity:
static const char * const test_svnfe_usage[] = {
"test-svn-fe <dumpfile>",
"test-svn-fe -d <preimage> <delta> <len>",
NULL
};> > +static int d; > +
The variable name is not so memorable. Maybe something like "apply_delta" would do.
Show 5 quoted lines
> -static int apply_delta(int argc, char *argv[])
> +static struct option test_svnfe_options[] = {
> + OPT_SET_INT('d', NULL, &d, "test apply_delta", 1),
> + OPT_END()
> +};Might make sense to take the opportunity to add a mnemonic long option name while at it:
OPT_SET_INT('d', "apply-delta", ...[...]
Show 16 quoted lines
> @@ -37,10 +47,16 @@ static int apply_delta(int argc, char *argv[])
> return 0;
> }
>
> -int main(int argc, char *argv[])
> +int main(int argc, const char *argv[])
> {
> - if (argc == 2) {
> - if (svndump_init(argv[1]))
> + argc = parse_options(argc, argv, NULL, test_svnfe_options,
> + test_svnfe_usage, 0);
> +
> + if (d)
> + return apply_delta(argc, argv);
> +
> + if (argc == 1) {Probably easier to read with the simple and exceptional case first.
if (apply_delta_instead) return apply_delta(argc, argv); if (argc != 1) usage_with_options(...);
if (svndump_init(argv[0])) return 1; ...
Show 12 quoted lines
> + if (svndump_init(argv[0])) > return 1; > svndump_read(NULL); > svndump_deinit(); > @@ -48,7 +64,5 @@ int main(int argc, char *argv[]) > return 0; > } > > - if (argc >= 2 && !strcmp(argv[1], "-d")) > - return apply_delta(argc, argv); > - usage(test_svnfe_usage); > + usage_with_options(test_svnfe_usage, test_svnfe_options);
Except for the minor nits noted above (in particular, hopefully this can be squashed with the corresponding svn-fe patch),
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com> --- test-svn-fe.c | 29 +++++++++++++++-------------- 1 files changed, 15 insertions(+), 14 deletions(-)
diff --git a/test-svn-fe.c b/test-svn-fe.c index 0aab2450..db56b6ba 100644 --- a/test-svn-fe.c +++ b/test-svn-fe.c @@ -3,21 +3,23 @@ */ #include "git-compat-util.h" -#include "parse-options.h" #include "vcs-svn/svndump.h" #include "vcs-svn/svndiff.h" #include "vcs-svn/sliding_window.h" #include "vcs-svn/line_buffer.h" +#include "parse-options.h" static const char * const test_svnfe_usage[] = { - "test-svn-fe (<dumpfile> | -d <preimage> <delta> <len>)", + "test-svn-fe <dumpfile>", + "test-svn-fe -d <preimage> <delta> <len>", NULL }; -static int d; +static int apply_delta_instead; static struct option test_svnfe_options[] = { - OPT_SET_INT('d', NULL, &d, "test apply_delta", 1), + OPT_SET_INT('d', "apply-delta", + &apply_delta_instead, "apply a subversion-format delta", 1), OPT_END() }; @@ -52,17 +54,16 @@ int main(int argc, const char *argv[]) argc = parse_options(argc, argv, NULL, test_svnfe_options, test_svnfe_usage, 0); - if (d) + if (apply_delta_instead) return apply_delta(argc, argv); - if (argc == 1) { - if (svndump_init(argv[0])) - return 1; - svndump_read(NULL); - svndump_deinit(); - svndump_reset(); - return 0; - } + if (argc != 1) + usage_with_options(test_svnfe_usage, test_svnfe_options); - usage_with_options(test_svnfe_usage, test_svnfe_options); + if (svndump_init(argv[0])) + return 1; + svndump_read(NULL); + svndump_deinit(); + svndump_reset(); + return 0; }
-- 1.7.6