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

Re: [PATCH] Issue #1776 - Create parent directories on url with

From: S.Ramaswamy <ramaswamy_at_collab.net>
Date: 2005-03-10 20:22:58 CET

My responses below:

> "S.Ramaswamy" <ramaswamy@collab.net> writes:
>
>
> If not then your code should use a subpool, call the cancellation
> function, etc. Does it really need to open multiple sessions?
>

ok, I will make these modifications.

>> +
>> + if (kind == svn_node_dir)
>> + {
>> + *existing_url = (const char *)url;
>
> Why the cast?
>
>> + if (url == APR_ARRAY_IDX (temp_paths, 0, const char*))
>> + {
>> + *new_path = NULL;
>> + }
>> + else
>> + {
>> + APR_ARRAY_PUSH (temp_paths, const char *) = url;
>> + SVN_ERR (svn_path_condense_targets (&pcommon, &new_paths,
>> +
>> temp_paths,FALSE,pool)); + *new_path = APR_ARRAY_IDX
>> (new_paths, 0, const char *); + }
>
> That looks like overkill, there are only two paths, you know what they
> are, is svn_path_condense_targets necessary? Are the
> apr_array_header_t necessary?

Is there a function to get the basename that's extra to one of two paths
- for instance with urls http://host/repos/trunk and
http://host/repos/trunk/dira/dira2/dira3, how do you extract
"dira/dira2/dira3" - If there is a way to do that without using
svn_path_condense_targets, I will do that. Let me check svn_path.h
again.

>
>> + }
>> +
>> + return SVN_NO_ERROR;
>> +}
>> +
>> static svn_error_t *
>> mkdir_urls (svn_client_commit_info_t **commit_info,
>> const apr_array_header_t *paths,
>> + svn_boolean_t make_parents,
>> svn_client_ctx_t *ctx,
>> apr_pool_t *pool)
>> {
>> @@ -499,41 +554,143 @@
>> svn_error_t *err;
>> const char *common;
>> int i;
>> + const char *target_prefix;
>> + apr_array_header_t *valid_urls = apr_array_make(pool,
>> + paths->nelts, +
>> sizeof(const char *)); +
>> apr_array_header_t *prefix;
>> + apr_array_header_t *new_paths = apr_array_make(pool,
>> + paths->nelts,
>> + sizeof(const
>> char*)); + int j;
>> + const char *target_path;
>> + apr_hash_t *target_hash;
>> + apr_hash_index_t *hi;
>> + int nelts;
>> + apr_pool_t *subpool;
>> + apr_pool_t *iterpool;
>>
>> - /* Condense our list of mkdir targets. */
>> - SVN_ERR (svn_path_condense_targets (&common, &targets, paths,
>> FALSE, pool)); - if (! targets->nelts)
>> + if (make_parents)
>> {
>> - const char *bname;
>> - svn_path_split (common, &common, &bname, pool);
>> - APR_ARRAY_PUSH (targets, const char *) = bname;
>> - }
>> - else
>> - {
>> - svn_boolean_t resplit = FALSE;
>> + const char *existing_url;
>> + const char *new_path;
>> +
>> + target_hash = apr_hash_make(pool);
>> + subpool = svn_pool_create(pool);
>
> Whitespace: this file uses "function (" not "function(". This applies
> to lots of your code.
>
>>
>> - /* We can't "mkdir" the root of an editor drive, so if one of -
>> our targets is the empty string, we need to back everything -
>> up by a path component. */
>> - for (i = 0; i < targets->nelts; i++)
>> + for(i=0; i < paths->nelts; i++)
>
> Look at the different whitespace, please be consistent with the
> existing code.

Sorry - will fix that.

>
>
> You don't appear to have included changes to svn_client.h, or the
> command line client. I guess you intend your patch to apply on top of
> the one in the issue, but it's more difficult to review if you do it
> like that.

I agree, I didn't include it since I didn't know if it would be
appropriate. I will include it the next version of the patch and clearly
indicate the portion of the patch that I wrote.

>
> Could you add a test to the testsuite?

Let me try. I will send the revised patch after making the changes you have
suggested.

Thanks
Ramaswamy

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Thu Mar 10 20:24:26 2005

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.