On Wed, 2010-12-08, Noorul Islam K M wrote:
> Attached is the patch which has the new functions to replace the
> existing blocks. All tests pass when tested with 'make check'. No need
> for test cases as they are already available.
Noorul, thank you. This is great.
> Also I have not modified
> libsvn_client/copy.c and svn/diff-cmd.c because they are not straight
> forward. I will go through them again and will come up with follow-up
> patch.
OK, that's fine.
The log message is good. I'll just rearrange the introductory sentence
and start with the words "Factor out ...", as I think that provides a
quick introduction to the change.
> [[[
> Two new functions one each for command line and client API which checks
> whether the target types are homogeneous.
>
> * subversion/svn/cl.h,
> subversion/svn/util.c
> (svn_cl__assert_homogeneous_target_type): New function
>
> * subversion/libsvn_client/client.h
> subversion/libsvn_client/util.c
> (svn_client__assert_homogeneous_target_type): New function
>
> * subversion/svn/mkdir-cmd.c,
> subversion/svn/delete-cmd.c,
> subversion/svn/lock-cmd.c,
> subversion/svn/unlock-cmd.c
> (svn_cl__mkdir, svn_cl__delete, svn_cl__lock, svn_cl__unlock):
> Replace existing logic with call to new function
> svn_cl__assert_homogeneous_target_type
>
> * subversion/libsvn_client/delete.c,
> subversion/libsvn_client/locking_commands.c,
> subversion/libsvn_client/add.c
> (svn_client_delete4, organize_lock_targets, svn_client_mkdir4):
> Replace existing logic with call to new function
> svn_client__assert_homogeneous_target_type
>
> Patch by: Noorul Islam K M <noorul{_AT_}collab.net>
> ]]]
The patch is good. The only things I want to change are very minor
details. There is no need for you to resubmit the patch; I will just
change them before I commit it.
> Index: subversion/svn/cl.h
> ===================================================================
> +/* This function checks to see if targets contain mixture of URLs
> + * and paths, returning an error if it finds a mixture of both. */
> +svn_error_t *
> +svn_cl__assert_homogeneous_target_type(apr_array_header_t *targets);
> Index: subversion/libsvn_client/client.h
> ===================================================================
> +/* This function checks to see if targets contain mixture of URLs
> + * and paths, returning an error if it finds mixture of both. */
> +svn_error_t *
> +svn_client__assert_homogeneous_target_type(const apr_array_header_t *targets);
I re-wrote the doc string as:
/* Return an error if TARGETS contains a mixture of URLs and paths;
* otherwise return SVN_NO_ERROR. */
and I added "const" to the parameter of the svn_cl__ function, because
that's logical and consistent with the other function.
> Index: subversion/svn/util.c
> ===================================================================
> +svn_error_t *
> +svn_cl__assert_homogeneous_target_type(apr_array_header_t *targets)
> +{
> + svn_boolean_t wc_present = FALSE, url_present = FALSE;
> + int i;
> +
> + for (i = 0; i < targets->nelts; ++i)
> + {
> + const char *target = APR_ARRAY_IDX(targets, i, const char *);
> + if (! svn_path_is_url(target))
> + wc_present = TRUE;
> + else
> + url_present = TRUE;
> + }
> +
> + if (url_present && wc_present)
> + return svn_error_createf(SVN_ERR_CL_ARG_PARSING_ERROR, NULL,
> + _("Cannot mix repository and working copy "
> + "targets"));
I adjusted the indentation of these two lines so "_(" is below "SVN_".
> +
> + return SVN_NO_ERROR;
> +}
Committed revision 1044028.
Thanks again.
- Julian
Received on 2010-12-09 17:45:19 CET