On Fri, Jan 31, 2003 at 11:27:01AM -0600, Karl Fogel wrote:
> mbk@tigris.org writes:
> > In that light, this change adds a new BDB table to the FS: uuids.
> > The table is a RECNO table, with fixed-length records of length
> > sizeof(uuid). The first record in this table is considered the
> > repository's own UUID. This value is preserved across dump/load
> > cycles. If one wants to load without changing the UUID, it can
> > simply be removed from dump file (it is always the third line).
>
> ...But the recommended way is to use 'svnadmin load --ignore-uuid' :-).
This was a "re-use of [PATCH] e-mail" bug. I noticed and tweaked the
log entry.
>
> However, I think we can can automate this --ignore-uuid feature a bit.
> Consider: When loading into a repository at revision 0, you almost
> always want to update to the UUID from the dumpfile. But when loading
> into a repository that already *has* data, you probably don't want to
> change the UUID.
Yeah, this sounds reasonable. You could argue that if the target repo
already has data, then either
1.) it already has the right UUID because it has been synchronized
before
or
2.) it has other data, in which case making it *lie* about being the
same repository as named in the dump is probably not such a great
idea.
>
> So, maybe that conditional should be the default behavior, and we
> would instead have two options: one to force using the uuid, and one
> to ignore it.
>
Yup.
> > +/** Populate @a *uuid with the UUID associated with @a fs.
> > + *
> > + * Populate @a *uuid with the UUID associated with @a fs.
> > + */
>
> Sometimes doxgyen side effects can be annoying :-).
I could have sworn I deleted these when Brane pointed them out.
I must have only imagined that.
>
> > +/** Associate @a *uuid with @a fs.
> > + *
> > + * Associate @a *uuid with @a fs.
> > + */
>
> Did I mention how doxgyen side effects can sometimes be annoying?
Is it my imagination, or is there an echo in here?
> > +
> > + SVN_ERR (BDB_WRAP (fs,
> > + "get repository uuid",
> > + uuids->put (uuids, trail->db_txn, &key, &value, 0)));
> Cut-and-pasted error string -- you want "set", not "get".
Done.
>
> > +/* Open a `uuids' table in @a env.
> > + *
> > + * Open a `uuids' table in @a env. If @a create is non-zero, create
> > + * one if it doesn't exist. Set @a *uuids_p to the new table.
> > + * Return a Berkeley DB error code.
> > + */
> > +int svn_fs__bdb_open_uuids_table (DB **uuids_p,
> > + DB_ENV *env,
> > + int create);
>
> 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.
> Also, hmmm, what is our backwards compatibility plan?
>
> In svn_fs_create_berkeley(), you pass the `create' flag to
> svn_fs__bdb_open_uuids_table(), but in svn_fs_open_berkeley() you
> don't. That makes sense, but maybe for existing repositories, the
> latter call should also pass the `create' flag for a while, just so
> people can grow the new uuid table without needing to do anything
> special? We could change it back to 0 after a couple of releases.
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). You should spend more time online. :-)
> Oh, and `libsvn_fs/structure' should document the new table. That
> file, despite its scruffy and unofficial look, has actually been kept
> very up to date, and is the go-to document for anyone learning how the
> filesystem works.
Oooh...
> > + if (svn_err) goto error;
> > + svn_err = BDB_WRAP (fs, "creating `uuids' table",
> > + svn_fs__bdb_open_uuids_table (&fs->uuids,
> > fs->env, 0));
> > if (svn_err) goto error;
>
> ...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.
> > + /* If this is not the "this dir" entry, and the uuid is
> > + the same as that of the "this dir" entry, don't write out
> > + the uuid. */
> > + if (entry->uuid)
> > + {
> > + if (!strcmp(entry->uuid, this_dir->uuid))
> > + apr_hash_set (atts, SVN_WC__ENTRY_ATTR_UUID,
> > + APR_HASH_KEY_STRING, NULL);
> > + }
> > +
>
> This may just be a personal thing -- I really like the
>
> if (strcmp (foo, bar) == 0)
>
> and
>
> if (strcmp (foo, bar) != 0)
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.
> > 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.
Then I wanted to have a separate "record" for the UUID.
>
> I'm assuming that your goal was to avoid needing to bump
> SVN_REPOS_DUMPFILE_FORMAT_VERSION, by putting the uuid in a new
> header, right? (Since unknown headers are ignored by the loader.)
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?
>
> Anyway, this change looks great! There's no really appropriate place
> to say "nice job" in the middle of the code review, so I'll just say
> it here.
Anyway, this code-review looks great. It's probably appropriate to
say "nice job" right here, so please take that as read, which by now
it has been.
>
> Anyway, I think the development of the new RA interface for fetching
> uuid can just happen on trunk after this is merged.
>
I have it (mostly) done already; I just figured it would be easier
to review in smaller chunks.
--ben
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Fri Jan 31 22:19:37 2003