On Wed, Feb 12, 2003 at 06:08:29PM -0800, Greg Stein wrote:
> > + static int magic_len = sizeof(SVN_REPOS_DUMPFILE_MAGIC_HEADER) - 1;
>
> static const. Let's keep that mutable data segment small! :-)
Blargh. Too much java programming.
>
> Style nit, of course, so feel free to ignore. Or ponder and change :-)
>
When in Rome...
> >...
> > + value = atoi (p+1);
> > +
> > + if (atoi(p+1) > SVN_REPOS_DUMPFILE_FORMAT_VERSION)
>
> May as well use 'value' here rather than another atoi() call.
>
Blargh. Too much programming. :-) The whole *point* of "value"
was to avoid extra calls to atoi().
> 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.
Yes, or two "repo metadata" records, with the second one overwriting the
first, etc. This isn't necessarily a bad thing, though.
I feel that this stance conflicts somewhat with the "be (spectacularly)
generous in what you accept" philosophy built into the dumpfile parser.
It didn't even validate the version-header before! I agree, though, that
generally the looser the early revs of a parser, the more contorted the
later revs (and parsers) have to be to maintain compatibility (HTML is a
case in point).
--ben
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Thu Feb 13 04:21:15 2003