On Aug 13, 2009, at 9:23 AM, Daniel Näslund wrote:
> Hi!
>
> [ I could not send this message through my ordinary MUA, mutt.
> My webmail does unfortunately encode all attachments as binary
> data. Sorry. ]
And we're sorry that tigris.org is giving you fits. :(
> I have run make check with three FAILs:
> external_tests.py 16: place a file external into a directory external.
> switch_tests.py 18: switch shouldn't allow changing repos root
> switch_tests.py 21: forced switch detectes tree conflicts.
>
> Reverted all changes, recompiled and the three tests still failed! Can
> anyone confirm that those tests are failing on trunk?
>
> I have replaced all occurences of svn_wc_cleanup2 with cleanup3 in
> libsvn_client. Is it bad to do two different things in one patch?
> Since
> replacing all cleanup2 means that the wrapper in deprecated.c will not
> be tested?
I tend to do it in two separate commits, since (theoretically) they
are logically separated changes, and it gives me a chance to run the
test suite using just the wrapper. (And with larger changes, the
"updating callers" patch can be quite significant and actually
introduce bugs of its own.)
>
> [[[
> Use svn_wc_context_t and absolute paths in svn_wc_cleanup3. Make
> libsvn_client use svn_wc_cleanup3. Remove diff3_cmd parameter.
>
> * subversion/include/svn_wc.h
> (svn_wc_cleanup3): New.
> (svn_wc_cleanup2): Deprecate.
>
> * subversion/libsvn_wc/deprecated.c
> (svn_wc_cleanup2): Reinplement as a wrapper.
>
> * subversion/libsvn_wc/log.c
> (svn_wc_cleanup3): New. Use absolute paths. No diff3_cmd. Add
> svn_wc_context_t parameter
> (svn_wc_cleanup2): Remove.
>
> * subversion/libsvn_client/cleanup.c
> (svn_client_cleanup): Use svn_wc_cleanup3. Remove use of diff3_cmd
> ]]]
>
> Mvh
Overall, the patch looks good. I've noted a few things that are
convention-related, and some that are more technical, but not any that
are fatal. I look forward to the updated version of this patch.
> Index: subversion/include/svn_wc.h
===================================================================
--- subversion/include/svn_wc.h (revision 38718)
+++ subversion/include/svn_wc.h (arbetskopia)
@@ -5558,12 +5558,10 @@
/**
- * Recurse from @a path, upgrading to the latest working copy format if
+ * Recurse from @a abspath, upgrading to the latest working copy
format if
* @a upgrade_wc is set, and cleaning up unfinished log business.
Perform
* any temporary allocations in @a scratch_pool. Any working copy
locks
- * under @a path will be taken over and then cleared by this
function. If
- * @a diff3_cmd is non-NULL, then use it as the diff3 command for any
- * merging; otherwise, use the built-in merge code.
+ * under @a path will be taken over and then cleared by this function.
*
* WARNING: there is no mechanism that will protect locks that are
still
* being used.
@@ -5572,9 +5570,23 @@
* various points during the operation. If it returns an error
* (typically @c SVN_ERR_CANCELLED), return that error immediately.
*
- * @since New in 1.2.
+ * @since New in 1.7.
*/
svn_error_t *
+svn_wc_cleanup3(const char *abspath,
We've been calling these "local_abspath" in general, to distinguish
from a repos_abspath.
+ svn_wc_context_t *wc_ctx,
+ svn_cancel_func_t cancel_func,
+ void *cancel_baton,
+ apr_pool_t *scratch_pool);
+
+/**
+ * Similar to svn_wc_cleanup3() but uses relative paths and creates
its own
+ * swn_wc_context_t.
Need @c in front of svn_wc_context_t.
+ *
+ * @deprecated Provided for backward compability with the 1.2 API.
Keep the @since tag here.
+ */
+SVN_DEPRECATED
+svn_error_t *
svn_wc_cleanup2(const char *path,
const char *diff3_cmd,
svn_cancel_func_t cancel_func,
Index: subversion/libsvn_wc/deprecated.c
===================================================================
--- subversion/libsvn_wc/deprecated.c (revision 38718)
+++ subversion/libsvn_wc/deprecated.c (arbetskopia)
@@ -2268,6 +2268,27 @@
/*** From log.c ***/
svn_error_t *
+svn_wc_cleanup2(const char *path,
+ const char *diff3_cmd,
+ svn_cancel_func_t cancel_func,
+ void *cancel_baton,
+ apr_pool_t *pool)
+{
+ svn_wc__db_t *db;
+ svn_wc_context_t *wc_ctx;
+ const char *abspath;
Again, local_abspath.
+
+ SVN_ERR(svn_wc__db_open(&db, svn_wc__db_openmode_readwrite,
+ NULL, pool, pool));
+
+ SVN_ERR(svn_dirent_get_absolute(&abspath, path, pool));
+
+ SVN_ERR(svn_wc__context_create_with_db(&wc_ctx, NULL, db, pool));
+
+ return svn_wc_cleanup3(abspath, wc_ctx, cancel_func, cancel_baton,
pool);
You want to manually destroy the wc_ctx here, and use svn_error_return
(see other examples in deprecated.c).
+}
+
+svn_error_t *
svn_wc_cleanup(const char *path,
svn_wc_adm_access_t *optional_adm_access,
const char *diff3_cmd,
Index: subversion/libsvn_wc/log.c
===================================================================
--- subversion/libsvn_wc/log.c (revision 38718)
+++ subversion/libsvn_wc/log.c (arbetskopia)
@@ -2388,37 +2388,31 @@
return svn_wc_adm_close2(adm_access, scratch_pool);
}
-
svn_error_t *
-svn_wc_cleanup2(const char *path,
- const char *diff3_cmd, /* ### OBSOLETE */
+svn_wc_cleanup3(const char *abspath,
+ svn_wc_context_t *wc_ctx,
svn_cancel_func_t cancel_func,
void *cancel_baton,
- apr_pool_t *scratch_pool)
+ apr_pool_t * scratch_pool)
{
- svn_wc__db_t *db;
- const char *local_abspath;
int wc_format_version;
- SVN_ERR(svn_wc__db_open(&db, svn_wc__db_openmode_readwrite,
- NULL /* ### config */, scratch_pool,
scratch_pool));
+ SVN_ERR_ASSERT(svn_dirent_is_absolute(abspath));
- SVN_ERR(svn_dirent_get_absolute(&local_abspath, path, scratch_pool));
-
- SVN_ERR(svn_wc__internal_check_wc(&wc_format_version, db,
local_abspath,
+ SVN_ERR(svn_wc__internal_check_wc(&wc_format_version, wc_ctx->db,
abspath,
scratch_pool));
/* a "version" of 0 means a non-wc directory */
if (wc_format_version == 0)
return svn_error_createf(SVN_ERR_WC_NOT_DIRECTORY, NULL,
_("'%s' is not a working copy
directory"),
- svn_dirent_local_style(path,
scratch_pool));
+ svn_dirent_local_style(abspath,
scratch_pool));
if (wc_format_version < SVN_WC__VERSION)
return svn_error_create(SVN_ERR_WC_UNSUPPORTED_FORMAT, NULL,
_("Log format too old, please use "
- "Subversion 1.6 or earlier"));
+ "Subversion 1.7 or earlier"));
This should stay as "1.6 or earlier". The reasons for this are beyond
the scope of the review, but that text is intentional. :)
- return svn_error_return(cleanup_internal(db, path, cancel_func,
cancel_baton,
- scratch_pool));
+ return svn_error_return(cleanup_internal(wc_ctx->db, abspath,
cancel_func,
+ cancel_baton,
scratch_pool));
Is cleanup_internal() fetching an absolute path internally? Can it be
simplified now that we guarantee it's getting an absolute path?
}
Index: subversion/libsvn_client/cleanup.c
===================================================================
--- subversion/libsvn_client/cleanup.c (revision 38718)
+++ subversion/libsvn_client/cleanup.c (arbetskopia)
@@ -44,17 +44,12 @@
svn_client_ctx_t *ctx,
apr_pool_t *scratch_pool)
{
- const char *diff3_cmd;
+ const char *abspath;
svn_error_t *err;
- svn_config_t *cfg = ctx->config
- ? apr_hash_get(ctx->config, SVN_CONFIG_CATEGORY_CONFIG,
- APR_HASH_KEY_STRING)
- : NULL;
- svn_config_get(cfg, &diff3_cmd, SVN_CONFIG_SECTION_HELPERS,
- SVN_CONFIG_OPTION_DIFF3_CMD, NULL);
+ SVN_ERR(svn_dirent_get_absolute(&abspath, path, scratch_pool));
- err = svn_wc_cleanup2(path, diff3_cmd, ctx->cancel_func,
+ err = svn_wc_cleanup3(abspath, ctx->wc_ctx, ctx->cancel_func,
ctx->cancel_baton, scratch_pool);
svn_io_sleep_for_timestamps(path, scratch_pool);
return svn_error_return(err);
Not a bit deal, but probably save this last bit for a separate patch.
-Hyrum
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2383319
Received on 2009-08-13 17:03:31 CEST