Login | Register
My pages Projects Community openCollabNet

Discussions > dev [DISABLED] > Re: [PATCH] Use context_t for svn_wc_cleanup3

subversion
Discussion topic

Back to topic list

Re: [PATCH] Use context_t for svn_wc_cleanup3

Author hwright
Full name Hyrum K. Wright
Date 2009-08-13 08:03:17 PDT
Message 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_cl​ient/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_o​pen(&db, svn_wc__db_openmode_readwrite,
+ NULL, pool, pool));
+
+ SVN_ERR(svn_dirent_g​et_absolute(&abs​path, path, pool));
+
+ SVN_ERR(svn_wc__cont​ext_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_o​pen(&db, svn_wc__db_openmode_readwrite,
- NULL /* ### config */, scratch_pool,
scratch_pool));
+ SVN_ERR_ASSERT(svn_d​irent_is_absolute(ab​spath));

- SVN_ERR(svn_dirent_g​et_absolute(&loc​al_abspath, path, scratch_pool));
-
- SVN_ERR(svn_wc__inte​rnal_check_wc(&w​c_format_version, db,
local_abspath,
+ SVN_ERR(svn_wc__inte​rnal_check_wc(&w​c_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(SV​N_ERR_WC_NOT_DIRECTO​RY, 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(cle​anup_internal(db, path, cancel_func,
cancel_baton,
- scratch_pool));
+ return svn_error_return(cle​anup_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_cl​ient/cleanup.c
====================​====================​====================​=======
--- subversion/libsvn_cl​ient/cleanup.c (revision 38718)
+++ subversion/libsvn_cl​ient/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_g​et_absolute(&abs​path, 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_tim​estamps(path, scratch_pool);
    return svn_error_return(err);

Not a bit deal, but probably save this last bit for a separate patch.

-Hyrum

« Previous message in topic | 4 of 7 | Next message in topic »

Messages

Show all messages in topic

[PATCH] Use context_t for svn_wc_cleanup3 Daniel Näslund <daniel at longitudo dot com> Daniel Näslund <daniel at longitudo dot com> 2009-08-13 07:23:12 PDT
     Re: [PATCH] Use context_t for svn_wc_cleanup3 julianfoad Julian Foad 2009-08-13 07:46:23 PDT
         Re: [PATCH] Use context_t for svn_wc_cleanup3 Paul Burba <ptburba at gmail dot com> Paul Burba <ptburba at gmail dot com> 2009-08-13 07:58:54 PDT
     Re: [PATCH] Use context_t for svn_wc_cleanup3 hwright Hyrum K. Wright 2009-08-13 08:03:17 PDT
         [PATCH v2] Use context_t in svn_wc_cleanup3 Daniel Näslund <daniel at longitudo dot com> Daniel Näslund <daniel at longitudo dot com> 2009-08-14 02:14:18 PDT
             New log message Daniel Näslund <daniel at longitudo dot com> Daniel Näslund <daniel at longitudo dot com> 2009-08-14 02:30:44 PDT
             Re: [PATCH v2] Use context_t in svn_wc_cleanup3 hwright Hyrum K. Wright 2009-08-14 08:54:30 PDT
Messages per page: