On Wed, Sep 20, 2006 at 10:15:36PM +0200, Erik Huelsmann wrote:
> And the patch is attached. More comments?
>
Looks good to me, though as usual I can't pretend to understand every
nuance of the log handling.
A couple of comments:
> +/* Return TRUE if any item of QUEUE (except CURRENT)
> + is a parent of PATH and will be processed recursively,
> + return FALSE otherwise.
> +
> + If HAVE_RECURSIVE is FALSE, exit early returning FALSE.
> + Recalculate its value otherwise, changing it to FALSE
> + iff no recursive items are found.
> +*/
> +static svn_boolean_t
> +have_recursive_parent(svn_boolean_t *have_recursive,
> + apr_array_header_t *queue,
> + int current,
> + const char *path,
> + apr_pool_t *pool)
I initially didn't grasp that while have_recursive_parent() returns
something related to the current item, the HAVE_RECURSIVE Argument to
that function is used as a cache to identify whether _any_ of the items
are recursive. Perhaps you could rename the argument (and use, later
in svn_wc_process_committed_queue()) to something like HAVE_ANY_RECURSIVE?
Also, you pass QUEUE, CURRENT, and PATH, yet the PATH can trivially be
derived from the other two. It's also not entirely clear why the 'except
CURRENT' clause in the doc-comments is present. You could resolve both
of these by removing PATH and calculating it inside the function.
Perhaps something like:
> +/* Return TRUE if any item of QUEUE has a path that is a
> + parent of ITEM's path and will be processed recursively,
> + return FALSE otherwise.
> +
> + If HAVE_ANY_RECURSIVE is FALSE, exit early returning FALSE.
> + Recalculate its value otherwise, changing it to FALSE
> + iff no recursive items are found in the queue.
> +*/
> +static svn_boolean_t
> +have_recursive_parent(svn_boolean_t *have_any_recursive,
> + apr_array_header_t *queue,
> + int item,
> + apr_pool_t *pool)
> +svn_error_t *
> +svn_wc_process_committed_queue(svn_wc_committed_queue_t *queue,
> + svn_wc_adm_access_t *adm_access,
> + svn_revnum_t new_revnum,
> + const char *rev_date,
> + const char *rev_author,
> + apr_pool_t *pool)
> +{
>
> [...]
>
> + /* allocate in pool instead of subpool:
> + we don't want this cleared at the next iteration */
You don't have a subpool at this stage; there's iterpool, did you
mean that?
> + ((apr_array_header_t *)queue)->nelts = 0;
The doc-comments for this function should mention that it clears the
queue; though I also wonder whether it's necessary to do this - any
particular reason?
> Index: subversion/libsvn_client/commit.c
> ===================================================================
> --- subversion/libsvn_client/commit.c (revision 21559)
> +++ subversion/libsvn_client/commit.c (working copy)
> @@ -1599,23 +1556,25 @@
> remove_lock = (! keep_locks && (item->state_flags
> & SVN_CLIENT_COMMIT_ITEM_LOCK_TOKEN));
> assert(*commit_info_p);
> - if ((bump_err = svn_wc_process_committed4
> - (item->path, adm_access,
> - loop_recurse,
> - (*commit_info_p)->revision,
> - (*commit_info_p)->date,
> - (*commit_info_p)->author,
> + if ((bump_err = svn_wc_queue_committed
> + (&queue,
> + item->path, adm_access, loop_recurse,
> item->wcprop_changes,
> - remove_lock,
> - (! keep_changelist),
> - apr_hash_get(digests, item->path, APR_HASH_KEY_STRING),
> - subpool)))
> + remove_lock, (! keep_changelist),
> + apr_hash_get(digests, item->path, APR_HASH_KEY_STRING), pool)))
> break;
>
> }
>
> /* Destroy the subpool. */
> svn_pool_destroy(subpool);
> +
> + bump_err
> + = svn_wc_process_committed_queue(queue, base_dir_access,
> + (*commit_info_p)->revision,
> + (*commit_info_p)->date,
> + (*commit_info_p)->author,
> + pool);
You already have a subpool at this point - any reason you
can't use it in the calls to svn_wc_queue_committed() and
svn_wc_process_committed_queue()?
Overall, this looks great, thanks!
Regards,
Malcolm
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Thu Sep 21 11:16:39 2006