Re: [PATCH]svn_wc_transmit_prop_deltas2-wc-ng

["Hyrum K. Wright" <hy...@hyrumwright.org>]

On Jul 31, 2009, at 3:37 AM, HuiHuang wrote:

> Hey Stefan,
>
> I think you are very busy these days because subversion heads up:  
> 1.6.4 on Aug. 4.
> When I try to design commit based on wc-ng I find it is a little  
> hard for me. I do not
> konw where to start when dig deep into the code. So I think it is  
> better to move
> forward little by little.
>
> I make the following patch to try to use wc_db and svn_wc_context_t.  
> I know the
> docstring may be not good. I want to know if I do the right thing?  
> If it is right, I will
> write a new patch instead.
>
> log message:
>
>    [[[
>       Add a new function svn_wc_transmit_prop_deltas2. It uses  
> svn_wc_context_t
>       instead of svn_wc_adm_access_t. And replace  
> svn_wc_transmit_prop_deltas by
>       it in some reference.
>
>       * subversion/include/svn_wc.h
>         (svn_wc_transmit_prop_deltas2): New API.
>       * subversion/libsvn_client/commit_util.c
>         (adm_access): Remove.
>         (tmp_entry): Remove.
>         (svn_wc_transmit_prop_deltas): Use  
> svn_wc_transmit_prop_deltas2 instead.
>         (cb_baton.adm_access): Remove.
>       * subversion/libsvn_wc/adm_crawler.c
>         (svn_wc_transmit_prop_deltas2): New function.
>    ]]]
>
> Index: subversion/include/svn_wc.h
> ===================================================================
> --- subversion/include/svn_wc.h	(revision 38507)
> +++ subversion/include/svn_wc.h	(working copy)
> @@ -5952,7 +5952,18 @@
>                             const char **tempfile,
>                             apr_pool_t *pool);
>
> +/** This function is the same as svn_wc_transmit_prop_deltas except  
> that it use
> + * svn_wc_context_t instead of svn_wc_adm_access_t.
> + */

New functions should have the complete description, while older ones  
are described in terms of the newer functions.  (See other examples in  
svn_wc.h.)

> +svn_error_t *
> +svn_wc_transmit_prop_deltas2(const char *path,
> +                             svn_wc_context_t *wc_ctx,
> +                             const svn_delta_editor_t *editor,
> +                             void *baton,
> +                             const char **tempfile,
> +                             apr_pool_t *pool);
>
> +
> /** Given a @a path with its accompanying @a entry, transmit all local
>  * property modifications using the appropriate @a editor method (in
>  * conjunction with @a baton). @a adm_access is an access baton set

You need to note that the old API is deprecated, using @deprecated and  
SVN_DEPRECATED.

> Index: subversion/libsvn_client/commit_util.c
> ===================================================================
> --- subversion/libsvn_client/commit_util.c	(revision 38507)
> +++ subversion/libsvn_client/commit_util.c	(working copy)
> @@ -1274,7 +1274,6 @@
>   void *file_baton = NULL;
>   const char *copyfrom_url = NULL;
>   apr_pool_t *file_pool = NULL;
> -  svn_wc_adm_access_t *adm_access = cb_baton->adm_access;
>   const svn_delta_editor_t *editor = cb_baton->editor;
>   apr_hash_t *file_mods = cb_baton->file_mods;
>   svn_client_ctx_t *ctx = cb_baton->ctx;
> @@ -1435,8 +1434,6 @@
>   /* Now handle property mods. */
>   if (item->state_flags & SVN_CLIENT_COMMIT_ITEM_PROP_MODS)
>     {
> -      const svn_wc_entry_t *tmp_entry;
> -
>       if (kind == svn_node_file)
>         {
>           if (! file_baton)
> @@ -1469,25 +1466,19 @@
>             }
>         }
>
> -      /* Ensured by harvest_committables(), item->path will never  
> be an
> -         excluded path. However, will it be deleted/absent items?   
> I think
> -         committing an modification on a deleted/absent item does  
> not make
> -         sense. So it's probably safe to turn off the show_hidden  
> flag here.*/
> -      SVN_ERR(svn_wc_entry(&tmp_entry, item->path, adm_access,  
> FALSE, pool));
> -
>       /* When committing a directory that no longer exists in the
>          repository, a "not found" error does not occur immediately
>          upon opening the directory.  It appears here during the delta
>          transmisssion. */
> -      err = svn_wc_transmit_prop_deltas
> -        (item->path, adm_access, tmp_entry, editor,
> +      err = svn_wc_transmit_prop_deltas2
> +        (item->path, ctx->wc_ctx, editor,
>          (kind == svn_node_dir) ? *dir_baton : file_baton, NULL,  
> pool);

I wouldn't change any callers initially, just have them exercise the  
old API which should be reimplemented as a wrapper around the new API.

Also, no-space-before-paren :)

