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' :-).
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.
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.
I think this would avoid a lot of pilot error.
> --- branches/issue-1037-uuids/subversion/include/svn_fs.h (original)
> +++ branches/issue-1037-uuids/subversion/include/svn_fs.h Fri Jan 31 10:32:07 2003
> @@ -1427,7 +1427,29 @@
> apr_pool_t *pool);
>
>
> +
> +/* UUID manipulation. */
>
> +/** 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 :-).
> +/** Associate @a *uuid with @a fs.
> + *
> + * Associate @a *uuid with @a fs.
> + */
Did I mention how doxgyen side effects can sometimes be annoying?
> +svn_error_t *svn_fs__bdb_set_uuid (svn_fs_t *fs,
> + int idx,
> + const char *uuid,
> + trail_t *trail)
> +{
> + DB *uuids = fs->uuids;
> + DBT key;
> + DBT value;
> +
> + svn_fs__clear_dbt (&key);
> + key.data = &idx;
> + key.size = sizeof(idx);
> +
> + svn_fs__clear_dbt (&value);
> + value.size = strlen(uuid) + 1;
> + value.data = apr_pstrmemdup(trail->pool, uuid, value.size);
> +
> + SVN_ERR (BDB_WRAP (fs,
> + "get repository uuid",
> + uuids->put (uuids, trail->db_txn, &key, &value, 0)));
> +
> + return SVN_NO_ERROR;
> +}
Cut-and-pasted error string -- you want "set", not "get".
> +/* 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.
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.
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.
> if (svn_err) goto error;
> + svn_err = BDB_WRAP (fs, "creating `uuids' table",
> + svn_fs__bdb_open_uuids_table (&fs->uuids,
> + fs->env, 1));
> + if (svn_err) goto error;
No comments, just pointing out that this is the call in
svn_fs_create_berkeley(), because...
> /* Initialize the DAG subsystem. */
> svn_err = svn_fs__dag_init_fs (fs);
> @@ -576,6 +582,10 @@
> if (svn_err) goto error;
> svn_err = BDB_WRAP (fs, "creating `strings' table",
> svn_fs__bdb_open_strings_table (&fs->strings,
> + fs->env, 0));
> + 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".
I'll refrain from checking in a fix for the existing typo, since it's
in your context and would cause a conflict. If you just fix it on the
branch, it'll get merged in with the rest of the change.
> +
> + /* 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)
style, so the sense of the operator matches the intuitive sense of the
operation. This isn't like, official, or anything (it's not in
HACKING), just pointing it out in case you decide you agree and want
to change it :-). (I won't mention it for the rest of the patch.)
Also, note that this file's style is to have a space between the
function name and the paren; not a big deal, just pointing it out, as
you have obviously been careful to preserve the style elsewhere.
> 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'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.)
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.
+1 on merging this into trunk, once the minor stuff above is taken
care of (well, my proposed conditional 'svnadmin load' behavior isn't
such a minor change, we should get that settled soon).
Anyway, I think the development of the new RA interface for fetching
uuid can just happen on trunk after this is merged.
-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 18:56:21 2003