On 8/18/06, Paul Burba <paulb@softlanding.com> wrote:
> "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.
Ok.
>
> > 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().
Ok.
[snip]
>
> > 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.
>
Ok, I think better to factor out this code in separated patch.
> Thanks, let me know if you see any other problems.
There are some small glitches, see my comments inside.
Index: subversion/libsvn_wc/copy.c
===================================================================
--- subversion/libsvn_wc/copy.c (revision 21115)
+++ subversion/libsvn_wc/copy.c (working copy)
@@ -39,6 +39,242 @@
/*** Code. ***/
+/* Helper function for svn_wc_copy2() which handles WC->WC copying of
+ files which are scheduled for addition or unversioned.
+
+ Copy SRC_PATH to DST_PATH.
+
+ DST_PATH is a non-existant path, but it's parent must exist
+ and be in the same WC as SRC_PATH.
+
+ If SRC_IS_ADDED is true then SRC_PATH is scheduled for addition and
+ DST_PATH will also be scheduled for addition.
+
+ If SRC_IS_ADDED is false then SRC_PATH is the unversioned child
+ file of a versioned or added parent and DST_PATH is simply copied.
+
+ DST_PARENT_ACCESS is a 0 depth locked access for the
+ versioned parent dir of DST_PATH.
+
+ Use POOL for all necessary allocations.
+*/
+static svn_error_t *
+copy_added_file_administratively(const char *src_path,
+ const char *dst_path,
+ svn_boolean_t src_is_added,
+ svn_wc_adm_access_t *dst_parent_access,
+ svn_cancel_func_t cancel_func,
+ void *cancel_baton,
+ svn_wc_notify_func2_t notify_func,
+ void *notify_baton,
+ apr_pool_t *pool)
+{
+ char *src_parent = svn_path_dirname(src_path, pool);
Btw, what do you think about split dst_path parameter to dst_parent
and dst_basename in parameters in
copy_added_file_administratively/copy_added_dir_administratively?
Because you are joining it when calling function and further splitting
it function body. It also will be more coordinated with
copy_file_administratively/copy_dir_administratively.
+
+ /* Copy this file and possibly put it under version control. */
+ SVN_ERR(svn_io_copy_file(src_path, dst_path, TRUE, pool));
+
Trailing spaces.
+ if (src_is_added)
+ {
+ SVN_ERR(svn_wc_add2(dst_path, dst_parent_access, NULL,
+ SVN_INVALID_REVNUM, cancel_func,
+ cancel_baton, notify_func,
+ notify_baton, pool));
+ }
+
+ return SVN_NO_ERROR;
+}
+
+
+/* Helper function for svn_wc_copy2() which handles WC->WC copying of
+ directories which are scheduled for addition or unversioned.
+
+ Copy SRC_PATH to DST_PATH.
+
+ DST_PATH is a non-existant path, but it's parent must exist
+ and be in the same WC as SRC_PATH.
+
+ If SRC_IS_ADDED is true then SRC_PATH is scheduled for addition and
+ DST_PATH will also be scheduled for addition.
+
+ If SRC_IS_ADDED is false then SRC_PATH is the unversioned child
+ directory of a versioned or added parent and DST_PATH is simply copied.
+
+ DST_PARENT_ACCESS is a 0 depth locked access for the
+ versioned parent dir of DST_PATH.
+
+ Use POOL for all necessary allocations.
+*/
+static svn_error_t *
+copy_added_dir_administratively(const char *src_path,
+ const char *dst_path,
+ svn_boolean_t src_is_added,
+ svn_wc_adm_access_t *dst_parent_access,
+ svn_cancel_func_t cancel_func,
+ void *cancel_baton,
+ svn_wc_notify_func2_t notify_func,
+ void *notify_baton,
+ apr_pool_t *pool)
+{
+ if (! src_is_added)
+ {
+ /* src_path is the top of an unversioned tree, just copy
+ the whole thing and we are done. */
+ SVN_ERR(svn_io_copy_dir_recursively(src_path,
+ svn_path_dirname(dst_path, pool),
+ svn_path_basename(dst_path, pool),
+ TRUE, cancel_func, cancel_baton,
+ pool));
+ }
+ else
+ {
+ char *src_parent, *dst_parent;
+ const svn_wc_entry_t *entry;
+ svn_wc_adm_access_t *src_access;
+ svn_wc_adm_access_t *dst_child_dir_access;
+ svn_wc_adm_access_t *src_child_dir_access;
+ apr_dir_t *dir;
+ apr_finfo_t this_entry;
+ svn_error_t *err;
+ apr_pool_t *subpool;
+ apr_int32_t flags = APR_FINFO_TYPE | APR_FINFO_NAME;
+
+ /* Check cancellation; note that this catches recursive calls too. */
+ if (cancel_func)
+ SVN_ERR(cancel_func(cancel_baton));
+
+ src_parent = svn_path_dirname(src_path, pool);
+ dst_parent = svn_path_dirname(dst_path, pool);
+
+ /* Get a svn_wc_adm_access_t for src_path. */
+ if (strcmp(src_parent, dst_parent) == 0)
+ {
+ src_access = dst_parent_access;
+ }
+ else
+ {
+ SVN_ERR(svn_wc_adm_open3(&src_access, NULL, src_parent, FALSE, 0,
+ cancel_func, cancel_baton,pool));
+ }
+
+ /* "Copy" the dir dst_path and schedule it, and possibly
+ it's children, for addition. */
+ SVN_ERR(svn_io_dir_make(dst_path, APR_OS_DEFAULT, pool));
+
Trailing spaces. (I hate this stuff too :)
+ SVN_ERR(svn_wc_adm_open3(&src_child_dir_access, NULL, src_path, FALSE,
+ 0, cancel_func, cancel_baton, pool));
+
+ /* Add the directory, adding locking access for dst_path
+ to dst_parent_access at the same time. */
+ SVN_ERR(svn_wc_add2(dst_path, dst_parent_access, NULL,
+ SVN_INVALID_REVNUM, cancel_func, cancel_baton,
+ notify_func, notify_baton, pool));
+
+ /* Get the access for the newly added dir, we'll need it if we
+ recurse or call copy_added_file_administratively(). */
+ SVN_ERR(svn_wc_adm_retrieve(&dst_child_dir_access, dst_parent_access,
+ dst_path, pool));
Formatting glitch.
+
+ /* Create a subpool for iterative memory control. */
+ subpool = svn_pool_create(pool);
+
+ /* Read src_path's entries one by one. */
+ SVN_ERR(svn_io_dir_open(&dir, src_path, pool));
+ for (err = svn_io_dir_read(&this_entry, flags, dir, subpool);
+ err == SVN_NO_ERROR;
+ err = svn_io_dir_read(&this_entry, flags, dir, subpool))
+ {
+ const char *src_fullpath, *dst_fullpath;
+
+ /* Skip entries for this dir and its parent. */
+ if (this_entry.name[0] == '.'
+ && (this_entry.name[1] == '\0'
+ || (this_entry.name[1] == '.'
+ && this_entry.name[2] == '\0')))
+ continue;
+
+ /* Check cancellation so you can cancel during an
+ * add of a directory with lots of files. */
+ if (cancel_func)
+ SVN_ERR(cancel_func(cancel_baton));
+
+ /* Skip over SVN admin directories. */
+ if (svn_wc_is_adm_dir(this_entry.name, subpool))
+ continue;
+
+ /* Construct the full path of the entry. */
+ src_fullpath = svn_path_join(src_path, this_entry.name, subpool);
+ dst_fullpath = svn_path_join(dst_path, this_entry.name, subpool);
+
+ SVN_ERR(svn_wc_entry(&entry, src_fullpath, src_child_dir_access,
+ TRUE, subpool));
+ src_is_added = entry ? TRUE : FALSE;
I think this should be deleted, because it's never used further.
+
+ /* Recurse on directories; add files; ignore the rest. */
+ if (this_entry.filetype == APR_DIR)
+ {
+ SVN_ERR(copy_added_dir_administratively(src_fullpath,
+ dst_fullpath,
+ entry ? TRUE : FALSE,
+ dst_child_dir_access,
+ cancel_func,
+ cancel_baton,
+ notify_func,
+ notify_baton,
+ subpool));
+ }
+ else if (this_entry.filetype != APR_UNKFILE)
+ {
+ SVN_ERR(copy_added_file_administratively(src_fullpath,
+ dst_fullpath,
+ entry ? TRUE : FALSE,
+ dst_child_dir_access,
+ cancel_func,
+ cancel_baton,
+ notify_func,
+ notify_baton,
+ subpool));
+ }
+
+ /* Clean out the per-iteration pool. */
+ svn_pool_clear(subpool);
+
+ } /* End for loop */
+
+ /* Check that the loop exited cleanly. */
+ if (! (APR_STATUS_IS_ENOENT(err->apr_err)))
+ {
+ return svn_error_createf(err->apr_err, err,
+ _("Error during recursive copy of '%s'"),
+ svn_path_local_style(src_path,
+ subpool));
+ }
+ 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));
+ }
+
+ SVN_ERR(svn_wc_adm_close(src_child_dir_access));
+
+ if (src_access != dst_parent_access)
+ {
+ SVN_ERR(svn_wc_adm_close(src_access));
+ }
+
+ } /* End else src_is_added. */
+
+ return SVN_NO_ERROR;
+}
+
+
/* Helper function for copy_file_administratively() and
copy_dir_administratively(). Determines the COPYFROM_URL and
COPYFROM_REV of a file or directory SRC_PATH which is the descendant
@@ -588,15 +824,48 @@
SVN_ERR(svn_io_check_path(src_path, &src_kind, pool));
if (src_kind == svn_node_file)
- SVN_ERR(copy_file_administratively(src_path, adm_access,
- dst_parent, dst_basename,
- notify_func, notify_baton, pool));
-
+ {
+ if (src_entry->schedule == svn_wc_schedule_add
+ && (! src_entry->copied))
+ {
+ SVN_ERR(copy_added_file_administratively(src_path,
+ svn_path_join(dst_path,
+ dst_basename,
+ pool),
+ TRUE, dst_parent,
+ cancel_func, cancel_baton,
+ notify_func, notify_baton,
+ pool));
+ }
+ else
+ {
+ SVN_ERR(copy_file_administratively(src_path, adm_access,
+ dst_parent, dst_basename,
+ notify_func, notify_baton, pool));
+ }
+ }
else if (src_kind == svn_node_dir)
- SVN_ERR(copy_dir_administratively(src_path, adm_access,
- dst_parent, dst_basename,
- cancel_func, cancel_baton,
- notify_func, notify_baton, pool));
+ {
+ if (src_entry->schedule == svn_wc_schedule_add
+ && (! src_entry->copied))
+ {
+ SVN_ERR(copy_added_dir_administratively(src_path,
+ svn_path_join(dst_path,
+ dst_basename,
+ pool),
+ TRUE, dst_parent,
+ cancel_func, cancel_baton,
+ notify_func, notify_baton,
+ pool));
+ }
+ else
+ {
+ SVN_ERR(copy_dir_administratively(src_path, adm_access,
+ dst_parent, dst_basename,
+ cancel_func, cancel_baton,
+ notify_func, notify_baton, pool));
+ }
+ }
SVN_ERR(svn_wc_adm_close(adm_access));
Index: subversion/tests/cmdline/copy_tests.py
===================================================================
--- subversion/tests/cmdline/copy_tests.py (revision 21115)
+++ subversion/tests/cmdline/copy_tests.py (working copy)
[..]
>
> 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.
> ]]]
>
>
>
--
Ivan Zhakov
Received on Sat Aug 19 09:07:51 2006