Re: [PATCH][merge-tracking] libsvn_fs_util

(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?