You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Hyrum K Wright <hy...@wandisco.com> on 2012/04/06 22:53:08 UTC
Re: svn commit: r1310581 - /subversion/branches/ev2-export/subversion/libsvn_client/copy.c
On Fri, Apr 6, 2012 at 3:48 PM, <hw...@apache.org> wrote:
> Author: hwright
> Date: Fri Apr 6 20:48:32 2012
> New Revision: 1310581
>
> URL: http://svn.apache.org/viewvc?rev=1310581&view=rev
> Log:
> On the ev2-export branch:
> Directly add any new directories, rather than through the path_info structs.
>
> * subversion/libsvn_client/copy.c
> (path_driver_info_t): Remove dir_add member.
> (drive_single_path): Don't check for the dir_add flag.
> (drive_editor): Handle the new dirs before the various copy/move paths.
> (repos_to_repos_copy): Don't bother putting the new directoris into the
> set of paths to be operated on, just give them to drive_editor().
>
> Modified:
> subversion/branches/ev2-export/subversion/libsvn_client/copy.c
>
> Modified: subversion/branches/ev2-export/subversion/libsvn_client/copy.c
> URL: http://svn.apache.org/viewvc/subversion/branches/ev2-export/subversion/libsvn_client/copy.c?rev=1310581&r1=1310580&r2=1310581&view=diff
> ==============================================================================
> --- subversion/branches/ev2-export/subversion/libsvn_client/copy.c (original)
> +++ subversion/branches/ev2-export/subversion/libsvn_client/copy.c Fri Apr 6 20:48:32 2012
> @@ -532,7 +532,6 @@ typedef struct path_driver_info_t
> svn_node_kind_t src_kind;
> svn_revnum_t src_revnum;
> svn_boolean_t resurrection;
> - svn_boolean_t dir_add;
> svn_string_t *mergeinfo; /* the new mergeinfo for the target */
> } path_driver_info_t;
>
> @@ -627,19 +626,9 @@ drive_single_path(svn_editor_t *editor,
> apr_pool_t *scratch_pool)
> {
> apr_hash_t *props = apr_hash_make(scratch_pool);
> - apr_array_header_t *children = apr_array_make(scratch_pool, 1,
> - sizeof(const char *));
> svn_boolean_t do_delete = FALSE;
> svn_boolean_t do_add = FALSE;
>
> - /* Check to see if we need to add the path as a directory. */
> - if (path_info->dir_add)
> - {
> - return svn_error_trace(svn_editor_add_directory(editor, path, children,
> - props,
> - SVN_INVALID_REVNUM));
> - }
> -
> /* If this is a resurrection, we know the source and dest paths are
> the same, and that our driver will only be calling us once. */
> if (path_info->resurrection)
> @@ -702,6 +691,7 @@ static svn_error_t *
> drive_editor(svn_editor_t *editor,
> apr_array_header_t *paths,
> apr_hash_t *action_hash,
> + apr_array_header_t *new_dirs,
> svn_boolean_t is_move,
> svn_revnum_t youngest,
> apr_pool_t *scratch_pool)
> @@ -710,6 +700,23 @@ drive_editor(svn_editor_t *editor,
> apr_pool_t *iterpool;
> int i;
>
> + if (new_dirs)
> + {
> + apr_array_header_t *children = apr_array_make(scratch_pool, 1,
> + sizeof(const char *));
> + apr_hash_t *props = apr_hash_make(scratch_pool);
> +
> + /* These are directories which we just need to add. */
> + for (i = 0; i < new_dirs->nelts; i++)
> + {
> + const char *dir = APR_ARRAY_IDX(new_dirs, i, const char *);
> +
> + /* ### The children param needs to be populated correctly. */
> + SVN_ERR(svn_editor_add_directory(editor, dir, children, props,
> + SVN_INVALID_REVNUM));
Greg,
Just so you know, I would have expected this section to crash Ev2 in
copy test 67, since there are several directories being added, but the
children array is (currently) empty. I wonder if there is a problem
with the internal Ev2 state checking.
Also, this currently feeds paths to Ev2 child-first. For instance,
copy test 67 calls this with 'X/Y/Z', 'X/Y', and 'X', in that order.
The shims reorder things so that it gets done properly, but I wonder
if there shouldn't be some state checks in Ev2 to ensure parents are
added before children.
-Hyrum
> + }
> + }
> +
> iterpool = svn_pool_create(scratch_pool);
> for (i = 0; i < paths->nelts; i++)
> {
> @@ -1071,24 +1078,7 @@ repos_to_repos_copy(const apr_array_head
> else
> message = "";
>
> - /* Setup our PATHS for the path-based editor drive. */
> - /* First any intermediate directories. */
> - if (make_parents)
> - {
> - for (i = 0; i < new_dirs->nelts; i++)
> - {
> - const char *relpath = APR_ARRAY_IDX(new_dirs, i, const char *);
> - path_driver_info_t *info = apr_pcalloc(pool, sizeof(*info));
> -
> - info->dst_path = relpath;
> - info->dir_add = TRUE;
> -
> - APR_ARRAY_PUSH(paths, const char *) = relpath;
> - apr_hash_set(action_hash, relpath, APR_HASH_KEY_STRING, info);
> - }
> - }
> -
> - /* Then our copy destinations and move sources (if any). */
> + /* Setup our copy destinations and move sources (if any). */
> for (i = 0; i < path_infos->nelts; i++)
> {
> path_driver_info_t *info = APR_ARRAY_IDX(path_infos, i,
> @@ -1114,7 +1104,7 @@ repos_to_repos_copy(const apr_array_head
> ctx->cancel_func, ctx->cancel_baton,
> pool, pool));
>
> - return svn_error_trace(drive_editor(editor, paths, action_hash,
> + return svn_error_trace(drive_editor(editor, paths, action_hash, new_dirs,
> is_move, youngest, pool));
> }
>
>
>
--
uberSVN: Apache Subversion Made Easy
http://www.uberSVN.com/
Re: svn commit: r1310581 - /subversion/branches/ev2-export/subversion/libsvn_client/copy.c
Posted by Hyrum K Wright <hy...@wandisco.com>.
On Fri, Apr 6, 2012 at 8:17 PM, Greg Stein <gs...@gmail.com> wrote:
> On Fri, Apr 6, 2012 at 17:36, Hyrum K Wright <hy...@wandisco.com> wrote:
>> On Fri, Apr 6, 2012 at 4:31 PM, Greg Stein <gs...@gmail.com> wrote:
>>> On Fri, Apr 6, 2012 at 16:53, Hyrum K Wright <hy...@wandisco.com> wrote:
>>>>...
>>>> Just so you know, I would have expected this section to crash Ev2 in
>>>> copy test 67, since there are several directories being added, but the
>>>> children array is (currently) empty. I wonder if there is a problem
>>>> with the internal Ev2 state checking.
>>>
>>> I'm suspicious that the checks are not working because of the "pass
>>> URLs as relpath" that you're doing.
>>>
>>> I should add some canonical assertions for all the relpath params, and
>>> get you to fix that bug :-)
>>
>> Go for it! You're welcome to use the branch, but it'd probably be
>> best for these checks to end up on trunk, either originally, or via a
>> cherrypick merge from the branch.
>
> r1310655 on trunk. With shims activated: 339 failures now :-)
Do the failures happen on trunk or the branch? (I suspect the branch
as well as the trunk, knowing you. :P )
> I only added relpath assertions. I did not write the code to check for
> improper parent/child addition order. I'll do that in a second commit.
>
>> I plan on fixing the various ### comments in copy.c on the branch, as
>> those are issues I've identified with the current code. It's
>> unfortunate that we don't have tests which test these cases. :(
>
> Sounds like some new tests are going to come along :-P
Thanks for volunteering. :)
-Hyrum
--
uberSVN: Apache Subversion Made Easy
http://www.uberSVN.com/
Re: svn commit: r1310581 - /subversion/branches/ev2-export/subversion/libsvn_client/copy.c
Posted by Greg Stein <gs...@gmail.com>.
On Fri, Apr 6, 2012 at 17:36, Hyrum K Wright <hy...@wandisco.com> wrote:
> On Fri, Apr 6, 2012 at 4:31 PM, Greg Stein <gs...@gmail.com> wrote:
>> On Fri, Apr 6, 2012 at 16:53, Hyrum K Wright <hy...@wandisco.com> wrote:
>>>...
>>> Just so you know, I would have expected this section to crash Ev2 in
>>> copy test 67, since there are several directories being added, but the
>>> children array is (currently) empty. I wonder if there is a problem
>>> with the internal Ev2 state checking.
>>
>> I'm suspicious that the checks are not working because of the "pass
>> URLs as relpath" that you're doing.
>>
>> I should add some canonical assertions for all the relpath params, and
>> get you to fix that bug :-)
>
> Go for it! You're welcome to use the branch, but it'd probably be
> best for these checks to end up on trunk, either originally, or via a
> cherrypick merge from the branch.
r1310655 on trunk. With shims activated: 339 failures now :-)
I only added relpath assertions. I did not write the code to check for
improper parent/child addition order. I'll do that in a second commit.
> I plan on fixing the various ### comments in copy.c on the branch, as
> those are issues I've identified with the current code. It's
> unfortunate that we don't have tests which test these cases. :(
Sounds like some new tests are going to come along :-P
Cheers,
-g
Re: svn commit: r1310581 - /subversion/branches/ev2-export/subversion/libsvn_client/copy.c
Posted by Hyrum K Wright <hy...@wandisco.com>.
On Fri, Apr 6, 2012 at 4:31 PM, Greg Stein <gs...@gmail.com> wrote:
> On Fri, Apr 6, 2012 at 16:53, Hyrum K Wright <hy...@wandisco.com> wrote:
>> On Fri, Apr 6, 2012 at 3:48 PM, <hw...@apache.org> wrote:
>>> Author: hwright
>>> Date: Fri Apr 6 20:48:32 2012
>>> New Revision: 1310581
>>>
>>> URL: http://svn.apache.org/viewvc?rev=1310581&view=rev
>>> Log:
>>> On the ev2-export branch:
>>> Directly add any new directories, rather than through the path_info structs.
>>>
>>> * subversion/libsvn_client/copy.c
>>> (path_driver_info_t): Remove dir_add member.
>>> (drive_single_path): Don't check for the dir_add flag.
>>> (drive_editor): Handle the new dirs before the various copy/move paths.
>>> (repos_to_repos_copy): Don't bother putting the new directoris into the
>>> set of paths to be operated on, just give them to drive_editor().
>>>
>>> Modified:
>>> subversion/branches/ev2-export/subversion/libsvn_client/copy.c
>>>
>>> Modified: subversion/branches/ev2-export/subversion/libsvn_client/copy.c
>>> URL: http://svn.apache.org/viewvc/subversion/branches/ev2-export/subversion/libsvn_client/copy.c?rev=1310581&r1=1310580&r2=1310581&view=diff
>>> ==============================================================================
>>> --- subversion/branches/ev2-export/subversion/libsvn_client/copy.c (original)
>>> +++ subversion/branches/ev2-export/subversion/libsvn_client/copy.c Fri Apr 6 20:48:32 2012
>>> @@ -532,7 +532,6 @@ typedef struct path_driver_info_t
>>> svn_node_kind_t src_kind;
>>> svn_revnum_t src_revnum;
>>> svn_boolean_t resurrection;
>>> - svn_boolean_t dir_add;
>>> svn_string_t *mergeinfo; /* the new mergeinfo for the target */
>>> } path_driver_info_t;
>>>
>>> @@ -627,19 +626,9 @@ drive_single_path(svn_editor_t *editor,
>>> apr_pool_t *scratch_pool)
>>> {
>>> apr_hash_t *props = apr_hash_make(scratch_pool);
>>> - apr_array_header_t *children = apr_array_make(scratch_pool, 1,
>>> - sizeof(const char *));
>>> svn_boolean_t do_delete = FALSE;
>>> svn_boolean_t do_add = FALSE;
>>>
>>> - /* Check to see if we need to add the path as a directory. */
>>> - if (path_info->dir_add)
>>> - {
>>> - return svn_error_trace(svn_editor_add_directory(editor, path, children,
>>> - props,
>>> - SVN_INVALID_REVNUM));
>>> - }
>>> -
>>> /* If this is a resurrection, we know the source and dest paths are
>>> the same, and that our driver will only be calling us once. */
>>> if (path_info->resurrection)
>>> @@ -702,6 +691,7 @@ static svn_error_t *
>>> drive_editor(svn_editor_t *editor,
>>> apr_array_header_t *paths,
>>> apr_hash_t *action_hash,
>>> + apr_array_header_t *new_dirs,
>>> svn_boolean_t is_move,
>>> svn_revnum_t youngest,
>>> apr_pool_t *scratch_pool)
>>> @@ -710,6 +700,23 @@ drive_editor(svn_editor_t *editor,
>>> apr_pool_t *iterpool;
>>> int i;
>>>
>>> + if (new_dirs)
>>> + {
>>> + apr_array_header_t *children = apr_array_make(scratch_pool, 1,
>>> + sizeof(const char *));
>>> + apr_hash_t *props = apr_hash_make(scratch_pool);
>>> +
>>> + /* These are directories which we just need to add. */
>>> + for (i = 0; i < new_dirs->nelts; i++)
>>> + {
>>> + const char *dir = APR_ARRAY_IDX(new_dirs, i, const char *);
>>> +
>>> + /* ### The children param needs to be populated correctly. */
>>> + SVN_ERR(svn_editor_add_directory(editor, dir, children, props,
>>> + SVN_INVALID_REVNUM));
>>
>> Greg,
>>
>> Just so you know, I would have expected this section to crash Ev2 in
>> copy test 67, since there are several directories being added, but the
>> children array is (currently) empty. I wonder if there is a problem
>> with the internal Ev2 state checking.
>
> I'm suspicious that the checks are not working because of the "pass
> URLs as relpath" that you're doing.
>
> I should add some canonical assertions for all the relpath params, and
> get you to fix that bug :-)
Go for it! You're welcome to use the branch, but it'd probably be
best for these checks to end up on trunk, either originally, or via a
cherrypick merge from the branch.
I plan on fixing the various ### comments in copy.c on the branch, as
those are issues I've identified with the current code. It's
unfortunate that we don't have tests which test these cases. :(
-Hyrum
--
uberSVN: Apache Subversion Made Easy
http://www.uberSVN.com/
Re: svn commit: r1310581 - /subversion/branches/ev2-export/subversion/libsvn_client/copy.c
Posted by Greg Stein <gs...@gmail.com>.
On Fri, Apr 6, 2012 at 16:53, Hyrum K Wright <hy...@wandisco.com> wrote:
> On Fri, Apr 6, 2012 at 3:48 PM, <hw...@apache.org> wrote:
>> Author: hwright
>> Date: Fri Apr 6 20:48:32 2012
>> New Revision: 1310581
>>
>> URL: http://svn.apache.org/viewvc?rev=1310581&view=rev
>> Log:
>> On the ev2-export branch:
>> Directly add any new directories, rather than through the path_info structs.
>>
>> * subversion/libsvn_client/copy.c
>> (path_driver_info_t): Remove dir_add member.
>> (drive_single_path): Don't check for the dir_add flag.
>> (drive_editor): Handle the new dirs before the various copy/move paths.
>> (repos_to_repos_copy): Don't bother putting the new directoris into the
>> set of paths to be operated on, just give them to drive_editor().
>>
>> Modified:
>> subversion/branches/ev2-export/subversion/libsvn_client/copy.c
>>
>> Modified: subversion/branches/ev2-export/subversion/libsvn_client/copy.c
>> URL: http://svn.apache.org/viewvc/subversion/branches/ev2-export/subversion/libsvn_client/copy.c?rev=1310581&r1=1310580&r2=1310581&view=diff
>> ==============================================================================
>> --- subversion/branches/ev2-export/subversion/libsvn_client/copy.c (original)
>> +++ subversion/branches/ev2-export/subversion/libsvn_client/copy.c Fri Apr 6 20:48:32 2012
>> @@ -532,7 +532,6 @@ typedef struct path_driver_info_t
>> svn_node_kind_t src_kind;
>> svn_revnum_t src_revnum;
>> svn_boolean_t resurrection;
>> - svn_boolean_t dir_add;
>> svn_string_t *mergeinfo; /* the new mergeinfo for the target */
>> } path_driver_info_t;
>>
>> @@ -627,19 +626,9 @@ drive_single_path(svn_editor_t *editor,
>> apr_pool_t *scratch_pool)
>> {
>> apr_hash_t *props = apr_hash_make(scratch_pool);
>> - apr_array_header_t *children = apr_array_make(scratch_pool, 1,
>> - sizeof(const char *));
>> svn_boolean_t do_delete = FALSE;
>> svn_boolean_t do_add = FALSE;
>>
>> - /* Check to see if we need to add the path as a directory. */
>> - if (path_info->dir_add)
>> - {
>> - return svn_error_trace(svn_editor_add_directory(editor, path, children,
>> - props,
>> - SVN_INVALID_REVNUM));
>> - }
>> -
>> /* If this is a resurrection, we know the source and dest paths are
>> the same, and that our driver will only be calling us once. */
>> if (path_info->resurrection)
>> @@ -702,6 +691,7 @@ static svn_error_t *
>> drive_editor(svn_editor_t *editor,
>> apr_array_header_t *paths,
>> apr_hash_t *action_hash,
>> + apr_array_header_t *new_dirs,
>> svn_boolean_t is_move,
>> svn_revnum_t youngest,
>> apr_pool_t *scratch_pool)
>> @@ -710,6 +700,23 @@ drive_editor(svn_editor_t *editor,
>> apr_pool_t *iterpool;
>> int i;
>>
>> + if (new_dirs)
>> + {
>> + apr_array_header_t *children = apr_array_make(scratch_pool, 1,
>> + sizeof(const char *));
>> + apr_hash_t *props = apr_hash_make(scratch_pool);
>> +
>> + /* These are directories which we just need to add. */
>> + for (i = 0; i < new_dirs->nelts; i++)
>> + {
>> + const char *dir = APR_ARRAY_IDX(new_dirs, i, const char *);
>> +
>> + /* ### The children param needs to be populated correctly. */
>> + SVN_ERR(svn_editor_add_directory(editor, dir, children, props,
>> + SVN_INVALID_REVNUM));
>
> Greg,
>
> Just so you know, I would have expected this section to crash Ev2 in
> copy test 67, since there are several directories being added, but the
> children array is (currently) empty. I wonder if there is a problem
> with the internal Ev2 state checking.
I'm suspicious that the checks are not working because of the "pass
URLs as relpath" that you're doing.
I should add some canonical assertions for all the relpath params, and
get you to fix that bug :-)
> Also, this currently feeds paths to Ev2 child-first. For instance,
> copy test 67 calls this with 'X/Y/Z', 'X/Y', and 'X', in that order.
> The shims reorder things so that it gets done properly, but I wonder
> if there shouldn't be some state checks in Ev2 to ensure parents are
> added before children.
That wouldn't be too hard. We could mark the parent as "only allowed
to modify" (as long as it hasn't already been marked as
"added/unalterable").
Cheers,
-g