On Thu, 28 Feb 2002, Greg Stein wrote:
> [ 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.
It does break , because !exists will not be true anymore.
> > + if (this_rev == ((svn_revnum_t *)(revs->elts))[i])
> > + matched = 1;
>
> And break out of here, too.
Same here, !matched will not be true anymore.
>
> Cheers,
> -g
>
>
---------------------------------------------------------------------
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