cmpilato@tigris.org writes:
> --- branches/fs-schema-changes/subversion/libsvn_fs/tree.c (original)
> +++ branches/fs-schema-changes/subversion/libsvn_fs/tree.c Tue Aug 12 15:08:47 2003
> @@ -3604,6 +3604,85 @@
>
>
>
> +/* Finding Changes */
> +
> +struct paths_changed_args
> +{
> + apr_hash_t *changes;
> + svn_fs_root_t *root;
> +};
I realize it wasn't documented in the original code either, but might
be nice to say what are the keys and values of the 'changes' hash.
(It wasn't obvious to me from looking.)
> +/* Our coolio opaque history object. */
> +struct svn_fs_history_t
> +{
> + svn_fs_t *fs;
> + const char *path;
> + svn_revnum_t revision;
> + const char *path_hint;
> + svn_revnum_t rev_hint;
> + int is_interesting;
> +};
So coolio that none of its fields need to be documented :-) ?
(Seriously -- I try not to be a stickler about internal structs when
the fields are obvious, but in this case some docs might clarify,
particularly the hint fields and is_interesting).
> +/* Return a new history object (marked as "interesting") for PATH and
> + REVISION, allocated in POOL. Note that paths are not duped into
> + POOL -- it is the responsibility of the caller to ensure that this
> + happens. */
> +static svn_fs_history_t *
> +assemble_history (svn_fs_t *fs,
> + const char *path,
> + svn_revnum_t revision,
> + int is_interesting,
> + const char *path_hint,
> + svn_revnum_t rev_hint,
> + apr_pool_t *pool)
I guess it's sort of implied, but you might as well just say that
arguments get copied to the corresponding fields in the structure.
The phrase 'marked as "interesting"' doesn't convey anything, for
example.
Call me "Doc Monster", just don't call me late for dinner.
I assume the doc string is referring to both the 'path' and
'path_hint' fields when it talks about allocation? Would be good to
name them explicitly, though.
> +/* Derive the oldest of a set of copies which took place in filesystem
> + FS -- bounded by the transactions START_TXN_ID and END_TXN_ID, and
> + by the copy ids START_COPY_ID and END_COPY_ID -- which resulted in
> + the creation of END_PATH. Return the previous location of the
> + END_PATH as *SRC_REV/SRC_PATH, and the revision in which the copy
> + occured as *DST_REV. Do all of this as part of TRAIL.
>
> +find_youngest_copy (svn_revnum_t *src_rev, /* return */
> + const char **src_path, /* return */
> + svn_revnum_t *dst_rev, /* return */
> + const char *dst_path,
> + svn_fs_t *fs,
> + svn_revnum_t start_rev,
> + const char *start_copy_id,
> + svn_revnum_t end_rev,
> + const char *end_copy_id, /* may be NULL */
> + trail_t *trail)
> +{
How are 'dst_path', 'start_rev' and 'end_rev' used?
> if (! (txn->copies && txn->copies->nelts))
> - continue;
> + goto loop_inc;
Sure you don't want to call that 'loop_decr', since it's a decrement
at the label?
> + loop_inc:
> + cur_rev--;
...here.
> +struct history_prev_args
> {
> - apr_hash_t *revs;
> - svn_fs_t *fs;
> - svn_fs_root_t *root;
> - const char *path;
> - const svn_fs_id_t *id;
> - int cross_copy_history;
> + svn_fs_history_t **prev_history_p;
> + svn_fs_history_t *history;
> + int cross_copies;
> + apr_pool_t *pool;
> };
You might think I'm going to complain here, but I'm not :-). These
fields are all obvious. Documentation would only obscure (except
perhaps for the 'pool' field).
> +svn_error_t *
> +svn_repos_revisions_changed (apr_array_header_t **revs,
> + svn_fs_t *fs,
> + const char *path,
> + svn_revnum_t start,
> + svn_revnum_t end,
> + svn_boolean_t cross_copies,
> + apr_pool_t *pool)
> +{
> + svn_fs_history_t *history;
> + apr_pool_t *subpool1 = svn_pool_create (pool);
> + apr_pool_t *subpool2 = svn_pool_create (pool);
> + apr_pool_t *oldpool, *newpool;
> + const char *history_path;
> + svn_revnum_t history_rev;
> + svn_fs_root_t *root;
> +
> + /* Validate the revisions. */
> + if ((! SVN_IS_VALID_REVNUM (start)) || (! SVN_IS_VALID_REVNUM (end)))
> + return svn_error_create (SVN_ERR_FS_NO_SUCH_REVISION, 0, "");
Do we have a function for manufacturing a better error string here
(one that specifies the invalid revision?) If not, we could still do
it by hand, and also distinguish between 'start' being invalid and
'end' being invalid...
> + /* Ensure that the input is ordered. */
> + if (start > end)
> + {
> + svn_revnum_t tmprev = start;
> + start = end;
> + end = tmprev;
> + }
A professor once showed me
start = start + end
end = start - end
start = start - end
It's needlessly obfuscatory, risks overflow, and is in no way
preferable to what you did. I don't know why I brought it up.
> + /* Allocate our return array. */
> + *revs = apr_array_make (pool, 4, sizeof (history_rev));
> +
> + /* Get a revision root for END, and an initial HISTORY baton. */
> + SVN_ERR (svn_fs_revision_root (&root, fs, end, pool));
> + SVN_ERR (svn_fs_node_history (&history, root, path, subpool1));
> + oldpool = subpool1;
> + newpool = subpool2;
I got baffled by the 'subpool1, subpool2' vs 'oldpool, newpool'
distinction. Why are there two pairs of variables, instead of just
two variables? I saw 'tmppool' and the comment later about "crazy
pool work", but wow -- is there some simpler way to do this? I
haven't considered the problem deeply, just a hunch. Or did you start
out thinking that too, and then learn better? :-)
Anyway, above very minor nits aside, nice job! The new interfaces are
clean & beautiful, and the code is extremely clear.
-Karl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Tue Aug 12 23:40:21 2003