On Mon, 18 Sep 2006, Malcolm Rowe wrote:
> On Mon, Sep 18, 2006 at 08:15:38PM +0530, Kamesh Jayachandran wrote:
...
> > 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.
...
> 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?
Good question. We did not do so in include/private/svn_atomic.h. I
don't have a strong preference here, but whichever we choose, it
should be consistent. Here's the relevant section of HACKING:
"All library-internal declarations made in a library-private header
file (such as libsvn_wc/wc.h) must be signified by two underscores
after the library prefix (such as svn_wc__ensure_directory). All
declarations private to a single file (such as the static function
get_entry_url inside libsvn_wc/update_editor.c) do not require any
additional namespace decorations. Symbols that need to be used
outside a library, but still are not public are put in a public
header, but use the double underscore notation. Such symbols may be
used by Subversion core code only, and we try to be restrictive
regarding the introduction of such entities."
- http://subversion.tigris.org/hacking.html#other-conventions
> 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'?
The library name is "libsvn_fs_util", and the prefix for this header
file is "svn_fs_merge_info_". The prefix usage should be consistent.
> > +/* 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?
This one might've come up before?
Since the index creation API can be called from 'svnadmin create', is
simplifying this private API a good enough trade-off to incur the
(minor) runtime overhead?
...
> > + * 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.
+1
...
> > +/* 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.
It's a flag to the FS to indicate whether or not to pull back merge
info from all parent paths.
...
> 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 should start returning an error, yeah.
- application/pgp-signature attachment: stored
Received on Tue Sep 19 02:42:58 2006