mark benedetto king <mbk@boredom.org> writes:
> > This function also creates the first uuid, right? It should document
> > that fact.
> >
>
> Sure, but the "strings" analog to this function creates a `next-key' table
> entry, and doesn't mention that.
Oh, sure, but `next-key' is the strings table's internal management
business, whereas the presence or absence of a particular uuid is
publically visible -- in fact, is the whole point of the interface.
So I think this is different.
> I considered this, and discussed it on #svn with Ben Collins-Sussman
> and Bill Tutt. Both were against it. (I think more from the precedent
> of trying to maintain schema compatibility, rather than on technical
> grounds).
Nope. Ben denies it, at least; haven't asked Bill :-).
What's the bad precedent? Backwards compatibility is a bad precedent?
Here's what I'm thinking: with this tweak, the first time the fs code
tries to read the uuid, it will succeed, whether the repository is old
or new. So old repositories will be "upgraded" smoothly, with no
admin intervention required, and thereafter will look the same as new
repositories.
Thus, there is no need to change the repository's format number. (And
if we're not going to stay compatible, then that number should get
bumped, since new fs code would error when trying to read uuids).
> You should spend more time online. :-)
Heh. ~shudder~ No, trust me, I shouldn't :-).
> > ...this is the open call, and therefore should say "opening", not
> > "creating". Yes, the call above it is a typo, not your fault :-).
> > Every other call in that function says "opening", not "creating".
> >
>
> Except for the "representations" one. All three are fixed.
Oops, sorry, I saw two, and forgot that I was looking at a pristine
file w/o your patch (i.e., with your patch, it would be three, right).
> It's definitely a personal thing, but if I can use the crazy
> indentation and bizarro-lispy spaces-before-parentheses, I can follow
> the local strcmp idiom.
Totally your call.
>> SVN_ERR (svn_stream_printf (stream, pool, SVN_REPOS_DUMPFILE_MAGIC_HEADER
>> - ": %d\n", SVN_REPOS_DUMPFILE_FORMAT_VERSION));
>> + ": %d\n\n", SVN_REPOS_DUMPFILE_FORMAT_VERSION));
>
>> Is this a compatible format change? It seems to tack on a new newline
>> above. Will old loaders really handle this compatibly? (I haven't
>> tested, just guessing here; maybe I'm just misunderstanding something
>> about the line breaks in the format file?)
>
> I believe it is.
>
> /* The first two lines of the stream are the dumpfile-format version
> number, and a blank line. */
> SVN_ERR (validate_format_version (linebuf->data));
>
> I wanted to keep that true.
Hmmm. But then why wasn't that extra newline there in the first
place, I wonder?...
In other words, is your patch keeping the dump format the way it was,
or is it changing the format in such as way as to bring it in
compliance with a possibly wrong comment? Because the loop right
below this change (that is, the loop in which write_revision_record()
is called) does not print a "\n" first thing, as far as I can tell.
Here, watch this output from the current code:
gauss$ svnadmin dump repositories/basic_tests-1
SVN-fs-dump-format-version: 1
Revision-number: 0
Prop-content-length: 56
Content-length: 56
[...]
$
Hmmm, no blank line.
So I think that comment was just wrong. In fact, I know it is,
because the implementation of validate_format_version() is empty:
static svn_error_t *
validate_format_version (const char *versionstring)
{
/* ### parse string and verify that we support the dumpfile format
version number. */
return SVN_NO_ERROR;
}
Furthermore, its very interface prohibits it from verifying that
there's a version number followed by a blank line here, since it only
takes one line as input, and has no idea what might be on the next
line.
(Whew, the things you uncover by making a big change...)
So there's something funny going on here, but it's independent of the
change you're making and probably shouldn't be affected by it. (Or,
if you think this is a good time to fix the problem, let's fix it in
trunk as a separate change, which would include fixing
validate_format_version's interface and filling in the
implementation.)
> Not just a new header, a new record type. I think old loaders will
> not be able to load the new format (unknown record types are considered
> "bogosity" (line 463)). New loaders will, however, be able to load
> the old format. I suppose that I should bump the version, so that
> old loaders can gracefully degrade, rather than kick out an unrecognized
> UUID record. What do you think?
Yeah, I think so. (That's really the only reason we have the format
number.)
> I have it (mostly) done already; I just figured it would be easier
> to review in smaller chunks.
Oh yes, definitely, thanks :).
-K
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Fri Jan 31 23:17:06 2003