that harvest_committables() patch

[Neels J Hofmeyr <ne...@elego.de>]

Hi Philip,

it took me quite a while and a lot of poking Greg to figure out the error.
I'm at the point where merge_tests.py 34 passes :D and I'd run a complete
make check. Only I will go to bed now and see the results tomorrow.

If you're at your keyboard earlier than me and if you like, take attached
version of the patch and make check it yourself. Maybe it should be split
into two commits...

(and make sure to send me a mail if/when you start taking over, I'll do the
same.)

Heh, grokking the really tricky ones is quite rewarding, thanks for letting
me have it :)
~Neels


I hope the attachment comes through, else find it at
http://kleinekatze.de/entry_t.20100501-0200

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

[Greg Stein <gs...@gmail.com>]

On Sat, May 1, 2010 at 22:42, Neels J Hofmeyr <ne...@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

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

[Neels J Hofmeyr <ne...@elego.de>]

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

more harvest_committables()

[Neels J Hofmeyr <ne...@elego.de>]

Hey Philip,

update on harvest_committables(): I'm busy with the patch I posted on dev@,
and I'm also planning to tackle the entry->keep_local stuff that skips the
tree-conflict checking (ll. 496). (Needs some changes elsewhere: ignorable
tree-conflicts should be removed at 'delete --keep-local' time instead of
explicitly ignoring them at commit time.)

Anything else is still up for grabs. I guess we can work island-wise and
groom away repeated _node_is_,... calls later.

~Neels

Neels J Hofmeyr wrote:
> Just to tell Philip r940102 and r940107. Still taking a look if I can move
> on a little.
> 
> ~Neels
> 
> Neels J Hofmeyr wrote:
>> Just to tell Philip that I'm pursuing this patch further now.
>> ~Neels
>>
> 

Re: that harvest_committables() patch

[Neels J Hofmeyr <ne...@elego.de>]

Just to tell Philip r940102 and r940107. Still taking a look if I can move
on a little.

~Neels

Neels J Hofmeyr wrote:
> Just to tell Philip that I'm pursuing this patch further now.
> ~Neels
> 

Re: that harvest_committables() patch

[Neels J Hofmeyr <ne...@elego.de>]

Just to tell Philip that I'm pursuing this patch further now.
~Neels