You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Daniel Rall <dl...@collab.net> on 2006/09/22 19:21:47 UTC
[merge tracking] Merge test #17
On Fri, 22 Sep 2006, Daniel Rall wrote:
...
> I traced the problem to the following call stack:
>
> svn_wc_adm_retrieve (libsvn_wc/lock.c:962)
> walker_helper (libsvn_wc/entries.c)
> svn_wc_walk_entries2 (libsvn_wc/entries.c)
> svn_client__get_prop_from_wc (libsvn_client/prop_commands.c)
> parse_merge_info (libsvn_client/diff.c)
> get_wc_target_merge_info (libsvn_client/diff.c)
>
> The problem is that a WC directory *underneath* the directory which is
> the target of the merge is missing from the WC. As
> svn_wc_walk_entries2() is strolling through the WC to retrieve the
> value for the "svn:mergeinfo" property from each sub-path, it attempts
> to access the .svn area of a directory which doesn't exist:
>
> From walker_helper():
>
> if (current_entry->kind == svn_node_dir)
> {
> svn_wc_adm_access_t *entry_access;
> SVN_ERR(svn_wc_adm_retrieve(&entry_access, adm_access, entrypath,
> subpool));
> From svn_wc_adm_retrieve():
>
> if (kind == svn_node_none)
> return svn_error_createf(SVN_ERR_WC_NOT_LOCKED, NULL,
> _("Directory '%s' is missing"),
> svn_path_local_style(path, pool));
Incidentally, this produces the test failure:
subversion/libsvn_wc/lock.c:962: (apr_err=155005)
svn: Directory 'svn-test-work/working_copies/merge_tests-17/A/B/F/Q' is missing
> walker_helper()'s attempt to retrieve the svn_wc_adm_access_t for the
> sub-directory without a svn_io_check_path() call might be a reasonable
> optimization. However, because of the generic SVN_ERR_WC_NOT_LOCKED
> error thrown by svn_wc_adm_retrieve(), there doesn't seem to be way
> for walker_helper() to determine that the directory is missing (at
> least, not without subsequently resorting to the path check). David
> James and I were thinking that this scenario should result in an error
> with a different code (e.g. SVN_ERR_WC_PATH_MISSING or something),
> which could be detected further up the call stack.
>
> While this seems like a fairly reasonable approach, I'm not clear what
> other implications it might have for walker_helper(). Any advice
> welcome...
The following patch is an example of how to get merge test #17
working. However, as it changes the core of the WC walker, so likely
isn't an appropriate generic behavior.
On IRC, we discussed some other possible solutions, including:
o Change svn_wc_walk_entries2() to take a flag indicating whether it
should ignore entries for directories which are missing on disk.
o Add an additional callback hook to the svn_wc_entry_callbacks_t
structure for handling errors. This hook could either be generic, and
used for pretty much every error encountered under the call stack of
svn_wc_walk_entries2(), or be specific to missing paths.
Either of these approaches would probably require a new error code for
the code snippet pasted above (from lock.c). Thoughts?
Index: subversion/libsvn_wc/entries.c
===================================================================
--- subversion/libsvn_wc/entries.c (revision 21605)
+++ subversion/libsvn_wc/entries.c (working copy)
@@ -2764,12 +2764,23 @@
if (current_entry->kind == svn_node_dir)
{
svn_wc_adm_access_t *entry_access;
- SVN_ERR(svn_wc_adm_retrieve(&entry_access, adm_access, entrypath,
- subpool));
- SVN_ERR(walker_helper(entrypath, entry_access,
- walk_callbacks, walk_baton,
- show_hidden, cancel_func, cancel_baton,
- subpool));
+ svn_error_t *err = svn_wc_adm_retrieve(&entry_access, adm_access,
+ entrypath, subpool);
+ if (err)
+ {
+ /* Supress errors from missing sub-diretories. */
+ svn_node_kind_t entry_kind;
+ SVN_ERR(svn_io_check_path(entrypath, &entry_kind, pool));
+ if (entry_kind != svn_node_none)
+ return err;
+ }
+ else
+ {
+ SVN_ERR(walker_helper(entrypath, entry_access,
+ walk_callbacks, walk_baton,
+ show_hidden, cancel_func, cancel_baton,
+ subpool));
+ }
}
svn_pool_clear(subpool);
Re: [trunk/merge tracking] Merge test #17
Posted by Daniel Rall <dl...@collab.net>.
On Mon, 25 Sep 2006, Garrett Rooney wrote:
> On 9/25/06, Daniel Rall <dl...@collab.net> wrote:
> >On Mon, 25 Sep 2006, Garrett Rooney wrote:
> >
> >> On 9/25/06, Daniel Rall <dl...@collab.net> wrote:
> >>
> >> >Does this seem unreasonable to anyone?
> >> >
> >> >Index: subversion/include/svn_error.h
> >> >===================================================================
> >> >--- subversion/include/svn_error.h (revision 21630)
> >> >+++ subversion/include/svn_error.h (working copy)
> >> >@@ -148,6 +148,13 @@
> >> > */
> >> > void svn_error_compose(svn_error_t *chain, svn_error_t *new_err);
> >> >
> >> >+/** Return whether @a apr_err exists as a cause of an error in @a
> >> >+ * err's chain (e.g. it or its children).
> >> >+ *
> >> >+ * @since New in 1.5.
> >> >+ */
> >> >+svn_boolean_t svn_error_chain_contains(svn_error_t *err, apr_status_t
> >> >apr_err);
> >>
> >> I'd rather see something like
> >>
> >> svn_boolean_t svn_error_root_cause_is(svn_error_t *err,
> >> apr_status_t apr_err);
> >>
> >> It seems like it more closely matches what you're really looking for.
> >
> >That's what I originally called it, but the implementation would be
> >slightly different: you'd walk down the error chain until you hit the
> >last error in the chain, then compare against its code against your
> >APR_ERR parameter.
> >
> >In practice, either implementations would work for the merge-tracking
> >branch -- the svn_wc_adm_retrieve() API is documented to return only
> >the SVN_ERR_WC_NOT_LOCKED error. The "real" error is somewhere
> >underneath that. Whether or not SVN_ERR_WC_PATH_NOT_FOUND needs to be
> >the final root cause for svn_client__get_prop_from_wc() to squelch the
> >error and keep on chugging is a matter of opinion.
> >
> >Maybe we want both? Any other opinions on which version I should use
> >on the merge-tracking branch?
>
> I can see situations where both would be meaningful questions to ask.
We'll start with an svn_error_root_cause_is(), then (r21645).
Re: [trunk/merge tracking] Merge test #17
Posted by Garrett Rooney <ro...@electricjellyfish.net>.
On 9/25/06, Daniel Rall <dl...@collab.net> wrote:
> On Mon, 25 Sep 2006, Garrett Rooney wrote:
>
> > On 9/25/06, Daniel Rall <dl...@collab.net> wrote:
> >
> > >Does this seem unreasonable to anyone?
> > >
> > >Index: subversion/include/svn_error.h
> > >===================================================================
> > >--- subversion/include/svn_error.h (revision 21630)
> > >+++ subversion/include/svn_error.h (working copy)
> > >@@ -148,6 +148,13 @@
> > > */
> > > void svn_error_compose(svn_error_t *chain, svn_error_t *new_err);
> > >
> > >+/** Return whether @a apr_err exists as a cause of an error in @a
> > >+ * err's chain (e.g. it or its children).
> > >+ *
> > >+ * @since New in 1.5.
> > >+ */
> > >+svn_boolean_t svn_error_chain_contains(svn_error_t *err, apr_status_t
> > >apr_err);
> >
> > I'd rather see something like
> >
> > svn_boolean_t svn_error_root_cause_is(svn_error_t *err,
> > apr_status_t apr_err);
> >
> > It seems like it more closely matches what you're really looking for.
>
> That's what I originally called it, but the implementation would be
> slightly different: you'd walk down the error chain until you hit the
> last error in the chain, then compare against its code against your
> APR_ERR parameter.
>
> In practice, either implementations would work for the merge-tracking
> branch -- the svn_wc_adm_retrieve() API is documented to return only
> the SVN_ERR_WC_NOT_LOCKED error. The "real" error is somewhere
> underneath that. Whether or not SVN_ERR_WC_PATH_NOT_FOUND needs to be
> the final root cause for svn_client__get_prop_from_wc() to squelch the
> error and keep on chugging is a matter of opinion.
>
> Maybe we want both? Any other opinions on which version I should use
> on the merge-tracking branch?
I can see situations where both would be meaningful questions to ask.
-garrett
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [trunk/merge tracking] Merge test #17
Posted by Daniel Rall <dl...@collab.net>.
On Mon, 25 Sep 2006, Garrett Rooney wrote:
> On 9/25/06, Daniel Rall <dl...@collab.net> wrote:
>
> >Does this seem unreasonable to anyone?
> >
> >Index: subversion/include/svn_error.h
> >===================================================================
> >--- subversion/include/svn_error.h (revision 21630)
> >+++ subversion/include/svn_error.h (working copy)
> >@@ -148,6 +148,13 @@
> > */
> > void svn_error_compose(svn_error_t *chain, svn_error_t *new_err);
> >
> >+/** Return whether @a apr_err exists as a cause of an error in @a
> >+ * err's chain (e.g. it or its children).
> >+ *
> >+ * @since New in 1.5.
> >+ */
> >+svn_boolean_t svn_error_chain_contains(svn_error_t *err, apr_status_t
> >apr_err);
>
> I'd rather see something like
>
> svn_boolean_t svn_error_root_cause_is(svn_error_t *err,
> apr_status_t apr_err);
>
> It seems like it more closely matches what you're really looking for.
That's what I originally called it, but the implementation would be
slightly different: you'd walk down the error chain until you hit the
last error in the chain, then compare against its code against your
APR_ERR parameter.
In practice, either implementations would work for the merge-tracking
branch -- the svn_wc_adm_retrieve() API is documented to return only
the SVN_ERR_WC_NOT_LOCKED error. The "real" error is somewhere
underneath that. Whether or not SVN_ERR_WC_PATH_NOT_FOUND needs to be
the final root cause for svn_client__get_prop_from_wc() to squelch the
error and keep on chugging is a matter of opinion.
Maybe we want both? Any other opinions on which version I should use
on the merge-tracking branch?
Re: [trunk/merge tracking] Merge test #17
Posted by Garrett Rooney <ro...@electricjellyfish.net>.
On 9/25/06, Daniel Rall <dl...@collab.net> wrote:
> Does this seem unreasonable to anyone?
>
> Index: subversion/include/svn_error.h
> ===================================================================
> --- subversion/include/svn_error.h (revision 21630)
> +++ subversion/include/svn_error.h (working copy)
> @@ -148,6 +148,13 @@
> */
> void svn_error_compose(svn_error_t *chain, svn_error_t *new_err);
>
> +/** Return whether @a apr_err exists as a cause of an error in @a
> + * err's chain (e.g. it or its children).
> + *
> + * @since New in 1.5.
> + */
> +svn_boolean_t svn_error_chain_contains(svn_error_t *err, apr_status_t apr_err);
I'd rather see something like
svn_boolean_t svn_error_root_cause_is(svn_error_t *err,
apr_status_t apr_err);
It seems like it more closely matches what you're really looking for.
-garrett
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [trunk/merge tracking] Merge test #17
Posted by Daniel Rall <dl...@collab.net>.
On Fri, 22 Sep 2006, Daniel Rall wrote:
...
> Add a generic error handler to routine to the WC walker API, adding
> the ability to modify or squelch errors which occur while walking the
> WC tree.
I committed this change to trunk in r21628.
...
> On the merge-tracking branch: Ignore missing paths when collecting the
> values for a property.
I ported the previous change to the branch, then committed this change
to the branch in r21629.
...
> * subversion/libsvn_client/prop_commands.c
> (wc_walker_error_handler): Add error handler to squelch ERR by
> returning SVN_NO_ERROR if ERR is casued by a missing path.
...
> I'd follow these two patches up by adding some type of error chain
> interrogation API to trunk to cleanup the FIXME in
> wc_walker_error_handler(). Alternately, we could just call
> svn_io_check_path() (along the lines of the patch I posted in the
> previous message in this thread).
Does this seem unreasonable to anyone?
Index: subversion/include/svn_error.h
===================================================================
--- subversion/include/svn_error.h (revision 21630)
+++ subversion/include/svn_error.h (working copy)
@@ -148,6 +148,13 @@
*/
void svn_error_compose(svn_error_t *chain, svn_error_t *new_err);
+/** Return whether @a apr_err exists as a cause of an error in @a
+ * err's chain (e.g. it or its children).
+ *
+ * @since New in 1.5.
+ */
+svn_boolean_t svn_error_chain_contains(svn_error_t *err, apr_status_t apr_err);
+
/** Create a new error that is a deep copy of @a err and return it.
*
* @since New in 1.2.
Index: subversion/libsvn_subr/error.c
===================================================================
--- subversion/libsvn_subr/error.c (revision 21630)
+++ subversion/libsvn_subr/error.c (working copy)
@@ -226,6 +226,20 @@
apr_pool_destroy(oldpool);
}
+svn_boolean_t
+svn_error_chain_contains(svn_error_t *err, apr_status_t apr_err)
+{
+ while (err)
+ {
+ if (err->apr_err == apr_err)
+ return TRUE;
+
+ err = err->child;
+ }
+
+ return FALSE;
+}
+
svn_error_t *
svn_error_dup(svn_error_t *err)
{
Index: subversion/libsvn_client/prop_commands.c
===================================================================
--- subversion/libsvn_client/prop_commands.c (revision 21630)
+++ subversion/libsvn_client/prop_commands.c (working copy)
@@ -25,6 +25,7 @@
#define APR_WANT_STRFUNC
#include <apr_want.h>
+#include "svn_error.h"
#include "svn_client.h"
#include "client.h"
#include "svn_path.h"
@@ -597,16 +598,9 @@
void *walk_baton,
apr_pool_t *pool)
{
- if (err)
- {
- /* Suppress errors from missing paths. */
- /* ### FIXME: Knowing where this check occurs in the error chain
- ### violates proper encapsulation. */
- if (!err->child || err->child->apr_err != SVN_ERR_WC_PATH_NOT_FOUND)
- return err;
- }
-
- return SVN_NO_ERROR;
+ /* Suppress errors from missing paths. */
+ return (svn_error_chain_contains(err, SVN_ERR_WC_PATH_NOT_FOUND) ?
+ SVN_NO_ERROR : err);
}
svn_error_t *
Re: [trunk/merge tracking] Merge test #17
Posted by Daniel Rall <dl...@collab.net>.
On Fri, 22 Sep 2006, Daniel Rall wrote:
> On Fri, 22 Sep 2006, Daniel Rall wrote:
> ...
> > I traced the problem to the following call stack:
> >
> > svn_wc_adm_retrieve (libsvn_wc/lock.c:962)
> > walker_helper (libsvn_wc/entries.c)
> > svn_wc_walk_entries2 (libsvn_wc/entries.c)
> > svn_client__get_prop_from_wc (libsvn_client/prop_commands.c)
> > parse_merge_info (libsvn_client/diff.c)
> > get_wc_target_merge_info (libsvn_client/diff.c)
> >
> > The problem is that a WC directory *underneath* the directory which is
> > the target of the merge is missing from the WC. As
> > svn_wc_walk_entries2() is strolling through the WC to retrieve the
> > value for the "svn:mergeinfo" property from each sub-path, it attempts
> > to access the .svn area of a directory which doesn't exist:
> >
> > From walker_helper():
> >
> > if (current_entry->kind == svn_node_dir)
> > {
> > svn_wc_adm_access_t *entry_access;
> > SVN_ERR(svn_wc_adm_retrieve(&entry_access, adm_access, entrypath,
> > subpool));
> > From svn_wc_adm_retrieve():
> >
> > if (kind == svn_node_none)
> > return svn_error_createf(SVN_ERR_WC_NOT_LOCKED, NULL,
> > _("Directory '%s' is missing"),
> > svn_path_local_style(path, pool));
>
> Incidentally, this produces the test failure:
>
> subversion/libsvn_wc/lock.c:962: (apr_err=155005)
> svn: Directory 'svn-test-work/working_copies/merge_tests-17/A/B/F/Q' is missing
>
> > walker_helper()'s attempt to retrieve the svn_wc_adm_access_t for the
> > sub-directory without a svn_io_check_path() call might be a reasonable
> > optimization.
DannyB and EH confirmed this type of optimization has made a
significant performance difference, especially for WCs of the gcc
class.
> > However, because of the generic SVN_ERR_WC_NOT_LOCKED
> > error thrown by svn_wc_adm_retrieve(), there doesn't seem to be way
> > for walker_helper() to determine that the directory is missing (at
> > least, not without subsequently resorting to the path check). David
> > James and I were thinking that this scenario should result in an error
> > with a different code (e.g. SVN_ERR_WC_PATH_MISSING or something),
> > which could be detected further up the call stack.
I made use of SVN_ERR_WC_PATH_NOT_FOUND in r21615 (on trunk) to
indicate the root cause of the errors coming out of this WC locking
API, and ported this change to the merge-tracking branch.
...
> o Add an additional callback hook to the svn_wc_entry_callbacks_t
> structure for handling errors. This hook could either be generic, and
> used for pretty much every error encountered under the call stack of
> svn_wc_walk_entries2(), or be specific to missing paths.
I went with this approach, which I broke into two patches. The first
patch I'd like to commit to trunk:
[[[
Add a generic error handler to routine to the WC walker API, adding
the ability to modify or squelch errors which occur while walking the
WC tree.
* subversion/include/svn_wc.h
(svn_wc_entry_callbacks2_t): Rev the WC walker callbacks, which
adding a handle_error() callback hook.
(svn_wc_walk_entries3): Rev the WC walker API, switching to
svn_wc_entry_callbacks2_t.
(svn_wc_walk_entries2): Deprecate the previous API.
* subversion/libsvn_wc/entries.c
(walker_helper): Accept a svn_wc_entry_callbacks2_t for the
WALK_CALLBACKS parameter, and use its handle_error() API
throughout. When attempting recursion into entry sub-directories,
skip them if a lock cannot be procured.
(svn_wc_walk_entries3): Rev the svn_wc_walk_entries2() API, and use
the handle_error() API.
(svn_wc_walk_entries2): Replace with compat code.
(walker_default_error_handler): A new compat error handler callback.
]]]
An alternate approach which is somewhat similar to this would be to
define a much more specific callback API to use when error likes
SVN_ERR_WC_PATH_NOT_FOUND are encountered inside
svn_wc_walk_entries2()/walker_helper(). The consensus on IRC was that
a more generic API would be preferrable.
The second patch depends upon the first. If no one has problems with
the first patch, I'll port the first patch to the merge-tracking
branch, and commit the second patch there as well.
[[[
On the merge-tracking branch: Ignore missing paths when collecting the
values for a property.
* subversion/libsvn_client/prop_commands.c
(wc_walker_error_handler): Add error handler to squelch ERR by
returning SVN_NO_ERROR if ERR is casued by a missing path.
(svn_client__get_prop_from_wc): Use a svn_wc_entry_callbacks2_t data
structure carrying a wc_walker_error_handler() callback to invoke
svn_wc_walk_entries3().
]]]
I'd follow these two patches up by adding some type of error chain
interrogation API to trunk to cleanup the FIXME in
wc_walker_error_handler(). Alternately, we could just call
svn_io_check_path() (along the lines of the patch I posted in the
previous message in this thread).
Thoughts?
Thanks, Dan