Daniel Rall wrote:
> 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.
>
Fine.
> ...
>
>>>>> 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".
>
> ...
>
Fine.
>> 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.
>
>
Fine.
>> 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.
This 'get_merge_info' is for retrieving the mergeinfo for old revisions
which might probably used for future WC->WC copy and such. Currently it
is not used anywhere.
As I could see the data is purely derived out of backend mergeinfo
store, I decided not to expose through libsvn_fs_*.
I made 'libsvn_fs' to consume this 'svn_fs_util_get_merge_info' (though
failed to fix build.conf in this regard).
> While you and I and DannyB should drop in sqlite while
> prototyping with the BDB backend, this should not be reflected by the
> API.
>
Do you mean we might have a different storage for 'mergeinfo' other than
'sqlite' for bdb backed repos?
> Is this simply replaced by #2 below, and still backend-specific?
>
>
#2 is different from #1.
#1 'get_merge_info' is about mergeinfo for already committed revs.
#2 'txn_mergeinfo' is for the current commit.
>> 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).
>
>
Accidentally it is. I have a patch against current HEAD of
merge-tracking which I will post based on your answers for the above.
>> @@ -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?
>
Yes.
>
>> 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.
>
Will take care.
> ...
>
>> --- 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.
>
Do you mean "Copyright (c) 2006 CollabNet. All rights reserved.?"
> ...
>
>> +#include "../include/private/merge_info_indexer.h"
>>
>
> You avoid the "../" prefix here (see subversion/libsvn_subr/atomic.c
> for an example).
>
> ...
>
Taken care.
>> +static svn_error_t *
>> +util_sqlite_exec (sqlite3 *db, const char *sql,
>> + sqlite3_callback callback,
>> + void *callbackdata)
>> +{
>> + char *errmsg;
>>
>
> err_msg
>
Taken care.
>
>> + 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.
>
Copy paste from earlier code, anyways corrected.
> ...
>
>> + // 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.
>
>
Do we have any plans to have different 'mergeinfo' stores for each repo
backend type? Then I agree this need to be part of individual backend.
If we have different type of backend 'mergeinfo' store for each repo
type then most of this exercise is not needed and sqlite related stuff
could be left in libsvn_fs_fs itself.
>> 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.
>
Taken care.
> ...
>
>> +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
>
Named it as svn_fs_mergeinfo_db_create.
> svn_fs_merge_info_update_index
>
done.
>
>> +/* 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)?
>
Not sure what you mean by this?
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Wed Sep 13 12:03:53 2006