(read the last review hunk first)
Ramkumar Ramachandra wrote on Thu, Aug 19, 2010 at 11:17:55 +0530:
> [[[
> * subversion/libsvn_repos/load.c
> (svn_repos_parse_dumpstream2, svn_repos_parse_dumpstream3): Rename
> the older function and add a version_number argument; error out if
> there's a version mismatch.
>
> * subversion/include/svn_repos.h
> (svn_repos_parse_dumpstream2, svn_repos_parse_dumpstream3): Add new
> function and mark svn_repos_parse_dumpstream2 as deprecated.
>
> * subversion/svnrdump/load_editor.c
> (drive_dumpstream_loader): Update to use the new API, and call it
> with version_number 3.
> ]]]
>
> Index: subversion/svnrdump/load_editor.c
> ===================================================================
> --- subversion/svnrdump/load_editor.c (revision 986884)
> +++ subversion/svnrdump/load_editor.c (working copy)
> @@ -540,8 +540,10 @@ drive_dumpstream_loader(svn_stream_t *stream,
> pb = parse_baton;
>
> SVN_ERR(svn_ra_get_repos_root2(session, &(pb->root_url), pool));
> - SVN_ERR(svn_repos_parse_dumpstream2(stream, parser, parse_baton,
> - NULL, NULL, pool));
> + SVN_ERR(svn_repos_parse_dumpstream3(stream, parser, parse_baton,
> + NULL, NULL,
> + SVN_REPOS_DUMPFILE_FORMAT_VERSION,
Wrong macro: when we introduce dumpfile format 4, this will become "4",
but I think you want it to remain "3" even then, right?
The alternative is to add a new (feature-oriented) macro whose value
will *not* change. See libsvn_fs_fs/fs.h:90 and below.
> + pool));
>
> return SVN_NO_ERROR;
> }
> Index: subversion/include/svn_repos.h
> ===================================================================
> --- subversion/include/svn_repos.h (revision 986884)
> +++ subversion/include/svn_repos.h (working copy)
> @@ -2286,6 +2286,7 @@ svn_repos_node_from_baton(void *edit_baton);
> /* The RFC822-style headers in our dumpfile format. */
> #define SVN_REPOS_DUMPFILE_MAGIC_HEADER "SVN-fs-dump-format-version"
> #define SVN_REPOS_DUMPFILE_FORMAT_VERSION 3
> +#define SVN_REPOS_DUMPFILE_FORMAT_UNKNOWN_VERSION -1
> #define SVN_REPOS_DUMPFILE_UUID "UUID"
> #define SVN_REPOS_DUMPFILE_CONTENT_LENGTH "Content-length"
>
> @@ -2646,6 +2647,11 @@ typedef svn_repos_parse_fns2_t svn_repos_parser_fn
> * @a cancel_baton as argument to see if the client wishes to cancel
> * the dump.
> *
> + * If @a exact_version is SVN_REPOS_DUMPFILE_FORMAT_UNKNOWN_VERSION,
#define macroname SVN_REPOS_DUMPFILE_FORMAT_UNKNOWN_VERSION
In doxygen, you can write "#macroname" or "@c macroname". (IIRC the first
makes a link and the second makes constant-width font; untested.)
> + * it is ignored and the dumpstream is parsed without this
> + * information. Else, the function checks the dumpstream's version
> + * number, and errors out if there's a mismatch.
> + *
Sometimes, in situations like this we guarantee exactly which error code
will be returned, for the benefit of API users who want to trap a
specific (non-fatal) error condition. See svn_ra_open4() for an
example.
> * This parser has built-in knowledge of the dumpfile format, but only
> * in a general sense:
> *
> @@ -2661,17 +2667,33 @@ typedef svn_repos_parse_fns2_t svn_repos_parser_fn
> * This is enough knowledge to make it easy on vtable implementors,
> * but still allow expansion of the format: most headers are ignored.
> *
> - * @since New in 1.1.
> + * @since New in 1.7.
> */
> svn_error_t *
> -svn_repos_parse_dumpstream2(svn_stream_t *stream,
> +svn_repos_parse_dumpstream3(svn_stream_t *stream,
> const svn_repos_parse_fns2_t *parse_fns,
> void *parse_baton,
> svn_cancel_func_t cancel_func,
> void *cancel_baton,
> + int exact_version,
> apr_pool_t *pool);
>
> +/**
> + * Similar to svn_repos_parse_dumpstream3(), but with @a exact_version
> + * always set to SVN_REPOS_DUMPFILE_FORMAT_UNKNOWN_VERSION.
> + *
> + * @since New in 1.1.
> + * @deprecated Provided for backward compatibility with the 1.6 API.
> + */
Looks good.
> +SVN_DEPRECATED
> +svn_error_t *svn_repos_parse_dumpstream2(svn_stream_t *stream,
> + const svn_repos_parse_fns2_t *parse_fns,
> + void *parse_baton,
Style nitpick:
The parameters aren't aligned properly. (the previous declaration does
it correctly.)
> + svn_cancel_func_t cancel_func,
> + void *cancel_baton,
> + apr_pool_t *pool);
>
> +
> /**
> * Set @a *parser and @a *parse_baton to a vtable parser which commits new
> * revisions to the fs in @a repos. The constructed parser will treat
> Index: subversion/libsvn_repos/load.c
> ===================================================================
> --- subversion/libsvn_repos/load.c (revision 986884)
> +++ subversion/libsvn_repos/load.c (working copy)
> @@ -654,14 +654,29 @@ parse_format_version(const char *versionstring, in
> }
>
>
> +svn_error_t *
> +svn_repos_parse_dumpstream2(svn_stream_t *stream,
> + const svn_repos_parse_fns2_t *parse_fns,
> + void *parse_baton,
> + svn_cancel_func_t cancel_func,
> + void *cancel_baton,
> + apr_pool_t *pool)
> +{
> + return svn_repos_parse_dumpstream3(stream, parse_fns, parse_baton,
> + cancel_func, cancel_baton,
> + SVN_REPOS_DUMPFILE_FORMAT_UNKNOWN_VERSION,
> + pool);
> +}
>
> +
> /* The Main Parser Logic */
> svn_error_t *
> -svn_repos_parse_dumpstream2(svn_stream_t *stream,
> +svn_repos_parse_dumpstream3(svn_stream_t *stream,
> const svn_repos_parse_fns2_t *parse_fns,
> void *parse_baton,
> svn_cancel_func_t cancel_func,
> void *cancel_baton,
> + int exact_version,
> apr_pool_t *pool)
> {
> svn_boolean_t eof;
> @@ -690,6 +705,15 @@ svn_error_t *
> return svn_error_createf(SVN_ERR_STREAM_MALFORMED_DATA, NULL,
> _("Unsupported dumpfile version: %d"), version);
>
> + /* Error out if exact_version doesn't match version exactly */
> + /* ### TODO: Extend svn_parse_fns2_t to provide another callback for
> + ### version number; that way we can impose arbitrary constraints
> + ### on it, not just check for exact version */
> + if (exact_version != SVN_REPOS_DUMPFILE_FORMAT_UNKNOWN_VERSION
> + && exact_version != version)
> + return svn_error_createf(SVN_ERR_STREAM_MALFORMED_DATA, NULL,
> + _("Unsupported dumpfile version: %d"), version);
> +
I'm not sure what we gain by committing this patch when you already have
a parse_fns3_t patch outstanding. (I haven't looked at that thread
yet.) Wouldn't it make more sense to first commit that patch, than to
commit this one, then that one, and then a revision of that one to use
the new API?
Note: it's acceptable to post patches that depend on previous patches.
(So you could write this patch in terms of parse_fns3_t directly.)
There are a number of ways to manage the interdependencies... (you
mentioned quilt on IRC; I know some folks here use Mercurial patch
queues and similar tricks)
> /* A dumpfile "record" is defined to be a header-block of
> rfc822-style headers, possibly followed by a content-block.
>
>
> -- Ram
Received on 2010-08-19 09:23:47 CEST