You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Stefan Sperling <st...@elego.de> on 2008/11/14 11:53:49 UTC
Re: svn commit: r34186 - in trunk/subversion: libsvn_client
libsvn_wc tests/cmdline
On Thu, Nov 13, 2008 at 10:10:16PM -0800, neels@tigris.org wrote:
> Author: neels
> Date: Thu Nov 13 22:10:16 2008
> New Revision: 34186
>
> Log:
> Fix some behaviour around "resolved" and tree-conflicts.
>
> Take stsp's patch and fix it: do not try to read entries from the parent
> directory of a working copy root that was supplied as a target.
Thanks for fixing, there was quite a things I had missed (e.g.
checking whether 'entry' is really valid before passing it to
a function.
> Also fix missing resolving of deleted and committed files that became
> tree-conflict victims during a merge.
>
> * subversion/libsvn_client/resolved.c (svn_client_resolve),
> * subversion/libsvn_wc/adm_ops.c (resolve_found_entry_callback):
> Ensure we do not end up trying to find tree conflict info in the
> parent directory of the working copy root.
>
> * subversion/libsvn_wc/entries.c
> (visit_tc_too_found_entry): Also visit tree-conflicts of deleted items.
>
> * subversion/tests/cmdline/resolved_tests.py
> (resolved_on_wc_root, resolved_on_deleted_item):
> New tests file with two new tests for the issues fixed by this commit.
Yay, tests! Should have thought of adding a test for that myself...
One more question:
> Modified: trunk/subversion/libsvn_client/resolved.c
> URL: http://svn.collab.net/viewvc/svn/trunk/subversion/libsvn_client/resolved.c?pathrev=34186&r1=34185&r2=34186
> ==============================================================================
> --- trunk/subversion/libsvn_client/resolved.c Thu Nov 13 15:03:26 2008 (r34185)
> +++ trunk/subversion/libsvn_client/resolved.c Thu Nov 13 22:10:16 2008 (r34186)
> @@ -53,19 +53,59 @@ svn_client_resolve(const char *path,
> {
> svn_wc_adm_access_t *adm_access;
> int adm_lock_level = SVN_WC__LEVELS_TO_LOCK_FROM_DEPTH(depth);
> + svn_boolean_t wc_root;
> + const svn_wc_entry_t *entry;
>
> - /* In order to resolve tree-conflicts on the target PATH, we need an
> - * adm_access on its parent directory. The lock level then needs to extend
> - * at least onto the immediate children. */
> - if (adm_lock_level >= 0)
> - adm_lock_level++;
> SVN_ERR(svn_wc_adm_probe_open3(&adm_access, NULL,
> - svn_path_dirname(path, pool),
> + path,
> TRUE,
> adm_lock_level,
> ctx->cancel_func, ctx->cancel_baton,
> pool));
>
> + /* Make sure we do not end up looking for tree conflict info
> + * above the working copy root. */
> + SVN_ERR(svn_wc_is_wc_root(&wc_root, path, adm_access, pool));
> + if (wc_root)
> + {
> + /* Switched subtrees are considered working copy roots by
> + * svn_wc_is_wc_root(). But it's OK to check for tree conflict
> + * info in the parent of a switched subtree, because the
> + * subtree itself might be a tree conflict victim. */
> + SVN_ERR(svn_wc_entry(&entry, path, adm_access, TRUE, pool));
> +
> + if (entry->kind == svn_node_dir)
> + {
> + svn_error_t *err;
> + svn_boolean_t switched;
> +
> + err = svn_wc__path_switched(path, &switched, entry, pool);
> +
> + if (err && (err->apr_err == SVN_ERR_ENTRY_MISSING_URL))
> + svn_error_clear(err);
Could you add a comment explaining what condition you are catching
by ignoring this error? In both places -- there's another instance of
this, see below. I guess it might have to do with unversioned
directories in the working copy, but I'm not sure if my guess is correct.
> + else
> + {
> + SVN_ERR(err);
> + wc_root = switched ? FALSE : TRUE;
> + }
> + }
> + }
> +
> + if (! wc_root) /* but possibly a switched subdir */
> + {
> + /* In order to resolve tree-conflicts on the target PATH, we need an
> + * adm_access on its parent directory. The lock level then needs to
> + * extend at least onto the immediate children. */
> + SVN_ERR(svn_wc_adm_close2(adm_access, pool));
> + if (adm_lock_level >= 0)
> + adm_lock_level++;
> + SVN_ERR(svn_wc_adm_probe_open3(&adm_access, NULL,
> + svn_path_dirname(path, pool),
> + TRUE, adm_lock_level,
> + ctx->cancel_func, ctx->cancel_baton,
> + pool));
> + }
> +
> SVN_ERR(svn_wc_resolved_conflict4(path, adm_access, TRUE, TRUE, TRUE,
> depth, conflict_choice,
> ctx->notify_func2, ctx->notify_baton2,
>
> Modified: trunk/subversion/libsvn_wc/adm_ops.c
> URL: http://svn.collab.net/viewvc/svn/trunk/subversion/libsvn_wc/adm_ops.c?pathrev=34186&r1=34185&r2=34186
> ==============================================================================
> --- trunk/subversion/libsvn_wc/adm_ops.c Thu Nov 13 15:03:26 2008 (r34185)
> +++ trunk/subversion/libsvn_wc/adm_ops.c Thu Nov 13 22:10:16 2008 (r34186)
> @@ -2871,6 +2871,7 @@ resolve_found_entry_callback(const char
> {
> struct resolve_callback_baton *baton = walk_baton;
> svn_boolean_t resolved = FALSE;
> + svn_boolean_t wc_root = FALSE;
>
> /* We're going to receive dirents twice; we want to ignore the
> first one (where it's a child of a parent dir), and only process
> @@ -2880,8 +2881,35 @@ resolve_found_entry_callback(const char
> return SVN_NO_ERROR;
>
>
> - /* If asked to, clear any tree conflict on the path. */
> - if (baton->resolve_tree)
> + /* Make sure we do not end up looking for tree conflict
> + * info above the working copy root. */
> +
> + if (entry && (entry->kind == svn_node_dir))
> + {
> + SVN_ERR(svn_wc_is_wc_root(&wc_root, path, baton->adm_access, pool));
> +
> + if (wc_root)
> + {
> + /* Switched subtrees are considered working copy roots by
> + * svn_wc_is_wc_root(). But it's OK to check for tree conflict
> + * info in the parent of a switched subtree, because the
> + * subtree itself might be a tree conflict victim. */
> + svn_boolean_t switched;
> + svn_error_t *err;
> + err = svn_wc__path_switched(path, &switched, entry, pool);
> + if (err && (err->apr_err == SVN_ERR_ENTRY_MISSING_URL))
> + svn_error_clear(err);
.... here.
Thanks,
Stefan
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Re: svn commit: r34186 - in trunk/subversion:
libsvn_client libsvn_wc tests/cmdline
Posted by Stefan Sperling <st...@elego.de>.
On Thu, Nov 20, 2008 at 04:53:51AM +0100, Neels J. Hofmeyr wrote:
> Julian Foad wrote:
> >
> > We need to document that "svn_wc_is_wc_root()" doesn't do what we were
> > thinking it would do. Done in r34211.
> >
> > We need to abstract out this chunk of code, give it a name and a
> > description. svn_wc_is_really_really_wc_root().
>
> I've got a patch here but would like it reviewed. Should the new function be
> public API?
>
> ~Neels
> Move some code around svn_wc_is_wc_root() in resolved.c out into a separate
> function, for code readability.
>
> Because it is an extension of svn_wc_is_wc_root() and might be useful
> elsewhere, add it next to the definition of svn_wc_is_wc_root() and
> publish it in the private headers, as svn_wc__strictly_is_wc_root().
>
> * subversion/include/private/svn_wc_private.h
> (svn_wc__strictly_is_wc_root): New function declaration.
>
> * subversion/libsvn_client/resolved.c
> (svn_client_resolve):
> Move some code out to svn_wc__strictly_is_wc_root().
>
> * subversion/libsvn_wc/update_editor.c
> (svn_wc__strictly_is_wc_root):
> New function from code that was in svn_client_resolve().
>
> * subversion/include/svn_wc.h
> (svn_wc_is_wc_root): Comment.
>
>
> --This line, and those below, will be ignored--
No they won't be ignored, they'll be reviewed :P
> Index: subversion/include/private/svn_wc_private.h
> ===================================================================
> --- subversion/include/private/svn_wc_private.h (revision 34275)
> +++ subversion/include/private/svn_wc_private.h (working copy)
> @@ -218,6 +218,16 @@ svn_wc__read_tree_conflicts(apr_array_he
> const char *dir_path,
> apr_pool_t *pool);
>
> +/** Like svn_wc_is_wc_root(), but it doesn't consider switched subdirs or
> + * deleted entries as working copy roots.
> + *
> + * @since New in 1.6.*/
> +svn_error_t *
> +svn_wc__strictly_is_wc_root(svn_boolean_t *wc_root,
> + const char *path,
> + svn_wc_adm_access_t *adm_access,
> + apr_pool_t *pool);
> +
> #ifdef __cplusplus
> }
> #endif /* __cplusplus */
> Index: subversion/include/svn_wc.h
> ===================================================================
> --- subversion/include/svn_wc.h (revision 34275)
> +++ subversion/include/svn_wc.h (working copy)
> @@ -3769,7 +3769,8 @@ svn_wc_crawl_revisions(const char *path,
> * @c FALSE otherwise. Here, @a path is a "working copy root" if its parent
> * directory is not a WC or if its parent directory's repository URL is not
> * the parent of its own repository URL. Thus, a switched subtree is
> - * considered to be a working copy root.
> + * considered to be a working copy root. Also, a deleted tree-conflict
> + * victim is considered a "working copy root" because it has no URL.
Oh oh oh oh... doesn't this nicely show that the current concept of
"working copy root" is absolutely ill-defined?
I'd say wc-ng will make this problem disappear, because it will
save meta data in either the working copy root only, or a centralised
meta data store. So for now I think it's OK to simply document the fact
that this function is broken.
Patch looks good.
Stefan
> *
> * If @a path is not found, return the error @c SVN_ERR_ENTRY_NOT_FOUND.
> *
> Index: subversion/libsvn_client/resolved.c
> ===================================================================
> --- subversion/libsvn_client/resolved.c (revision 34276)
> +++ subversion/libsvn_client/resolved.c (working copy)
> @@ -43,7 +43,6 @@ svn_client_resolve(const char *path,
> svn_wc_adm_access_t *adm_access;
> int adm_lock_level = SVN_WC__LEVELS_TO_LOCK_FROM_DEPTH(depth);
> svn_boolean_t wc_root;
> - const svn_wc_entry_t *entry;
>
> SVN_ERR(svn_wc_adm_probe_open3(&adm_access, NULL,
> path,
> @@ -53,45 +52,10 @@ svn_client_resolve(const char *path,
> pool));
>
> /* Make sure we do not end up looking for tree conflict info
> - * above the working copy root. */
> - SVN_ERR(svn_wc_is_wc_root(&wc_root, path, adm_access, pool));
> - if (wc_root)
> - {
> - /* Check whether this is a switched subtree or an absent item.
> - * Switched subtrees are considered working copy roots by
> - * svn_wc_is_wc_root(). But it's OK to check for tree conflict
> - * info in the parent of a switched subtree, because the
> - * subtree itself might be a tree conflict victim. */
> - SVN_ERR(svn_wc_entry(&entry, path, adm_access, TRUE, pool));
> -
> - /* If this has no entry, it can't possibly be a switched subdir.
> - * It can't be a WC root either, for that matter.*/
> - if (entry == NULL)
> - wc_root = FALSE;
> - else
> - if (entry->kind == svn_node_dir)
> - {
> - svn_error_t *err;
> - svn_boolean_t switched;
> -
> - err = svn_wc__path_switched(path, &switched, entry, pool);
> -
> - if (err && (err->apr_err == SVN_ERR_ENTRY_MISSING_URL))
> - {
> - /* This is e.g. a locally deleted dir. It has an entry but
> - * no repository URL. It cannot be a WC root. */
> - svn_error_clear(err);
> - wc_root = FALSE;
> - }
> - else
> - {
> - SVN_ERR(err);
> - /* The query for a switched dir succeeded. If switched,
> - * don't consider this a WC root. */
> - wc_root = switched ? FALSE : TRUE;
> - }
> - }
> - }
> + * above the working copy root. It's OK to check for tree conflict
> + * info in the parent of a *switched* subtree, because the
> + * subtree itself might be a tree conflict victim. */
> + SVN_ERR(svn_wc__strictly_is_wc_root(&wc_root, path, adm_access, pool));
>
> if (! wc_root) /* but possibly a switched subdir */
> {
> Index: subversion/libsvn_wc/update_editor.c
> ===================================================================
> --- subversion/libsvn_wc/update_editor.c (revision 34275)
> +++ subversion/libsvn_wc/update_editor.c (working copy)
> @@ -4603,6 +4603,56 @@ svn_wc_is_wc_root(svn_boolean_t *wc_root
> }
>
>
> +svn_error_t*
> +svn_wc__strictly_is_wc_root(svn_boolean_t *wc_root,
> + const char *path,
> + svn_wc_adm_access_t *adm_access,
> + apr_pool_t *pool)
> +{
> + SVN_ERR(svn_wc_is_wc_root(wc_root, path, adm_access, pool));
> +
> + if (*wc_root)
> + {
> + const svn_wc_entry_t *entry;
> +
> + /* Check whether this is a switched subtree or an absent item.
> + * Switched subtrees are considered working copy roots by
> + * svn_wc_is_wc_root(). */
> + SVN_ERR(svn_wc_entry(&entry, path, adm_access, TRUE, pool));
> +
> + /* If this has no entry, it can't possibly be a switched subdir.
> + * It can't be a WC root either, for that matter.*/
> + if (entry == NULL)
> + *wc_root = FALSE;
> + else
> + if (entry->kind == svn_node_dir)
> + {
> + svn_error_t *err;
> + svn_boolean_t switched;
> +
> + err = svn_wc__path_switched(path, &switched, entry, pool);
> +
> + if (err && (err->apr_err == SVN_ERR_ENTRY_MISSING_URL))
> + {
> + /* This is e.g. a locally deleted dir. It has an entry but
> + * no repository URL. It cannot be a WC root. */
> + svn_error_clear(err);
> + *wc_root = FALSE;
> + }
> + else
> + {
> + SVN_ERR(err);
> + /* The query for a switched dir succeeded. If switched,
> + * don't consider this a WC root. */
> + *wc_root = switched ? FALSE : TRUE;
> + }
> + }
> + }
> +
> + return SVN_NO_ERROR;
> +}
> +
> +
> svn_error_t *
> svn_wc_get_actual_target(const char *path,
> const char **anchor,
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
[PATCH] Re: svn commit: r34186 - in trunk/subversion: libsvn_client
libsvn_wc tests/cmdline
Posted by "Neels J. Hofmeyr" <ne...@elego.de>.
Julian Foad wrote:
>
> We need to document that "svn_wc_is_wc_root()" doesn't do what we were
> thinking it would do. Done in r34211.
>
> We need to abstract out this chunk of code, give it a name and a
> description. svn_wc_is_really_really_wc_root().
I've got a patch here but would like it reviewed. Should the new function be
public API?
~Neels
Re: svn commit: r34186 - in trunk/subversion: libsvn_client
libsvn_wc tests/cmdline
Posted by Julian Foad <ju...@btopenworld.com>.
On Fri, 2008-11-14 at 11:53 +0000, Stefan Sperling wrote:
> On Thu, Nov 13, 2008 at 10:10:16PM -0800, neels@tigris.org wrote:
> > Modified: trunk/subversion/libsvn_client/resolved.c
> > URL: http://svn.collab.net/viewvc/svn/trunk/subversion/libsvn_client/resolved.c?pathrev=34186&r1=34185&r2=34186
> > ==============================================================================
[...]
> > + /* Make sure we do not end up looking for tree conflict info
> > + * above the working copy root. */
> > + SVN_ERR(svn_wc_is_wc_root(&wc_root, path, adm_access, pool));
> > + if (wc_root)
> > + {
> > + /* Switched subtrees are considered working copy roots by
> > + * svn_wc_is_wc_root(). But it's OK to check for tree conflict
> > + * info in the parent of a switched subtree, because the
> > + * subtree itself might be a tree conflict victim. */
> > + SVN_ERR(svn_wc_entry(&entry, path, adm_access, TRUE, pool));
> > +
> > + if (entry->kind == svn_node_dir)
> > + {
> > + svn_error_t *err;
> > + svn_boolean_t switched;
> > +
> > + err = svn_wc__path_switched(path, &switched, entry, pool);
> > +
> > + if (err && (err->apr_err == SVN_ERR_ENTRY_MISSING_URL))
> > + svn_error_clear(err);
>
> Could you add a comment explaining what condition you are catching
> by ignoring this error? In both places -- there's another instance of
> this, see below. I guess it might have to do with unversioned
> directories in the working copy, but I'm not sure if my guess is correct.
>
> > + else
> > + {
> > + SVN_ERR(err);
> > + wc_root = switched ? FALSE : TRUE;
> > + }
> > + }
> > Modified: trunk/subversion/libsvn_wc/adm_ops.c
> > URL: http://svn.collab.net/viewvc/svn/trunk/subversion/libsvn_wc/adm_ops.c?pathrev=34186&r1=34185&r2=34186
> > ==============================================================================
[...]
> > + /* Make sure we do not end up looking for tree conflict
> > + * info above the working copy root. */
> > +
> > + if (entry && (entry->kind == svn_node_dir))
> > + {
> > + SVN_ERR(svn_wc_is_wc_root(&wc_root, path, baton->adm_access, pool));
> > +
> > + if (wc_root)
> > + {
> > + /* Switched subtrees are considered working copy roots by
> > + * svn_wc_is_wc_root(). But it's OK to check for tree conflict
> > + * info in the parent of a switched subtree, because the
> > + * subtree itself might be a tree conflict victim. */
> > + svn_boolean_t switched;
> > + svn_error_t *err;
> > + err = svn_wc__path_switched(path, &switched, entry, pool);
> > + if (err && (err->apr_err == SVN_ERR_ENTRY_MISSING_URL))
> > + svn_error_clear(err);
>
> .... here.
We need to document that "svn_wc_is_wc_root()" doesn't do what we were
thinking it would do. Done in r34211.
We need to abstract out this chunk of code, give it a name and a
description. svn_wc_is_really_really_wc_root().
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org