"Ivan Zhakov" <chemodax@gmail.com> wrote on 08/17/2006 05:40:01 PM:
> On 8/18/06, Paul Burba <paulb@softlanding.com> wrote:
> > Take a look if you have some time, thanks,
> Good work, Paul. Your patch looks good, but I have some thoughts:
>
> 1. Did you try split copy_added_path_administratively to
> copy_added_file_administratively/copy_added_dir_administratively()?
No...
> Because copy_added_path_administratively() have separated
> implementation for each case. I think it will be reasonable to split
> it.
...but it's easy enough to do so.
> 2. Might good idea to add src_is_versioned parameter to
> copy_added_path_administratively (or
> copy_added_file_administratively/copy_added_dir_administratively),
> because caller always know this and there is no reason to duplicate
> stat calls.
In addition to splitting the function I added the new parameter as you
suggested. I renamed it from src_is_versioned to src_is_added, since we
are really dealing with src_paths that are either scheduled for addition
or unversioned. My original name implies that scheduled for addition ==
versioned.
> 3. There is duplication of code at:
> + else if (this_entry.filetype != APR_UNKFILE)
> + {
> + /* Copy the file and add it to version control if the
> + source of the copy is versioned. */
> + SVN_ERR(svn_io_copy_file(src_fullpath, dst_fullpath,
TRUE,
> + subpool));
> This code is similiar to what is done at top of
> copy_added_path_administratively. I think if you implement (1) and (2)
> you can just call copy_added_file_administratively here.
In splitting the function I eliminated the code at the top, but in the new
function copy_added_dir_administratively() we still need to check the type
and versioned/add status of each *child* of src_path before recursing or
calling copy_added_file_administratively().
> 4. Also at the same place:
> + else if (this_entry.filetype != APR_UNKFILE)
> + {
> + /* Copy the file and add it to version control if the
> + source of the copy is versioned. */
> + SVN_ERR(svn_io_copy_file(src_fullpath, dst_fullpath,
TRUE,
> + subpool));
> + SVN_ERR(svn_wc_entry(&entry,
> + svn_path_join(src_path,
> + this_entry.name,
> + pool),
> + src_child_dir_access, TRUE,
pool));
> I didn't see reasons to construct full source path again, because you
> have it in variable src_fullpath.
A mistake on my part, it's not necessary. Fixed.
> 5.
> + else /* Yes, it exited cleanly, so close the dir. */
> + {
> + apr_status_t apr_err;
> +
> + svn_error_clear(err);
> + apr_err = apr_dir_close(dir);
> + if (apr_err)
> + return svn_error_wrap_apr(apr_err,
> + _("Can't close directory
'%s'"),
> + svn_path_local_style(src_path,
> + subpool));
> + }
> Btw, why we have not svn_io_dir_close() ??
I'm not sure why we don't have it, probably just due to a lack of need
outside of io.c. The only call to svn_io_dir_open() outside of io.c is in
add.c. I can fix that once this patch is in.
Thanks, let me know if you see any other problems.
Paul B.
[[[
Support copy and move of paths scheduled for addition.
Follow-up to r20811.
Suggested by: zhakov
* subversion/libsvn_wc/copy.c
(copy_added_file_administratively, copy_added_dir_administratively):
New recursive helper functions for copying added paths and unversioned
children within added directories.
(svn_wc_copy2): Use new helper functions when copying added paths.
* subversion/tests/cmdline/copy_tests.py
(copy_move_added_paths, copy_added_paths_to_URL,): New tests.
(test_list): Run new tests.
]]]
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Fri Aug 18 21:32:19 2006