Re: svn commit: r1310581 - /subversion/branches/ev2-export/subversion/libsvn_client/copy.c

On Fri, Apr 6, 2012 at 4:31 PM, Greg Stein <gstein_at_gmail.com> wrote:
> On Fri, Apr 6, 2012 at 16:53, Hyrum K Wright <hyrum.wright_at_wandisco.com> wrote:
>> On Fri, Apr 6, 2012 at 3:48 PM,  <hwright_at_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/