On Tue, 12 Sep 2006, Kamesh Jayachandran wrote:
...
> >>>I'd suggest that we'd just be adding complexity if we made this a
> >>>publicly-accessible library: we'd have to support a whole new set of
> >>>interfaces in a binary-compatible fashion. On the other hand, if we
> >>>make it private, we can change the implementation at will.
...
> >If the include/private/ area is used for the svn_fs_util.h, should we
> >bother worrying about BC issues? I tend to think no -- anyone
> >violating our encapsulation strategy to that extent rather deserves
> >any compat headaches that they receive as a result.
>
> Do you want merge_info_indexer.h to be renamed to 'svn_fs_util.h' to
> be renamed?
Assuming it currently contains only Merge Tracking-related APIs, it
would be consistent to name it svn_fs_merge_info.h.
Naming it svn_merge_info_indexer.h doesn't seem exactly right, as it
isn't completely specific to the indexing, and may increase in
interaction with the other FS data store.
...
> >>>Finally, while I prefer the 'util' name, I wonder whether it'd be better
> >>>to call it something like 'libsvn_fs_subr' for parity with 'libsvn_subr'.
> >>>
> >>Sounds good.
> >
> >This exact discussion came up one morning between Kamesh and Mike and
> >myself. IIRC, while we were aware of the minor inconsistency it would
> >cause, Mike and I still preferred "util", on the grounds that it's
> >more obivous (no more "What's 'subr' stand for?" questions). I
> >maintain the same preference today.
>
> I remember this, I don't have any preference on the names, Ok with
> either libsvn_util or libsvn_subr.
Let's go with "libsvn_fs_util".
...
> Move mergeinfo indexing code out of libsvn_fs_fs to libsvn_fs_util(new module)
> so that it can be reused by bdb repos.
This change would've been more digestible if broken into two pieces,
the first of which made the API changes necessary for manipulating
merge tracking info, and the second performed the actual mass code
move.
> The changes can be summarised as follows,
> 1. root_vtable_t's get_merge_info hook has been removed as implementation is
> 'fs' independent and can be availed out of new module libsvn_fs_util's
> svn_fs_util_get_merge_info.
I don't think the merge info retrieval API should be removed from
libsvn_fs's interface. Merge info should be allowed to be
FS-dependent. While you and I and DannyB should drop in sqlite while
prototyping with the BDB backend, this should not be reflected by the
API.
Is this simply replaced by #2 below, and still backend-specific?
> 2. Exposing 'txn_mergeinfo' hook out of txn_vtable_t so that new merge_info
> indexer can make use of it to weave the information from the transaction.
>
> 3. libsvn_fs_util exposes the following public functions for libsvn_fs_fs,
> libsvn_fs_base and libsvn_fs modules to consume.
> - svn_fs_util_mtd_create - creates the fresh mergeinfo db with the schema
> consumed by libsvn_fs repo create hook.
> - svn_fs_util_mtd_update_merge_info_index - updates mergeinfo sqlite db
> consumed by the commit_body of libsvn_fs_fs currently later would be used
> by libsvn_fs_base.
> - svn_fs_util_get_merge_info - gets a mergeinfo for a committed revision
> for the set of paths, consumed by libsvn_fs.
...
> --- build.conf (revision 21450)
> +++ build.conf (working copy)
> @@ -28,6 +28,7 @@
> includes = subversion/include/*.h
> include-wildcards = *.h *.i *.swg
> private-includes =
> + subversion/include/private/*.h
> subversion/bindings/swig/include/*.swg
> subversion/libsvn_delta/compose_delta.c
> private-built-includes =
Is this part of r21016? It looks like we should merge that and
dependent revisions into the merge-tracking branch. For subsequent
revisions of this patch, assume we've merged that (even if I haven't
gotten around to it yet).
> @@ -239,7 +240,7 @@
> type = fs-module
> path = subversion/libsvn_fs_fs
> install = fsmod-lib
> -libs = libsvn_delta libsvn_subr aprutil apriconv apr sqlite
> +libs = libsvn_delta libsvn_subr aprutil apriconv apr libsvn_fs_util
Doesn't this completely removes sqlite as a dependency?
> msvc-static = yes
>
> # General API for accessing repositories
> @@ -302,6 +303,15 @@
> msvc-libs = advapi32.lib shfolder.lib
> msvc-static = yes
>
> +# Low-level grab bag of utilities
> +[libsvn_fs_util]
> +type = lib
> +install = fsmod-lib
> +path = subversion/libsvn_fs_util
> +libs = aprutil apriconv apr
I believe that the dependency on sqlite should be here now.
...
> --- subversion/libsvn_fs_util/merge_info_indexer.c (revision 0)
> +++ subversion/libsvn_fs_util/merge_info_indexer.c (revision 0)
> @@ -0,0 +1,549 @@
> +/* merge_info_indexer.c
> + *
> + * ====================================================================
> + * Copyright (c) 2000-2006 CollabNet. All rights reserved.
This date should be only 2006.
...
> +#include "../include/private/merge_info_indexer.h"
You avoid the "../" prefix here (see subversion/libsvn_subr/atomic.c
for an example).
...
> +static svn_error_t *
> +util_sqlite_exec (sqlite3 *db, const char *sql,
> + sqlite3_callback callback,
> + void *callbackdata)
> +{
> + char *errmsg;
err_msg
> + if (sqlite3_exec(db, sql, NULL, NULL, &errmsg) != SQLITE_OK)
> + return svn_error_create(SVN_ERR_FS_SQLITE_ERROR, NULL,
> + errmsg);
> + return SVN_NO_ERROR;
> +}
...
> + // Cleanup the leftovers of any previous, failed FSFS transactions
> + // involving NEW_REV.
Inappropriate comment style.
...
> + // Record any merge info from the current transaction.
> + if (txn_contains_merge_info)
> + SVN_ERR(index_txn_merge_info(txn, new_rev, db, pool));
> +
> + // This is moved here from commit_txn, because we don't want to
> + // write the final current file if the sqlite commit fails.
> + // On the other hand, if we commit the transaction and end up failing
> + // the current file, we just end up with inaccessible data in the
> + // database, not a real problem.
More.
...
> --- subversion/libsvn_fs/fs-loader.h (revision 21450)
> +++ subversion/libsvn_fs/fs-loader.h (working copy)
> @@ -184,6 +184,8 @@
> const svn_string_t *value, apr_pool_t *pool);
> svn_error_t *(*root)(svn_fs_root_t **root_p, svn_fs_txn_t *txn,
> apr_pool_t *pool);
> + svn_error_t *(*txn_mergeinfo)(apr_hash_t **minfoprops, svn_fs_txn_t *txn,
> + apr_pool_t *pool);
> } txn_vtable_t;
>
>
> @@ -291,11 +293,6 @@
> svn_error_t *(*change_merge_info)(svn_fs_root_t *root, const char *path,
> apr_hash_t *info,
> apr_pool_t *pool);
> - svn_error_t *(*get_merge_info)(apr_hash_t **minfohash,
> - svn_fs_root_t *root,
> - const apr_array_header_t *paths,
> - svn_boolean_t include_parents,
> - apr_pool_t *pool);
> } root_vtable_t;
I'm unsure about this API change. Please see my comments in the
change log message.
> --- subversion/libsvn_fs/fs-loader.c (revision 21450)
> +++ subversion/libsvn_fs/fs-loader.c (working copy)
> @@ -353,6 +353,7 @@
> /* Perform the actual creation. */
> *fs_p = svn_fs_new(fs_config, pool);
> SVN_ERR(vtable->create(*fs_p, path, pool));
> + SVN_ERR(svn_fs_util_mtd_create(path, pool));
I wonder if this wouldn't be better inside each backend's
implementation of vtable->create()? The BDB backend doesn't
necessarily even need sqlite to easily keep an index of merge info.
> return serialized_init(*fs_p, pool);
> }
...
> --- subversion/libsvn_fs_fs/fs_fs.c (revision 21450)
> +++ subversion/libsvn_fs_fs/fs_fs.c (working copy)
...
> +#include "../include/private/merge_info_indexer.h"
See comment near beginning of review.
...
> --- subversion/include/private/merge_info_indexer.h (revision 0)
> +++ subversion/include/private/merge_info_indexer.h (revision 0)
> @@ -0,0 +1,45 @@
> +/*
> + * merge_info_indexer.h: Declarations for the
> + * public functions of libsvn_fs_util to be consumed by only fs_* libs.
> + *
> + * ====================================================================
> + * Copyright (c) 2000-2006 CollabNet. All rights reserved.
This date should be only 2006.
...
> +svn_error_t *
> +svn_fs_util_mtd_create(const char *path, apr_pool_t *pool);
> +
> +svn_error_t *
> +svn_fs_util_mtd_update_merge_info_index(svn_fs_txn_t *txn, svn_revnum_t new_rev,
> + svn_boolean_t txn_contains_merge_info,
> + apr_pool_t *pool);
These APIs are missing doc strings. What's the difference between
"mtd" (the non-obvious abbreviate for "merge tracking data(base?)")
and "merge info"? I'd like more obvious names for these APIs. How
about something like:
svn_fs_merge_info_create_index
svn_fs_merge_info_update_index
> +/* Get the merge info for a set of paths. */
> +svn_error_t *
> +svn_fs_util_get_merge_info(apr_hash_t **mergeinfo,
> + svn_fs_root_t *root,
> + const apr_array_header_t *paths,
> + svn_boolean_t include_parents,
> + apr_pool_t *pool);
> +
> +/* SQLITE->SVN quick error wrap, much like SVN_ERR.
> + XXX: This macro probably belongs elsewhere, like svn_sqlite.h or
> + something. Later. */
> +#define SQLITE_ERR(x, db) do \
> +{ \
> + if ((x) != SQLITE_OK) \
> + return svn_error_create(SVN_ERR_FS_SQLITE_ERROR, NULL, \
> + sqlite3_errmsg((db))); \
> +} while (0)
This is the only sqlite-specifc code in svn_fs_merge_info.h. It would
be nice if it wasn't in here. Could it live in a header file specific
to the libsvn_fs_util module (e.g. fs_util.h)?
- application/pgp-signature attachment: stored
Received on Wed Sep 13 01:22:05 2006