You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Kamesh Jayachandran <ka...@collab.net> on 2006/09/07 14:41:33 UTC
[PATCH][merge-tracking] libsvn_fs_util
Hi All,
This is the follow up to my earlier patch updated today's merge-tracking
HEAD.
Please find the attached patch and log.
With regards
Kamesh Jayachandran
Re: [PATCH][merge-tracking] libsvn_fs_util
Posted by Greg Stein <gs...@lyra.org>.
On 9/14/06, Daniel Rall <dl...@collab.net> wrote:
>...
> 1) As we really need to focus on getting an end-to-end Merge Tracking
> prototype completed, the sqlite merge info index you're moving to
> libsvn_fs_util may be used by both the FSFS and BDB backends for a
> first cut. The API, on the other hand, shouldn't be dictating this as
> a requirement (BDB might later want to use its own schema, the Google
> backend might have other ideas, etc.).
Yeah... it can't really use sqllite, fwiw. No files on local disk. We
had to munge the dbm's storing dav activities, too.
Cheers,
-g
--
Greg Stein, http://www.lyra.org/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH][merge-tracking] libsvn_fs_util
Posted by Kamesh Jayachandran <ka...@collab.net>.
Daniel Rall wrote:
> On Thu, 14 Sep 2006, Kamesh Jayachandran wrote:
> ...
>
>>>> 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.
>>>>
>>> I see no reason to unnecessarily dictate the type of merge info index
>>> used by a backend. Encapsulating the storage type inside the FS
>>> backend module adds very little overhead.
>>>
>> Then I see no meaning in the existence of libsvn_fs_util(Which I believe
>> is meant to hold the reusable sqlite code.)
>> Correct me if I am missing something.
>>
>
> libsvn_fs_util is important:
>
> 1) As we really need to focus on getting an end-to-end Merge Tracking
> prototype completed, the sqlite merge info index you're moving to
> libsvn_fs_util may be used by both the FSFS and BDB backends for a
> first cut. The API, on the other hand, shouldn't be dictating this as
> a requirement (BDB might later want to use its own schema, the Google
> backend might have other ideas, etc.).
>
> 2) C-Mike has mentioned that there's non-Merge Tracking related FS
> code which he'd like to put into libsvn_fs_util module so that it can
> be shared between the FSFS and BDB backends.
>
Fine will post a patch adhering to your comments soon.
With regards
Kamesh Jayachandran
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH][merge-tracking] libsvn_fs_util
Posted by Daniel Rall <dl...@collab.net>.
On Thu, 14 Sep 2006, Kamesh Jayachandran wrote:
...
> >>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.
> >
> >I see no reason to unnecessarily dictate the type of merge info index
> >used by a backend. Encapsulating the storage type inside the FS
> >backend module adds very little overhead.
>
> Then I see no meaning in the existence of libsvn_fs_util(Which I believe
> is meant to hold the reusable sqlite code.)
> Correct me if I am missing something.
libsvn_fs_util is important:
1) As we really need to focus on getting an end-to-end Merge Tracking
prototype completed, the sqlite merge info index you're moving to
libsvn_fs_util may be used by both the FSFS and BDB backends for a
first cut. The API, on the other hand, shouldn't be dictating this as
a requirement (BDB might later want to use its own schema, the Google
backend might have other ideas, etc.).
2) C-Mike has mentioned that there's non-Merge Tracking related FS
code which he'd like to put into libsvn_fs_util module so that it can
be shared between the FSFS and BDB backends.
Re: [PATCH][merge-tracking] libsvn_fs_util
Posted by Kamesh Jayachandran <ka...@collab.net>.
> The get_merge_info() interface is used by svn_fs_get_merge_info(),
> which is in turn used by svn_repos_fs_get_merge_info(), which is the
> API by which we retrieve merge info for use on the client-side. It or
> an equivalent API is absolutely necessary.
>
>
Now svn_fs_get_merge_info() calls 'svn_fs_util_get_merge_info' directly
instead of vtable hooks as I felt mergeinfo indexing is independent of
'fs backend' and only sqlite based.
Now I found to be wrong in this. I would make vtable based.
>> 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.
>>
>
> I see no reason to unnecessarily dictate the type of merge info index
> used by a backend. Encapsulating the storage type inside the FS
> backend module adds very little overhead.
>
>
Then I see no meaning in the existence of libsvn_fs_util(Which I believe
is meant to hold the reusable sqlite code.)
Correct me if I am missing something.
With regards
Kamesh Jayachandran
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH][merge-tracking] libsvn_fs_util
Posted by Daniel Rall <dl...@collab.net>.
(Kamesh, would you mind including some newlines around your response
when you inline them into my text? They're otherwise unnecessarily
hard to find. Thanks! :)
On Wed, 13 Sep 2006, Kamesh Jayachandran wrote:
> Daniel Rall wrote:
> >On Tue, 12 Sep 2006, Kamesh Jayachandran wrote:
...
> >>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_*.
The get_merge_info() interface is used by svn_fs_get_merge_info(),
which is in turn used by svn_repos_fs_get_merge_info(), which is the
API by which we retrieve merge info for use on the client-side. It or
an equivalent API is absolutely necessary.
> 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?
It should be possible, yes. Merge info storage and indexing is a
function of the FS backend. This is why the merge info retrieval API
needs to be part of the FS vtable.
> >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.
...
> >>--- 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.
I merged in all revisions from trunk yesterday, so this change will
disappear from the patch.
...
> >>--- 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.?"
Yup.
...
> >>--- 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.
I see no reason to unnecessarily dictate the type of merge info index
used by a backend. Encapsulating the storage type inside the FS
backend module adds very little overhead.
...
> >>+/* 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);
Do we need this API with get_merge_info() part of the FS backend's
vtable?
> >>+/* 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?
I don't want this API in this header file. Where can we move it to?
Re: [PATCH][merge-tracking] libsvn_fs_util
Posted by Kamesh Jayachandran <ka...@collab.net>.
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
Re: [PATCH][merge-tracking] libsvn_fs_util
Posted by Daniel Rall <dl...@collab.net>.
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)?
Re: [PATCH][merge-tracking] libsvn_fs_util
Posted by Kamesh Jayachandran <ka...@collab.net>.
> It doesn't look like anyone's come up with a portable way to do that
> yet (at least, not using static linkage, or build magic to dump the
> code directly into _fs and _base). What we discussed while Kamesh was
> visiting in Brisbane is to use the new subversion/include/private/
> area for the Subversion-private header files.
>
>
Sorry I failed to understand that now understood it :), now the attached
patch has that implemented.
>>> 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.
>>>
>>> It looks like you're planning to make it private, since you aren't
>>> installing a public include file. I'd just like to make sure that that's
>>> a deliberate choice rather than an accident of the current design.
>>>
>> It is an accident, I did not think about the BC issues and visiblity of
>> this libs to other modules other than libsvn_fs_fs and libsvn_fs_base.
>>
>
> 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?
> ...
>
>>> If we do keep it as a shared library, you'll need to add version checks
>>> (i.e. svn_ver_check_list()) to it, and also add it to the existing checks
>>> in libsvn_fs_{base,fs}. If we make it static, that won't be necessary.
>>>
>>> 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.
>>> [The really important question - whether this change actually makes sense
>>> in the context of the merge-tracking work - I'll leave to someone with
>>> more experience of that code.]
>>>
>
> It doesn't matter one way or the other. Mike mentioned that there is
> other FS code -- unrelated to merge tracking -- which should really
> live in a utility library, too.
>
Re: [PATCH][merge-tracking] libsvn_fs_util
Posted by Daniel Rall <dl...@collab.net>.
On Fri, 08 Sep 2006, Kamesh Jayachandran wrote:
> Malcolm Rowe wrote:
> >On Thu, Sep 07, 2006 at 08:11:33PM +0530, Kamesh Jayachandran wrote:
> >
> >>Move mergeinfo indexing code out of libsvn_fs_fs to libsvn_fs_util(new
> >>module)
> >>so that it can be reused by bdb repos.
> >
> >It occurs to me that if you're creating a new utility library for
> >the filesystem implementations to use then we would want to consider
> >whether it's intended to be private (solely used by the filesystem
> >implementations) or public (usable by anyone). For example, should
> >users be able to call functions in it directly?
>
> Currently it is meant to be consumed by libsvn_fs_fs and libsvn_fs_base.
> How can I make it private only to 'libsvn_fs_fs' and 'libsvn_fs_base'?
It doesn't look like anyone's come up with a portable way to do that
yet (at least, not using static linkage, or build magic to dump the
code directly into _fs and _base). What we discussed while Kamesh was
visiting in Brisbane is to use the new subversion/include/private/
area for the Subversion-private header files.
> >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.
> >
> >It looks like you're planning to make it private, since you aren't
> >installing a public include file. I'd just like to make sure that that's
> >a deliberate choice rather than an accident of the current design.
>
> It is an accident, I did not think about the BC issues and visiblity of
> this libs to other modules other than libsvn_fs_fs and libsvn_fs_base.
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.
...
> >If we do keep it as a shared library, you'll need to add version checks
> >(i.e. svn_ver_check_list()) to it, and also add it to the existing checks
> >in libsvn_fs_{base,fs}. If we make it static, that won't be necessary.
> >
> >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.
> >[The really important question - whether this change actually makes sense
> >in the context of the merge-tracking work - I'll leave to someone with
> >more experience of that code.]
It doesn't matter one way or the other. Mike mentioned that there is
other FS code -- unrelated to merge tracking -- which should really
live in a utility library, too.
Re: [PATCH][merge-tracking] libsvn_fs_util
Posted by Kamesh Jayachandran <ka...@collab.net>.
Malcolm Rowe wrote:
> On Thu, Sep 07, 2006 at 08:11:33PM +0530, Kamesh Jayachandran wrote:
>
>> Move mergeinfo indexing code out of libsvn_fs_fs to libsvn_fs_util(new module)
>> so that it can be reused by bdb repos.
>>
>>
>
> It occurs to me that if you're creating a new utility library for
> the filesystem implementations to use then we would want to consider
> whether it's intended to be private (solely used by the filesystem
> implementations) or public (usable by anyone). For example, should
> users be able to call functions in it directly?
>
Currently it is meant to be consumed by libsvn_fs_fs and libsvn_fs_base.
How can I make it private only to 'libsvn_fs_fs' and 'libsvn_fs_base'?
> 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.
>
> It looks like you're planning to make it private, since you aren't
> installing a public include file. I'd just like to make sure that that's
> a deliberate choice rather than an accident of the current design.
>
It is an accident, I did not think about the BC issues and visiblity of
this libs to other modules other than libsvn_fs_fs and libsvn_fs_base.
> I also wonder whether we really want to install a new shared library -
> it might be better to compile libsvn_fs_util as a static library and
> link it into both libsvn_fs implementations. That might require some
> changes to the build generator, though.
>
> If we do keep it as a shared library, you'll need to add version checks
> (i.e. svn_ver_check_list()) to it, and also add it to the existing checks
> in libsvn_fs_{base,fs}. If we make it static, that won't be necessary.
>
> 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.
> [The really important question - whether this change actually makes sense
> in the context of the merge-tracking work - I'll leave to someone with
> more experience of that code.]
>
> Regards,
> Malcolm
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH][merge-tracking] libsvn_fs_util
Posted by Greg Hudson <gh...@MIT.EDU>.
On Sep 8, 2006, at 5:17 PM, Malcolm Rowe wrote:
> In any case, I think there may be an easier way to achieve the same
> thing: instead of adding a new library target to build.conf, just add
> the libsvn_fs_util sources to both the libsvn_fs_{base,fs} targets;
> that way they should just pull in the code directly. (Assuming that
> doing so doesn't break a static build, of course).
I imagine this would produce symbol conflicts if libsvn_fs_base and
libsvn_fs_fs were both linked in.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH][merge-tracking] libsvn_fs_util
Posted by Malcolm Rowe <ma...@farside.org.uk>.
On Fri, Sep 08, 2006 at 12:56:47PM -0400, Greg Hudson wrote:
>
> On Sep 7, 2006, at 11:48 AM, Malcolm Rowe wrote:
> >
> >I also wonder whether we really want to install a new shared library -
> >it might be better to compile libsvn_fs_util as a static library and
> >link it into both libsvn_fs implementations. That might require some
> >changes to the build generator, though.
>
> I believe linking shared libraries against static libraries is not
> particularly portable. What would be the advantage here?
>
Good point, I'd forgotten about that. The advantage would be that the
libsvn_fs_util 'library' would become just an implementation detail,
which is what it is, rather than an externally-usable library for which
we were (even if only technically) providing a user-visible interface,
with potential for version mismatches, ABI compatibility considerations,
and so on.
In any case, I think there may be an easier way to achieve the same
thing: instead of adding a new library target to build.conf, just add
the libsvn_fs_util sources to both the libsvn_fs_{base,fs} targets;
that way they should just pull in the code directly. (Assuming that
doing so doesn't break a static build, of course).
Regards,
Malcolm
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH][merge-tracking] libsvn_fs_util
Posted by Peter Samuelson <pe...@p12n.org>.
> On 2006-09-08 12:56:47 -0400, Greg Hudson wrote:
> > I believe linking shared libraries against static libraries is not
> > particularly portable. What would be the advantage here?
[Marcus Rueckert]
> it doesnt work on anything else than ix86.
You mean libtool doesn't support it for anything else than ix86? That
would surprise me, as libtool aims to support all of its operations
everywhere.
Certainly it's possible to link a static library into a shared library
on a lot more platforms than that, if not _all_ platforms. The thing
to watch out for is that the static library must be compiled with PIC
(position-independent code, "-fPIC" in gcc) just like the shared
library.
Re: [PATCH][merge-tracking] libsvn_fs_util
Posted by Marcus Rueckert <da...@web.de>.
On 2006-09-08 12:56:47 -0400, Greg Hudson wrote:
> On Sep 7, 2006, at 11:48 AM, Malcolm Rowe wrote:
> >I also wonder whether we really want to install a new shared library -
> >it might be better to compile libsvn_fs_util as a static library and
> >link it into both libsvn_fs implementations. That might require some
> >changes to the build generator, though.
>
> I believe linking shared libraries against static libraries is not
> particularly portable. What would be the advantage here?
it doesnt work on anything else than ix86.
darix
--
openSUSE - SUSE Linux is my linux
openSUSE is good for you
www.opensuse.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH][merge-tracking] libsvn_fs_util
Posted by Greg Hudson <gh...@MIT.EDU>.
On Sep 7, 2006, at 11:48 AM, Malcolm Rowe wrote:
>
> I also wonder whether we really want to install a new shared library -
> it might be better to compile libsvn_fs_util as a static library and
> link it into both libsvn_fs implementations. That might require some
> changes to the build generator, though.
I believe linking shared libraries against static libraries is not
particularly portable. What would be the advantage here?
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH][merge-tracking] libsvn_fs_util
Posted by Malcolm Rowe <ma...@farside.org.uk>.
On Thu, Sep 07, 2006 at 08:11:33PM +0530, Kamesh Jayachandran wrote:
> Move mergeinfo indexing code out of libsvn_fs_fs to libsvn_fs_util(new module)
> so that it can be reused by bdb repos.
>
It occurs to me that if you're creating a new utility library for
the filesystem implementations to use then we would want to consider
whether it's intended to be private (solely used by the filesystem
implementations) or public (usable by anyone). For example, should
users be able to call functions in it directly?
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.
It looks like you're planning to make it private, since you aren't
installing a public include file. I'd just like to make sure that that's
a deliberate choice rather than an accident of the current design.
I also wonder whether we really want to install a new shared library -
it might be better to compile libsvn_fs_util as a static library and
link it into both libsvn_fs implementations. That might require some
changes to the build generator, though.
If we do keep it as a shared library, you'll need to add version checks
(i.e. svn_ver_check_list()) to it, and also add it to the existing checks
in libsvn_fs_{base,fs}. If we make it static, that won't be necessary.
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'.
[The really important question - whether this change actually makes sense
in the context of the merge-tracking work - I'll leave to someone with
more experience of that code.]
Regards,
Malcolm
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org