[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: Input validation observations

From: Julian Foad <julian.foad_at_wandisco.com>
Date: Thu, 09 Dec 2010 16:44:36 +0000

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

This is an archived mail posted to the Subversion Dev mailing list.

This site is subject to the Apache Privacy Policy and the Apache Public Forum Archive Policy.