Karl Fogel <kfogel@google.com> wrote on 08/22/2006 03:08:24 AM:
> pburba@tigris.org writes:
<snip>
> > +copy_added_dir_administratively(const char *src_path,
> > + svn_boolean_t src_is_added,
> > + svn_wc_adm_access_t
*dst_parent_access,
> > + svn_wc_adm_access_t *src_access,
> > + const char *dst_basename,
> > + svn_cancel_func_t cancel_func,
> > + void *cancel_baton,
> > + svn_wc_notify_func2_t notify_func,
> > + void *notify_baton,
> > + apr_pool_t *pool)
> > +{
> > + /* The 'dst_path' is simply dst_parent/dst_basename */
> > + const char *dst_path
> > + = svn_path_join(svn_wc_adm_access_path(dst_parent_access),
> > + dst_basename, 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_wc_adm_access_path(dst_parent_access),
> > + dst_basename,
> > + TRUE, cancel_func,
cancel_baton,
> > + pool));
> > + }
> > + else
> > + {
> > + char *src_parent, *dst_parent;
> > + const svn_wc_entry_t *entry;
> > + 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);
> > +
> > + /* "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));
> > +
> > + /* 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 accesses for the newly added dir and its source,
we'll
> > + need both to process any of SRC_PATHS's children below. */
> > + SVN_ERR(svn_wc_adm_retrieve(&dst_child_dir_access,
dst_parent_access,
> > + dst_path, pool));
> > + SVN_ERR(svn_wc_adm_retrieve(&src_child_dir_access, src_access,
> > + src_path, pool));
> > +
> > + /* 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))
> > + {
>
> It might be better to use either this:
>
> while((err = svn_io_read_dir(...)) == SVN_NO_ERROR)
> {
> }
> if (err)
> { ... both cases of error-handling outside the loop ...
> }
>
> or this:
>
> while(1)
> {
> err = (svn_io_dir_read(...));
> if (err)
> { ... both cases of error-handling inside the loop ...
> }
> }
I prefer the latter, though only for purely aesthetic reasons...though I
do hate breaking out of a loop, it always feels like I'm cheating somehow.
> (Probably you were just following the example in libsvn_client/add.c,
> which I also think could be improved in the same way :-). )
I was doing just that, and I suspect the original inspiration for the code
in add.c came from http://svnbook.red-bean.com/en/1.1/ch08s05.html.
A second attached patch fixes add.c.
> Whether the error-handling is inside or outside the loop is not
> important (to me, anyway). But in the for-vs-while question, I think
> 'while' is better because it doesn't duplicate the calls and doesn't
> make the reader look twice.
'While' it is.
> > + 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));
> > +
> > + /* Recurse on directories; add files; ignore the rest. */
> > + if (this_entry.filetype == APR_DIR)
> > + {
> > + SVN_ERR(copy_added_dir_administratively(src_fullpath,
> > + entry ? TRUE :
FALSE,
> > + dst_child_dir_access,
> > + src_child_dir_access,
> > + this_entry.name,
> > + cancel_func,
> > + cancel_baton,
> > + notify_func,
> > + notify_baton,
> > + subpool));
> > + }
> > + else if (this_entry.filetype != APR_UNKFILE)
> > + {
> > + SVN_ERR(copy_added_file_administratively(src_fullpath,
> > + entry ?
> TRUE : FALSE,
> > +
> dst_child_dir_access,
> > + this_entry.name,
> > + cancel_func,
> > + cancel_baton,
> > + notify_func,
> > + notify_baton,
> > + subpool));
> > + }
> > +
> > + /* Clean out the per-iteration pool. */
> > + svn_pool_clear(subpool);
> > +
> > + } /* End for loop */
>
> Yeah, I guess we can't do the clear-pool-at-top-of-loop thing here
> because this_entry is allocated in subpool. Oh well.
We can now with the while(1) loop since this_entry is allocated with it.
> > + /* 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 copyof
'%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));
> > + }
>
> Hmm, why still use subpool here?
Mistake on my part.
> Instead, shouldn't subpool be
> destroyed immediately after the loop? (You don't need it for anything
> in the err-handling code that follows -- 'dir' is allocated in pool,
> not subpool, and err is allocated in its own pool as usual.)
Fixed that, it's now destroyed. Sorry about the sloppiness, thanks for
catching it.
Paul B.
================================================================================
[[[
Properly destroy subpool and simplify looping in r21152.
Suggested by: kfogel
* subversion/libsvn_wc/copy.c
(copy_added_dir_administratively): Use more straightforward while loop
in
place of for loop. Clear iterative subpool at start of loop and destroy
it
when done with it.
]]]
[[[
Use more readable looping structure when crawling a dir tree during adds.
Suggested by: kfogel
* subversion/libsvn_client/add.c
(add_dir_recursive): Use more straightforward while loop in place of for
loop. Clear subpool at start of loop.
]]]
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Wed Aug 23 16:10:26 2006