Quoting Julian Foad <julianfoad_at_btopenworld.com>:
> On Tue, 2008-11-11 at 18:14 +0100, Stephen Butler wrote:
>> If anyone would like to review it, the changes to check_tree_conflict()
>> and the new function already_in_a_tree_conflict() are probably the most
>> interesting part. Along with the usage of eb->current_tree_conflict.
>
> OK, I've gone through these parts. Nothing obviously wrong, just a
> couple of questions and minor comments.
>
> ...
>
>> [[[
>>
>> Fix update/switch to skip all tree conflict victims and their
>> descendants.
>> * subversion/libsvn_wc/update_editor.c
> [...]
>> (already_in_a_tree_conflict): New function.
>> Index: subversion/libsvn_wc/update_editor.c
>> ===================================================================
>> --- subversion/libsvn_wc/update_editor.c (revision 34137)
>> +++ subversion/libsvn_wc/update_editor.c (working copy)
> [...]
>> @@ -1382,87 +1387,61 @@ check_tree_conflict(svn_wc_conflict_description_t
> [...]
>> + /* Use case 3: Deleting a locally-deleted item. */
>> + if (entry->schedule == svn_wc_schedule_delete
>> + || entry->schedule == svn_wc_schedule_replace)
>> + reason = svn_wc_conflict_reason_deleted;
>> + else
>> {
>> - svn_boolean_t modified;
>> - svn_node_kind_t kind;
>> + svn_boolean_t modified = FALSE;
>> + svn_wc_adm_access_t *adm_access;
>>
>> - SVN_ERR(svn_io_check_path(full_path, &kind, pool));
>> + /* Use case 2: Deleting a locally-modified item. */
>> + if (entry->kind == svn_node_file)
>> + SVN_ERR(entry_has_local_mods(&modified, parent_adm_access,
>> + entry->kind, full_path, pool));
>>
>> - if (kind == svn_node_none)
>> - reason = svn_wc_conflict_reason_missing;
>> - else
>> + else if (entry->kind == svn_node_dir)
>> {
>> - svn_wc_adm_access_t *adm_access;
>
> Let this "adm_access" variable stay where it was in this scope.
OK.
>
>> -
>> + /* We must detect deep modifications in a directory tree,
>> + * but the update editor will not visit the subdirectories
>> + * of a directory that it wants to delete. Therefore, we
>> + * need to start a separate crawl here. */
>> SVN_ERR(svn_wc_adm_probe_retrieve(&adm_access,
>> parent_adm_access,
>> full_path, pool));
>>
>> - SVN_ERR(tree_has_local_mods(&modified, full_path, adm_access,
>> - eb->cancel_func,
>> eb->cancel_baton,
>> - pool));
>> - if (modified)
>> - {
>> - reason = svn_wc_conflict_reason_edited;
>> - }
>> + /* Ensure that the access baton is specific to FULL_PATH,
>> + * otherwise the crawl will start at the parent. */
>> + if (strcmp(svn_wc_adm_access_path(adm_access),
>> full_path) == 0)
>> + SVN_ERR(tree_has_local_mods(&modified, full_path,
>> adm_access,
>> + eb->cancel_func,
>> eb->cancel_baton,
>> + pool));
>
> This "If (adm_access dir == full_path)" is puzzling. What does it mean?
> Given that we have full_path's entry and it's kind is 'dir', I don't see
> how adm_probe_retrieve could have failed to get an adm_access dir ==
> full_path.
If a tree-conflicted file is to be replaced by a dir, then during the
"add" phase of the replacement the file still exists (because we now
skip existing tree conflicts). The access baton "retrieve" functions
will silently take a file's parent's access baton, causing
tree_has_local_mods() to walk from the parent dir, with unexpected
results. I need to make that clear in the comment, or make the code
more obvious.
>
>> }
>> +
>> + if (modified)
>> + reason = svn_wc_conflict_reason_edited;
>> +
>> }
>> break;
>> }
>> @@ -1494,6 +1472,63 @@ check_tree_conflict(svn_wc_conflict_description_t
>> return SVN_NO_ERROR;
>> }
>>
>> +/* If PATH is inside a conflicted tree, return the tree conflict's victim
>> + * in VICTIM_PATH. Otherwise set VICTIM_PATH to NULL.
>> + *
>> + * The search begins at the working copy root, returning the first
>> + * ("highest") tree conflict victim, which may be PATH itself.
>
> The ADM_ACCESS is rather unclear. It looks like it may need to be a
> baton in a set that includes read-locks for PATH and every ancestor of
> PATH in the working copy. Or it might just work with pretty much any adm
> access baton set that includes PATH, due to the WC APIs being much more
> tolerant than their doc strings claim. I'm struggling with that myself
> and don't know the right way to approach it.
>
> As this is a private function, just knowing that it works is enough,
> though a doc comment about this is desirable. (If it were public we'd
> have to be much more careful to document it properly.)
>
>> + */
>> +static svn_error_t *
>> +already_in_a_tree_conflict(char **victim_path,
>> + const char *path,
>> + svn_wc_adm_access_t *adm_access,
>> + apr_pool_t *pool)
>> +{
>> + svn_boolean_t is_wc_root, tree_conflicted;
>> + char *ancestor, *abs_path;
>> + const svn_wc_entry_t *entry;
>> + apr_array_header_t *ancestors;
>> + int i;
>> +
>> + *victim_path = NULL;
>> + abs_path = apr_pstrdup(pool, path);
>> + ancestors = apr_array_make(pool, 0, sizeof(char *));
>> +
>> + /* Find all of the ancestor-dirs in the working copy. */
>
> (Really it finds all except the WC root itself.)
True. I'll correct the comment.
>
>> + while (! svn_path_is_empty(abs_path))
>> + {
>> + SVN_ERR(svn_wc_entry(&entry, abs_path, adm_access, FALSE, pool));
>
> e.g. Here, strictly speaking you'd have to supply a different
> adm_access each time.
Should I call svn_wc_adm_probe_try3() to try to lock each succeeding
parent dir?
>
>> + if (entry == NULL)
>> + is_wc_root = FALSE; /* svn_wc_is_root() expects an entry. */
>> + else
>> + SVN_ERR(svn_wc_is_wc_root(&is_wc_root, abs_path,
>> adm_access, pool));
>> +
>> + if (is_wc_root)
>> + break;
>> + else
>> + APR_ARRAY_PUSH(ancestors, char *) = abs_path;
>> +
>> + abs_path = svn_path_dirname(abs_path, pool);
>> + }
>> +
>> + /* From the root end, check the conflict status of each ancestor. */
Thanks for the feedback!
Steve
--
Stephen Butler | Software Developer
elego Software Solutions GmbH
Gustav-Meyer-Allee 25 | 13355 Berlin | Germany
fon: +49 30 2345 8696 | mobile: +49 163 25 45 015
fax: +49 30 2345 8695 | http://www.elegosoft.com
Geschäftsführer: Olaf Wagner | Sitz der Gesellschaft: Berlin
Amtsgericht Charlottenburg HRB 77719 | USt-IdNr: DE163214194
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe_at_subversion.tigris.org
For additional commands, e-mail: dev-help_at_subversion.tigris.org
Received on 2008-11-12 11:05:22 CET