Malcolm Rowe wrote:
> Did you confirm that removing the direct sqlite dependency doesn't
> cause the problem I mentioned previously? (the 'neon' problem mentioned
> in build.conf).
>
>
I will take this to other thread.
>> 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?
>
>
The 'mergeinfo backend' could be different for different for 'fs'
backend in the near future.
This exercise just makes both 'fs_fs' and 'fs_base' to consume 'sqlite'
for a while till 'fs_base' has its own 'mergeinfo store in bdb itself'.
This sqlite connections are opened once, All associated function calls
responsible to do one logical 'sql' operation are passed the same
'sqlite connection pointer'.
>> 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'?
>
Ok. Library prefix is 'svn_fs_util'.
Will use 'mergeinfo'.
>
>> +/* 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.
>
>
Sorry I meant the 'file system path' like
/absolute/path/to/repo/root/db, the same value received by filesystem
vtable_t's create hook.
> 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?
>
>
Yes it is possible. I see a point in your suggestion.
Creating 'mergeinfo.db' from first call to 'update_index' has the
advantage of seamlessly upgrading the old repos.
Downside I see for every update_index call I need to do the check.
Not sure what would be the better way to take.
>> +
>> +/* Cleanup the earlier left over sqlite records.
>>
>
> That sounds like an implementation detail that a caller shouldn't need
> to know about.
>
Ok.
>
>> + * 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.
>
>
Ok.
>> + */
>> +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?
>
Ok.
>
>> + svn_boolean_t txn_contains_merge_info,
>>
>
> This isn't documented - what does it do?
>
Will 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).
>
>
It is just part of mass move of existing function to a new file.
Would go through the current implementation and make the above changes
and get it committed and then post this patch.
Thanks for the nice review.
Hope Now I can include your name in 'Reviewed by:' list.
With regards
Kamesh Jayachandran
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Tue Sep 19 08:15:17 2006