>
>       if (err)
>         return fixup_out_of_date_error(path, kind, err);
>
> -      SVN_ERR(svn_wc_transmit_prop_deltas
> -              (item->path, adm_access, tmp_entry, editor,
> +      SVN_ERR(svn_wc_transmit_prop_deltas2
> +              (item->path, ctx->wc_ctx, editor,
>                (kind == svn_node_dir) ? *dir_baton : file_baton,  
> NULL, pool));

Same.

>       /* Make any additional client -> repository prop changes. */
> @@ -1610,7 +1601,6 @@
>     }
>
>   /* Setup the callback baton. */
> -  cb_baton.adm_access = adm_access;
>   cb_baton.editor = editor;
>   cb_baton.edit_baton = edit_baton;
>   cb_baton.file_mods = file_mods;
> Index: subversion/libsvn_wc/adm_crawler.c
> ===================================================================
> --- subversion/libsvn_wc/adm_crawler.c	(revision 38507)
> +++ subversion/libsvn_wc/adm_crawler.c	(working copy)
> @@ -1060,7 +1060,43 @@
>                                       fulltext, editor, file_baton,  
> pool);
> }
>
> +svn_error_t *
> +svn_wc_transmit_prop_deltas2(const char *path,

This should be an absolute path, and documented as such in the header.

> +                             svn_wc_context_t *wc_ctx,

We've been standardizing the parameter order as wc_ctx, path not path,  
wc_ctx.

> +                             const svn_delta_editor_t *editor,
> +                             void *baton,
> +                             const char **tempfile,
> +                             apr_pool_t *pool)
> +{
> +  int i;
> +  apr_array_header_t *propmods;
> +  svn_wc__db_t *db = wc_ctx->db;

Unneeded temp variable.

> +  const char *local_abspath;

Unneeded, since the caller should provide an abspath.

> +  svn_wc__db_kind_t *kind = NULL;

This shouldn't be a pointer; the check_node() API below will segfault  
if it is.

> +  SVN_ERR(svn_dirent_get_absolute(&local_abspath, path, pool));

Don't need this, since the caller should provide an abspath.

> +  SVN_ERR(svn_wc__db_check_node(kind, db, local_abspath, pool));
> +
> +  if (tempfile)
> +    *tempfile = NULL;
> +
> +  /* Get an array of local changes by comparing the hashes. */
> +  SVN_ERR(svn_wc__internal_propdiff(&propmods, NULL, db,  
> local_abspath,
> +                                    pool, pool));
> +
> +  /* Apply each local change to the baton */
> +  for (i = 0; i < propmods->nelts; i++)
> +    {
> +      const svn_prop_t *p = &APR_ARRAY_IDX(propmods, i, svn_prop_t);
> +      if (*kind == svn_wc__db_kind_file)
> +        SVN_ERR(editor->change_file_prop(baton, p->name, p->value,  
> pool));
> +      else
> +        SVN_ERR(editor->change_dir_prop(baton, p->name, p->value,  
> pool));
> +    }
> +
> +  return SVN_NO_ERROR;
> +}
> +
> svn_error_t *
> svn_wc_transmit_prop_deltas(const char *path,
>                             svn_wc_adm_access_t *adm_access,

This function should be reimplemented as wrapper around the new one,  
and moved to deprecated.c

>
> Thank you.
> Huihuang

That's all the stuff I caught on the initial pass, feel free to  
followup with questions.

-Hyrum

------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2379585