Re: svn commit: rev 4775 - in branches/issue-1037-uuids/subversion: include libsvn_repos

On Thu, Feb 06, 2003 at 09:57:54PM -0600, mbk@tigris.org wrote:
>...
> +++ branches/issue-1037-uuids/subversion/libsvn_repos/load.c Thu Feb 6 21:57:45 2003
>...
> +parse_format_version (const char *versionstring, int *version)
> {
> - /* ### parse string and verify that we support the dumpfile format
> - version number. */
> -
> + /* parse string and verify that we support the dumpfile format
> + version number, setting *version appropriately. */
> + static int magic_len = sizeof(SVN_REPOS_DUMPFILE_MAGIC_HEADER) - 1;

static const. Let's keep that mutable data segment small! :-)

> + const char *p = strchr(versionstring, ':');
> + int value;
> +
> + if (p == NULL ||
> + p != (versionstring + magic_len) ||
> + strncmp (versionstring,
> + SVN_REPOS_DUMPFILE_MAGIC_HEADER,
> + magic_len))

Personally, I prefer the following form:

    if (p == NULL
        || p != (...)
        || strncmp (...))

As you read down the lines, it is easier to see how the conditions are being
combined. In your format, the reader must track over the condition to the
end of the line to figure out how the condition on the *next* line is being
combined.

Style nit, of course, so feel free to ignore. Or ponder and change :-)

>...
> + value = atoi (p+1);
> +
> + if (atoi(p+1) > SVN_REPOS_DUMPFILE_FORMAT_VERSION)

May as well use 'value' here rather than another atoi() call.

I'll also note that you aren't using the results of the version parsing.
Instead, you're just checking for "later than this." I would have expected
the caller to say "if version == 2, then let's read a repos block;
otherwise, [for version 1] let's start reading nodes". The caller enables
that extra block by looking for certain types of data in there (the UUID
field). That seems a bit "loose" if you will. It allows the file to move the
"repo metadata" to *any* location, rather than up front as you defined it in
your email.

IOW, the stated format of the file doesn't match the implementation. The
impl is much more forgiving. I'm not sure that we want to allow arbitrary
blocks scattered about, as long as they have a UUID field.

Cheers,
-g

-- 
Greg Stein, http://www.lyra.org/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org