On Wed, 2008-11-12 at 11:05 +0100, Stephen Butler wrote:
> 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?
Well, yes you should, but if trying to do that turns out awkward then
I'd be happy enough with just a comment dropped in here saying so.
- Julian
> >
> >> + 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
>
---------------------------------------------------------------------
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 16:11:19 CET