You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Greg Stein <gs...@gmail.com> on 2012/06/23 06:44:20 UTC
Re: svn commit: r1352935 - in /subversion/trunk/subversion:
include/svn_client.h libsvn_client/deprecated.c libsvn_client/mergeinfo.c
svn/main.c svn/mergeinfo-cmd.c
On Jun 22, 2012 11:48 AM, <cmpilato <cm...@apache.org>
apache.org <cm...@apache.org>> wrote:
>...
> +++ subversion/trunk/subversion/libsvn_client/mergeinfo.c Fri Jun 22
15:47:43 2012
> @@ -1622,20 +1622,21 @@ svn_client_mergeinfo_get_merged(apr_hash
>
>
> svn_error_t *
> -svn_client_mergeinfo_log(svn_boolean_t finding_merged,
> - const char *target_path_or_url,
> - const svn_opt_revision_t *target_peg_revision,
> - const char *source_path_or_url,
> - const svn_opt_revision_t *source_peg_revision,
> - svn_log_entry_receiver_t log_receiver,
> - void *log_receiver_baton,
> - svn_boolean_t discover_changed_paths,
> - svn_depth_t depth,
> - const apr_array_header_t *revprops,
> - svn_client_ctx_t *ctx,
> - apr_pool_t *scratch_pool)
> +svn_client_mergeinfo_log2(svn_boolean_t finding_merged,
> + const char *target_path_or_url,
> + const svn_opt_revision_t *target_peg_revision,
> + const char *source_path_or_url,
> + const svn_opt_revision_t *source_peg_revision,
> + const svn_opt_revision_t
*source_start_revision,
> + const svn_opt_revision_t *source_end_revision,
> + svn_log_entry_receiver_t log_receiver,
> + void *log_receiver_baton,
> + svn_boolean_t discover_changed_paths,
> + svn_depth_t depth,
> + const apr_array_header_t *revprops,
> + svn_client_ctx_t *ctx,
> + apr_pool_t *scratch_pool)
> {
> - apr_pool_t *sesspool = svn_pool_create(scratch_pool);
> const char *log_target = NULL;
> const char *repos_root;
> const char *target_repos_rel;
> @@ -1662,6 +1663,23 @@ svn_client_mergeinfo_log(svn_boolean_t f
> SVN_ERR_UNSUPPORTED_FEATURE, NULL,
> _("Only depths 'infinity' and 'empty' are currently supported"));
>
> + /* Validate and sanitize the incoming source operative revision range.
*/
> + if (!((source_start_revision->kind == svn_opt_revision_unspecified) ||
> + (source_start_revision->kind == svn_opt_revision_number) ||
> + (source_start_revision->kind == svn_opt_revision_date) ||
> + (source_start_revision->kind == svn_opt_revision_head)))
> + return svn_error_create
> + (SVN_ERR_CLIENT_BAD_REVISION, NULL, NULL);
> + if (!((source_end_revision->kind == svn_opt_revision_unspecified) ||
> + (source_end_revision->kind == svn_opt_revision_number) ||
> + (source_end_revision->kind == svn_opt_revision_date) ||
> + (source_end_revision->kind == svn_opt_revision_head)))
> + return svn_error_create
> + (SVN_ERR_CLIENT_BAD_REVISION, NULL, NULL);
Style: white space before paren, and it would be better to lead (not trail)
the lines with ||.
> ...
Cheers,
-g
Re: svn commit: r1352935 - in /subversion/trunk/subversion: include/svn_client.h
libsvn_client/deprecated.c libsvn_client/mergeinfo.c svn/main.c svn/mergeinfo-cmd.c
Posted by "C. Michael Pilato" <cm...@collab.net>.
On 06/24/2012 04:41 PM, C. Michael Pilato wrote:
> On 06/23/2012 12:44 AM, Greg Stein wrote:
>>> + return svn_error_create
>>> + (SVN_ERR_CLIENT_BAD_REVISION, NULL, NULL);
>>
>> Style: white space before paren, and it would be better to lead (not trail)
>> the lines with ||.
>
> Agreed on the whitespace. (Silly anyway ... the whole statement fits in 80
> columns.)
r1353326 for this bit.
--
C. Michael Pilato <cm...@collab.net>
CollabNet <> www.collab.net <> Enterprise Cloud Development
Re: svn commit: r1352935 - in /subversion/trunk/subversion: include/svn_client.h
libsvn_client/deprecated.c libsvn_client/mergeinfo.c svn/main.c svn/mergeinfo-cmd.c
Posted by "C. Michael Pilato" <cm...@collab.net>.
On 06/23/2012 12:44 AM, Greg Stein wrote:
>> @@ -1662,6 +1663,23 @@ svn_client_mergeinfo_log(svn_boolean_t f
>> SVN_ERR_UNSUPPORTED_FEATURE, NULL,
>> _("Only depths 'infinity' and 'empty' are currently supported"));
>>
>> + /* Validate and sanitize the incoming source operative revision range. */
>> + if (!((source_start_revision->kind == svn_opt_revision_unspecified) ||
>> + (source_start_revision->kind == svn_opt_revision_number) ||
>> + (source_start_revision->kind == svn_opt_revision_date) ||
>> + (source_start_revision->kind == svn_opt_revision_head)))
>> + return svn_error_create
>> + (SVN_ERR_CLIENT_BAD_REVISION, NULL, NULL);
>> + if (!((source_end_revision->kind == svn_opt_revision_unspecified) ||
>> + (source_end_revision->kind == svn_opt_revision_number) ||
>> + (source_end_revision->kind == svn_opt_revision_date) ||
>> + (source_end_revision->kind == svn_opt_revision_head)))
>> + return svn_error_create
>> + (SVN_ERR_CLIENT_BAD_REVISION, NULL, NULL);
>
> Style: white space before paren, and it would be better to lead (not trail)
> the lines with ||.
Agreed on the whitespace. (Silly anyway ... the whole statement fits in 80
columns.)
We're a bit less consistent on the operator placement in the codebase. With
mixed-operator conditions, I agree with (and employ) the "trailing operator"
style. But in this situation, I think readability is greatly enhanced by
having having conditions vertically align the way they appear above.
--
C. Michael Pilato <cm...@collab.net>
CollabNet <> www.collab.net <> Enterprise Cloud Development