[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: more harvest_committables() -- was: that harvest_committables() patch

From: Greg Stein <gstein_at_gmail.com>
Date: Sun, 2 May 2010 10:43:53 -0400

On Sat, May 1, 2010 at 22:42, Neels J Hofmeyr <neels_at_elego.de> wrote:
> I have this patch in progress which tries to get rid of one {entry->deleted
> || (entry->schedule one of _delete, _replace)} use in harvest_committables().
>
> It seems to pass now (complete make check still running, will verify
> tomorrow), but it definitely needs polish. Even if this patch passes make
> check it's a sad twisty bastard.
>
> Thoughts welcome!
>
> ~Neels
>
>
>
> * subversion/include/private/svn_wc_private.h
>  (svn_wc__node_is_status_deleted):

Just fix this function to check for obstructed_delete, too. I've
mentioned this before, but have forgotten to fix it. Do not introduce
a new function.

>
> * subversion/libsvn_client/commit_util.c
>  (harvest_committables):
>
> * subversion/libsvn_wc/node.c
>  (svn_wc__node_is_replaced):

Do not change this function. It was designed/written to *specifically*
mean svn_wc_schedule_replace. It should not be changed.

>
> * subversion/tests/cmdline/copy_tests.py
>  (mixed_wc_to_url):
>
> --This line, and those below, will be ignored--
> conf: # dub:/home/neels/pat/.pat-base/config-default
> conf: http://archive.apache.org/dist/apr/apr-1.3.9.tar.bz2
> conf: http://archive.apache.org/dist/apr/apr-util-1.3.9.tar.bz2
> conf: http://www.sqlite.org/sqlite-3.6.22.tar.gz
> conf: http://www.webdav.org/neon/neon-0.29.3.tar.gz
> conf: fsfs
> conf: local
> Index: subversion/include/private/svn_wc_private.h
> ===================================================================
> --- subversion/include/private/svn_wc_private.h (revision 940121)
> +++ subversion/include/private/svn_wc_private.h (working copy)
> @@ -414,6 +414,19 @@ svn_wc__node_is_status_deleted(svn_boole
>                                apr_pool_t *scratch_pool);
>
>  /**
> + * Set @a *is_obstructed_delete to TRUE if @a local_abspath is
> + * "obstructed_deleted", using @a wc_ctx.  If @a local_abspath is not in the
> + * working copy, return @c SVN_ERR_WC_PATH_NOT_FOUND.  Use @a scratch_pool for
> + * all temporary allocations.
> + */
> +svn_error_t *
> +svn_wc__node_is_status_deleted_or_obstructed_del(
> +  svn_boolean_t *is_obstructed_delete,
> +  svn_wc_context_t *wc_ctx,
> +  const char *local_abspath,
> +  apr_pool_t *scratch_pool);
> +
> +/**
>  * Set @a *is_obstructed to whether @a local_abspath is obstructed, using
>  * @a wc_ctx.  If @a local_abspath is not in the working copy, return
>  * @c SVN_ERR_WC_PATH_NOT_FOUND.  Use @a scratch_pool for all temporary
> Index: subversion/libsvn_client/commit_util.c
> ===================================================================
> --- subversion/libsvn_client/commit_util.c      (revision 940121)
> +++ subversion/libsvn_client/commit_util.c      (working copy)
> @@ -493,6 +493,11 @@ harvest_committables(apr_hash_t *committ
>
>   /* If the entry is deleted with "--keep-local", we can skip checking
>      for conflicts inside. */
> +  /* ### Since wc-ng won't store whether a node was deleted with --keep-local
> +     ### or not, a 'svn delete --keep-local' should instead clear the
> +     ### tree-conflicts found in the targets. This code here should simply
> +     ### always check for tree-conflicts and assume ignorable conflicts to be
> +     ### gone already. */
>   if (entry->keep_local)
>     SVN_ERR_ASSERT(entry->schedule == svn_wc_schedule_delete);
>   else
> @@ -519,12 +524,23 @@ harvest_committables(apr_hash_t *committ
>      - The entry is scheduled for deletion or replacement, which case
>        we need to send a delete either way.
>   */
> -  if ((! adds_only)
> -      && ((entry->deleted && entry->schedule == svn_wc_schedule_normal)
> -          || (entry->schedule == svn_wc_schedule_delete)
> -          || (entry->schedule == svn_wc_schedule_replace)))
> +  if (! adds_only)
>     {
> -      state_flags |= SVN_CLIENT_COMMIT_ITEM_DELETE;
> +      svn_boolean_t is_replaced;
> +      svn_boolean_t is_status_deleted;
> +      svn_boolean_t is_present;
> +
> +      /* ### There is room for optimization here, but let's keep it plain
> +       * while this function is in flux. */
> +      SVN_ERR(svn_wc__node_is_status_deleted_or_obstructed_del(
> +                &is_status_deleted, ctx->wc_ctx, local_abspath,
> +                scratch_pool));
> +      SVN_ERR(svn_wc__node_is_replaced(&is_replaced, ctx->wc_ctx,
> +                                       local_abspath, scratch_pool));
> +      SVN_ERR(svn_wc__node_is_status_present(&is_present, ctx->wc_ctx,
> +                                             local_abspath, scratch_pool));
> +      if (is_status_deleted || is_replaced || ! is_present)
> +        state_flags |= SVN_CLIENT_COMMIT_ITEM_DELETE;
>     }
>
>   /* Check for the trivial addition case.  Adds can be explicit
> Index: subversion/libsvn_wc/node.c
> ===================================================================
> --- subversion/libsvn_wc/node.c (revision 940121)
> +++ subversion/libsvn_wc/node.c (working copy)
> @@ -675,6 +675,28 @@ svn_wc__node_is_status_deleted(svn_boole
>  }
>
>  svn_error_t *
> +svn_wc__node_is_status_deleted_or_obstructed_del(svn_boolean_t *is_deleted,
> +                                                 svn_wc_context_t *wc_ctx,
> +                                                 const char *local_abspath,
> +                                                 apr_pool_t *scratch_pool)
> +{
> +  svn_wc__db_status_t status;
> +
> +  SVN_ERR(svn_wc__db_read_info(&status,
> +                               NULL, NULL, NULL, NULL, NULL, NULL, NULL,
> +                               NULL, NULL, NULL, NULL, NULL, NULL, NULL,
> +                               NULL, NULL, NULL, NULL, NULL, NULL, NULL,
> +                               NULL, NULL,
> +                               wc_ctx->db, local_abspath,
> +                               scratch_pool, scratch_pool));
> +
> +  *is_deleted = (status == svn_wc__db_status_deleted) ||
> +                (status == svn_wc__db_status_obstructed_delete);
> +
> +  return SVN_NO_ERROR;
> +}
> +
> +svn_error_t *
>  svn_wc__node_is_status_obstructed(svn_boolean_t *is_obstructed,
>                                   svn_wc_context_t *wc_ctx,
>                                   const char *local_abspath,
> @@ -803,9 +825,35 @@ svn_wc__node_is_replaced(svn_boolean_t *
>                          const char *local_abspath,
>                          apr_pool_t *scratch_pool)
>  {
> -  return svn_error_return(svn_wc__internal_is_replaced(replaced, wc_ctx->db,
> -                                                       local_abspath,
> -                                                       scratch_pool));
> +  SVN_ERR(svn_wc__internal_is_replaced(replaced, wc_ctx->db, local_abspath,
> +                                       scratch_pool));
> +
> +  if (*replaced)
> +    {
> +      /* ### Catch a mixed-rev copy that replaces. The mixed-rev children are
> +       * each regarded as op-roots of the replace and result in currently
> +       * unexpected behaviour. Model wc-1 behaviour via
> +       * svn_wc__node_get_copyfrom_info(). */
> +      svn_wc__db_status_t status;
> +
> +      SVN_ERR(svn_wc__db_scan_addition(&status, NULL, NULL, NULL,
> +                                       NULL, NULL, NULL, NULL, NULL,
> +                                       wc_ctx->db, local_abspath,
> +                                       scratch_pool, scratch_pool));
> +
> +      if (status == svn_wc__db_status_copied
> +          || status == svn_wc__db_status_moved_here)
> +        {
> +          svn_boolean_t is_copy_target;
> +
> +          SVN_ERR(svn_wc__node_get_copyfrom_info(NULL, NULL, &is_copy_target,
> +                                                 wc_ctx, local_abspath,
> +                                                 scratch_pool, scratch_pool));
> +          if (! is_copy_target)
> +            *replaced = FALSE;
> +        }
> +    }

Instead of this, have the commit function fetch the copyfrom/target
data. That stuff will be null if the node is not a copy.

>...

I also found a bug in your recent change to nde_get_copyfrom_info. You
should get the parent's status, ensure it is status_added (it can't be
obstructed_add, or the child info could not have been fetched!), and
then use scan_addition() to get the copyfrom data (IF
parent_original_abspath is NULL).

Basically, the requested node could have copyfrom info supporting a
mixed-rev copy, the parent could have NULL info (inherit), and the
grandparent could have copyfrom info.

Cheers,
-g
Received on 2010-05-02 16:44:22 CEST

This is an archived mail posted to the Subversion Dev mailing list.