You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by "Neels J. Hofmeyr" <ne...@elego.de> on 2008/10/25 04:49:36 UTC
branch tree-conflicts-notify: merge to trunk?
Hey Steve! (sbutler)
That's so awesome! Somehow you've magically fixed almost everything on the
branch! All I had to do was raise and notify a tree-conflict on file
addition in a conflicted directory.
With that, one thing remains: There's still some differentiation needed
between raised and persisted tree-conflicts. But I guess we can treat this
separately (in need of more tests, see below).
Otherwise, the "only" thing that can lead update_tests.py to perfection is
the implementation of dirs_same_p() a.k.a. diff_summarize, to be able to see
whether a subtree we're deleting is really the same one.
...And discussion on how exactly the output should be.
E.g., I think it is nice like this:
A C alpha
D C beta
M C gamma
While, right now, update notifies more like this:
A C alpha
D C beta
C gamma
Should we add the M or lose the A and D? I'd say add the M and whatever else
would have been shown without a tree-conflict, to be able to see what was
tried by the update. (Maybe there are more cases to this, though.)
Either way, I think this branch is ready for reintegration into trunk!!
What do you think, Steve?
(still in need of a complete `make check' though)
Thanks again for the great improvements I found there today! I really didn't
know what to do about the test framework, it's great how that just works
now. The stuff I complained about before also makes sense to me, now.
Oh, I'd just like to note that there was a misconception in some comments in
update_tests.py:
[[[
- ### Current behavior (rather messy, isn't it?)
]]]
It wasn't reaaally that messy, it just missed the cases of file additions
into conflicted directories. ;)
[[[
# 1.1) local tree delete, incoming leaf edit
...
- ### Current behavior carries out deletions in conflicted trees.
- expected_state = state_after_leaf_edit.copy()
- expected_state.remove('F/alpha', 'DF/D1/beta', 'DDF/D1/D2/gamma')
+ # The directory structure is scheduled for delete but is still
+ # found on disk, because the way directory deletes are handled by svn.
+ # No files are present because alpha, beta, gamma were locally removed
+ # and delta, epsilon, zeta weren't added due to tree-conflicts in their
+ # parent directory.
+ expected_disk = state_after_leaf_edit.copy()
+ expected_disk.remove('F/alpha', 'DF/D1/beta', 'DDF/D1/D2/gamma',
+ 'D/D1/delta', 'DD/D1/D2/epsilon', 'DDD/D1/D2/D3/zeta')
]]]
The incoming action is a leaf edit, so no deletions are tried. The error was
that files were being added into a conflicted dir. (Why was this called
expected_state? It's called expected_disk everywhere else.)
[[[
- ### Current behavior records redundant conflicts!
+ ### Current behavior fails to show a status on files that would
+ ### have been added if there hadn't been a tree-conflict.
]]]
The complete local tree was removed, so if you try to add a file a few
levels inside a removed dir, all the non-existing path elements are in tree
conflict. Makes sense to me.
The check_tree_conflict() vs. svn_wc_conflicted_p() discussion made me
think: In the tests, I guess we should also try to run the same operation
that causes the conflicts again, to check that the persisting conflicts
aren't re-raised or missed altogether. Is this accounted for yet?
And to squeeze yet another topic into this mail: What's the deal with the
run_and_verify_unquiet_status tests -- is status not intended to show
tree-conflicts (on missing nodes) by default?
Thanks Steve, great stuff. Yesterday I was confused, but now it all makes sense.
~Neels
Re: branch tree-conflicts-notify: how many columns?
Posted by "Neels J. Hofmeyr" <ne...@elego.de>.
Stephen Butler wrote:
> Quoting Neels J Hofmeyr <ne...@elego.de>:
>
>> Neels J. Hofmeyr wrote:
>>>
>>> Stephen Butler wrote:
>> [...]
>>>>> The check_tree_conflict() vs. svn_wc_conflicted_p() discussion made me
>>>>> think: In the tests, I guess we should also try to run the same
>>>>> operation
>>>>> that causes the conflicts again, to check that the persisting
>>>>> conflicts
>>>>> aren't re-raised or missed altogether. Is this accounted for yet?
>>>> No, currently we test only fresh tree conflicts. Good point.
>>>
>>> I'll try that now if you haven't already.
>>
>> No, actually, got caught up in more pressing matters, as you can see
>> in the
>> commit log of r33891.
>
> Hello tree-conflict fans,
>
> Neels, I believe the changes in r33891 are actually a part of checking that
> persistent tree conflicts "aren't re-raised or missed altogether." But I
No, they are not:
[[[
Skip files added into tree-conflicted dirs (and don't notify).
Skip deletion of tree-conflicted nodes.
...
]]]
Firstly, it fixes a notification that I added wrongly, but I left some code
in a comment because we will most certainly want to use it if encountering a
persistent conflict, once we want to deal with that stuff.
Secondly, it skips the actual deletion if the delete was discovered to cause
a tree-conflict, which has an effect on notification, which is the reason
why I included that.
> think we should solve that problem in one go rather than piecemeal.
> Otherwise we'll waste a lot of time massaging test expectations from one
> intermediate (bad) state to another.
+1
But on certain changes it is good to review most relevant test suites to
find errors or understand situations. It helped me today ;)
>
> Attached is a diff to the tree-conflicts-notify branch, just some hacking
> I've done today to try to skip changes to the working copy in the presence
> of existing tree conflicts. BTW I cribbed a few good things from Julian's
> skip-inside-conflicts-3.patch from a couple days ago.
So, you're not quite done with it yet, right? Let me know if you'd be happy
to continue on trunk (or whether you've completed it on the branch), see below.
>
> But I think we should tidy up and merge-to-trunk the pre-r33891 state of
> our branch, because we've accomplished our first goal: per-victim tree
> conflict notification in update/switch output. Skipping conflicted trees,
> handling merge output, UI improvements (e.g. 'CDM'), etc. can be done in
> future patches to trunk or in short-term branches as needed.
>
> What do you (all) think?
There's just one case I found today that we'd break when merging as-is:
we've got per-victim conflict notification for update/switch, and we changed
the notification code to count them by the new tree_conflicted flag.
However, merge doesn't use that flag yet and displays tree-conflicts as a
"text-conflict" on the parent dir. The result can be seen in merge_tests.py
103 (on branch tree-conflicts-notify r33895): A tree conflict is counted as
a text conflict.
So do we want to tackle merge notification on trunk? I don't mind either
way. But we're slowly drifting farther and farther from trunk. And it's
slowly becoming a "let's do everything on the branch"-branch instead of an
"I just want to try this first"-branch. If no-one objects (or is quicker
than me), I'll reintegrate.
~Neels
Re: branch tree-conflicts-notify: how many columns?
Posted by Stephen Butler <sb...@elego.de>.
Quoting Neels J Hofmeyr <ne...@elego.de>:
> Neels J. Hofmeyr wrote:
>>
>> Stephen Butler wrote:
> [...]
>>>> The check_tree_conflict() vs. svn_wc_conflicted_p() discussion made me
>>>> think: In the tests, I guess we should also try to run the same operation
>>>> that causes the conflicts again, to check that the persisting conflicts
>>>> aren't re-raised or missed altogether. Is this accounted for yet?
>>> No, currently we test only fresh tree conflicts. Good point.
>>
>> I'll try that now if you haven't already.
>
> No, actually, got caught up in more pressing matters, as you can see in the
> commit log of r33891.
Hello tree-conflict fans,
Neels, I believe the changes in r33891 are actually a part of checking that
persistent tree conflicts "aren't re-raised or missed altogether." But I
think we should solve that problem in one go rather than piecemeal.
Otherwise we'll waste a lot of time massaging test expectations from one
intermediate (bad) state to another.
Attached is a diff to the tree-conflicts-notify branch, just some hacking
I've done today to try to skip changes to the working copy in the presence
of existing tree conflicts. BTW I cribbed a few good things from Julian's
skip-inside-conflicts-3.patch from a couple days ago.
But I think we should tidy up and merge-to-trunk the pre-r33891 state of
our branch, because we've accomplished our first goal: per-victim tree
conflict notification in update/switch output. Skipping conflicted trees,
handling merge output, UI improvements (e.g. 'CDM'), etc. can be done in
future patches to trunk or in short-term branches as needed.
What do you (all) think?
Steve
Draft commit message for attached patch:
On the tree-conflicts-notify branch: Try to skip tree-conflicted
items. So far, I have edited the dir callbacks only. The file
callbacks, test expectations, etc. remain. Oh, and testing to see
if it works at all.
[[[
* subversion/libsvn_wc/update_editor.c
(edit_baton): New boolean field inside_a_tree_conflict.
(maybe_bump_dir_info): Don't call complete_directory() if inside a
tree conflict.
(check_tree_conflict): Rename to ...
(maybe_raise_tree_conflict): ... and add a todo comment.
(tree_has_local_mods,
add_file,
open_file): Track rename of check_tree_conflict().
(do_entry_deletion,
add_directory,
open_directory,
close_directory): Track rename of check_tree_conflict(). Skip
almost everything if inside a tree conflict. Otherwise, if the dir
has an existing or freshly-raised tree conflict, notify the user
and skip almost everything else. Otherwise proceed normally.
(change_dir_prop): Skip everything if inside a tree conflict.
]]]
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: branch tree-conflicts-notify: how many columns?
Posted by Neels J Hofmeyr <ne...@elego.de>.
Neels J. Hofmeyr wrote:
>
> Stephen Butler wrote:
[...]
>>> The check_tree_conflict() vs. svn_wc_conflicted_p() discussion made me
>>> think: In the tests, I guess we should also try to run the same operation
>>> that causes the conflicts again, to check that the persisting conflicts
>>> aren't re-raised or missed altogether. Is this accounted for yet?
>> No, currently we test only fresh tree conflicts. Good point.
>
> I'll try that now if you haven't already.
No, actually, got caught up in more pressing matters, as you can see in the
commit log of r33891.
~Neels
branch tree-conflicts-notify: how many columns? -- was: merge to
trunk?
Posted by "Neels J. Hofmeyr" <ne...@elego.de>.
Stephen Butler wrote:
> Quoting "Neels J. Hofmeyr" <ne...@elego.de>:
>
>> Hey Steve! (sbutler)
>>
>> That's so awesome! Somehow you've magically fixed almost everything on
>> the
>> branch! All I had to do was raise and notify a tree-conflict on file
>> addition in a conflicted directory.
>
> Thanks!
>
> BTW, I think we should (silently) skip a file-add in a tree-conflicted
> tree, rather than flag a new tree conflict. And of course skip all
> other changes, too. Otherwise the user might see a whole treeful of
> t.c. victims, and not be sure where the problem started. A single
> tree conflict, raised at the root of the tree during open_directory(),
> should be enough.
Hey, of course. What was I thinking. If a directory is tree-conflicted,
everything inside is naturally going to be a tree-conflict and doesn't need
to be listed in detail.
Oh, but what about the case when a directory has a previously raised,
persisting tree-conflict, and say update wants to add a file in it.
Shouldn't that then be a new, separate tree-conflict?
>
>> Should we add the M or lose the A and D? I'd say add the M and
>> whatever else
>> would have been shown without a tree-conflict, to be able to see what was
>> tried by the update. (Maybe there are more cases to this, though.)
>
> I believe the first two columns tell the user what changes the update
> has made in their working copy. If a path is skipped due to a tree
> conflict, we should leave these columns blank.
>
> Of course, we plan to skip all tree conflict victims (very soon), so
> the 'x C' will become ' C' in all cases.
>
> If ' C' is too cryptic, we could add the '(M->!)' later.
Yes, I believe it should be visible what was being tried when encountering a
tree-conflict. Either way, 'D C' or ' C(D->M)' but btw, I'm actually -1
on the brackets and even on the arrow. I'd prefer having ' CDM'. Much less
characters to read, and less unnecessary indentation for most of the cases
where there are no tree-conflicts. Here's a sort of scale of possibilities:
notify nr of additional columns
========== ===
C 1
D C 1
D CM 2
CDM 3 <-- neels likes this one best.
CD>M 4
C D>M 5
C:D>M 5
C D->M 6
C:D->M 6
C(D>M) 6
C D->M 6
C(D->M) 7
...
Tree-Conflict: ((( Delete ---> Modified ))) 25 ;)
So I'd like to stay near the top of this list, because most of the time, the
additional columns will be empty anyway, making it harder to see which
normal notification (on the far left) goes with which path.
Also, the one I like best conforms to the way notification is used until
now: we're just adding two columns for conflict action and reason, no more
and no less. And naturally this should be similar across commands.
>
>>
>>
>> Either way, I think this branch is ready for reintegration into trunk!!
>> What do you think, Steve?
>> (still in need of a complete `make check' though)
>
> Yes, if the switch and merge tests aren't badly affected. Looking
> into that now.
Well, actually, that was a bit quick by me. Let's bugfix some more.
>> The complete local tree was removed, so if you try to add a file a few
>> levels inside a removed dir, all the non-existing path elements are in
>> tree
>> conflict. Makes sense to me.
>
> But the whole tree is already tree-conflicted, so raising new tree
> conflicts insided it is redundant.
Yes. Somehow I forgot to not want redundancy.
>> The check_tree_conflict() vs. svn_wc_conflicted_p() discussion made me
>> think: In the tests, I guess we should also try to run the same operation
>> that causes the conflicts again, to check that the persisting conflicts
>> aren't re-raised or missed altogether. Is this accounted for yet?
>
> No, currently we test only fresh tree conflicts. Good point.
I'll try that now if you haven't already.
>
>>
>>
>> And to squeeze yet another topic into this mail: What's the deal with the
>> run_and_verify_unquiet_status tests -- is status not intended to show
>> tree-conflicts (on missing nodes) by default?
>
> True. The final status check in every run_and_verify_X() runs with the
> -q option, ignoring files and dirs that aren't under version control.
Ah, I see ... Nice catch!
>
> I think I'll change our DeepTreesTestCase class (on trunk) to do the
> status check unquietly, because so many t.c. victims don't exist.
+1
~Neels
Re: Stages of tree conflict detection in "update" [was:
branch tree-conflicts-notify: merge to trunk?]
Posted by Julian Foad <ju...@btopenworld.com>.
On Sat, 2008-11-01 at 02:04 +0100, Neels J. Hofmeyr wrote:
>
> Julian Foad wrote:
> > How's the attached doc-string to go at the top of update_editor.c?
> >
> > - Julian
> >
> >
> >
> +/*
> + * This code handles "checkout" and "update" and "switch".
> + * A checkout is similar to an update that is only adding new items.
>
> ### maybe also say
> + * An "update" and a "switch" are essentially the same operation.
>
> + *
> + * The intended behaviour of "update" and "switch", focusing on the checks
> + * to be made before applying a change, is:
> + *
> + * For each incoming change:
> + * if target is already in conflict or obstructed:
> + * skip this change
> + * else
> + * if this action will cause a tree conflict:
> + * record the tree conflict
> + * skip this change
> + * else:
> + * make this change
>
> :) nice
>
> + *
> + * In more detail:
> + *
> + * For each incoming change:
> + *
> + * 1. if # Incoming change is inside an item already in conflict:
> + * a. tree/text/prop change to node beneath tree-conflicted dir
> + * then # Skip all changes in this conflicted subtree [*1]:
> + * do not update the Base nor the Working
> + * notify "skipped because already in conflict" just once
> + * for the whole conflicted subtree
> + *
> + * if # Incoming change affects an item already in conflict:
> + * b. tree/text/prop change to tree-conflicted dir/file, or
> + * c. tree change to a text/prop-conflicted file/dir, or
> + * d. text/prop change to a text/prop-conflicted file/dir [*2], or
> + * e. tree change to a dir tree containing any conflicts,
> + * then # Skip this change [*1]:
> + * do not update the Base nor the Working
> + * notify "skipped because already in conflict"
> + *
> + * 2. if # Incoming change affects an item that's "obstructed":
> + * a. on-disk node kind doesn't match recorded Working node kind
> + * (including an absence/presence mis-match),
> + * then # Skip this change [*1]:
> + * do not update the Base nor the Working
> + * notify "skipped because obstructed"
> + *
> + * 3. if # Incoming change raises a tree conflict:
> + * a. tree/text/prop change to node beneath sched-delete dir, or
> + * b. tree/text/prop change to sched-delete dir/file, or
> + * c. text/prop change to tree-scheduled dir/file,
> + * then # Skip this change:
> + * do not update the Base nor the Working [*3]
> + * notify "tree conflict"
> + *
> + * 4. Apply the change:
> + * update the Base
> + * update the Working, possibly raising text/prop conflicts
> + * notify
> + *
> + * Notes:
> + *
> + * "Tree change" here refers to an add or delete of the target node,
> + * including the add or delete part of a copy or move or rename.
> + *
> + * [*1] We should skip changes to an entire node, as the base revision number
> + * applies to the entire node. Not sure how this affects attempts to
> + * handle text and prop changes separately.
> + *
> + * [*2] Details of which combinations of property and text changes conflict
> + * are not specified here.
>
> ### Maybe say where they are specified. Are they?
I don't know if these details are written down anywhere, so I have no
reference to insert.
I committed the doc-string as it was.
- Julian
>
> + *
> + * [*3] For now, we skip the update, and require the user to:
> + * - Modify the WC to be compatible with the incoming change;
> + * - Mark the conflict as resolved;
> + * - Repeat the update.
> + * Ideally, it would be possible to resolve any conflict without
> + * repeating the update. To achieve this, we would have to store the
> + * necessary data at conflict detection time, and delay the update of
> + * the Base until the time of resolving.
> + */
>
>
> Looks good!
> ~Neels
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Stages of tree conflict detection in "update" [was: branch tree-conflicts-notify:
merge to trunk?]
Posted by "Neels J. Hofmeyr" <ne...@elego.de>.
Julian Foad wrote:
> How's the attached doc-string to go at the top of update_editor.c?
>
> - Julian
>
>
>
+/*
+ * This code handles "checkout" and "update" and "switch".
+ * A checkout is similar to an update that is only adding new items.
### maybe also say
+ * An "update" and a "switch" are essentially the same operation.
+ *
+ * The intended behaviour of "update" and "switch", focusing on the checks
+ * to be made before applying a change, is:
+ *
+ * For each incoming change:
+ * if target is already in conflict or obstructed:
+ * skip this change
+ * else
+ * if this action will cause a tree conflict:
+ * record the tree conflict
+ * skip this change
+ * else:
+ * make this change
:) nice
+ *
+ * In more detail:
+ *
+ * For each incoming change:
+ *
+ * 1. if # Incoming change is inside an item already in conflict:
+ * a. tree/text/prop change to node beneath tree-conflicted dir
+ * then # Skip all changes in this conflicted subtree [*1]:
+ * do not update the Base nor the Working
+ * notify "skipped because already in conflict" just once
+ * for the whole conflicted subtree
+ *
+ * if # Incoming change affects an item already in conflict:
+ * b. tree/text/prop change to tree-conflicted dir/file, or
+ * c. tree change to a text/prop-conflicted file/dir, or
+ * d. text/prop change to a text/prop-conflicted file/dir [*2], or
+ * e. tree change to a dir tree containing any conflicts,
+ * then # Skip this change [*1]:
+ * do not update the Base nor the Working
+ * notify "skipped because already in conflict"
+ *
+ * 2. if # Incoming change affects an item that's "obstructed":
+ * a. on-disk node kind doesn't match recorded Working node kind
+ * (including an absence/presence mis-match),
+ * then # Skip this change [*1]:
+ * do not update the Base nor the Working
+ * notify "skipped because obstructed"
+ *
+ * 3. if # Incoming change raises a tree conflict:
+ * a. tree/text/prop change to node beneath sched-delete dir, or
+ * b. tree/text/prop change to sched-delete dir/file, or
+ * c. text/prop change to tree-scheduled dir/file,
+ * then # Skip this change:
+ * do not update the Base nor the Working [*3]
+ * notify "tree conflict"
+ *
+ * 4. Apply the change:
+ * update the Base
+ * update the Working, possibly raising text/prop conflicts
+ * notify
+ *
+ * Notes:
+ *
+ * "Tree change" here refers to an add or delete of the target node,
+ * including the add or delete part of a copy or move or rename.
+ *
+ * [*1] We should skip changes to an entire node, as the base revision number
+ * applies to the entire node. Not sure how this affects attempts to
+ * handle text and prop changes separately.
+ *
+ * [*2] Details of which combinations of property and text changes conflict
+ * are not specified here.
### Maybe say where they are specified. Are they?
+ *
+ * [*3] For now, we skip the update, and require the user to:
+ * - Modify the WC to be compatible with the incoming change;
+ * - Mark the conflict as resolved;
+ * - Repeat the update.
+ * Ideally, it would be possible to resolve any conflict without
+ * repeating the update. To achieve this, we would have to store the
+ * necessary data at conflict detection time, and delay the update of
+ * the Base until the time of resolving.
+ */
Looks good!
~Neels
Re: Stages of tree conflict detection in "update" [was: branch
tree-conflicts-notify: merge to trunk?]
Posted by Julian Foad <ju...@btopenworld.com>.
How's the attached doc-string to go at the top of update_editor.c?
- Julian
Re: Stages of tree conflict detection in "update" [was: branch
tree-conflicts-notify: merge to trunk?]
Posted by Julian Foad <ju...@btopenworld.com>.
On Tue, 2008-10-28 at 17:59 +0100, Stephen Butler wrote:
> Quoting Julian Foad <ju...@btopenworld.com>:
>
> >
> > I agree: one notification per conflicted sub-tree. Revised text:
> >
> > [[[
> > * 1. if # Incoming change inside an item already in conflict:
> > * a.
> > * tree/text/prop change to node beneath tree-conflicted dir
> > * then # Skip all changes in this conflicted subtree [2]:
> > * do not update the Base nor the Working
> > * notify "skipped because already in conflict" just once
> > * for the whole conflicted subtree
> > *
> > * if # Incoming change affects an item already in conflict:
> > * b.
> > * tree/text/prop change to tree-conflicted dir/file, or
> > * e.
> > * tree change to a dir tree containing any conflicts, or
> > * c.
> > * tree change to a text/prop-conflicted file/dir, or
> > * d.
> > * text/prop change to a text/prop-conflicted file/dir[1],
> > * then # Skip this change [2]:
> > * do not update the Base nor the Working
> > * notify "skipped because already in conflict"
> > *
> > ]]]
> >
> > I've also added a condition "e." which was missing.
>
> I'm afraid I don't understand condition "e". It sounds rather expensive,
> applying svn_wc_conflicted_p2() to everything in a dir tree. Or did you
> have something else in mind?
I do mean we would have to look for any conflicts on the whole tree that
is to be deleted. (Deletion is the only tree change that this condition
could apply to.) I don't see it as any more expensive than other things
we're doing such as (in a merge) comparing the contents of the subtree
for equality before deleting it.
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Stages of tree conflict detection in "update" [was: branch
tree-conflicts-notify: merge to trunk?]
Posted by Stephen Butler <sb...@elego.de>.
Quoting Julian Foad <ju...@btopenworld.com>:
>
> I agree: one notification per conflicted sub-tree. Revised text:
>
> [[[
> * 1. if # Incoming change inside an item already in conflict:
> * a.
> * tree/text/prop change to node beneath tree-conflicted dir
> * then # Skip all changes in this conflicted subtree [2]:
> * do not update the Base nor the Working
> * notify "skipped because already in conflict" just once
> * for the whole conflicted subtree
> *
> * if # Incoming change affects an item already in conflict:
> * b.
> * tree/text/prop change to tree-conflicted dir/file, or
> * e.
> * tree change to a dir tree containing any conflicts, or
> * c.
> * tree change to a text/prop-conflicted file/dir, or
> * d.
> * text/prop change to a text/prop-conflicted file/dir[1],
> * then # Skip this change [2]:
> * do not update the Base nor the Working
> * notify "skipped because already in conflict"
> *
> ]]]
>
> I've also added a condition "e." which was missing.
I'm afraid I don't understand condition "e". It sounds rather expensive,
applying svn_wc_conflicted_p2() to everything in a dir tree. Or did you
have something else in mind?
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
Stages of tree conflict detection in "update" [was: branch
tree-conflicts-notify: merge to trunk?]
Posted by Julian Foad <ju...@btopenworld.com>.
On Mon, 2008-10-27 at 20:36 +0100, Stephen Butler wrote:
> Quoting Julian Foad <ju...@btopenworld.com>:
> > Now, stages of detection...
BTW, I was talking only about update/switch, not merge.
> I hadn't thought about sub-items 1c or 1d above. Good catch.
>
> So IIUC we'll print one line of output upon detecting an existing conflict
> (1b, 1c, 1d above), or creating fresh tree conflict (#3 below).
>
> I think we should be completely silent for the rest of a conflicted
> tree (1a), to avoid burying the user in lots of "Skipped" messages.
I agree: one notification per conflicted sub-tree. Revised text:
[[[
* 1. if # Incoming change inside an item already in conflict:
* a.
* tree/text/prop change to node beneath tree-conflicted dir
* then # Skip all changes in this conflicted subtree [2]:
* do not update the Base nor the Working
* notify "skipped because already in conflict" just once
* for the whole conflicted subtree
*
* if # Incoming change affects an item already in conflict:
* b.
* tree/text/prop change to tree-conflicted dir/file, or
* e.
* tree change to a dir tree containing any conflicts, or
* c.
* tree change to a text/prop-conflicted file/dir, or
* d.
* text/prop change to a text/prop-conflicted file/dir[1],
* then # Skip this change [2]:
* do not update the Base nor the Working
* notify "skipped because already in conflict"
*
]]]
I've also added a condition "e." which was missing.
> Too much output would distract the user from the initial message,
> which describes where the tree conflict is. After resolving the tree
> conflict, the user can check status or resume update/switch, and find
> any other conflicts in the tree.
Yes.
> > *
> > * 2. if # Incoming change affects an item that's "obstructed":
> > * on-disk node kind doesn't match recorded Working node kind,
> > * including an absence/presence mis-match
> > * then # Skip this change [2]:
> > * do not update the Base nor the Working
> > * notify "skipped because obstructed"
> > *
> > * 3. if # Incoming change raises a tree conflict:
> > * tree/text/prop change to node beneath tree-scheduled[3] dir
Like in (1.), here we insert:
* then # Skip all changes in conflicted subtree
* do not update the Base nor the Working
* notify "skipped because already in conflict" just once
* for the whole conflicted subtree
*
* if
> > * tree/text/prop change to tree-scheduled[3] dir/file, or
> > * text/prop change to tree-scheduled dir/file,
> > * then # Skip this change:
> > * do not update the Base nor the Working [4]
> > * notify "tree conflict"
> > *
> > * 4. Apply the change:
[...]
> Here's my basic implementation plan:
>
> The following update callbacks need to check for existing conflicts:
>
> open_directory
> open_file
> add_directory
> add_file
> delete_entry
> absent_file
> absent_directory
>
> In each callback, we'll make the following checks/calls.
>
1.a.
> (eb->inside_a_tree_conflict)
>
> True if we're walking a tree-conflicted tree. Works even if the
> target and its parent dir do not exist (e.g., we've skipped adding
> the parent dir due to the tree conflict). Skip target silently if
> true.
>
1.b/c/d
> svn_wc_conflicted_p2()
>
> Check for existing conflicts. Requires a versioned parent dir for
> a tree conflict. Requires a versioned parent dir and versioned
> target for text or prop conflicts. Skip target with notification
> if conflicts are found.
1.e. may need a new check.
2. - obstructions - has a fair amount of existing code but should be
rationalized.
3.
> check_tree_conflict(&tree_conflict, eb, ...)
>
> Returns a fresh tree conflict if the target is a victim. Requires
> a versioned parent dir. If a conflict is found, record it
> persistently.
>
> notify_tree_conflict(tree_conflict, eb, ...)
>
> If there's a tree conflict and an eb->notify_func, notify the user.
This should differ:
for a newly raised tree conflict, notify "tree conflict"
for a pre-existing conflict, notify "skipped (because of a conflict)"
But any notification is better than none, to start with.
Thanks.
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: branch tree-conflicts-notify: merge to trunk?
Posted by Julian Foad <ju...@btopenworld.com>.
On Tue, 2008-10-28 at 05:36 +0100, Neels J. Hofmeyr wrote:
[...]
> This is the tiny tip of an iceberg that will require the diff callbacks to
> change (but a different change than what diff --summarize needs). The merge
> code needs to be tree-conflict aware. The relevant action takes place in the
> diff_callbacks, and it is kind of difficult to get the information out of
> there: diff callbacks return each conflict state (text, prop) separately via
> a pointer argument passed to the callback function. So we need another
> pointer argument to return a tree-conflicted state to the diff callbacks'
> notification layer (I'm saying that the callbacks signatures need to have
> another argument added). BTW, I really don't like the design of the diff
> callbacks in that respect. It takes lots of zombie typing to refactor.
Yes. The callbacks don't pass around a baton that they could use to
collect this info and process it at (e.g.) dir_close() time.
That's another argument to move to using "editor" callbacks.
[...]
> BTW, the branch tree-conflicts-notify can take no more of this. It should go
> back to trunk -- *now*.
Yes.
[Test failures ...]
> Now, do we have a design problem here? Maybe it *is* necessary to block a
> recursive commit on a directory that has *missing* items that remain in
> tree-conflict, and thus we *do* need to block a commit in a directory that
> *contains* certain tree conflicts? Repeat:
> - A commit should only be blocked when the whole directory is committed
> recursively. This already happens for nodes that are actually present, but
> missing ones currently don't get checked.
> - When committing non-conflicted siblings of a tree-conflict victim
> explicitly, then that shouldn't block.
> So maybe we *do* still need the HAS_TREE_CONFLICTED_CHILDREN as an optional
> return value in svn_wc_conflicted_p2().
>
> This is slowly turning into quite a landscape of numerous different levels
> of tree-conflicts:
>
> tree-conflicts of relevance on a given node:
> - just been newly raised on this node.
> - just been newly raised on an ancestor while crawling.
> - persisting on this node (from a previous operation).
> - persisting on any ancestors.
> - persisting on a *missing* child node, when recursing a dir.
>
>
> Hey, what if I have
> svn up
> C A
> but I do
> cd A/sub/dir
> svn ci
> -- will the commit fail to block because we're not crawling up into the
> ancestors of the current working copy directory? Do we have to persist the
> complete subtree of a tree-conflicted WC dir as conflicted, even though not
> notifying or info-ing about it? There could be a flag saying "yeah, you
> can't commit me, but it's one of my ancestors' fault, so you have to fix
> stuff further up". (Or is this already accounted for?)
>
> Wow, tree-conflicts are really complicated. There's, like, whole *layers* of
> stuff. ;)
Urg. We are way out of our depth.
> > Aside from sorting out these tests, we also need to make some new
> > tests for detection of existing conflicts, following the general
> > scheme that you outline below. Oops, guess I just volunteered. :-o
>
> I thought of kind of changing the DeepTreesTestCase running code to just
> repeat whatever it did to raise its tree-conflicts and try to evaluate
> (maybe we need to pass in more expected_stuff or maybe there's a simple
> generic way of checking whether a persistent tree-conflict is handled
> correctly). But I'd be happy to leave this to you or anyone else, as long as
> it's not me. ;)
>
> >> I hope it's OK if I do a catch-up merge on your branch. Last Friday (I
>
> Yes, go ahead. :)
Done. r33925.
[...]
> >> Now, stages of detection...
> >>
> >> We could be clearer on exactly what Subversion should be doing during
> >> "svn update/switch", with respect to conflict detection and skipping of
> >> obstructions.
> >>
> >> One of you mentioned before that a note about this would make a good
> >> code comment in update_editor.c, and I agree.
>
> I was actually meaning that exact structure I saw there. I find below
> enumeration quite difficult to filter... Let me see, originally, it was this:
>
> if target was already in conflict:
> skip this action
> else
> if this action will cause a tree conflict:
> record the tree conflict
> skip this action
> else:
> do this action
OK, I'll include that brief version too.
> > I think we should be completely silent for the rest of a conflicted
> > tree (1a), to avoid burying the user in lots of "Skipped" messages.
> > Too much output would distract the user from the initial message,
> > which describes where the tree conflict is. After resolving the tree
> > conflict, the user can check status or resume update/switch, and find
> > any other conflicts in the tree.
>
> So here we're talking about the tree-conflicts that persist from a previous
> operation? Then -1, I'd still like to see (1a) notifications, or at least a
> skip notification for the topmost items of (1a)
Sure. Note that Steve said "silent for the REST of a ...", meaning
exactly one notification inside each conflicted subtree. At least that's
how I think.
> (I mean that it says
> "skipped this whole tree because it is inside a tree-conflicted dir", but
> would have to report scattered files separately, probably). Otherwise some
> possible scenarios would just return without ANY notification at all, not
> indicating that it is skipping something due to a persisting tree-conflict.
>
> $ svn <operation1>
> C my/dir
> $ svn <operation2> (wants to add file `my/dir/subtree/addition')
> Skipped my/dir/subtree/addition, because my/dir remains in conflict.
>
> Hm, or maybe it should say something like
> $ svn <operation2>
> Skipped my/dir, because it remains in conflict.
> even though my/dir was *no* explicit target. This could become very
> confusing when stepping into my/dir/subtree, calling <operation2> and
> reading something about "Skipped my/dir" although it is not below, but above
> this directory. Run for cover! Worms!!
Urg.
> >> *
> >> * 2. if # Incoming change affects an item that's "obstructed":
> >> * on-disk node kind doesn't match recorded Working node kind,
> >> * including an absence/presence mis-match
> >> * then # Skip this change [2]:
> >> * do not update the Base nor the Working
> >> * notify "skipped because obstructed"
> >> *
> >> * 3. if # Incoming change raises a tree conflict:
> >> * tree/text/prop change to node beneath tree-scheduled[3] dir, or
> >> * tree/text/prop change to tree-scheduled[3] dir/file, or
> >> * text/prop change to tree-scheduled dir/file,
> >> * then # Skip this change:
> >> * do not update the Base nor the Working [4]
> >> * notify "tree conflict"
>
> Here, in contrast, a *newly raised* tree-conflict in a directory should
> cause all "skip" messages of sub-nodes to be omitted.
>
> >> *
> >> * 4. Apply the change:
> >> * update the Base
> >> * update the Working, possibly raising text/prop conflicts
> >> * notify as in v1.5
>
> So this is the final "else", the "default:" case, the "what we do if nothing
> above went wrong", right?
Yes.
[...]
> > Here's my basic implementation plan:
> >
> > The following update callbacks need to check for existing conflicts:
> >
> > open_directory
> > open_file
> > add_directory
> > add_file
> > delete_entry
> > absent_file
> > absent_directory
> >
> > In each callback, we'll make the following checks/calls.
> >
> > (eb->inside_a_tree_conflict)
> >
> > True if we're walking a tree-conflicted tree. Works even if the
> > target and its parent dir do not exist (e.g., we've skipped adding
> > the parent dir due to the tree conflict). Skip target silently if
> > true.
>
> Nice. We could actually add an eb->inside_a_tree_conflict_was_notified or
> something, which the topmost conflict cause sets to TRUE if it notified
> about the conflict. Then, when having a subtree as target, this is FALSE and
> indicates that we should still notify about a skip.
>
> >
> > svn_wc_conflicted_p2()
> >
> > Check for existing conflicts. Requires a versioned parent dir for
> > a tree conflict. Requires a versioned parent dir and versioned
> > target for text or prop conflicts. Skip target with notification
> > if conflicts are found.
> >
> > check_tree_conflict(&tree_conflict, eb, ...)
> >
> > Returns a fresh tree conflict if the target is a victim. Requires
> > a versioned parent dir. If a conflict is found, record it
> > persistently.
>
> So, this may eventually split into detect_- and raise_tree_conflict().
> Although I'd rather call that persist_tree_conflict() instead of raise_,
> raise sounds too much like "notify and bail out", both of which it doesn't.
Agreed.
[...]
> Thanks guys for the good collaboration, I'm benefiting greatly from your posts.
Oh, cool. I'm feeling overwhelmed today by the number of long emails
with lots of hard stuff in each.
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: branch tree-conflicts-notify: merge to trunk?
Posted by "Neels J. Hofmeyr" <ne...@elego.de>.
Stephen Butler wrote:
> Quoting Julian Foad <ju...@btopenworld.com>:
[...]
>> I got these failures when I tried the branch a couple of hours ago
>> (r33896):
>> FAIL: externals_tests.py 14: old 'PATH URL' format should ignore peg
>> revisions
>> FAIL: externals_tests.py 16: place a file external into a directory
>> external
>
> I'll throw in a couple of svn resolved command to get rid of the
> inadvertently-created tree conflicts (of the add:add variety).
>
>> FAIL: merge_tests.py 103: merge tries to delete a file of different
>> content
>
> For me test 103 fails intermittently. When it does fail, the expected
> stdout and actual stdout appear to match anyway. Rather strange.
>
> EXPECTED STDOUT:
> Skipped 'A/D/G2/tau'
> --- Merging r2 into 'A/D/G2':
> C A/D/G2
> Summary of conflicts:
> Tree conflicts: 1
> Skipped paths: 1
> ACTUAL STDOUT:
> Skipped 'A/D/G2/tau'
> --- Merging r2 into 'A/D/G2':
> C A/D/G2
> Summary of conflicts:
> Text conflicts: 1
> Skipped paths: 1
> EXCEPTION: SVNLineUnequal
> Traceback [...]
The error here is that in the ACTUAL STDOUT, it says "Text conflicts", not
"Tree conflicts". (That has caught me once before: When quickly comparing
outputs, "Text" and "Tree" is easily mistaken to be the same.) I haven't
changed the test because it calls for deeper fixes.
This is the tiny tip of an iceberg that will require the diff callbacks to
change (but a different change than what diff --summarize needs). The merge
code needs to be tree-conflict aware. The relevant action takes place in the
diff_callbacks, and it is kind of difficult to get the information out of
there: diff callbacks return each conflict state (text, prop) separately via
a pointer argument passed to the callback function. So we need another
pointer argument to return a tree-conflicted state to the diff callbacks'
notification layer (I'm saying that the callbacks signatures need to have
another argument added). BTW, I really don't like the design of the diff
callbacks in that respect. It takes lots of zombie typing to refactor.
Since the returned conflict arguments are of type svn_wc_notify_state_t, I
once thought of adding a svn_wc_notify_tree_conflicted field to the enum,
but that's no good: We have two separate values, content_state and
prop_state, and either one could then be tree conflicted. We'd have to check
both all over, and get confused because the data structure doesn't reflect
the actual orthogonality.
BTW, the branch tree-conflicts-notify can take no more of this. It should go
back to trunk -- *now*.
>
>> FAIL: stat_tests.py 31: status with tree conflicts
>
> Expected status
>
>> FAIL: tree_conflict_tests.py 9: merge file: modify onto not-file
>> FAIL: tree_conflict_tests.py 11: merge file: del/rpl/mv onto not-file
>> FAIL: tree_conflict_tests.py 13: merge dir: modify onto not-dir
>> FAIL: tree_conflict_tests.py 15: merge dir: del/rpl/mv onto not-dir
>
> In all 4 cases, commit of a dir containing a text-conflicted file
> produced no output, not even an error message. Not sure how we broke
> this.
I think I found why. We've changed the code to return the tree-conflicted
state on the victim, but these tests erroneously rely on the previous
behaviour where the conflict is reported on the victim's parent and blocks
that parent.
E.g. tree_conflict_tests.py 9 skips a missing target in a directory (the
directory branch2/ft has no file F in it). That is a tree-conflict and gets
noted:
$ svn status
! C branch2/ft/F
However, merge does not have per-victim notification yet (see way above),
and when above conflict was raised, it showed
$ svn merge <stuff>
C branch2/ft
as in the olden days: a "text conflict" on the parent. Furthermore, the test
expects branch2/ft to be in conflict, but it now isn't anymore. We removed
*HAS_TREE_CONFLICTED_CHILDREN from svn_wc_conflicted_p2(), so the commit
doesn't get a tree-conflict on branch2/ft anymore. The real conflicted item
is branch2/ft/F, which is missing and doesn't get tested by commit in the
first place.
Now, do we have a design problem here? Maybe it *is* necessary to block a
recursive commit on a directory that has *missing* items that remain in
tree-conflict, and thus we *do* need to block a commit in a directory that
*contains* certain tree conflicts? Repeat:
- A commit should only be blocked when the whole directory is committed
recursively. This already happens for nodes that are actually present, but
missing ones currently don't get checked.
- When committing non-conflicted siblings of a tree-conflict victim
explicitly, then that shouldn't block.
So maybe we *do* still need the HAS_TREE_CONFLICTED_CHILDREN as an optional
return value in svn_wc_conflicted_p2().
This is slowly turning into quite a landscape of numerous different levels
of tree-conflicts:
tree-conflicts of relevance on a given node:
- just been newly raised on this node.
- just been newly raised on an ancestor while crawling.
- persisting on this node (from a previous operation).
- persisting on any ancestors.
- persisting on a *missing* child node, when recursing a dir.
Hey, what if I have
svn up
C A
but I do
cd A/sub/dir
svn ci
-- will the commit fail to block because we're not crawling up into the
ancestors of the current working copy directory? Do we have to persist the
complete subtree of a tree-conflicted WC dir as conflicted, even though not
notifying or info-ing about it? There could be a flag saying "yeah, you
can't commit me, but it's one of my ancestors' fault, so you have to fix
stuff further up". (Or is this already accounted for?)
Wow, tree-conflicts are really complicated. There's, like, whole *layers* of
stuff. ;)
>
> Aside from sorting out these tests, we also need to make some new
> tests for detection of existing conflicts, following the general
> scheme that you outline below. Oops, guess I just volunteered. :-o
I thought of kind of changing the DeepTreesTestCase running code to just
repeat whatever it did to raise its tree-conflicts and try to evaluate
(maybe we need to pass in more expected_stuff or maybe there's a simple
generic way of checking whether a persistent tree-conflict is handled
correctly). But I'd be happy to leave this to you or anyone else, as long as
it's not me. ;)
>> I hope it's OK if I do a catch-up merge on your branch. Last Friday (I
Yes, go ahead. :)
>> think) I tweaked the "check_tree_conflict()" function in update_editor.c
>> to return the conflict description it raised, and I committed that to
>> trunk, while you had done it a slightly different way (returning a
>> boolean flag instead) on the branch. Returning the whole conflict should
>> allow us more flexibility to choose how much detail we put into the
>> notification, like I do in the attached patch by sending it to the new
>> function "notify_new_tree_conflict()".
+1, but
> Yes, I suppose returning the whole conflict is the long-term solution.
> I'd like to merge the branch to trunk ASAP, though. Can we do the
> boolean->conflict type change in a separate commit to trunk?
+1
>> Also I think check_tree_conflict() may benefit (sooner or later) from
>> being split into separate "detect" and "raise" functions, with the
>> conflict description being returned by the former and passed to the
>> latter.
+1, as soon as it is needed.
>
> Yes, the tree conflict notification should be encapsulated in a simple
> function.
Now, `raise' and `notify' are entirely different things. `raise' makes the
entry reflect the tree-conflict. `notify' is very specific to the respective
surrounding code in the way that the notification is propagated. So, `raise'
should be a simple function. But `notify' unfortunately can't: e.g. in the
update_editor.c, notify_func() calls are right in between everything else.
That would qualify for a simple tree-conflicts notification function. But
merge, for example, uses the diff callbacks that have the iceberg mentioned
above, so we don't have access to the notify_func and can't call it
ourselves. So I guess it's a show-stopper for combining `raise' and `notify'
in the same function.
There's some wrapping/evaluating done around the notification states
returned by the diff callbacks, and I don't think we can easily change the
diff callbacks to just provide the darn notification function like it was
intended.
>
> Currently the tree conflict notifications are mixed in with other
> notifications, leading to output such as 'D C'. But very soon we'll
> eliminate that mixing, by skipping updates to the victims.
Hey, haven't I already done that? I think I have. (on the branch)
> Then we
> can notify the user of a tree conflict immediately instead of waiting
> for dir_close or file_close (in some cases).
Depends on the caller, s.a..
>
>>
>> Yell if that seems wrong.
>>
>>
>> Now, stages of detection...
>>
>> We could be clearer on exactly what Subversion should be doing during
>> "svn update/switch", with respect to conflict detection and skipping of
>> obstructions.
>>
>> One of you mentioned before that a note about this would make a good
>> code comment in update_editor.c, and I agree.
I was actually meaning that exact structure I saw there. I find below
enumeration quite difficult to filter... Let me see, originally, it was this:
if target was already in conflict:
skip this action
else
if this action will cause a tree conflict:
record the tree conflict
skip this action
else:
do this action
and from there, I see that your text aims to more precisely define this
pseudocode, and it adds an "else if obstructed":
>>
>> Does this look right?
>>
>> [[[
>> /*
>> * An "update" or "switch" should include, in this order:
>> *
>> * 1. if # Incoming change affects an item already in conflict:
> a.)
>> * tree/text/prop change to node beneath tree-conflicted dir,
>> or
> b.)
>> * tree/text/prop change to tree-conflicted dir/file, or
> c.)
>> * tree change to a text/prop-conflicted file/dir, or
> d.)
>> * text/prop change to a text/prop-conflicted file/dir[1],
>> * then # Skip this change [2]:
>> * do not update the Base nor the Working
>> * notify "skipped because already in conflict"
>
> I hadn't thought about sub-items 1c or 1d above. Good catch.
>
> So IIUC we'll print one line of output upon detecting an existing conflict
> (1b, 1c, 1d above), or creating fresh tree conflict (#3 below).
>
> I think we should be completely silent for the rest of a conflicted
> tree (1a), to avoid burying the user in lots of "Skipped" messages.
> Too much output would distract the user from the initial message,
> which describes where the tree conflict is. After resolving the tree
> conflict, the user can check status or resume update/switch, and find
> any other conflicts in the tree.
So here we're talking about the tree-conflicts that persist from a previous
operation? Then -1, I'd still like to see (1a) notifications, or at least a
skip notification for the topmost items of (1a) (I mean that it says
"skipped this whole tree because it is inside a tree-conflicted dir", but
would have to report scattered files separately, probably). Otherwise some
possible scenarios would just return without ANY notification at all, not
indicating that it is skipping something due to a persisting tree-conflict.
$ svn <operation1>
C my/dir
$ svn <operation2> (wants to add file `my/dir/subtree/addition')
Skipped my/dir/subtree/addition, because my/dir remains in conflict.
Hm, or maybe it should say something like
$ svn <operation2>
Skipped my/dir, because it remains in conflict.
even though my/dir was *no* explicit target. This could become very
confusing when stepping into my/dir/subtree, calling <operation2> and
reading something about "Skipped my/dir" although it is not below, but above
this directory. Run for cover! Worms!!
>
>
>> *
>> * 2. if # Incoming change affects an item that's "obstructed":
>> * on-disk node kind doesn't match recorded Working node kind,
>> * including an absence/presence mis-match
>> * then # Skip this change [2]:
>> * do not update the Base nor the Working
>> * notify "skipped because obstructed"
>> *
>> * 3. if # Incoming change raises a tree conflict:
>> * tree/text/prop change to node beneath tree-scheduled[3] dir, or
>> * tree/text/prop change to tree-scheduled[3] dir/file, or
>> * text/prop change to tree-scheduled dir/file,
>> * then # Skip this change:
>> * do not update the Base nor the Working [4]
>> * notify "tree conflict"
Here, in contrast, a *newly raised* tree-conflict in a directory should
cause all "skip" messages of sub-nodes to be omitted.
>> *
>> * 4. Apply the change:
>> * update the Base
>> * update the Working, possibly raising text/prop conflicts
>> * notify as in v1.5
So this is the final "else", the "default:" case, the "what we do if nothing
above went wrong", right?
Then I agree with this, though I haven't checked it to the letter, nor
thought deeply about cases that might be missing.
>> *
>> *
>> * [1] Details of which combinations of property and text changes
>> conflict
>> * are out of scope here and defined by v1.5 behaviour.
>> *
>> * [2] Should skip changes to an entire node, as the base revision number
>> * applies to the whole node. Not sure how this affects attempts to
>> * handle text and prop changes separately.
>> *
>> * [3] Not all combinations implied can happen, but that doesn't matter.
>> *
>> * [4] For now, we skip the update, and require the user to:
>> * - Modify the WC to be compatible with the incoming change;
>> * - Mark the conflict as resolved;
>> * - Repeat the update.
>> * Ideally, it would be possible to resolve any conflict without
>> * repeating the update. To achieve this, we could store the possible
>> * new base(s) at conflict detection time, and update the actual
>> base at
>> * the time of resolving.
I don't really understand this. It sounds quite simple, but I can't
visualize much detail.
>
> All in all this looks good to me.
Yeah.
>
>>
>>
>> I noticed considerable inconsistencies in this, in the current "update"
>> code, while trying to make "update" skip changes inside a
>> tree-conflicted directory tree. I haven't worked through trying to
>> implement the above, I'm just telling you that's somthing I'm looking
>> at, as I think we need to get this behaviour clearly specified.
>
> Yes the current update code works only for fresh tree conflicts. It's
> definitely incomplete.
Yes. I've only done the "fresh conflicts" part so far.
>
> Here's my basic implementation plan:
>
> The following update callbacks need to check for existing conflicts:
>
> open_directory
> open_file
> add_directory
> add_file
> delete_entry
> absent_file
> absent_directory
>
> In each callback, we'll make the following checks/calls.
>
> (eb->inside_a_tree_conflict)
>
> True if we're walking a tree-conflicted tree. Works even if the
> target and its parent dir do not exist (e.g., we've skipped adding
> the parent dir due to the tree conflict). Skip target silently if
> true.
Nice. We could actually add an eb->inside_a_tree_conflict_was_notified or
something, which the topmost conflict cause sets to TRUE if it notified
about the conflict. Then, when having a subtree as target, this is FALSE and
indicates that we should still notify about a skip.
>
> svn_wc_conflicted_p2()
>
> Check for existing conflicts. Requires a versioned parent dir for
> a tree conflict. Requires a versioned parent dir and versioned
> target for text or prop conflicts. Skip target with notification
> if conflicts are found.
>
> check_tree_conflict(&tree_conflict, eb, ...)
>
> Returns a fresh tree conflict if the target is a victim. Requires
> a versioned parent dir. If a conflict is found, record it
> persistently.
So, this may eventually split into detect_- and raise_tree_conflict().
Although I'd rather call that persist_tree_conflict() instead of raise_,
raise sounds too much like "notify and bail out", both of which it doesn't.
>
> notify_tree_conflict(tree_conflict, eb, ...)
>
> If there's a tree conflict and an eb->notify_func, notify the user.
That will probably not work, at least for merge code, see above. Although,
then again, we might call notify_tree_conflict() in the diff_callbacks
driving structure. Then again then-again, `svn diff' doesn't *need* that at
all. And it's odd to add something to the "diff_callbacks" that is outside
the scope of a diff altogether. Or are we going to hint at tree-conflicts in
a diff as well? I don't think so. Maybe these callbacks have the wrong name.
Thanks guys for the good collaboration, I'm benefiting greatly from your posts.
I was planning to merge to trunk now but reviewing these issues took too
long. Steve, if you like you can merge tree-conflicts-notify to trunk,
otherwise I'll do that tomorrow (or as soon as possible).
This must be the longest consecutive time that I've written on a single
email, ever. "It's so long because I didn't have the time to make it shorter".
~Neels
Re: branch tree-conflicts-notify: merge to trunk?
Posted by Stephen Butler <sb...@elego.de>.
Quoting Julian Foad <ju...@btopenworld.com>:
> On Sat, 2008-10-25 at 14:15 +0200, Stephen Butler wrote:
>> Quoting "Neels J. Hofmeyr" <ne...@elego.de>:
>>
>> > Hey Steve! (sbutler)
>> >
>> > That's so awesome! Somehow you've magically fixed almost everything on the
>> > branch! All I had to do was raise and notify a tree-conflict on file
>> > addition in a conflicted directory.
>>
>> Thanks!
>
> That's great. And yes I want it on trunk too. Especially I want the
> changes in how we proceed through checking for new tree conflicts,
> checking for existing conflicts, and checking for obstructions, as I
> think we need to do that more consistently - see below.
>
> I got these failures when I tried the branch a couple of hours ago
> (r33896):
> FAIL: externals_tests.py 14: old 'PATH URL' format should ignore peg
> revisions
> FAIL: externals_tests.py 16: place a file external into a directory
> external
I'll throw in a couple of svn resolved command to get rid of the
inadvertently-created tree conflicts (of the add:add variety).
> FAIL: merge_tests.py 103: merge tries to delete a file of different
> content
For me test 103 fails intermittently. When it does fail, the expected
stdout and actual stdout appear to match anyway. Rather strange.
EXPECTED STDOUT:
Skipped 'A/D/G2/tau'
--- Merging r2 into 'A/D/G2':
C A/D/G2
Summary of conflicts:
Tree conflicts: 1
Skipped paths: 1
ACTUAL STDOUT:
Skipped 'A/D/G2/tau'
--- Merging r2 into 'A/D/G2':
C A/D/G2
Summary of conflicts:
Text conflicts: 1
Skipped paths: 1
EXCEPTION: SVNLineUnequal
Traceback [...]
> FAIL: stat_tests.py 31: status with tree conflicts
Expected status
> FAIL: tree_conflict_tests.py 9: merge file: modify onto not-file
> FAIL: tree_conflict_tests.py 11: merge file: del/rpl/mv onto not-file
> FAIL: tree_conflict_tests.py 13: merge dir: modify onto not-dir
> FAIL: tree_conflict_tests.py 15: merge dir: del/rpl/mv onto not-dir
In all 4 cases, commit of a dir containing a text-conflicted file
produced no output, not even an error message. Not sure how we broke
this.
Aside from sorting out these tests, we also need to make some new
tests for detection of existing conflicts, following the general
scheme that you outline below. Oops, guess I just volunteered. :-o
>
> I hope it's OK if I do a catch-up merge on your branch. Last Friday (I
> think) I tweaked the "check_tree_conflict()" function in update_editor.c
> to return the conflict description it raised, and I committed that to
> trunk, while you had done it a slightly different way (returning a
> boolean flag instead) on the branch. Returning the whole conflict should
> allow us more flexibility to choose how much detail we put into the
> notification, like I do in the attached patch by sending it to the new
> function "notify_new_tree_conflict()".
Yes, I suppose returning the whole conflict is the long-term solution.
I'd like to merge the branch to trunk ASAP, though. Can we do the
boolean->conflict type change in a separate commit to trunk?
>
> Also I think check_tree_conflict() may benefit (sooner or later) from
> being split into separate "detect" and "raise" functions, with the
> conflict description being returned by the former and passed to the
> latter.
Yes, the tree conflict notification should be encapsulated in a simple
function.
Currently the tree conflict notifications are mixed in with other
notifications, leading to output such as 'D C'. But very soon we'll
eliminate that mixing, by skipping updates to the victims. Then we
can notify the user of a tree conflict immediately instead of waiting
for dir_close or file_close (in some cases).
>
> Yell if that seems wrong.
>
>
> Now, stages of detection...
>
> We could be clearer on exactly what Subversion should be doing during
> "svn update/switch", with respect to conflict detection and skipping of
> obstructions.
>
> One of you mentioned before that a note about this would make a good
> code comment in update_editor.c, and I agree.
>
> Does this look right?
>
> [[[
> /*
> * An "update" or "switch" should include, in this order:
> *
> * 1. if # Incoming change affects an item already in conflict:
a.)
> * tree/text/prop change to node beneath tree-conflicted dir,
> or
b.)
> * tree/text/prop change to tree-conflicted dir/file, or
c.)
> * tree change to a text/prop-conflicted file/dir, or
d.)
> * text/prop change to a text/prop-conflicted file/dir[1],
> * then # Skip this change [2]:
> * do not update the Base nor the Working
> * notify "skipped because already in conflict"
I hadn't thought about sub-items 1c or 1d above. Good catch.
So IIUC we'll print one line of output upon detecting an existing conflict
(1b, 1c, 1d above), or creating fresh tree conflict (#3 below).
I think we should be completely silent for the rest of a conflicted
tree (1a), to avoid burying the user in lots of "Skipped" messages.
Too much output would distract the user from the initial message,
which describes where the tree conflict is. After resolving the tree
conflict, the user can check status or resume update/switch, and find
any other conflicts in the tree.
> *
> * 2. if # Incoming change affects an item that's "obstructed":
> * on-disk node kind doesn't match recorded Working node kind,
> * including an absence/presence mis-match
> * then # Skip this change [2]:
> * do not update the Base nor the Working
> * notify "skipped because obstructed"
> *
> * 3. if # Incoming change raises a tree conflict:
> * tree/text/prop change to node beneath tree-scheduled[3] dir, or
> * tree/text/prop change to tree-scheduled[3] dir/file, or
> * text/prop change to tree-scheduled dir/file,
> * then # Skip this change:
> * do not update the Base nor the Working [4]
> * notify "tree conflict"
> *
> * 4. Apply the change:
> * update the Base
> * update the Working, possibly raising text/prop conflicts
> * notify as in v1.5
> *
> *
> * [1] Details of which combinations of property and text changes conflict
> * are out of scope here and defined by v1.5 behaviour.
> *
> * [2] Should skip changes to an entire node, as the base revision number
> * applies to the whole node. Not sure how this affects attempts to
> * handle text and prop changes separately.
> *
> * [3] Not all combinations implied can happen, but that doesn't matter.
> *
> * [4] For now, we skip the update, and require the user to:
> * - Modify the WC to be compatible with the incoming change;
> * - Mark the conflict as resolved;
> * - Repeat the update.
> * Ideally, it would be possible to resolve any conflict without
> * repeating the update. To achieve this, we could store the possible
> * new base(s) at conflict detection time, and update the actual base at
> * the time of resolving.
But if we can't add a tree due to a tree conflict, where would we stash it
in the working copy? Anyway, that's a performance enhancement for future
consideration.
> */
> ]]]
All in all this looks good to me.
>
>
> I noticed considerable inconsistencies in this, in the current "update"
> code, while trying to make "update" skip changes inside a
> tree-conflicted directory tree. I haven't worked through trying to
> implement the above, I'm just telling you that's somthing I'm looking
> at, as I think we need to get this behaviour clearly specified.
Yes the current update code works only for fresh tree conflicts. It's
definitely incomplete.
Here's my basic implementation plan:
The following update callbacks need to check for existing conflicts:
open_directory
open_file
add_directory
add_file
delete_entry
absent_file
absent_directory
In each callback, we'll make the following checks/calls.
(eb->inside_a_tree_conflict)
True if we're walking a tree-conflicted tree. Works even if the
target and its parent dir do not exist (e.g., we've skipped adding
the parent dir due to the tree conflict). Skip target silently if
true.
svn_wc_conflicted_p2()
Check for existing conflicts. Requires a versioned parent dir for
a tree conflict. Requires a versioned parent dir and versioned
target for text or prop conflicts. Skip target with notification
if conflicts are found.
check_tree_conflict(&tree_conflict, eb, ...)
Returns a fresh tree conflict if the target is a victim. Requires
a versioned parent dir. If a conflict is found, record it
persistently.
notify_tree_conflict(tree_conflict, eb, ...)
If there's a tree conflict and an eb->notify_func, notify the user.
Regards,
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: branch tree-conflicts-notify: merge to trunk?
Posted by Julian Foad <ju...@btopenworld.com>.
On Sat, 2008-10-25 at 14:15 +0200, Stephen Butler wrote:
> Quoting "Neels J. Hofmeyr" <ne...@elego.de>:
>
> > Hey Steve! (sbutler)
> >
> > That's so awesome! Somehow you've magically fixed almost everything on the
> > branch! All I had to do was raise and notify a tree-conflict on file
> > addition in a conflicted directory.
>
> Thanks!
That's great. And yes I want it on trunk too. Especially I want the
changes in how we proceed through checking for new tree conflicts,
checking for existing conflicts, and checking for obstructions, as I
think we need to do that more consistently - see below.
I got these failures when I tried the branch a couple of hours ago
(r33896):
FAIL: externals_tests.py 14: old 'PATH URL' format should ignore peg
revisions
FAIL: externals_tests.py 16: place a file external into a directory
external
FAIL: merge_tests.py 103: merge tries to delete a file of different
content
FAIL: stat_tests.py 31: status with tree conflicts
FAIL: tree_conflict_tests.py 9: merge file: modify onto not-file
FAIL: tree_conflict_tests.py 11: merge file: del/rpl/mv onto not-file
FAIL: tree_conflict_tests.py 13: merge dir: modify onto not-dir
FAIL: tree_conflict_tests.py 15: merge dir: del/rpl/mv onto not-dir
I hope it's OK if I do a catch-up merge on your branch. Last Friday (I
think) I tweaked the "check_tree_conflict()" function in update_editor.c
to return the conflict description it raised, and I committed that to
trunk, while you had done it a slightly different way (returning a
boolean flag instead) on the branch. Returning the whole conflict should
allow us more flexibility to choose how much detail we put into the
notification, like I do in the attached patch by sending it to the new
function "notify_new_tree_conflict()".
Also I think check_tree_conflict() may benefit (sooner or later) from
being split into separate "detect" and "raise" functions, with the
conflict description being returned by the former and passed to the
latter.
Yell if that seems wrong.
Now, stages of detection...
We could be clearer on exactly what Subversion should be doing during
"svn update/switch", with respect to conflict detection and skipping of
obstructions.
One of you mentioned before that a note about this would make a good
code comment in update_editor.c, and I agree.
Does this look right?
[[[
/*
* An "update" or "switch" should include, in this order:
*
* 1. if # Incoming change affects an item already in conflict:
* tree/text/prop change to node beneath tree-conflicted dir, or
* tree/text/prop change to tree-conflicted dir/file, or
* tree change to a text/prop-conflicted file/dir, or
* text/prop change to a text/prop-conflicted file/dir[1],
* then # Skip this change [2]:
* do not update the Base nor the Working
* notify "skipped because already in conflict"
*
* 2. if # Incoming change affects an item that's "obstructed":
* on-disk node kind doesn't match recorded Working node kind,
* including an absence/presence mis-match
* then # Skip this change [2]:
* do not update the Base nor the Working
* notify "skipped because obstructed"
*
* 3. if # Incoming change raises a tree conflict:
* tree/text/prop change to node beneath tree-scheduled[3] dir, or
* tree/text/prop change to tree-scheduled[3] dir/file, or
* text/prop change to tree-scheduled dir/file,
* then # Skip this change:
* do not update the Base nor the Working [4]
* notify "tree conflict"
*
* 4. Apply the change:
* update the Base
* update the Working, possibly raising text/prop conflicts
* notify as in v1.5
*
*
* [1] Details of which combinations of property and text changes conflict
* are out of scope here and defined by v1.5 behaviour.
*
* [2] Should skip changes to an entire node, as the base revision number
* applies to the whole node. Not sure how this affects attempts to
* handle text and prop changes separately.
*
* [3] Not all combinations implied can happen, but that doesn't matter.
*
* [4] For now, we skip the update, and require the user to:
* - Modify the WC to be compatible with the incoming change;
* - Mark the conflict as resolved;
* - Repeat the update.
* Ideally, it would be possible to resolve any conflict without
* repeating the update. To achieve this, we could store the possible
* new base(s) at conflict detection time, and update the actual base at
* the time of resolving.
*/
]]]
I noticed considerable inconsistencies in this, in the current "update"
code, while trying to make "update" skip changes inside a
tree-conflicted directory tree. I haven't worked through trying to
implement the above, I'm just telling you that's somthing I'm looking
at, as I think we need to get this behaviour clearly specified.
Comments?
- Julian
(n.b. the attached "skip-conflicts-6.patch" is for reference, not
particularly for review, and is very little changed from "-5" version of
it.)
Re: branch tree-conflicts-notify: merge to trunk?
Posted by Stephen Butler <sb...@elego.de>.
Quoting "Neels J. Hofmeyr" <ne...@elego.de>:
> Hey Steve! (sbutler)
>
> That's so awesome! Somehow you've magically fixed almost everything on the
> branch! All I had to do was raise and notify a tree-conflict on file
> addition in a conflicted directory.
Thanks!
BTW, I think we should (silently) skip a file-add in a tree-conflicted
tree, rather than flag a new tree conflict. And of course skip all
other changes, too. Otherwise the user might see a whole treeful of
t.c. victims, and not be sure where the problem started. A single
tree conflict, raised at the root of the tree during open_directory(),
should be enough.
>
> With that, one thing remains: There's still some differentiation needed
> between raised and persisted tree-conflicts. But I guess we can treat this
> separately (in need of more tests, see below).
>
> Otherwise, the "only" thing that can lead update_tests.py to perfection is
> the implementation of dirs_same_p() a.k.a. diff_summarize, to be able to see
> whether a subtree we're deleting is really the same one.
>
>
> ...And discussion on how exactly the output should be.
> E.g., I think it is nice like this:
>
> A C alpha
> D C beta
> M C gamma
>
> While, right now, update notifies more like this:
>
> A C alpha
> D C beta
> C gamma
>
> Should we add the M or lose the A and D? I'd say add the M and whatever else
> would have been shown without a tree-conflict, to be able to see what was
> tried by the update. (Maybe there are more cases to this, though.)
I believe the first two columns tell the user what changes the update
has made in their working copy. If a path is skipped due to a tree
conflict, we should leave these columns blank.
Of course, we plan to skip all tree conflict victims (very soon), so
the 'x C' will become ' C' in all cases.
If ' C' is too cryptic, we could add the '(M->!)' later.
>
>
> Either way, I think this branch is ready for reintegration into trunk!!
> What do you think, Steve?
> (still in need of a complete `make check' though)
Yes, if the switch and merge tests aren't badly affected. Looking
into that now.
>
>
> Thanks again for the great improvements I found there today! I really didn't
> know what to do about the test framework, it's great how that just works
> now. The stuff I complained about before also makes sense to me, now.
>
> Oh, I'd just like to note that there was a misconception in some comments in
> update_tests.py:
>
> [[[
> - ### Current behavior (rather messy, isn't it?)
> ]]]
>
> It wasn't reaaally that messy, it just missed the cases of file additions
> into conflicted directories. ;)
>
>
> [[[
> # 1.1) local tree delete, incoming leaf edit
> ...
> - ### Current behavior carries out deletions in conflicted trees.
> - expected_state = state_after_leaf_edit.copy()
> - expected_state.remove('F/alpha', 'DF/D1/beta', 'DDF/D1/D2/gamma')
> + # The directory structure is scheduled for delete but is still
> + # found on disk, because the way directory deletes are handled by svn.
> + # No files are present because alpha, beta, gamma were locally removed
> + # and delta, epsilon, zeta weren't added due to tree-conflicts in their
> + # parent directory.
> + expected_disk = state_after_leaf_edit.copy()
> + expected_disk.remove('F/alpha', 'DF/D1/beta', 'DDF/D1/D2/gamma',
> + 'D/D1/delta', 'DD/D1/D2/epsilon',
> 'DDD/D1/D2/D3/zeta')
> ]]]
>
> The incoming action is a leaf edit, so no deletions are tried. The error was
> that files were being added into a conflicted dir. (Why was this called
> expected_state? It's called expected_disk everywhere else.)
>
> [[[
> - ### Current behavior records redundant conflicts!
> + ### Current behavior fails to show a status on files that would
> + ### have been added if there hadn't been a tree-conflict.
> ]]]
>
> The complete local tree was removed, so if you try to add a file a few
> levels inside a removed dir, all the non-existing path elements are in tree
> conflict. Makes sense to me.
But the whole tree is already tree-conflicted, so raising new tree
conflicts insided it is redundant.
>
>
> The check_tree_conflict() vs. svn_wc_conflicted_p() discussion made me
> think: In the tests, I guess we should also try to run the same operation
> that causes the conflicts again, to check that the persisting conflicts
> aren't re-raised or missed altogether. Is this accounted for yet?
No, currently we test only fresh tree conflicts. Good point.
>
>
> And to squeeze yet another topic into this mail: What's the deal with the
> run_and_verify_unquiet_status tests -- is status not intended to show
> tree-conflicts (on missing nodes) by default?
True. The final status check in every run_and_verify_X() runs with the
-q option, ignoring files and dirs that aren't under version control.
I think I'll change our DeepTreesTestCase class (on trunk) to do the
status check unquietly, because so many t.c. victims don't exist.
Cheers,
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