You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Julian Foad <ju...@btopenworld.com> on 2008/11/03 17:14:21 UTC
Re: svn commit: r33989 - in trunk: . notes/tree-conflicts
subversion/include subversion/include/private
subversion/libsvn_auth_kwallet subversion/libsvn_client
subversion/libsvn_subr subversion/libsvn_wc subversion/tests/cmdline
subversion/tests/libsvn_subr tools/buildbot/slaves/win32-xp-VS2005
tools/server-side www
On Fri, 2008-10-31 at 20:39 -0700, neels@tigris.org wrote:
> Author: neels
> Date: Fri Oct 31 20:39:02 2008
> New Revision: 33989
>
> Log:
> Merge (lightweight) branch tc-merge-notify to trunk.
>
> Implement per-victim tree-conflicts notification during merge.
> Use newly implemented notification style in merge operations (as done
> for update/switch recently).
>
> Do not show `Skipped' messages within newly tree-conflicted directories.
> Pass a boolean SKIP down the item batons and completely skip *all*
> action in the editor if SKIP is TRUE (i.e. don't even call diff callbacks).
> Since the diff callbacks don't have an item baton to fill, I solved it
> by skipping child nodes already in the general editor that calls the diff
> callbacks. But it would probably do this code good to move to a standard
> editor implementation altogether, like `update/switch' does it.
>
>
> * subversion/include/svn_wc.h (svn_wc_diff_callbacks3_t): Add a svn_boolean_t
> *TREE_CONFLICTED argument to all of the callbacks functions, which
> returns whether the callback wants this node to be notified as
> tree-conflicted. Returning TRUE in *TREE_CONFLICTED makes a repos
> diff (and consequently a merge) skip any child nodes.
This description would go better in the code comment. I added a code
comment about this new flag but I didn't write about the "skipping"
effect of the flag.
I wrote that the function must set the flag to true or false - i.e. it
must not leave it untouched - but that may cause a problem (see below)
so I don't know if it was what you intended.
> * subversion/libsvn_wc/diff.c
> (file_diff, directory_elements_diff, report_wc_file_as_added,
> report_wc_directory_as_added, delete_entry, close_directory,
> close_file): Pass NULL for new *TREE_CONFLICTED parameter upon calling
> the svn_wc_diff_callbacks3_t callback functions. Currently, handling
> tree-conflicts notifications here is not needed, since there is no
> code using it. Note that this variant of diff will also not skip
> child nodes of a tree-conflicted item for the same reason.
Re. not skipping: It would be better in the long term, of course, to
implement the skipping here as well, to keep the behaviour consistent,
even though that part of the behaviour is not being used by the current
"svn" client.
> * subversion/libsvn_client/repos_diff.c
> (dir_baton, file_baton): Add two booleans TREE_CONFLICTED and SKIPPED.
> (make_dir_baton, make_file_baton): Default TREE_CONFLICTED to FALSE.
> Default SKIPPED to FALSE.
> (delete_entry, add_directory, open_directory, close_file, close_directory):
> Pass the tree-conflict state through to the notify_func, and change some
> of the logic around the notify action to not show tree-conflicts as
> skipped paths.
Here, the logic is setting TREE_CONFLICTED to false in the baton and
then passing a pointer to it to each of the callbacks. If more than one
callback is called for the same path, won't the subsequent callbacks
overwrite the value set by the first callback?
Observation: In tree_conflict_tests 14 (XFail) a directory-delete is
merged onto a branch where the directory exists and has a property
difference. There is a notification " C" during the merge, but then no
conflict status is recorded in the working copy.
It looks like there might be an error in merge_file_deleted() and
merge_dir_deleted(): they don't set *tree_conflicted=FALSE anywhere. In
default cases, merge_file_deleted() doesn't set it at all,
merge_dir_deleted() sets it TRUE, and the other callbacks set it FALSE.
> (delete_entry, add_directory, open_directory, add_file, open_file):
> Skip *all* action if the parent node is SKIPped or TREE_CONFLICTED.
> Propagate the SKIP flag to child batons where necessary.
> (window_handler, apply_textdelta, close_file, close_directory,
> change_file_prop, change_dir_prop): Skip *all* action if this node's
> SKIP flag is TRUE.
> (kind_action_state_t): Add a boolean TREE_CONFLICTED field (breaking the
> name kind_action_state_t which says "it has a kind, an action and a
> state") used for notification of deleted nodes.
> (absent_directory, absent_file): Add a question comment.
>
> * subversion/libsvn_client/merge.c
> (merge_props_changed, merge_file_changed, merge_file_added,
> merge_file_deleted, merge_dir_added, merge_dir_deleted, merge_dir_opened,
> merge_dir_closed, single_file_merge_notify, do_file_merge): Apply addition
> of new callbacks argument called *TREE_CONFLICTED and use it.
> (merge_cmd_baton_t): Remove obsoleted field TREE_CONFLICTED_DIRS which
> used to list all dirs that contain tree-conflicts.
> (add_parent_to_tree_conflicted_dirs, is_tree_conflicted_dir_p): Remove
> obsoleted functions related to TREE_CONFLICTED_DIRS.
> (do_merge, tree_conflict): Remove use of TREE_CONFLICTED_DIRS.
>
> * subversion/libsvn_client/diff.c
> (diff_props_changed, diff_file_changed, diff_file_added,
> diff_file_deleted_with_diff, diff_file_deleted_no_diff,
> diff_dir_added, diff_dir_deleted, diff_dir_opened, diff_dir_closed):
> Apply addition of callbacks parameter *TREE_CONFLICTED and ignore it.
> These are the callbacks for a plain diff, they don't cause
> tree-conflicts by definition.
>
> * subversion/tests/cmdline/merge_tests.py
> (delete_file_and_dir, merge_catches_nonexistent_target,
> merge_tree_deleted_in_target, merge_added_dir_to_deleted_in_target,
> three_way_merge_add_of_existing_binary_file, del_differing_file,
> tree_conflicts_and_obstructions,
> tree_conflicts_on_merge_local_ci_4_1,
> tree_conflicts_on_merge_local_ci_4_2, tree_conflicts_on_merge_local_ci_5_1,
> tree_conflicts_on_merge_local_ci_5_2, tree_conflicts_on_merge_local_ci_6,
> tree_conflicts_on_merge_no_local_ci_4_1,
> tree_conflicts_on_merge_no_local_ci_4_2,
> tree_conflicts_on_merge_no_local_ci_5_1,
> tree_conflicts_on_merge_no_local_ci_5_2,
> tree_conflicts_on_merge_no_local_ci_6): Fix up some tests. Intentionally
> leaving some merge tests unfixed for now. Their common problem shall be
> fixed on trunk, since update/switch/checkout are also affected.
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r33989 - in trunk: . notes/tree-conflicts subversion/include
subversion/include/private subversion/libsvn_auth_kwallet subversion/libsvn_client
subversion/libsvn_subr subversion/libsvn_wc subversion/tests/cmdline subversion/tests/libsvn_subr
tools/buildbot/slaves/win32-xp-VS2005 tools/server-side www
Posted by Neels J Hofmeyr <ne...@elego.de>.
Neels J. Hofmeyr wrote:
> Is the current code actually aware of the fact that the same path may be a
> file on the one side and a dir on the other? (I mean in the conflict
> description and reporting.)
Ok, nevermind that one, Julian just posted a complete overview of it.
Re: svn commit: r33989 - in trunk: . notes/tree-conflicts subversion/include
subversion/include/private subversion/libsvn_auth_kwallet subversion/libsvn_client
subversion/libsvn_subr subversion/libsvn_wc subversion/tests/cmdline subversion/tests/libsvn_subr
tools/buildbot/slaves/win32-xp-VS2005 tools/server-side www
Posted by "Neels J. Hofmeyr" <ne...@elego.de>.
Julian Foad wrote:
> On Fri, 2008-10-31 at 20:39 -0700, neels@tigris.org wrote:
>> Author: neels
>> Date: Fri Oct 31 20:39:02 2008
>> New Revision: 33989
>>
>> Log:
>> Merge (lightweight) branch tc-merge-notify to trunk.
>> * subversion/include/svn_wc.h (svn_wc_diff_callbacks3_t): Add a svn_boolean_t
>> *TREE_CONFLICTED argument to all of the callbacks functions, which
>> returns whether the callback wants this node to be notified as
>> tree-conflicted. Returning TRUE in *TREE_CONFLICTED makes a repos
>> diff (and consequently a merge) skip any child nodes.
>
> This description would go better in the code comment. I added a code
> comment about this new flag but I didn't write about the "skipping"
> effect of the flag.
I'd say should *also* be in the code comment... It's quite a change,
skipping everything in some cases.
>
> I wrote that the function must set the flag to true or false - i.e. it
> must not leave it untouched - but that may cause a problem (see below)
> so I don't know if it was what you intended.
... s.b.
>
>> * subversion/libsvn_wc/diff.c
>> (file_diff, directory_elements_diff, report_wc_file_as_added,
>> report_wc_directory_as_added, delete_entry, close_directory,
>> close_file): Pass NULL for new *TREE_CONFLICTED parameter upon calling
>> the svn_wc_diff_callbacks3_t callback functions. Currently, handling
>> tree-conflicts notifications here is not needed, since there is no
>> code using it. Note that this variant of diff will also not skip
>> child nodes of a tree-conflicted item for the same reason.
>
> Re. not skipping: It would be better in the long term, of course, to
> implement the skipping here as well, to keep the behaviour consistent,
> even though that part of the behaviour is not being used by the current
> "svn" client.
I agree. ...I tend to leave these "implementation details" that don't affect
the API to after we branched 1.6.
>
>> * subversion/libsvn_client/repos_diff.c
>> (dir_baton, file_baton): Add two booleans TREE_CONFLICTED and SKIPPED.
>> (make_dir_baton, make_file_baton): Default TREE_CONFLICTED to FALSE.
>> Default SKIPPED to FALSE.
>> (delete_entry, add_directory, open_directory, close_file, close_directory):
>> Pass the tree-conflict state through to the notify_func, and change some
>> of the logic around the notify action to not show tree-conflicts as
>> skipped paths.
>
> Here, the logic is setting TREE_CONFLICTED to false in the baton and
> then passing a pointer to it to each of the callbacks. If more than one
> callback is called for the same path, won't the subsequent callbacks
> overwrite the value set by the first callback?
Hm, yes, that should probably be decoupled... (s.b.)
>
> Observation: In tree_conflict_tests 14 (XFail) a directory-delete is
> merged onto a branch where the directory exists and has a property
> difference. There is a notification " C" during the merge, but then no
> conflict status is recorded in the working copy.
>
> It looks like there might be an error in merge_file_deleted() and
> merge_dir_deleted(): they don't set *tree_conflicted=FALSE anywhere. In
> default cases, merge_file_deleted() doesn't set it at all,
> merge_dir_deleted() sets it TRUE, and the other callbacks set it FALSE.
Heh, I expected someone to stumble across that. I found it just yesterday,
was at the point of committing but saw that more work was necessary and went
to bed. So I'm on it.
I've also got some more fixes in the patch. Looks like I really wasn't
paying attention to that at the time. E.g., apparently *all* directory
deletes in a merge will currently result in a tree-conflict -- oops!
We should test for that, I mean false positives. And, I found out, we should
also test for props in the deep tree cases, i.e. add P, DP and DDP
directories that do prop changes and deletes.
And I also think we don't have add-on-top-of-add tests, do we?
And Tests that have two different node kinds conflicting. And...
Is the current code actually aware of the fact that the same path may be a
file on the one side and a dir on the other? (I mean in the conflict
description and reporting.)
~Neels