You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Stephen Butler <sb...@elego.de> on 2008/11/11 17:14:32 UTC
[PATCH] really skip tree conflict victims (2)
Hi tree conflict fans,
This patch is almost ready to commit. I just need to fix up about ten
tests (for switch et al.).
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.
Regards,
Steve
[[[
Fix update/switch to skip all tree conflict victims and their
descendants.
Create only one tree conflict at the root of each conflicted tree, not a
tree conflict in every directory.
Notify the user once about any given tree conflict.
Tweak test expectations to reflect these new features.
Add tests that run update/switch in trees that already contain tree
conflict victims.
Add tests that chdir into conflicted trees and attempt to update/switch.
TODO:
More tests! As always....
The tree conflict 2_1 and 3 tests show that an incoming tree delete
bumps the revision number inside a victim tree. I don't know how to
solve this without leaving the parent dirs incomplete. Maybe I need to
hack something in complete_directory().
If an update targets a single file or dir that has been deleted from the
repo, there's an error (see test 2_1). Should be a tree conflict.
Fixing it requires moving some functions, so I'll do it in a separate
commit for clarity.
* subversion/libsvn_wc/update_editor.c
(edit_baton): Add field current_tree_conflict.
(make_editor): Initialize new field.
(dir_baton): Remove unused field tree_conflicted.
(make_dir_baton): Track field removal.
(open_root): TODO comment, for now.
(check_tree_conflict): Thin out the comments a bit. Remove useless
checks for entry validity. Remove the redundant (delete:obstructed)
and (delete:missing) conflicts.
(already_in_a_tree_conflict): New function.
(do_entry_deletion
add_directory,
open_directory,
add_file,
open_file): Skip the operation if there is an existing or new tree
conflict. Notify the user once per tree conflict.
(close_directory): Unset current_tree_conflict when returning to
the victim dir. Don't notify about tree conflicts.
* subversion/tests/cmdline/update_tests.py
(update_deleted_missing_dir,
another_hudson_problem): Don't treat a missing item as a tree
conflict.
(update_delete_modified_files,
tree_conflicts_on_update_1_2,
tree_conflicts_on_update_2_1,
tree_conflicts_on_update_2_2,
tree_conflicts_on_update_3): Tweak expected output and disk to
reflect the consistent skipping.
(tree_conflicts_on_update_2_3): New test function.
(test_list): Add the new test function, make it XFail().
* subversion/tests/cmdline/svntest/tree.py
(build_tree_from_checkout): Correct a comment.
* subversion/tests/cmdline/svntest/actions.py
(deep_trees_run_tests_scheme for update): Allow status check to be
optional.
(deep_trees_skipping_on_update): New function.
]]]
--
Stephen Butler | Software Developer
elego Software Solutions GmbH
Gustav-Meyer-Allee 25 | 13355 Berlin | Germany
fon: +49 30 2345 8696 | mobile: +49 163 25 45 015
fax: +49 30 2345 8695 | http://www.elegosoft.com
Geschäftsführer: Olaf Wagner | Sitz der Gesellschaft: Berlin
Amtsgericht Charlottenburg HRB 77719 | USt-IdNr: DE163214194
Re: [PATCH] really skip tree conflict victims (2)
Posted by "Neels J. Hofmeyr" <ne...@elego.de>.
Julian Foad wrote:
> On Tue, 2008-11-11 at 18:00 +0000, Julian Foad wrote:
>> n.b. I've also updated the 'tc-resolve' branch so I think it's ready to
>> merge back. I guess you'll want me to wait for this?
>
> Never mind that comment. The branch is not quite there yet. (It does
> resolve TCs where the target is a victim, but it also resolves TCs where
> the target is the parent, which is not desired.)
Fixed in r34153.
But the new tests for that fail, see my "HEADS UP" mail just before.
~Neels
Re: [PATCH] really skip tree conflict victims (2)
Posted by Julian Foad <ju...@btopenworld.com>.
On Tue, 2008-11-11 at 18:00 +0000, Julian Foad wrote:
> On Tue, 2008-11-11 at 18:14 +0100, Stephen Butler wrote:
> > Hi tree conflict fans,
> >
> > This patch is almost ready to commit. I just need to fix up about ten
> > tests (for switch et al.).
> >
> > 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.
>
> Hi Steve. That's fantastic. I struggled with trying to implement
> "skipping", and didn't get this far.
>
> +1 on the reduction to four essential cases, that you mentioned earlier.
>
> I'll try to check this over in an hour or two.
>
> n.b. I've also updated the 'tc-resolve' branch so I think it's ready to
> merge back. I guess you'll want me to wait for this?
Never mind that comment. The branch is not quite there yet. (It does
resolve TCs where the target is a victim, but it also resolves TCs where
the target is the parent, which is not desired.)
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] really skip tree conflict victims (2)
Posted by Julian Foad <ju...@btopenworld.com>.
On Tue, 2008-11-11 at 18:14 +0100, Stephen Butler wrote:
> Hi tree conflict fans,
>
> This patch is almost ready to commit. I just need to fix up about ten
> tests (for switch et al.).
>
> 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.
Hi Steve. That's fantastic. I struggled with trying to implement
"skipping", and didn't get this far.
+1 on the reduction to four essential cases, that you mentioned earlier.
I'll try to check this over in an hour or two.
n.b. I've also updated the 'tc-resolve' branch so I think it's ready to
merge back. I guess you'll want me to wait for this?
- Julian
> [[[
>
> Fix update/switch to skip all tree conflict victims and their
> descendants.
>
> Create only one tree conflict at the root of each conflicted tree, not a
> tree conflict in every directory.
>
> Notify the user once about any given tree conflict.
>
> Tweak test expectations to reflect these new features.
>
> Add tests that run update/switch in trees that already contain tree
> conflict victims.
>
> Add tests that chdir into conflicted trees and attempt to update/switch.
>
>
> TODO:
>
> More tests! As always....
>
> The tree conflict 2_1 and 3 tests show that an incoming tree delete
> bumps the revision number inside a victim tree. I don't know how to
> solve this without leaving the parent dirs incomplete. Maybe I need to
> hack something in complete_directory().
>
> If an update targets a single file or dir that has been deleted from the
> repo, there's an error (see test 2_1). Should be a tree conflict.
> Fixing it requires moving some functions, so I'll do it in a separate
> commit for clarity.
>
>
> * subversion/libsvn_wc/update_editor.c
> (edit_baton): Add field current_tree_conflict.
> (make_editor): Initialize new field.
> (dir_baton): Remove unused field tree_conflicted.
> (make_dir_baton): Track field removal.
> (open_root): TODO comment, for now.
> (check_tree_conflict): Thin out the comments a bit. Remove useless
> checks for entry validity. Remove the redundant (delete:obstructed)
> and (delete:missing) conflicts.
> (already_in_a_tree_conflict): New function.
> (do_entry_deletion
> add_directory,
> open_directory,
> add_file,
> open_file): Skip the operation if there is an existing or new tree
> conflict. Notify the user once per tree conflict.
> (close_directory): Unset current_tree_conflict when returning to
> the victim dir. Don't notify about tree conflicts.
>
> * subversion/tests/cmdline/update_tests.py
> (update_deleted_missing_dir,
> another_hudson_problem): Don't treat a missing item as a tree
> conflict.
> (update_delete_modified_files,
> tree_conflicts_on_update_1_2,
> tree_conflicts_on_update_2_1,
> tree_conflicts_on_update_2_2,
> tree_conflicts_on_update_3): Tweak expected output and disk to
> reflect the consistent skipping.
> (tree_conflicts_on_update_2_3): New test function.
> (test_list): Add the new test function, make it XFail().
>
> * subversion/tests/cmdline/svntest/tree.py
> (build_tree_from_checkout): Correct a comment.
>
> * subversion/tests/cmdline/svntest/actions.py
> (deep_trees_run_tests_scheme for update): Allow status check to be
> optional.
> (deep_trees_skipping_on_update): New function.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] really skip tree conflict victims (2)
Posted by Julian Foad <ju...@btopenworld.com>.
On Wed, 2008-11-12 at 18:29 +0100, Stephen Butler wrote:
> Hi tree conflict fans,
>
> I committed the patch (after some more work) as r34158. Sorry
> it's such a massive set of changes, but I didn't see how to
> split it nicely.
+1. Thanks, Steve.
> Any working copy/adm_access gurus out there, please have a look
> at the shenanigans going on in already_in_a_tree_conflict().
> The issue discussed below is not quite closed. I couldn't use
> svn_wc_adm_probe_try3() because it requires an associated access
> baton. But already_in_a_tree_conflict() can't use any
> associated batons, because it would "pollute" them with
> references to their parent dirs. svn_wc_adm_probe_open3() seems
> to work OK.
OK, stsp's been looking at a similar issue tonight and I'm watching and
will take it up in the morning and see if the same concerns or solutions
apply here.
> General remarks on r34158:
>
> 1. Need more tests. As always....
Sure... but anything in particular?
> 2. The new function already_in_a_tree_conflict() isn't very efficient.
>
> 3. tree_conflicts_on_update_2_1 and _3 show that an incoming tree delete
> bumps the revision number inside a victim tree. I don't know how to
> solve this without leaving the parent dirs incomplete. Maybe I need
> to hack something in complete_directory().
>
> 4. tree_conflicts_on_update_2_3 is XFail due to an update bug. If an
> update target is a single file or dir that has been deleted from the
> repo, update should print a 'Skipped' message. Instead, it either
> prints nothing or stops with an error. Fixing this requires moving
> some functions, I think, so I'll do it in a separate commit for
> clarity.
>
> 5. tree_conflicts_on_switch_2_1 and _3 are XFail because of a bug in
> 'svn status -u', which asks the repo for the status of items added by
> switch that haven't been committed yet, and receives "path 'X' does
> not exist in repository". This should be status '? ' instead of an
> error. There's a related problem with 'svn status' in switch test 21.
I'll try to look at these at some point.
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] really skip tree conflict victims (2)
Posted by Stephen Butler <sb...@elego.de>.
Hi tree conflict fans,
I committed the patch (after some more work) as r34158. Sorry
it's such a massive set of changes, but I didn't see how to
split it nicely.
Any working copy/adm_access gurus out there, please have a look
at the shenanigans going on in already_in_a_tree_conflict().
The issue discussed below is not quite closed. I couldn't use
svn_wc_adm_probe_try3() because it requires an associated access
baton. But already_in_a_tree_conflict() can't use any
associated batons, because it would "pollute" them with
references to their parent dirs. svn_wc_adm_probe_open3() seems
to work OK.
General remarks on r34158:
1. Need more tests. As always....
2. The new function already_in_a_tree_conflict() isn't very efficient.
3. tree_conflicts_on_update_2_1 and _3 show that an incoming tree delete
bumps the revision number inside a victim tree. I don't know how to
solve this without leaving the parent dirs incomplete. Maybe I need
to hack something in complete_directory().
4. tree_conflicts_on_update_2_3 is XFail due to an update bug. If an
update target is a single file or dir that has been deleted from the
repo, update should print a 'Skipped' message. Instead, it either
prints nothing or stops with an error. Fixing this requires moving
some functions, I think, so I'll do it in a separate commit for
clarity.
5. tree_conflicts_on_switch_2_1 and _3 are XFail because of a bug in
'svn status -u', which asks the repo for the status of items added by
switch that haven't been committed yet, and receives "path 'X' does
not exist in repository". This should be status '? ' instead of an
error. There's a related problem with 'svn status' in switch test 21.
Regards,
Steve
Quoting Julian Foad <ju...@btopenworld.com>:
> On Wed, 2008-11-12 at 11:05 +0100, Stephen Butler wrote:
>> Quoting Julian Foad <ju...@btopenworld.com>:
>>
>> > On Tue, 2008-11-11 at 18:14 +0100, Stephen Butler wrote:
>> >> + */
>> >> +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)
...
>> >
>> > 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
--
Stephen Butler | Software Developer
elego Software Solutions GmbH
Gustav-Meyer-Allee 25 | 13355 Berlin | Germany
fon: +49 30 2345 8696 | mobile: +49 163 25 45 015
fax: +49 30 2345 8695 | http://www.elegosoft.com
Geschäftsführer: Olaf Wagner | Sitz der Gesellschaft: Berlin
Amtsgericht Charlottenburg HRB 77719 | USt-IdNr: DE163214194
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] really skip tree conflict victims (2)
Posted by Julian Foad <ju...@btopenworld.com>.
On Wed, 2008-11-12 at 11:05 +0100, Stephen Butler wrote:
> Quoting Julian Foad <ju...@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@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] really skip tree conflict victims (2)
Posted by Stephen Butler <sb...@elego.de>.
Quoting Julian Foad <ju...@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?
>
>> + 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
--
Stephen Butler | Software Developer
elego Software Solutions GmbH
Gustav-Meyer-Allee 25 | 13355 Berlin | Germany
fon: +49 30 2345 8696 | mobile: +49 163 25 45 015
fax: +49 30 2345 8695 | http://www.elegosoft.com
Geschäftsführer: Olaf Wagner | Sitz der Gesellschaft: Berlin
Amtsgericht Charlottenburg HRB 77719 | USt-IdNr: DE163214194
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] really skip tree conflict victims (2)
Posted by Julian Foad <ju...@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.
>
> Create only one tree conflict at the root of each conflicted tree, not a
> tree conflict in every directory.
>
> Notify the user once about any given tree conflict.
>
> Tweak test expectations to reflect these new features.
>
> Add tests that run update/switch in trees that already contain tree
> conflict victims.
>
> Add tests that chdir into conflicted trees and attempt to update/switch.
>
>
> TODO:
>
> More tests! As always....
>
> The tree conflict 2_1 and 3 tests show that an incoming tree delete
> bumps the revision number inside a victim tree. I don't know how to
> solve this without leaving the parent dirs incomplete. Maybe I need to
> hack something in complete_directory().
>
> If an update targets a single file or dir that has been deleted from the
> repo, there's an error (see test 2_1). Should be a tree conflict.
> Fixing it requires moving some functions, so I'll do it in a separate
> commit for clarity.
>
>
> * subversion/libsvn_wc/update_editor.c
[...]
> (check_tree_conflict): Thin out the comments a bit. Remove useless
> checks for entry validity. Remove the redundant (delete:obstructed)
> and (delete:missing) conflicts.
> (already_in_a_tree_conflict): New function.
> (do_entry_deletion
> add_directory,
> open_directory,
> add_file,
> open_file): Skip the operation if there is an existing or new tree
> conflict. Notify the user once per tree conflict.
> (close_directory): Unset current_tree_conflict when returning to
> the victim dir. Don't notify about tree conflicts.
>
> * subversion/tests/cmdline/update_tests.py
> (update_deleted_missing_dir,
> another_hudson_problem): Don't treat a missing item as a tree
> conflict.
> (update_delete_modified_files,
> tree_conflicts_on_update_1_2,
> tree_conflicts_on_update_2_1,
> tree_conflicts_on_update_2_2,
> tree_conflicts_on_update_3): Tweak expected output and disk to
> reflect the consistent skipping.
> (tree_conflicts_on_update_2_3): New test function.
> (test_list): Add the new test function, make it XFail().
[...]
> * subversion/tests/cmdline/svntest/actions.py
> (deep_trees_run_tests_scheme for update): Allow status check to be
> optional.
> (deep_trees_skipping_on_update): 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
[...]
>
> case svn_wc_conflict_action_delete:
> - if (!entry)
> - reason = svn_wc_conflict_reason_missing;
> - else if (entry->schedule != svn_wc_schedule_normal)
[...]
> - reason = (entry->schedule == svn_wc_schedule_delete
> - ? svn_wc_conflict_reason_deleted
> - : svn_wc_conflict_reason_obstructed); /* replace, add, etc. */
> - else /* schedule is normal, but item might be modified or missing */
> + /* 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.
> -
> + /* 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 (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.)
> + 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.
> + 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. */
> + for (i = ancestors->nelts - 1; i >= 0; i--)
> + {
> + ancestor = APR_ARRAY_IDX(ancestors, i, char *);
> + SVN_ERR(svn_wc_conflicted_p2(NULL, NULL, &tree_conflicted, ancestor,
> + adm_access, pool));
> + if (tree_conflicted)
> + {
> + *victim_path = ancestor;
> +
> + return SVN_NO_ERROR;
> + }
> + }
> +
> + return SVN_NO_ERROR;
> +}
> +
>
[...]
I only skimmed lightly through the rest.
Looks good.
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org