On Thu, 24 May 2007, hwright@tigris.org wrote:
...
> Add a new API used to determine all of the mergeinfo for each path, and its
> children, in a set of paths.
>
> * subversion/libsvn_fs/fs-loader.h
> (root_vtable_t): Add get_children_mergeinfo().
+1 on this API being part of the vtable. It keeps merge info
backend-independent.
This name is awfully close to get_mergeinfo_for_children()...I'd much
prefer this (and its wrapper APIs) be called get_mergeinfo_for_tree().
> * subversion/libsvn_fs/fs-loader.c
> (svn_fs_get_children_mergeinfo): New function.
>
> * subversion/libsvn_fs_base/tree.c,
> subversion/libsvn_fs_fs/tree.c
> (root_vtable): Add svn_fs_mergeinfo__get_children_mergeinfo().
>
> * subversion/libsvn_fs_util/mergeinfo-sqlite-index.c
> (get_mergeinfo_for_children): New function.
> (svn_fs_mergeinfo__get_children_mergeinfo): New function.
>
> * subversion/include/svn_fs.h
> (svn_fs_get_children_mergeinfo): New prototype.
>
> * subversion/include/private/svn_fs_mergeinfo.h
> (svn_fs_mergeinfo__get_children_mergeinfo): New prototype.
...
> --- branches/merge-sensitive-log/subversion/include/private/svn_fs_mergeinfo.h (original)
> +++ branches/merge-sensitive-log/subversion/include/private/svn_fs_mergeinfo.h Thu May 24 21:27:21 2007
...
> +/* Get the combined mergeinfo for each one of PATHS (an array of
> + absolute-in-the-fs paths) under ROOT and return it in *MERGEINFO,
> + mapping char * paths to mergeinfo hashs. */
Aren't we supposed to be getting the merge info for children of PATHS
too?
This doc string should indicate that the API conforms to the
get_children_mergeinfo() interface.
> +svn_error_t *
> +svn_fs_mergeinfo__get_children_mergeinfo(apr_hash_t **mergeinfo,
> + svn_fs_root_t *root,
> + const apr_array_header_t *paths,
> + apr_pool_t *pool);
> +
...
> --- branches/merge-sensitive-log/subversion/include/svn_fs.h (original)
> +++ branches/merge-sensitive-log/subversion/include/svn_fs.h Thu May 24 21:27:21 2007
> @@ -1214,6 +1214,26 @@
> svn_boolean_t include_parents,
> apr_pool_t *pool);
>
> +/** Retrive combined mergeinfo for multiple nodes, and their children.
^^^^^^^
"Retrieve". The svn_fs_mergeinfo__get_children_mergeinfo()
implementation doesn't appear to actually return merge info for PATHS,
only for their children.
> + *
> + * @a mergeinfo is filled with mergeinfo for each of the @a paths and
> + * their children, stored as a mergeinfo hash. It will never be @c NULL,
> + * but may be empty.
> + *
> + * @a root indicate the revision root to use when looking up paths.
> + *
> + * @a paths indicate the paths you are requesting information for.
> + *
> + * Do any necessary temporary allocation in @a pool.
> + *
> + * @since New in 1.5.
> + */
> +svn_error_t *
> +svn_fs_get_children_mergeinfo(apr_hash_t **mergeinfo,
> + svn_fs_root_t *root,
> + const apr_array_header_t *paths,
> + apr_pool_t *pool);
> +
...
> --- branches/merge-sensitive-log/subversion/libsvn_fs_util/mergeinfo-sqlite-index.c (original)
> +++ branches/merge-sensitive-log/subversion/libsvn_fs_util/mergeinfo-sqlite-index.c Thu May 24 21:27:21 2007
...
> +/* Get the mergeinfo for all of the children of PATH in REV. Return the
> + results in PATH_MERGEINFO. PATH_MERGEINFO should already be created
> + prior to calling this function, but it's value may change as additional
> + mergeinfos are added to it. */
> +static svn_error_t *
> +get_mergeinfo_for_children(sqlite3 *db,
> + const char *path,
> + svn_revnum_t rev,
> + apr_hash_t **path_mergeinfo,
> + apr_pool_t *pool)
> +{
> + sqlite3_stmt *stmt;
> + int sqlite_result;
> + char *like_path;
> +
> + /* Get all paths under us. */
> + SQLITE_ERR(sqlite3_prepare(db,
> + "SELECT MAX(revision), path "
> + "FROM mergeinfo_changed "
> + "WHERE path LIKE ? AND revision <= ?;",
> + -1, &stmt, NULL), db);
Do you need to sort by path (as we do elsewhere)? Perhaps it doesn't
matter, since we're returning the merge info in a hash.
How do we handle the case where PATH is something like
"/branches/1.1", and the DB contains a path like "/branches/1.1.1"
(which we want to filter out, I think)?
> + like_path = apr_psprintf(pool, "%s%%", path);
Alternately, could you inline the % SQL wildcard character into the
SQL statement to avoid the sprintf()?
> + SQLITE_ERR(sqlite3_bind_text(stmt, 1, like_path, -1, SQLITE_TRANSIENT), db);
> + SQLITE_ERR(sqlite3_bind_int64(stmt, 2, rev), db);
> +
> + sqlite_result = sqlite3_step(stmt);
> + while (sqlite_result != SQLITE_DONE)
> + {
> + sqlite_int64 lastmerged_rev;
> + const char *merged_path;
> +
> + if (sqlite_result == SQLITE_ERROR)
> + return svn_error_create(SVN_ERR_FS_SQLITE_ERROR, NULL,
> + sqlite3_errmsg(db));
> +
> + lastmerged_rev = sqlite3_column_int64(stmt, 0);
> + merged_path = sqlite3_column_text(stmt, 1);
> +
> + /* If we've got a merged revision, go get the merge info from the db */
> + if (lastmerged_rev > 0)
> + {
> + apr_hash_t *db_mergeinfo;
> +
> + SVN_ERR(parse_mergeinfo_from_db(db, merged_path, lastmerged_rev,
> + &db_mergeinfo, pool));
> +
> + SVN_ERR(svn_mergeinfo_merge(path_mergeinfo, db_mergeinfo, pool));
> + }
> +
> + sqlite_result = sqlite3_step(stmt);
> + }
> +
> + SQLITE_ERR(sqlite3_finalize(stmt), db);
> +
> + return SVN_NO_ERROR;
> +}
...
> +/* Get all the mergeinfo for all the children for each path. Return a hash of
> + mergeinfo hashes, keyed on the input paths. */
This information should live only in the doc string in
svn_fs_mergeinfo.h (which I think it is redundant with).
> +svn_error_t *
> +svn_fs_mergeinfo__get_children_mergeinfo(apr_hash_t **mergeinfo,
> + svn_fs_root_t *root,
> + const apr_array_header_t *paths,
> + apr_pool_t *pool)
> +{
> + apr_hash_t *mergeinfo_str;
> + svn_revnum_t rev;
> + sqlite3 *db;
> + int i;
> +
> + SVN_ERR(svn_fs_mergeinfo__get_merge_info(&mergeinfo_str, root, paths, TRUE,
> + pool));
We never use "mergeinfo_str" after retrieving it, making this either
a) a bug or b) unnecessary code. I'm guessing (a), and that we
intended to include the direct or inherited merge info for PATHS in
our returned MERGEINFO.
> + /* This is an inlining of svn_fs_revision_root_revision(), which we cannot
> + call from here, because it would be a circular dependency. */
"create a circular dep" (I know, cut and paste :).
As also use this code snippet in svn_fs_mergeinfo__get_mergeinfo(), I
think we should create a macro for it in this .c file.
> + rev = root->is_txn_root ? SVN_INVALID_REVNUM : root->rev;
...
> + for (i = 0; i < paths->nelts; i++)
> + {
> + const char *path = APR_ARRAY_IDX(paths, i, const char *);
> + const char *mergeinfo_path_str = apr_hash_get(*mergeinfo, path,
> + APR_HASH_KEY_STRING);
Level of indentation is off here. I'd name this "path_mergeinfo_str"
for consistency with other variables.
> + apr_hash_t *path_mergeinfo;
> +
> + if (mergeinfo_path_str)
> + SVN_ERR(svn_mergeinfo_parse(&path_mergeinfo, mergeinfo_path_str, pool));
> + else
> + path_mergeinfo = apr_hash_make(pool);
> +
> + SVN_ERR(get_mergeinfo_for_children(db, path, rev, &path_mergeinfo,
> + pool));
> +
> + apr_hash_set(*mergeinfo, path, APR_HASH_KEY_STRING, path_mergeinfo);
> + }
...
> +}
Hmmm. This is leaving dealing with merge info inheritance up to the
callers. While this isn't necessarily inconsistent with existing
merge info API usage, because of the type of data returned by this
API, this needs to be very clearly documented. Also, are we are
including the direct or inherited merge info for PATHS in our returned
MERGEINFO? The doc string makes it look like "no", which is
surprising...
As a novice user, I might have expected this API to list the merge
info for every child path explicitly (which clearly isn't great in
terms of performance and scalability).
- application/pgp-signature attachment: stored
Received on Fri May 25 22:28:38 2007