On Sat, Aug 23, 2008 at 10:43:11PM +0200, Neels Hofmeyr wrote:
> We've got these functions:
>
> do_entry_deletion() "Helper for delete_entry()."
> delete_entry() Yes, it calls do_entry_deletion().
> close_edit() Why, this one *also* calls do_entry_deletion().
>
> On trunk, there was no adm_access passed down into do_entry_deletion(). It
> used to figure that out itself from the parent_path and the edit baton:
>
> SVN_ERR(svn_wc_adm_retrieve(&adm_access, eb->adm_access,
> parent_path, pool));
>
>
> On the tree-conflicts branch, this line was moved out to delete_entry(). The
> result is passed down into do_entry_deletion() as a new parameter called
> parent_adm_access.
>
> Now, in close_edit(), this new parameter is simply set to "NULL", which is
> the problem.
>
> The comment above do_entry_deletion has this to say:
> /* PARENT_ADM_ACCESS is the admin access baton for the parent directory,
> * or NULL if this is the target of the "update" being deleted.
>
> It tries to say that, e.g., when the user provided a target path on the
> command line (say `svn up A/D/G') and G *itself* is deleted by the update,
> PARENT_ADM_ACCESS is NULL.
>
> Basically, this amounts to a mere flag which says whether
> do_entry_deletion() has been called by delete_entry() or close_edit(). This
> "flag" is used to skip tree-conflicts detection for the latter case. (Why?)
>
>
> What bothers us is that do_entry_deletion() *uses* the new parent_adm_access
> parameter instead of the "figured out" adm_access. close_edit() sets it to
> NULL and thus causes a segmentation fault.
>
> So far, it seemingly makes sense to revert to "figuring out" adm_access
> within do_entry_deletion(), and introduce a flag parameter
> do_tree_conflicts_detection (instead of parent_adm_access).
>
>
> Which leads me back to above question: Why ever is tree-conflicts detection
> disabled for the case that the user provided explicit update targets which
> happen to be removed by the update?
>
> The log history has nothing on this. Addition of parameter parent_adm_access
> is not mentioned anywhere.
To me, it seems that this behaviour is accidental.
Looks like the paremeter was moved in r30862 (svn blame is your friend :).
do_entry_deletion() indeed got the new parameter in that revision,
but it was not mentioned in the log message:
------------------------------------------------------------------------
r30862 | julianfoad | 2008-04-30 09:57:31 +0200 (Wed, 30 Apr 2008) | 17 lines
Implement more tree conflict detection, especially getting all the file-
conflict (as opposed to directory-conflict) test cases to work. Disable
some sorts of testing that we aren't ready for.
* subversion/libsvn_client/merge.c
(merge_dir_deleted): Improve the directory delete-onto-deleted case.
* subversion/libsvn_wc/update_editor.c
(entry_has_local_mods, check_tree_conflict): New functions.
In other functions: experimental progress towards detecting more conflicts.
* subversion/tests/cmdline/tree_conflict_tests.py
Disable testing for any replacement actions, and for files whose change is
only prop mods. Disable testing of commits that should fail, both because
it isn't yet working in all cases and because the error messages when they
do fail are inconsistent.
------------------------------------------------------------------------
The diff of that revision is fairly large, and the log message does
not describe all the hunks in detail. The bug you fixed was probably
just due to an oversight.
> So, I investigated and came up with below patch. It does this:
>
>
> - do *not* skip tree-conflicts testing for that special case
> (parent_adm_access == NULL), retrieve adm_access as done on trunk.
>
> That itself causes an error message "Directory '%s' is missing", in
> check_tree_conflict() calling entry_has_local_mods().
>
> So, I gather, this is actually use case 3: update tries to delete, but the
> user has locally removed the directory (without marking it as removed, though).
That is not use case 3. It would be use case 3 if the working copy
meta data agreed with what is on disk.
Anytime disk and meta data disagree, we just call it "obstruction"
or "missing", because we can't proceed anyway. The user messed up the
working copy and cannot reasonably expect us to know what's going on.
> The subtle difference being that the target of deletion has explicitly been
> issued as target, e.g. on the commandline, and not picked up recursively.
>
>
> - in check_tree_conflict(), I thus add some code to catch this case, where a
> directory is missing. The current check node->kind == svn_node_none is not
> sufficient, because a missing dir is apparently still svn_node_dir if its
> parent is still there.
The directory being present may depend on whether you are looking at
meta data (working copy entries) or on disk (svn_io_*).
> - If this case is encountered, I mark it svn_wc_conflict_reason_missing.
> This causes update_tests.py 15 to fail, because the output contains this
> tree-conflict all of a sudden.
>
> I could choose to merely catch above case and not mark it as a conflict,
> then update_tests.py 15 succeeds. However, this *is* a tree-conflict and
> should be marked.
>
>
> - I fix update_tests.py 15 by calling svn resolved on the conflict marked
> directory in the appropriate place.
>
>
> I hope I've made this somewhat clear without babbling too much... ;)
Sounds all sane to me, but I'll leave double checking to Julian.
Some comments on the patch itself:
> Index: subversion/libsvn_wc/update_editor.c
> ===================================================================
> --- subversion/libsvn_wc/update_editor.c (revision 32658)
> +++ subversion/libsvn_wc/update_editor.c (working copy)
> @@ -1198,31 +1198,47 @@
> else
> {
> svn_boolean_t modified;
> + svn_node_kind_t kind;
>
> - /* If we are about to delete a path that has local mods,
> + /* If we are about to delete a path that is locally missing,
> * mark the containing directory as tree conflicted.
> - * This is tree conflict use case 2 as described in the
> - * paper attached to issue #2282
> + * This is tree conflict use case 3.
> * See also notes/tree-conflicts/detection.txt
detection.txt says:
If 'svn update' deletes a file that has been scheduled for deletion in
the working copy, the file is a tree conflict victim
But the file has not been scheduled for deletion in this case.
I'd suggest simply stating:
/* If we are about to delete a path that is missing from disk
* but present in meta data, raise a tree conflict.
* The merge cannot succeed because the working copy is broken.
*/
Essentially, this is the inverse of our 'obstructed' tree conflict
cases, which happen when a path is present on disk which meta data
says should not be there (yet).
> */
> - SVN_ERR(entry_has_local_mods(&modified, parent_adm_access, entry,
> - full_path, pool));
> + if (svn_io_check_path(full_path, &kind, pool) != SVN_NO_ERROR
I think the line above, as written, will leak an error if
svn_io_check_path returns something other than SVN_NO_ERROR.
Note that the error system is a bit peculiar. It's intended to emulate
exception handling. All errors passed around are actually pointers to
error objects. What the line above essentially does is:
if (svn_io_check_path(full_path, &kind, pool) != NULL
Quoting the docstring of svn_error_create() in svn_error.h:
* @note Errors are always allocated in a subpool of the global pool,
* since an error's lifetime is generally not related to the
* lifetime of any convenient pool. Errors must be freed
* with svn_error_clear().
Note the last sentence.
So in the above case, any error generated by svn_io_check_path() would
not be freed. The error is "leaking".
The usual construct for ignoring errors completely is:
svn_error_clear(svn_io_check_path(full_path, &kind, pool));
if (kind == svn_node_none)
...
You can also ignore only errors of a certain type, by matching
err->apr_err to known error codes:
err = svn_io_check_path(full_path, &kind, pool);
if (err)
{
if (err->apr_err == SVN_ERR_SOME_ERROR)
/* Ignore this error, because [...]. */
svn_error_clear(err);
else
return err;
}
if (kind == svn_node_none)
...
Though I'm not really sure why you'd want to ignore potential
errors raised by svn_io_check_path(). Looking at its implementation,
it returns any error returned by APR, except for ENOENT or ENOTDIR.
I'm not sure what other errors APR could return, but it's likely that
if the error isn't on of those two, there are bigger problems.
So wouldn't the following do the job?
SVN_ERR(svn_io_check_path(full_path, &kind, pool));
if (kind == svn_node_none)
...
Note that SVN_ERR(func(a, b, c)); expands to the equivalent of:
svn_error_t *err = func(a, b, c);
if (err)
return err;
> + || kind == svn_node_none)
> + reason = svn_wc_conflict_reason_missing;
Note that the same reason is also returned if the directory is missing
from meta data. Quoting code not visible in the context of you patch:
/* Test whether CONFLICT_ACTION conflicts with the state of ENTRY.
* If so, set CONFLICT_REASON to an appropriate value. */
switch (action)
{
[...]
case svn_wc_conflict_action_delete:
if (!entry)
reason = svn_wc_conflict_reason_missing; [ <--- note here! ]
else if (entry->schedule != svn_wc_schedule_normal)
[...]
else
{
[ Your patch starts modifying things here ]
Currently we just have the following reasons which relate to
meta data being out of sync with the state on disk:
svn_wc_conflict_reason_obstructed, /* another object is in the way */
svn_wc_conflict_reason_missing, /* object is unknown or missing */
svn_wc_conflict_reason_unversioned /* object is unversioned */
We might want to extend this at some point, such as:
svn_wc_conflict_reason_obstructed, /* another (versioned or unversioned)
object is in the way */
svn_wc_conflict_reason_missing, /* object is unknown or missing
both on disk and in meta data */
svn_wc_conflict_reason_entry_missing, /* object is unknown to meta data
or missing from meta data */
svn_wc_conflict_reason_path_missing, /* object is present in meta data
but is not on disk */
svn_wc_conflict_reason_unversioned /* object is unversioned and cannot
be operated upon */
This would allow us to produce more helpful error messages for
these cases. But this is clearly out of scope of your patch.
I just jotted down this idea here to make a note of it :)
Telling these reaons apart will become even more important once
Rui Guo's directory exclusion work has been merged to trunk.
Because then, a directory missing from disk but present in meta
data becomes entirely legitimate.
> + else
> + {
> +
>
> - /* ### TODO: Also detect deep modifications in a directory tree.
> - * The update editor will not visit subdirectories of a
> - * directory it wants to delete. Therefore, we need to start
> - * a separate crawl here to make sure the directory tree we
> - * are about to delete hasn't been locally modified anywhere.
> - */
> + /* If we are about to delete a path that has local mods,
> + * mark the containing directory as tree conflicted.
> + * This is tree conflict use case 2 as described in the
> + * paper attached to issue #2282
> + * See also notes/tree-conflicts/detection.txt
> + */
> + SVN_ERR(entry_has_local_mods(&modified, parent_adm_access,
> + entry, full_path, pool));
> +
> +
> + /* ### TODO: Also detect deep modifications in a directory tree.
> + * The update editor will not visit subdirectories of a
> + * directory it wants to delete. Therefore, we need to start
> + * a separate crawl here to make sure the directory tree we
> + * are about to delete hasn't been locally modified anywhere.
> + */
> #if 0
> - SVN_ERR(subtrees_have_local_mods(&modified, parent_adm_access, entry,
> - full_path, pool));
> - /* Alternatively, have entry_has_local_mods() do the recursion. */
> + SVN_ERR(subtrees_have_local_mods(&modified, parent_adm_access,
> + entry, full_path, pool));
> + /* Alternatively, have entry_has_local_mods() do the recursion.
> + */
> #endif
>
> - if (modified)
> - {
> - reason = svn_wc_conflict_reason_edited;
> + if (modified)
> + {
> + reason = svn_wc_conflict_reason_edited;
> + }
> }
> }
> break;
> @@ -1249,30 +1265,28 @@
> return SVN_NO_ERROR;
> }
>
> -/* Helper for delete_entry(). */
> -/* PARENT_ADM_ACCESS is the admin access baton for the parent directory,
> - * or NULL if this is the target of the "update" being deleted.
> - */
> +/* Helper for delete_entry() and close_edit(). */
> static svn_error_t *
> do_entry_deletion(struct edit_baton *eb,
> const char *parent_path,
> const char *path,
> int *log_number,
> - svn_wc_adm_access_t *parent_adm_access,
> apr_pool_t *pool)
> {
> + svn_wc_adm_access_t *adm_access;
> const svn_wc_entry_t *entry;
> const char *full_path = svn_path_join(eb->anchor, path, pool);
> svn_stringbuf_t *log_item = svn_stringbuf_create("", pool);
>
> - /* ### Error: here we're assuming parent_adm_access is non-null. Crashes in update_tests-15, for instance. */
> - SVN_ERR(svn_wc_entry(&entry, full_path, parent_adm_access, FALSE, pool));
> + SVN_ERR(svn_wc_adm_retrieve(&adm_access, eb->adm_access,
> + parent_path, pool));
This is the core of the fix, and essentially assumes that we will
return here if svn_wc_adm_retrieve() cannot get hold of an access baton.
Which is fine: It does indeed return an error if it cannot return a
valid access baton. Simplifies the code a lot.
>
> - if (parent_adm_access)
> - SVN_ERR(check_tree_conflict(log_item, full_path, entry, parent_adm_access,
> - svn_wc_conflict_action_delete, pool));
> + SVN_ERR(svn_wc__entry_versioned(&entry, full_path, adm_access, FALSE, pool));
>
> - SVN_ERR(svn_wc__loggy_delete_entry(&log_item, parent_adm_access, full_path,
> + SVN_ERR(check_tree_conflict(log_item, full_path, entry, adm_access,
> + svn_wc_conflict_action_delete, pool));
> +
> + SVN_ERR(svn_wc__loggy_delete_entry(&log_item, adm_access, full_path,
> pool));
>
> /* If the thing being deleted is the *target* of this update, then
> @@ -1287,7 +1301,7 @@
> (entry->kind == svn_node_file) ? svn_node_file : svn_node_dir; /* ### redundant? */
> tmp_entry.deleted = TRUE;
>
> - SVN_ERR(svn_wc__loggy_entry_modify(&log_item, parent_adm_access,
> + SVN_ERR(svn_wc__loggy_entry_modify(&log_item, adm_access,
> full_path, &tmp_entry,
> SVN_WC__ENTRY_MODIFY_REVISION
> | SVN_WC__ENTRY_MODIFY_KIND /* ### redundant change? */
> @@ -1297,7 +1311,7 @@
> eb->target_deleted = TRUE;
> }
>
> - SVN_ERR(svn_wc__write_log(parent_adm_access, *log_number, log_item, pool));
> + SVN_ERR(svn_wc__write_log(adm_access, *log_number, log_item, pool));
>
> if (eb->switch_url)
> {
> @@ -1340,7 +1354,7 @@
> }
> }
>
> - SVN_ERR(svn_wc__run_log(parent_adm_access, NULL, pool));
> + SVN_ERR(svn_wc__run_log(adm_access, NULL, pool));
> *log_number = 0;
>
> if (eb->notify_func)
> @@ -1361,15 +1375,11 @@
> apr_pool_t *pool)
> {
> struct dir_baton *pb = parent_baton;
> - svn_wc_adm_access_t *parent_adm_access;
>
> SVN_ERR(check_path_under_root(pb->path, svn_path_basename(path, pool),
> pool));
> - SVN_ERR(svn_wc_adm_retrieve(&parent_adm_access, pb->edit_baton->adm_access,
> - pb->path, pool));
> -
> return do_entry_deletion(pb->edit_baton, pb->path, path, &pb->log_number,
> - parent_adm_access, pool);
> + pool);
> }
>
>
> @@ -3405,8 +3415,7 @@
> pretend that the editor deleted the entry. The helper function
> do_entry_deletion() will take care of the necessary steps. */
> if ((*eb->target) && (svn_wc__adm_missing(eb->adm_access, target_path)))
> - SVN_ERR(do_entry_deletion(eb, eb->anchor, eb->target, &log_number, NULL,
> - pool));
> + SVN_ERR(do_entry_deletion(eb, eb->anchor, eb->target, &log_number, pool));
>
> /* The editor didn't even open the root; we have to take care of
> some cleanup stuffs. */
> Index: subversion/tests/cmdline/update_tests.py
> ===================================================================
> --- subversion/tests/cmdline/update_tests.py (revision 32658)
> +++ subversion/tests/cmdline/update_tests.py (working copy)
> @@ -1097,6 +1097,15 @@
> 'Updated to revision 3.\n'], [],
> 'up', G_path)
>
> + # This update created a tree conflict ("update tried to
> + # delete a directory that was locally deleted"), marking
> + # C A/D. Just ignore it, i.e. resolve it.
> + D_path = os.path.join(wc_dir, 'A', 'D')
> + svntest.actions.run_and_verify_svn(None,
> + ["Resolved conflicted state of " +
> + "'svn-test-work/working_copies/update_tests-15/A/D'\n"], [],
> + 'resolved', D_path)
> +
Unfortunately, many tests break because of tree conflicts :(
Fixing test breakage by resolving such conflicts is what we have
been doing since day 1, so this is fine.
> # Both G and gamma should be 'deleted', update should produce no output
> expected_output = svntest.wc.State(wc_dir, { })
> expected_status = svntest.actions.get_virginal_state(wc_dir, 3)
>
Stefan
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe_at_subversion.tigris.org
For additional commands, e-mail: dev-help_at_subversion.tigris.org
Received on 2008-08-24 13:21:01 CEST