This attempted fix has been lying around much longer than my other posts.
Just making sure it's not forgotten. (Stsp has since confirmed that the fix
has the right direction.)
It actually contains a patch (see original mail) for one of Julian's "###"
comments on the changes currently sitting on the tree-conflicts branch --
sorry for not marking it properly back then.
Neels Hofmeyr wrote:
> Neels Hofmeyr wrote:
>> Julian Foad wrote:
>>> On Wed, 2008-08-20 at 10:58 +0100, Julian Foad wrote:
>>>> Here is a log message for the changes currently sitting on the
>>>> tree-conflicts branch.
>>>>
>> ...
>>>> * subversion/libsvn_wc/update_editor.c
>> ...
>>>> (do_entry_deletion): Have the parent's admin access baton passed in
>>>> by the
>>>> caller. Check for tree conflicts.
>>>> ### Broken when parent_adm_access is NULL.
>> taking this one.
>>
>
>
> This is quite intricate.
>
> 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.
>
>
> 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).
>
> 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.
>
>
> - 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... ;)
>
> Neels
>
>
> [[[
> Fix segmentation fault during tree-conflict detection, in effect catching
> a tree-conflicts case where an update wants to delete a directory that is
> missing in the local working copy, when the victim is an explicit target,
> e.g. explicitly issued on the command line.
>
> * subversion/libsvn_wc/update_editor.c
> (check_tree_conflict): Catch case of update trying to remove a directory
> that is missing in the working copy, marking a tree-conflict with
> 'svn_wc_conflict_reason_missing'.
> (do_entry_deletion): Revert to behaviour as on trunk, getting adm_access
> via svn_wc_adm_retrieve(). Remove parameter PARENT_ADM_ACCESS, it was
> added on the branch. Remove error comment (error is hereby fixed).
> Adjust function's main comment, adding that this is also a helper for
> close_edit(), removing the comment about PARENT_ADM_ACCESS.
> (delete_entry): Call do_entry_deletion() without adm_access parameter,
> remove retrieval of adm_access. Revert to spacing as on trunk.
> (close_edit): Remove the PARENT_ADM_ACCESS parameter, passed as NULL
> to do_entry_deletion().
>
> * subversion/tests/cmdline/update_tests.py (another_hudson_problem):
> Above change marks a tree-conflict in this test, which confuses
> expected output. Adding a svn call that resolves the conflict.
> ]]]
>
>
--
Neels Hofmeyr -- elego Software Solutions GmbH
Gustav-Meyer-Allee 25 / Gebäude 12, 13355 Berlin, Germany
phone: +49 30 23458696 mobile: +49 177 2345869 fax: +49 30 23458695
http://www.elegosoft.com | Geschäftsführer: Olaf Wagner | Sitz: Berlin
Handelsreg: Amtsgericht Charlottenburg HRB 77719 | USt-IdNr: DE163214194
Received on 2008-08-30 04:03:05 CEST