Paul Burba <paulb@softlanding.com> writes:
>> 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.
Ooh, good point!
> [[[
> 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.
> ]]]
*nod*
Reviewing each patch in turn, below... (though it would have been fine
if you'd committed them too, of course).
> Index: subversion/libsvn_client/add.c
> ===================================================================
> --- subversion/libsvn_client/add.c (revision 21223)
> +++ subversion/libsvn_client/add.c (working copy)
> @@ -322,12 +322,40 @@
> /* Read the directory entries one by one and add those things to
> revision control. */
> SVN_ERR(svn_io_dir_open(&dir, dirname, 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))
> +
> + while (1)
> {
> const char *fullpath;
>
> + /* Clean out the per-iteration pool. */
> + svn_pool_clear(subpool);
> +
> + err = svn_io_dir_read(&this_entry, flags, dir, subpool);
> +
> + if (err)
> + {
> + /* 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 add of '%s'"),
> + svn_path_local_style(dirname, 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(dirname, subpool));
> + }
> + break;
> + }
Yup. Might reverse the test order so that the first test is for the
positive case (i.e., remove the "!", reverse the bodies, and nix the
else's comment). The first time I read it, I got thrown by the
failure case being first, I think because the success case has a
longer body, which isn't typical. But it might just be my overeager
eye looking too hard for clues.
> /* Skip entries for this dir and its parent. */
> if (this_entry.name[0] == '.'
> && (this_entry.name[1] == '\0'
> @@ -364,31 +392,8 @@
> else if (err)
> return err;
> }
> -
> - /* Clean out the per-iteration pool. */
> - svn_pool_clear(subpool);
> }
>
> - /* 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 add of '%s'"),
> - svn_path_local_style(dirname, 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(dirname, subpool));
> - }
Looks good.
> /* Opened by svn_wc_add */
> SVN_ERR(svn_wc_adm_close(dir_access));
>
>
> Index: subversion/libsvn_wc/copy.c
> ===================================================================
> --- subversion/libsvn_wc/copy.c (revision 21223)
> +++ subversion/libsvn_wc/copy.c (working copy)
> @@ -161,17 +161,48 @@
> SVN_ERR(svn_wc_adm_retrieve(&src_child_dir_access, src_access,
> src_path, pool));
>
> + /* Read src_path's entries one by one. */
> + SVN_ERR(svn_io_dir_open(&dir, 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))
> + while (1)
> {
> const char *src_fullpath;
>
> + /* Clean out the per-iteration pool. */
> + svn_pool_clear(subpool);
> +
> + err = svn_io_dir_read(&this_entry, flags, dir, subpool);
> +
> + if (err)
> + {
> + /* 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));
> + }
> + break;
> + }
> +
> /* Skip entries for this dir and its parent. */
> if (this_entry.name[0] == '.'
> && (this_entry.name[1] == '\0'
> @@ -221,32 +252,10 @@
> subpool));
> }
>
> - /* Clean out the per-iteration pool. */
> - svn_pool_clear(subpool);
> + } /* End while(1) loop */
>
> - } /* End for loop */
> + svn_pool_destroy(subpool);
>
> - /* 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));
> - }
> -
> } /* End else src_is_added. */
>
> return SVN_NO_ERROR;
Looks good too.
I think it would be fine to commit them together, since they're
conceptually doing the same thing. Only one is a followup to r21152,
but you can just place that note accordingly in the log message.
Thanks for the quick response!
-Karl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Fri Aug 25 01:09:45 2006