artagnon_at_apache.org wrote on Sun, 25 Jul 2010 at 12:21 -0000:
> Author: artagnon
> Date: Sun Jul 25 09:20:55 2010
> New Revision: 979011
>
> URL: http://svn.apache.org/viewvc?rev=979011&view=rev
> Log:
> Add a build.conf and get svnrload to build
>
> * build.conf
> (svnrdump): Add new section to build svnrdump.
> (__ALL__): Add svnrdump.
> * subversion/svnrload
> (svn:ignore): Copy properties from svnrdump.
> * subversion/svnrload/load_editor.c: Change the path to the load
> header.
> * subversion/svnrload/parse_dumpstream.c
> Correct ident to match GNU style
> (main): Stub out the callbacks and use the default parser. Also wrap
> functions in SVN_INT_ERR appropriately for error handling.
> * subversion/svnrload/svnrload.c
> (main): Stub out function completely in favor of the main in
> parse_dumpstream.c
>
Nice log message.
(Next time you might consider adding blank lines before every /^*/ line,
for readability.)
> Modified:
> subversion/branches/svnrload/build.conf
> subversion/branches/svnrload/subversion/svnrload/ (props changed)
> subversion/branches/svnrload/subversion/svnrload/load_editor.c
> subversion/branches/svnrload/subversion/svnrload/parse_dumpstream.c
> subversion/branches/svnrload/subversion/svnrload/svnrload.c
>
I'm pretty sure you want to add it in a few other places; at least in
build/transform_libtool_scripts.sh. And maybe in a few others... best
bet is to 'grep -R svnversion $trunk'.
> Modified: subversion/branches/svnrload/build.conf
> URL: http://svn.apache.org/viewvc/subversion/branches/svnrload/build.conf?rev=979011&r1=979010&r2=979011&view=diff
> ==============================================================================
> --- subversion/branches/svnrload/build.conf (original)
> +++ subversion/branches/svnrload/build.conf Sun Jul 25 09:20:55 2010
> @@ -175,6 +175,13 @@ libs = libsvn_client libsvn_ra libsvn_de
> install = bin
> manpages = subversion/svnrdump/svnrdump.1
>
> +[svnrload]
> +description = Subversion remote repository loader
> +type = exe
> +path = subversion/svnrload
> +libs = libsvn_client libsvn_ra libsvn_delta libsvn_subr apr
> +install = bin
For consistency, please:
+manpages = subversion/svnrload/svnrload.1
> +
> # Support for GNOME Keyring
> [libsvn_auth_gnome_keyring]
> description = Subversion GNOME Keyring Library
> @@ -1073,7 +1080,7 @@ libs = libsvn_fs_base libsvn_fs_fs
> [__ALL__]
> type = project
> path = build/win32
> -libs = svn svnserve svnadmin svnlook svnversion svnrdump svndumpfilter
> +libs = svn svnserve svnadmin svnlook svnversion svnrdump svnrload svndumpfilter
> mod_authz_svn mod_dav_svn svnsync
>
> [__ALL_TESTS__]
>
> Modified: subversion/branches/svnrload/subversion/svnrload/parse_dumpstream.c
> URL: http://svn.apache.org/viewvc/subversion/branches/svnrload/subversion/svnrload/parse_dumpstream.c?rev=979011&r1=979010&r2=979011&view=diff
> ==============================================================================
> --- subversion/branches/svnrload/subversion/svnrload/parse_dumpstream.c (original)
> +++ subversion/branches/svnrload/subversion/svnrload/parse_dumpstream.c Sun Jul 25 09:20:55 2010
> @@ -9,8 +9,8 @@ new_revision_record(void **revision_bato
> void *parse_baton,
> apr_pool_t *pool)
> {
> - printf("new_revision_record called");
> - return SVN_NO_ERROR;
> + printf("new_revision_record called");
> + return SVN_NO_ERROR;
> }
>
Please don't mix whitespace/indentation changes and semantic changes in
the same commit.
(I'm sure HACKING says that --- doesn't it?)
> -int main()
> +int
> +main(int argc, char **argv)
> {
> - apr_pool_t *pool;
> - apr_file_t *dumpfile;
> - svn_stream_t *dumpstream;
> - svn_repos_parse_fns2_t *parser;
> -
> - if (svn_cmdline_init ("parse_dumpstream", stderr) != EXIT_SUCCESS)
> - return EXIT_FAILURE;
> - pool = svn_pool_create(NULL);
> -
> - parser = apr_pcalloc(pool, sizeof(*parser));
> -
> - parser->new_revision_record = new_revision_record;
> - parser->new_node_record = new_node_record;
> - parser->uuid_record = uuid_record;
> - parser->set_revision_property = set_revision_property;
> - parser->set_node_property = set_node_property;
> - parser->remove_node_props = remove_node_props;
> - parser->set_fulltext = set_fulltext;
> - parser->close_node = close_node;
> - parser->close_revision = close_revision;
> - parser->delete_node_property = delete_node_property;
> - parser->apply_textdelta = apply_textdelta;
> -
> - apr_file_open_stdin(&dumpfile, pool);
> - dumpstream = svn_stream_from_aprfile2(dumpfile, FALSE, pool);
> - svn_repos_parse_dumpstream2(dumpstream, parser, NULL, NULL, NULL, pool);
> - svn_pool_destroy(pool);
> - return 0;
> + apr_pool_t *pool;
> + apr_file_t *dumpfile;
> + svn_stream_t *dumpstream;
> + svn_repos_parse_fns2_t *parser;
How am I supposed to review the changes to this function?
> + struct parser_baton_t *parser_baton;
> +
> + if (svn_cmdline_init ("parse_dumpstream", stderr) != EXIT_SUCCESS)
> + return EXIT_FAILURE;
> + pool = svn_pool_create(NULL);
> +
> + parser = apr_pcalloc(pool, sizeof(*parser));
> +
> + /* parser->new_revision_record = new_revision_record; */
> + /* parser->new_node_record = new_node_record; */
> + /* parser->uuid_record = uuid_record; */
> + /* parser->set_revision_property = set_revision_property; */
> + /* parser->set_node_property = set_node_property; */
> + /* parser->remove_node_props = remove_node_props; */
> + /* parser->set_fulltext = set_fulltext; */
> + /* parser->close_node = close_node; */
> + /* parser->close_revision = close_revision; */
> + /* parser->delete_node_property = delete_node_property; */
> + /* parser->apply_textdelta = apply_textdelta; */
> +
All this are "to be done", I guess?
> + apr_file_open_stdin(&dumpfile, pool);
> + dumpstream = svn_stream_from_aprfile2(dumpfile, FALSE, pool);
> + SVN_INT_ERR(svn_repos_parse_dumpstream2(dumpstream, parser, NULL,
> + NULL, NULL, pool));
> + SVN_INT_ERR(svn_stream_close(dumpstream));
> + svn_pool_destroy(pool);
> + return 0;
> }
>
> +/* const char *url = NULL; */
> +/* char *revision_cut = NULL; */
> +/* svn_revnum_t start_revision = svn_opt_revision_unspecified; */
> +/* svn_revnum_t end_revision = svn_opt_revision_unspecified; */
> +/* svn_revnum_t latest_revision = svn_opt_revision_unspecified; */
> +/* svn_boolean_t quiet = FALSE; */
> +/* apr_pool_t *pool = NULL; */
> +/* svn_ra_session_t *session = NULL; */
> +/* const char *config_dir = NULL; */
> +/* const char *username = NULL; */
> +/* const char *password = NULL; */
> +/* svn_boolean_t no_auth_cache = FALSE; */
> +/* svn_boolean_t non_interactive = FALSE; */
> +/* apr_getopt_t *os; */
> +
Why not "#if 0" the block?
> +/* SVN_INT_ERR(svn_cmdline_fprintf(stderr, pool, */
> +/* "LOWER cannot be greater " */
> +/* "than UPPER.\n")); */
Unrelated, but again the error message says LOWER and UPPER. Could you
rephrase it in a way that uses the semantics of the arguments rather
than names in the help syntax?
e.g., I'm sure we already have such a message somewhere... (/me opens
fr.po) Here it is:
#: ../libsvn_repos/dump.c:1048 ../libsvn_repos/dump.c:1290
#, c-format
msgid "Start revision %ld is greater than end revision %ld"
msgstr "La révision de début %ld est plus grande que celle de fin %ld"
Daniel
(btw, just last week I included the name of one of a function's formal
parameters in an error message given in response to an API violation.
Which, arguably, is disaligned with what I just preached.)
Received on 2010-07-25 15:08:02 CEST