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.
>
> Create only one tree conflict at the root of each conflicted tree, not a
> tree conflict in every directory.
>
> Notify the user once about any given tree conflict.
>
> Tweak test expectations to reflect these new features.
>
> Add tests that run update/switch in trees that already contain tree
> conflict victims.
>
> Add tests that chdir into conflicted trees and attempt to update/switch.
>
>
> TODO:
>
> More tests! As always....
>
> The tree conflict 2_1 and 3 tests show that an incoming tree delete
> bumps the revision number inside a victim tree. I don't know how to
> solve this without leaving the parent dirs incomplete. Maybe I need to
> hack something in complete_directory().
>
> If an update targets a single file or dir that has been deleted from the
> repo, there's an error (see test 2_1). Should be a tree conflict.
> Fixing it requires moving some functions, so I'll do it in a separate
> commit for clarity.
>
>
> * subversion/libsvn_wc/update_editor.c
[...]
> (check_tree_conflict): Thin out the comments a bit. Remove useless
> checks for entry validity. Remove the redundant (delete:obstructed)
> and (delete:missing) conflicts.
> (already_in_a_tree_conflict): New function.
> (do_entry_deletion
> add_directory,
> open_directory,
> add_file,
> open_file): Skip the operation if there is an existing or new tree
> conflict. Notify the user once per tree conflict.
> (close_directory): Unset current_tree_conflict when returning to
> the victim dir. Don't notify about tree conflicts.
>
> * subversion/tests/cmdline/update_tests.py
> (update_deleted_missing_dir,
> another_hudson_problem): Don't treat a missing item as a tree
> conflict.
> (update_delete_modified_files,
> tree_conflicts_on_update_1_2,
> tree_conflicts_on_update_2_1,
> tree_conflicts_on_update_2_2,
> tree_conflicts_on_update_3): Tweak expected output and disk to
> reflect the consistent skipping.
> (tree_conflicts_on_update_2_3): New test function.
> (test_list): Add the new test function, make it XFail().
[...]
> * subversion/tests/cmdline/svntest/actions.py
> (deep_trees_run_tests_scheme for update): Allow status check to be
> optional.
> (deep_trees_skipping_on_update): 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
[...]
>
> case svn_wc_conflict_action_delete:
> - if (!entry)
> - reason = svn_wc_conflict_reason_missing;
> - else if (entry->schedule != svn_wc_schedule_normal)
[...]
> - reason = (entry->schedule == svn_wc_schedule_delete
> - ? svn_wc_conflict_reason_deleted
> - : svn_wc_conflict_reason_obstructed); /* replace, add, etc. */
> - else /* schedule is normal, but item might be modified or missing */
> + /* 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.
> -
> + /* 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 (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.)
> + 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.
> + 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. */
> + for (i = ancestors->nelts - 1; i >= 0; i--)
> + {
> + ancestor = APR_ARRAY_IDX(ancestors, i, char *);
> + SVN_ERR(svn_wc_conflicted_p2(NULL, NULL, &tree_conflicted, ancestor,
> + adm_access, pool));
> + if (tree_conflicted)
> + {
> + *victim_path = ancestor;
> +
> + return SVN_NO_ERROR;
> + }
> + }
> +
> + return SVN_NO_ERROR;
> +}
> +
>
[...]
I only skimmed lightly through the rest.
Looks good.
- Julian
---------------------------------------------------------------------
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 01:45:53 CET