On Mon, Sep 18, 2006 at 08:15:38PM +0530, Kamesh Jayachandran wrote:
> [[[
> Patch by: Kamesh Jayachandran <kamesh@collab.net>
> Reviewed by: Daniel L Rall <dlr@collab.net>
> Greg Hudson <ghudson@MIT.EDU>
> Malcolm Rowe <malcolm-svn-dev@farside.org.uk>
I wouldn't say that I've reviewed this patch. To me, reviewed means
something akin to a +1 vote in STATUS, and from what I remember, most
of my 'review' involved questions, not approval.
> Index: build.conf
> ===================================================================
> --- build.conf (revision 21525)
> +++ build.conf (working copy)
> @@ -235,14 +235,14 @@
> path = subversion/libsvn_fs_base
> sources = *.c bdb/*.c util/*.c
> install = bdb-lib
> -libs = libsvn_delta libsvn_subr aprutil apriconv apr bdb sqlite
> +libs = libsvn_delta libsvn_subr aprutil apriconv apr bdb libsvn_fs_util
> msvc-static = yes
>
Did you confirm that removing the direct sqlite dependency doesn't
cause the problem I mentioned previously? (the 'neon' problem mentioned
in build.conf).
> Index: subversion/libsvn_fs_fs/fs.h
> ===================================================================
> --- subversion/libsvn_fs_fs/fs.h (revision 21525)
> +++ subversion/libsvn_fs_fs/fs.h (working copy)
> @@ -71,14 +67,6 @@
> #endif
> } fs_fs_data_t;
>
> -/* Transactions need their own private opened copy of the mergeinfo
> - database so that their sql commands are not shared between each other. */
> -typedef struct
> -{
> - /* Merge tracking database. */
> - sqlite3 *mtd;
> -} fs_txn_data_t;
> -
I'm just curious - I'm not really reviewing the implementation - but
where did this logic or concept move to?
> Index: subversion/include/private/svn_fs_merge_info.h
> ===================================================================
> --- subversion/include/private/svn_fs_merge_info.h (revision 0)
> +++ subversion/include/private/svn_fs_merge_info.h (revision 0)
> @@ -0,0 +1,42 @@
> +/*
> + * svn_fs_merge_info.h: Declarations for the
> + * public functions of libsvn_fs_util to be consumed by only fs_* libs.
> + *
> + * ====================================================================
> + * Copyright (c) 2006 CollabNet. All rights reserved.
> + *
> + * This software is licensed as described in the file COPYING, which
> + * you should have received as part of this distribution. The terms
> + * are also available at http://subversion.tigris.org/license-1.html.
> + * If newer versions of this license are posted there, you may use a
> + * newer version instead, at your option.
> + *
> + * This software consists of voluntary contributions made by many
> + * individuals. For exact contribution history, see the revision
> + * history and logs, available at http://subversion.tigris.org/.
> + * ====================================================================
> + */
> +
If these following functions are internal, shouldn't we name
them according to the double-underscore convention we use to mark
non-public-but-exported functions?
More on function naming: what's the library prefix? Is it
svn_fs_mergeinfo_, svn_fs_merge_, or svn_fs_util_ ? Why do you have
'mergeinfo' and 'merge_info'?
> +/* Create the sqlite mergeinfo.db with default schema
> + * under 'db' directory of the repository identified by @a PATH
> + * use @a POOL for any temporary allocations. */
> +svn_error_t *
> +svn_fs_mergeinfo_db_create(const char *path, apr_pool_t *pool);
The caller doesn't need to know what the filename or location of the
mergeinfo database is. I'm curious as to why you're passing a repository
path rather than a filesystem path, though - the repository shouldn't
need to be referenced by the FS implementation.
Do you really need a separate call for creating the database? Why can't
you just create it on the first call to update_index?
> +
> +/* Cleanup the earlier left over sqlite records.
That sounds like an implementation detail that a caller shouldn't need
to know about.
> + * Get the mergeinfo for the current transaction identified by @a NEW_REV
> + * and @a TXN if any and record the same in sqlite.
Ditto the fact that it's a sqlite database. If this is a utility
function, it should be documented something along the lines of "Update
the mergeinfo index according to the changes made in transaction TXN or
revision NEW_REV".
Should we be using Doxygen-style or INTERNAL-style markup here? Pick one,
not both.
> + */
> +svn_error_t *
> +svn_fs_merge_info_update_index(svn_fs_txn_t *txn, svn_revnum_t new_rev,
It sounds like from the description about that only one of these is
required - could you clarify the doc-comments?
> + svn_boolean_t txn_contains_merge_info,
This isn't documented - what does it do?
> + apr_pool_t *pool);
> +
> +/* Get the merge info for the set of @a PATHS and have it as @a *MERGEINFO.
> + */
> +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);
Random thoughts: 'include_parents' isn't documented. Why are you
passing a transaction/revision root here, but a transaction object or
revision number in the previous function? What is in PATHS? (an array
of absolute-in-the-fs paths?) What output goes in *mergeinfo? (a hash
of paths? to something?).
Does this function work if the index isn't up to date? (that is, if
there isn't an index, or isn't an index entry for the root in question,
does it derive the answer from the base data) If not, please make that
clear by describing it so ("Return the merge info from the index..." or
similar) and saying roughly what happens in the two cases I mentioned
(I assume it returns an error, so we should say so, and preferably say
which one it returns).
Regards,
Malcolm
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Mon Sep 18 18:57:48 2006