[ per IRC, there are still some problems in the svn_client_log() handling
for URLs; Ben is going to redo that portion ]
The patch looks good, but for the URL code in svn_client_log. I'd recommend
returning SVN_ERR_UNSUPPORTED_FEATURE for that for now, and we'll get the
rest of this in. The URL can be done in a second round. Thoughts?
On Thu, Feb 28, 2002 at 10:50:05PM -0500, Ben Collins wrote:
> Here's a new patch. Changelog is the same. Fixes all suggestions, plus a
> few.
As I mentioned on IRC, when resubmitting a patch, please ensure to repost
the log message. It is quite possible the person who will commit the patch
has deleted the prior mail.
>...
> +++ ./subversion/include/svn_fs.h Thu Feb 28 21:52:50 2002
> @@ -666,7 +666,9 @@
>
>
> /* Allocate and return an array *REVS of svn_revnum_t revisions in
> - which PATH under ROOT was modified. Use POOL for all allocations.
> + which PATHS under ROOT were modified. Use POOL for all allocations.
> + The array of *REVS are sorted in descending order. All duplicates
> + will also be removed.
>
> NOTE: This function uses node-id ancestry alone to determine
> modifiedness, and therefore does NOT claim that in any of the
> @@ -674,7 +676,7 @@
> directory entries lists changed, etc. */
> svn_error_t *svn_fs_revisions_changed (apr_array_header_t **revs,
> svn_fs_root_t *root,
> - const char *path,
> + const apr_array_header_t *paths,
> apr_pool_t *pool);
The doc string should tell the caller what the type of the PATHS elements
are. Since that isn't type-checked by the compiler or anything, it is
important to tell them. Just something like "the elements are 'const char *'"
>...
> +++ ./subversion/libsvn_fs/tree.c Thu Feb 28 22:17:05 2002
>...
> + {
> + int t, exists = 0;
> + svn_revnum_t revision;
> + dag_node_t *node;
> + int len = svn_fs_id_length (tmp_id);
> +
> + /* Get the dag node for this id. */
> + SVN_ERR (svn_fs__dag_get_node (&node, args->fs, tmp_id, trail));
> +
> + /* Now get the revision from the dag. */
> + SVN_ERR (svn_fs__dag_get_revision (&revision, node, trail));
> +
> + /* And put the revision into our array, if it doesn't already
> + * exist. */
> + for (t = 0; t < array->nelts && ! exists; t++)
> + if (APR_ARRAY_IDX(array, t, svn_revnum_t) == revision)
> + exists = 1;
I'm mildly concerned about O(n^2) operation here, but I can't imagine this
being all that large of an array. Regardless, when you find a match, you
should 'break' out of the loop.
An alternative to the nested loop is to remove dups after the qsort()
occurs.
>...
> + /* We split the path here. If this is the only target (a full path)
> + we'll end up using BASENAME as the URL, and FILE as the path to
> + search on. For both cases, BASENAME gets passed to
> + svn_client__get_revision_number() below. */
> + svn_path_split (base, &basename, &file, pool);
>...
> @@ -94,13 +149,14 @@
> SVN_ERR (svn_client__open_ra_session (&session, ra_lib, URL, basename,
> TRUE, TRUE, auth_baton, pool));
>
> + /* Get the revisions based on the users "hints". */
> SVN_ERR (svn_client__get_revision_number
> (&start_revnum, ra_lib, session, start, basename->data, pool));
> SVN_ERR (svn_client__get_revision_number
> (&end_revnum, ra_lib, session, end, basename->data, pool));
When using a URL for an argument, there might not be a working copy. That
implies that NULL should be passed for the 'base_dir' arg to open_ra_session
(and FALSE for the do_store and use_admin flags). Also, NULL should be
passed for the 'path' argument to get_revision_number.
>...
> + if (revs)
> + {
> + int i, matched = 0;
> + for (i = 0; i < revs->nelts && !matched; i++)
> + if (this_rev == ((svn_revnum_t *)(revs->elts))[i])
> + matched = 1;
And break out of here, 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
Received on Sat Oct 21 14:37:10 2006