[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

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

From: Julian Foad <julianfoad_at_btopenworld.com>
Date: Mon, 03 Nov 2008 17:14:21 +0000

On Fri, 2008-10-31 at 20:39 -0700, neels_at_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_at_subversion.tigris.org
For additional commands, e-mail: dev-help_at_subversion.tigris.org
Received on 2008-11-03 18:14:42 CET

This is an archived mail posted to the Subversion Dev mailing list.