Re: switch --relocate fails on deleted dir in mixed-rev WC

[Philip Martin <ph...@codematters.co.uk>]

Julian Foad <ju...@btopenworld.com> writes:

> -      if (recurse && (entry->kind == svn_node_dir))
> +      if (recurse && (entry->kind == svn_node_dir)
> +          && ((! entry->deleted && ! entry->absent)
> +              || (entry->schedule == svn_wc_schedule_add)
> +              || (entry->schedule == svn_wc_schedule_replace)))
>           {
>             svn_wc_adm_access_t *subdir_access;
>             const char *subdir = svn_path_join (path, key, pool);
>
> I'm rather unsure of the exact meanings and combinations of "absent"
> and "deleted" and "schedule", and am worried by other bits of code
> using different tests such as the following:

'schedule="delete"' indicates that an existing item will be deleted at
the next commit, think 'svn rm wc/foo'.

'schedule="add"' indicates that a new item will be added at the next
commit, think 'svn add wc/foo'.

'schedule="replace"' indicates both of the above, think 'svn rm wc/foo'
followed by 'svn add wc/foo'.

'deleted="true"' indicates that the entry was scheduled for deletion
and subsequently committed, but that the parent directory has not been
updated and so is at a revision that would otherwise contain the
deleted item, think 'svn rm wc/foo' followed by 'svn ci wc'

'absent="true"' indicates that mod_authz_svn doesn't allow read access
to that item so it's not physically present in the working copy.


Your change looks correct to me.

> subversion/libsvn_wc/update_editor.c:complete_directory()
>>   /* Remove any deleted or missing entries. */
>>   for (hi = apr_hash_first (pool, entries); hi; hi = apr_hash_next (hi))
>>     {
> [...]
>>       /* Any entry still marked as deleted (and not schedule add) can now
>>          be removed -- if it wasn't undeleted by the update, then it
>>          shouldn't stay in the updated working set.  Schedule add items
>>          should remain.
>>       */
>>       if (current_entry->deleted)
>>         {
>>           if (current_entry->schedule != svn_wc_schedule_add)
>>             svn_wc__entry_remove (entries, name);
>
> Can someone explain why that doesn't consider "replace"?  If it's a
> bug, can we construct a test for it?

Not a bug.  If the item is 'deleted="true"' the delete has already
been committed, so that item can't be 'schedule="delete"' as it no
longer exists, and so 'schedule="replace"' cannot happen.

That bit of code was the subject of much discussion when written and
it's still one of the hairy bits of libsvn_wc.  I think it relies on
the 'incomplete="true"' processing to make it robust in the face of an
interrupted update, but I would not like to say for certain that it
always works.

-- 
Philip Martin

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: switch --relocate fails on deleted dir in mixed-rev WC

[Julian Foad <ju...@btopenworld.com>]

Philip Martin wrote:
> 
> The code should avoid recursing if "absent" since such items can never
> be present on disk.  The code should avoid recursing if "deleted"
> unless "schedule=add".  So I think that makes it:
> 
>       if (recurse && entry->kind == svn_node_dir
>           && ! entry->absent
>           && (! entry->deleted || entry->schedule == svn_wc_schedule_add))

Committed in r16673.  Thanks, Richard, for reporting the bug, and Philip for 
reviewing.

I've proposed the fix for porting into v1.3.

- Julian

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: switch --relocate fails on deleted dir in mixed-rev WC

[Julian Foad <ju...@btopenworld.com>]

Philip Martin wrote:
> 
> "deleted" with "schedule=replace" would be a bug, it should not occur.

OK; that makes sense.

> But you are right, I was probably a bit hasty.  "absent" is often
> treated very much like "deleted" in the existing code, but as far as I
> know "absent" can't be deleted or added and so can never be
> "schedule=anything".  Your code probably has redundant checks.

Right, as does the code from which I copied it:

subversion/libsvn_wc/lock.c:prune_deleted()
   (This presently has two slightly different variants which are equivalent in
practice but unsettling to read.)

subversion/libsvn_wc/entries.c:handle_start_tag()


This looks suspect:

subversion/libsvn_wc/adm_ops.c:svn_wc_add2()
>   /* You can only add something that is not in revision control, or
>      that is slated for deletion from revision control, or has been
>      previously 'deleted', unless, of course, you're specifying an
>      addition with -history-; then it's okay for the object to be
>      under version control already; it's not really new.  */

I can't make sense of this "unless" part.  What's "the object" - the one you're
adding or one that already existed at the path?  Obviously the object you're
adding (with history) is already be under VC; but surely there must not be a VC
object already at the path?  Does it mean,

   "At this level of operation (the WC), it doesn't matter if you overwrite
    a versioned object, because you will have taken care of it at a higher
    level."

?

>   if (orig_entry)
>     {
>       if ((! copyfrom_url)
>           && (orig_entry->schedule != svn_wc_schedule_delete)
>           && (! orig_entry->deleted))
>         {
>           [error: "'%s' is already under version control"]


- Julian


---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: switch --relocate fails on deleted dir in mixed-rev WC

[Philip Martin <ph...@codematters.co.uk>]

Julian Foad <ju...@btopenworld.com> writes:

> Philip Martin wrote:
>> Julian Foad <ju...@btopenworld.com> writes:
>>
>>>-      if (recurse && (entry->kind == svn_node_dir))
>>>+      if (recurse && (entry->kind == svn_node_dir)
>>>+          && ((! entry->deleted && ! entry->absent)
>>>+              || (entry->schedule == svn_wc_schedule_add)
>>>+              || (entry->schedule == svn_wc_schedule_replace)))
>>>          {
>>>            svn_wc_adm_access_t *subdir_access;
>>>            const char *subdir = svn_path_join (path, key, pool);
>>>
>>>I'm rather unsure of the exact meanings and combinations of "absent"
>>>and "deleted" and "schedule", and am worried by other bits of code
>>>using different tests such as the following:
>> 'schedule="delete"' indicates that an existing item will be deleted
>> at
>> the next commit, think 'svn rm wc/foo'.
>> 'schedule="add"' indicates that a new item will be added at the next
>> commit, think 'svn add wc/foo'.
>> 'schedule="replace"' indicates both of the above, think 'svn rm
>> wc/foo'
>> followed by 'svn add wc/foo'.
>> 'deleted="true"' indicates that the entry was scheduled for deletion
>> and subsequently committed, but that the parent directory has not been
>> updated and so is at a revision that would otherwise contain the
>> deleted item, think 'svn rm wc/foo' followed by 'svn ci wc'
>> 'absent="true"' indicates that mod_authz_svn doesn't allow read
>> access
>> to that item so it's not physically present in the working copy.
>
> Yes, their individual meanings all sound simple enough at first
> reading, but in combination, how, for instance, do you get a directory
> "deleted" with "schedule=replace"?

"deleted" with "schedule=replace" would be a bug, it should not occur.

>> Your change looks correct to me.

But you are right, I was probably a bit hasty.  "absent" is often
treated very much like "deleted" in the existing code, but as far as I
know "absent" can't be deleted or added and so can never be
"schedule=anything".  Your code probably has redundant checks.

The code should avoid recursing if "absent" since such items can never
be present on disk.  The code should avoid recursing if "deleted"
unless "schedule=add".  So I think that makes it:

      if (recurse && entry->kind == svn_node_dir
          && ! entry->absent
          && (! entry->deleted || entry->schedule == svn_wc_schedule_add))


-- 
Philip Martin

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: switch --relocate fails on deleted dir in mixed-rev WC

[Julian Foad <ju...@btopenworld.com>]

Philip Martin wrote:
> Julian Foad <ju...@btopenworld.com> writes:
> 
>>-      if (recurse && (entry->kind == svn_node_dir))
>>+      if (recurse && (entry->kind == svn_node_dir)
>>+          && ((! entry->deleted && ! entry->absent)
>>+              || (entry->schedule == svn_wc_schedule_add)
>>+              || (entry->schedule == svn_wc_schedule_replace)))
>>          {
>>            svn_wc_adm_access_t *subdir_access;
>>            const char *subdir = svn_path_join (path, key, pool);
>>
>>I'm rather unsure of the exact meanings and combinations of "absent"
>>and "deleted" and "schedule", and am worried by other bits of code
>>using different tests such as the following:
> 
> 
> 'schedule="delete"' indicates that an existing item will be deleted at
> the next commit, think 'svn rm wc/foo'.
> 
> 'schedule="add"' indicates that a new item will be added at the next
> commit, think 'svn add wc/foo'.
> 
> 'schedule="replace"' indicates both of the above, think 'svn rm wc/foo'
> followed by 'svn add wc/foo'.
> 
> 'deleted="true"' indicates that the entry was scheduled for deletion
> and subsequently committed, but that the parent directory has not been
> updated and so is at a revision that would otherwise contain the
> deleted item, think 'svn rm wc/foo' followed by 'svn ci wc'
> 
> 'absent="true"' indicates that mod_authz_svn doesn't allow read access
> to that item so it's not physically present in the working copy.

Yes, their individual meanings all sound simple enough at first reading, but in 
combination, how, for instance, do you get a directory "deleted" with 
"schedule=replace"?


> Your change looks correct to me.

Thanks.


>>subversion/libsvn_wc/update_editor.c:complete_directory()
>>
>>>  /* Remove any deleted or missing entries. */
>>>  for (hi = apr_hash_first (pool, entries); hi; hi = apr_hash_next (hi))
>>>    {
>>[...]
>>>      /* Any entry still marked as deleted (and not schedule add) can now
>>>         be removed -- if it wasn't undeleted by the update, then it
>>>         shouldn't stay in the updated working set.  Schedule add items
>>>         should remain.
>>>      */
>>>      if (current_entry->deleted)
>>>        {
>>>          if (current_entry->schedule != svn_wc_schedule_add)
>>>            svn_wc__entry_remove (entries, name);
>>
>>Can someone explain why that doesn't consider "replace"?  If it's a
>>bug, can we construct a test for it?
> 
> Not a bug.  If the item is 'deleted="true"' the delete has already
> been committed, so that item can't be 'schedule="delete"' as it no
> longer exists, and so 'schedule="replace"' cannot happen.

I'm failing to see how this is different from the fix above.

- Julian


> That bit of code was the subject of much discussion when written and
> it's still one of the hairy bits of libsvn_wc.  I think it relies on
> the 'incomplete="true"' processing to make it robust in the face of an
> interrupted update, but I would not like to say for certain that it
> always works.

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: switch --relocate fails on deleted dir in mixed-rev WC

[Julian Foad <ju...@btopenworld.com>]

Richard Brady wrote:
> 
> Philip Martin wrote:
> 
>>'deleted="true"' indicates that the entry was scheduled for deletion
>>and subsequently committed, but that the parent directory has not been
>>updated and so is at a revision that would otherwise contain the
>>deleted item, think 'svn rm wc/foo' followed by 'svn ci wc'
> 
> But in the case of my recipe, it is the parent directory that is
> committed. To me if:

Ah, no, you misunderstand how Subversion does commits.  It only commits things 
that have changed; for a directory, a change to one of its children does NOT 
mean the directory has changed.

So although you asked Subversion to commit "wc", it scanned that sub-tree but 
only committed the node "wc/foo", not "wc" itself.  (To get "wc" itself up to 
date, you have to issue a separate "svn update" command.)

> 1. the deleted child directory is committed, THEN the parent directory
> should show 'deleted="true"' for the child.

Yes.

> or if
> 
> 2. the parent directory is committed, THEN the parent directory should
> no longer contain any record of the child.

Yes, where "committed" means "was sent to the repository, because it had 
changed in itself", not just "was considered for committing".

- Julian

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: switch --relocate fails on deleted dir in mixed-rev WC

[Richard Brady <br...@dsp.sun.ac.za>]

Philip, Julian

Philip Martin wrote:
> 'deleted="true"' indicates that the entry was scheduled for deletion
> and subsequently committed, but that the parent directory has not been
> updated and so is at a revision that would otherwise contain the
> deleted item, think 'svn rm wc/foo' followed by 'svn ci wc'

But in the case of my recipe, it is the parent directory that is
committed. To me if:

1. the deleted child directory is committed, THEN the parent directory
should show 'deleted="true"' for the child.

or if

2. the parent directory is committed, THEN the parent directory should
no longer contain any record of the child.

R


---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org