Kumaran Santhanam <kumaran@tigris.org> writes:
> Attached is the promised patch that attempts to provide the
> separated meta directory functionality. Please be advised that
> this is a first cut, so it should not be used on production data.
As Ben said, a log message would help greatly; see HACKING for more.
Your high-level overview was very helpful, however, thanks.
> - Upon checkout of a separated meta-data working copy, the file
> ~/.subversion/meta/index is updated. It contains a mapping
> between working copy paths and meta data caches. The caches are
> simply a unique number derived from the UTC time on the
> particular workstation. I'm open to better suggestions, but for
> now this works and has the benefit of being lexicographically
> sorted.
What happens when two people share a working copy?
> Before I go much further on this, I would really appreciate it if
> a branch could be created for this feature. It would make things
> much easier going forward. Please let me know if this would be
> possible.
On the one hand, branches are supposed to be "cheap". On the other
hand, there's inevitably some overhead to having one: the commit mails
go out to everyone (or alternatively, we spend valuable time making
them go somewhere else); discussion about branch commits occupies
mental space on the dev list, etc. Therefore, we do exercise some
judgement about making branches. For one thing, it'd be nice to see a
patch that conforms to the HACKING guidelines, and passes at least
some meaningful portion of the regression tests (you don't say whether
this does yet).
Also, we don't have any consensus here that such a patch should/would
be folded into 1.0, even if it were done and passed all tests. I
suspect that getting such a consensus will require first of all a very
clean & comprehensible patch that looks low risk. Yours is pretty
clean, code-wise, and certainly not incomprehensible. However,
without a log message and some documentation tweaks (see below), it's
not there yet. I also have some design concerns, also noted below.
My review is very cosmetic. I didn't want to spend time on a really
in-depth review, because I don't have a great desire to see this
feature in 1.0. We've got enough to do already with the known issues,
without adding new stuff! I wouldn't veto the feature if a large
number of people were emphatic about wanting it, but I'm certainly not
going to pursue consensus on it myself, that's up to you <grin>. I
know coding tasks are not interchangeable: just because you're
motivated to work on separate metadata areas doesn't necessarily mean
you're motivated to fix bugs... but I can still wish, right? :-)
Here goes:
> Index: subversion/include/svn_wc.h
> ===================================================================
> --- subversion/include/svn_wc.h (revision 6927)
> +++ subversion/include/svn_wc.h (working copy)
> @@ -935,6 +935,17 @@
>
>
>
> +/* Create a separate administrative area.
> + *
> + * If an administrative directory already exists for this path in
> + * the index, it is overwritten. The actual administrative files
> + * are not deleted, only the reference in the index.
> + *
> + */
> +svn_error_t *svn_wc_create_adm_root (const char *path,
> + apr_pool_t *pool);
> +
> +
This doc string refers "the index", a concept that the reader may not
be familiar with. There needs to be something at the top of svn_wc.h
explaining the two kinds of adm areas, something giving the kind of
overview you gave in your mail. I confess I'm not too clear on why
this function offers the functionality it does, even knowing the
overview...
In new file config_meta.c:
+/** Cached meta data index. */
+static apr_hash_t *cachedIndex = NULL;
+static const char *cachedConfigDir = NULL;
If it's truly safe for these to be global (which is rare!), the
comment should explain why. Subversion needs to stay thread-safe.
Based on later code, it doesn't look to me like these globals are
thread-safe.
When there's a hash, you need to document the types and meanings of
the keys and the values.
In other words, when another programmer reads the variable
declarations, they should come away knowing what the variables mean
and how they are used.
> +/** Helper function to clean up the cached index. */
> +static apr_status_t
> +cleanupCachedIndex (void *data)
> +{
> + if (data == cachedIndex) {
> + /*
> + * These objects are automatically deallocated when
> + * the pool is destroyed. We just need to make sure
> + * that we know to re-allocate them later.
> + */
> + cachedIndex = NULL;
> + cachedConfigDir = NULL;
> + }
> + return APR_SUCCESS;
> +}
Oooh, please never theStudlyCaps style. We always_use_underscores :-).
> +/** Get the meta data root path. */
> +svn_error_t *
> +svn_config_get_meta_root (const char **meta_root,
> + const char *config_dir,
> + apr_pool_t *pool)
You don't need to redocument this, it's already documented in
svn_config.h.
(Same for many other functions.)
> + SVN_ERR (svn_io_check_path (*meta_filename, &kind, pool));
> + if (kind == svn_node_none) return SVN_NO_ERROR;
> + if (kind != svn_node_file)
> + return svn_error_createf(SVN_ERR_NODE_UNEXPECTED_KIND, NULL,
> + "Not a file: %s", *meta_filename);
> +
> + return SVN_NO_ERROR;
> +}
Please don't use the
if (cond) statement;
format. Always use a separate line for the body; curly braces are
nice, but in some contexts they can be skipped -- there's no real rule
about that, just use your judgement.
> +
> +
> +/**
> + * Read the meta data index. If the index does not exist, return
> + * an empty hash. The index is cached for performance, since this
> + * function may be called many times during the course of a single
> + * operation.
> + */
> +svn_error_t *
> +svn_config_read_meta_index (apr_hash_t **hash,
> + const char *config_dir,
> + apr_pool_t *pool)
Now this is really repeating a lot of the doc string :-).
> Index: subversion/libsvn_client/checkout.c
> ===================================================================
> --- subversion/libsvn_client/checkout.c (revision 6927)
> +++ subversion/libsvn_client/checkout.c (working copy)
> @@ -102,9 +102,11 @@
> /* Bootstrap: create an incomplete working-copy root dir. Its
> entries file should only have an entry for THIS_DIR with a
> URL, revnum, and an 'incomplete' flag. */
> - SVN_ERR (svn_io_make_dir_recursively (path, pool));
> + SVN_ERR (svn_io_make_dir_recursively (path, pool));
> + if (ctx->meta == svn_meta_separate)
> + SVN_ERR (svn_wc_create_adm_root (path, pool));
> SVN_ERR (svn_wc_ensure_adm (path, URL, revnum, pool));
> -
> +
Urk.
The fact that the meta cache is even *known* outside libsvn_wc is a
bad sign... The libsvn_client code should not be aware of how
libsvn_wc manages itself (we may violate this in a few places
currently, but if so, it's a trend we want to shrink, not grow!).
The storage particulars of adm information is totally the job of
libsvn_wc. libsvn_client just needs an interface to "wc-ify" a
directory; how that happens is up to libsvn_wc. The new conditional
above is therefore disturbing in its implications.
-Karl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Sat Aug 30 06:38:38 2003