Re: [PATCH] add `--quiet' option to `svnadmin dump'

Eric Hanchrow wrote:

>2003-02-11 Eric Hanchrow <offby1@blarg.net>
>
> * subversion/svnadmin/main.c: new option `--quiet'.
> (struct svnadmin_opt_state): new member.
> (subcommand_dump): don't use (or create) stdio_stream if we were
> asked to be quiet.
> (main): handle new 'q' option.
>
> * doc/book/book/ch08.xml: document new `--quiet' option to
> `svnadmin dump'.
>

Note: I have no opinion about whether we need this feature or not. I
have a few comments about the patch itself, though.

>Index: subversion/svnadmin/main.c
>===================================================================
>--- subversion/svnadmin/main.c (revision 4835)
>+++ subversion/svnadmin/main.c (working copy)
>@@ -115,6 +115,9 @@
> {"in-repos-template", svnadmin__in_repos_template, 1,
> "specify template for the repository structure"},
>
>+ {"quiet", 'q', 0,
>+ "no progress (only errors) to stderr"},
>+
> {"ignore-uuid", svnadmin__ignore_uuid, 0,
> "ignore the UUID specified in the file."},
>
>@@ -148,7 +151,7 @@
> "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} },
>+ {'r', svnadmin__incremental, 'q'} },
>
> {"help", subcommand_help, {"?", "h"},
> "usage: svn help [SUBCOMMAND1 [SUBCOMMAND2] ...]\n\n"
>@@ -211,6 +214,7 @@
> svn_boolean_t help; /* --help or -? */
> svn_boolean_t incremental; /* --incremental */
> svn_boolean_t follow_copies; /* --copies */
>+ svn_boolean_t quiet;
>
You're missing a comment here, like the other opt_state members have.

> enum svn_repos_load_uuid uuid_action; /* --ignore-uuid,
> --force-uuid */
> const char *on_disk;
>@@ -264,7 +268,7 @@
> struct svnadmin_opt_state *opt_state = baton;
> svn_repos_t *repos;
> svn_fs_t *fs;
>- svn_stream_t *stdout_stream, *stderr_stream;
>+ svn_stream_t *stdout_stream, *stderr_stream = NULL;
> svn_revnum_t
> lower = SVN_INVALID_REVNUM,
> upper = SVN_INVALID_REVNUM;
>@@ -301,10 +305,13 @@
> a file if they want. :-) Progress feedback goes to stderr. */
> SVN_ERR (create_stdio_stream (&stdout_stream,
> apr_file_open_stdout, pool));
>+ if (!opt_state->quiet)
>+ {
> SVN_ERR (create_stdio_stream (&stderr_stream,
> apr_file_open_stderr, pool));
>
You forgot to indent here.

>+ }
>
>- SVN_ERR (svn_repos_dump_fs (repos, stdout_stream, stderr_stream,
>+ SVN_ERR (svn_repos_dump_fs (repos, stdout_stream, (opt_state->quiet ? NULL : stderr_stream),
> lower, upper, opt_state->incremental, pool));
>
Hm! If you're putting this conditional here, then you don't need the
initialization of stderr_stream to NULL above. But since stderr_stream
is already guaranteed to be NULL if the --quiet option was passed, this
change only confuses readers.

> return SVN_NO_ERROR;
>@@ -377,6 +384,7 @@
> SVN_ERR (svn_utf_cstring_to_utf8 (&path_utf8,
> APR_ARRAY_IDX (args, 0, const char *),
> NULL, pool));
>+
>
Please don't change whitespace if you're not making other changes.

> *(const char **)apr_array_push(paths) =
> svn_path_internal_style (path_utf8, pool);
>
>@@ -509,6 +517,7 @@
> SVN_ERR (svn_utf_cstring_to_utf8 (&filename_utf8,
> APR_ARRAY_IDX (args, 0, const char *),
> NULL, pool));
>+
>
And here.

> filename_utf8 = svn_path_internal_style (filename_utf8, pool);
> SVN_ERR (svn_stringbuf_from_file (&file_contents, filename_utf8, pool));
>
>@@ -623,6 +632,9 @@
> }
> }
> break;
>+ case 'q':
>+ opt_state.quiet = TRUE;
>+ break;
> case 'h':
> case '?':
> opt_state.help = TRUE;
>@@ -653,12 +665,14 @@
> return EXIT_FAILURE;
> }
> break;
>+
>
And here.

> case svnadmin__ignore_uuid:
> opt_state.uuid_action = svn_repos_load_uuid_ignore;
> break;
> case svnadmin__force_uuid:
> opt_state.uuid_action = svn_repos_load_uuid_force;
> break;
>+
>
And again.

> default:
> {
> subcommand_help (NULL, NULL, pool);
>Index: doc/book/book/ch08.xml
>===================================================================
>--- doc/book/book/ch08.xml (revision 4835)
>+++ doc/book/book/ch08.xml (working copy)
>@@ -3305,7 +3305,13 @@
> <screen>
> --revision (-r)
> --incremental
>+--quiet (-q)
> </screen>
>+
>+ <para>The <option>--quiet</option> option suppresses the
>+ <computeroutput>Dumped revision
>+ <replaceable>FOO</replaceable></computeroutput> messages.
>+
> </refsect1>
>
> <refsect1>
>@@ -3335,7 +3341,6 @@
> Content-length: 101
> ...
> </screen>
>-
>
And yet again.

> </refsect1>
> </refentry>
>
>

-- 
Brane Čibej   <brane_at_xbc.nu>   http://www.xbc.nu/brane/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org