I'm on a little vendetta against svn_io_check_path() calls in
libsvn_wc/libsvn_client: we're stat()ing like crazy (still) even
though in many (most?) cases we don't actually need to do so. A common
pattern is:
svn_io_check_path()
if (kind==directory)
remove_the_dir();
else
remove_the_file();
But by issuing a file/dir removal without a stat(), we can optimize
the common case: from the result value, we can deduce it wasn't a node
to be removed with svn_io_remove_file(), so then we go on to
svn_io_remove_dir().
There's one problem: APR isn't really clear on which errors should be
expected when incorrectly removing a dir with a remove_file() call.
That's why I'm submitting the patch below for review. What it does is:
try to remove the file, if there's an error (any error) it proceeds to
try directory removal. This may lead to strange error reports if the
node at hand *was* a file, but coludn't be removed for some reason.
What about these ideas/ issues? Any objections to me committing
something like this?
bye,
Erik.
Index: subversion/libsvn_wc/adm_ops.c
===================================================================
--- subversion/libsvn_wc/adm_ops.c (revision 26538)
+++ subversion/libsvn_wc/adm_ops.c (working copy)
@@ -31,6 +31,7 @@
#include <apr_md5.h>
#include <apr_file_io.h>
#include <apr_time.h>
+#include <apr_errno.h>
#include "svn_types.h"
#include "svn_pools.h"
@@ -1004,40 +1005,29 @@
void *cancel_baton,
apr_pool_t *pool)
{
- svn_node_kind_t kind;
+ svn_error_t *err;
- SVN_ERR(svn_io_check_path(path, &kind, pool));
- switch (kind)
+ /* Optimize the common case: try to delete the file */
+ err = svn_io_remove_file(path, pool);
+ if (err)
{
- case svn_node_none:
- return svn_error_createf(SVN_ERR_BAD_FILENAME, NULL,
- _("'%s' does not exist"),
- svn_path_local_style(path, pool));
- break;
-
- default:
- /* ### TODO: what do we do here? To handle Unix symlinks we
- fallthrough to svn_node_file... gulp! */
-
- case svn_node_file:
- SVN_ERR(svn_io_remove_file(path, pool));
- break;
-
- case svn_node_dir:
- /* ### It would be more in the spirit of things to feed the
- ### cancellation check through to svn_io_remove_dir2()... */
+ /* Then maybe it was a directory? */
+ svn_error_clear(err);
if (cancel_func)
SVN_ERR(cancel_func(cancel_baton));
- SVN_ERR(svn_io_remove_dir2(path, FALSE, pool));
+ err = svn_io_remove_dir2(path, FALSE, pool);
+ }
- if (cancel_func)
- SVN_ERR(cancel_func(cancel_baton));
-
- break;
+ if (err && APR_STATUS_IS_ENOENT(err->apr_err))
+ {
+ svn_error_clear(err);
+ return svn_error_createf(SVN_ERR_BAD_FILENAME, NULL,
+ _("'%s' does not exist"),
+ svn_path_local_style(path, pool));
}
- return SVN_NO_ERROR;
+ return err;
}
/* Remove/erase PATH from the working copy. For files this involves
@@ -1063,85 +1053,77 @@
void *cancel_baton,
apr_pool_t *pool)
{
- /* Check that the item exists in the wc. */
- svn_node_kind_t wc_kind;
- SVN_ERR(svn_io_check_path(path, &wc_kind, pool));
- if (wc_kind == svn_node_none)
- return SVN_NO_ERROR;
+ svn_error_t *err;
+
if (cancel_func)
SVN_ERR(cancel_func(cancel_baton));
- switch (kind)
+ err = svn_io_remove_file(path, pool);
+ if (err && APR_STATUS_IS_ENOENT(err->apr_err))
{
- default:
- /* ### TODO: what do we do here? */
- break;
+ svn_error_clear(err);
+ return SVN_NO_ERROR;
+ }
- case svn_node_file:
- SVN_ERR(svn_io_remove_file(path, pool));
- break;
+ if (err)
+ {
+ apr_hash_t *ver, *unver;
+ apr_hash_index_t *hi;
+ svn_wc_adm_access_t *dir_access;
- case svn_node_dir:
- {
- apr_hash_t *ver, *unver;
- apr_hash_index_t *hi;
- svn_wc_adm_access_t *dir_access;
+ svn_error_clear(err);
+ /* ### Suspect that an iteration or recursion subpool would be
+ good here. */
- /* ### Suspect that an iteration or recursion subpool would be
- good here. */
+ /* First handle the versioned items, this is better (probably) than
+ simply using svn_io_get_dirents2 for everything as it avoids the
+ need to do svn_io_check_path on each versioned item */
+ SVN_ERR(svn_wc_adm_retrieve(&dir_access, adm_access, path, pool));
+ SVN_ERR(svn_wc_entries_read(&ver, dir_access, FALSE, pool));
+ for (hi = apr_hash_first(pool, ver); hi; hi = apr_hash_next(hi))
+ {
+ const void *key;
+ void *val;
+ const char *name;
+ const svn_wc_entry_t *entry;
+ const char *down_path;
- /* First handle the versioned items, this is better (probably) than
- simply using svn_io_get_dirents2 for everything as it avoids the
- need to do svn_io_check_path on each versioned item */
- SVN_ERR(svn_wc_adm_retrieve(&dir_access, adm_access, path, pool));
- SVN_ERR(svn_wc_entries_read(&ver, dir_access, FALSE, pool));
- for (hi = apr_hash_first(pool, ver); hi; hi = apr_hash_next(hi))
- {
- const void *key;
- void *val;
- const char *name;
- const svn_wc_entry_t *entry;
- const char *down_path;
+ apr_hash_this(hi, &key, NULL, &val);
+ name = key;
+ entry = val;
- apr_hash_this(hi, &key, NULL, &val);
- name = key;
- entry = val;
+ if (!strcmp(name, SVN_WC_ENTRY_THIS_DIR))
+ continue;
- if (!strcmp(name, SVN_WC_ENTRY_THIS_DIR))
- continue;
+ down_path = svn_path_join(path, name, pool);
+ SVN_ERR(erase_from_wc(down_path, dir_access, entry->kind,
+ cancel_func, cancel_baton, pool));
+ }
- down_path = svn_path_join(path, name, pool);
- SVN_ERR(erase_from_wc(down_path, dir_access, entry->kind,
- cancel_func, cancel_baton, pool));
- }
+ /* Now handle any remaining unversioned items */
+ SVN_ERR(svn_io_get_dirents2(&unver, path, pool));
+ for (hi = apr_hash_first(pool, unver); hi; hi = apr_hash_next(hi))
+ {
+ const void *key;
+ const char *name;
+ const char *down_path;
- /* Now handle any remaining unversioned items */
- SVN_ERR(svn_io_get_dirents2(&unver, path, pool));
- for (hi = apr_hash_first(pool, unver); hi; hi = apr_hash_next(hi))
- {
- const void *key;
- const char *name;
- const char *down_path;
+ apr_hash_this(hi, &key, NULL, NULL);
+ name = key;
- apr_hash_this(hi, &key, NULL, NULL);
- name = key;
+ /* The admin directory will show up, we don't want to delete it */
+ if (svn_wc_is_adm_dir(name, pool))
+ continue;
- /* The admin directory will show up, we don't want to delete it */
- if (svn_wc_is_adm_dir(name, pool))
- continue;
+ /* Versioned directories will show up, don't delete those either */
+ if (apr_hash_get(ver, name, APR_HASH_KEY_STRING))
+ continue;
- /* Versioned directories will show up, don't delete those
either */- if (apr_hash_get(ver, name,
APR_HASH_KEY_STRING))
- continue;
-
- down_path = svn_path_join(path, name, pool);
- SVN_ERR(erase_unversioned_from_wc
- (down_path, cancel_func, cancel_baton, pool));
- }
- }
- /* ### TODO: move this dir into parent's .svn area */
- break;
+ down_path = svn_path_join(path, name, pool);
+ SVN_ERR(erase_unversioned_from_wc
+ (down_path, cancel_func, cancel_baton, pool));
+ }
}
return SVN_NO_ERROR;
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Wed Sep 12 16:10:45 2007