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...@wandisco.com> on 2011/08/26 18:13:44 UTC
Re: svn commit: r1161219 - Auto-resolve 'local move vs. incoming
edit'
On Wed, 2011-08-24, stsp@apache.org wrote:
> Author: stsp
> Date: Wed Aug 24 18:26:19 2011
> New Revision: 1161219
>
> URL: http://svn.apache.org/viewvc?rev=1161219&view=rev
> Log:
> Auto-resolve 'local move vs. incoming edit' conflicts for files during merge.
>
> * subversion/libsvn_client/repos_diff.c
> (close_file): If the file did receive changes during the merge
> and has been moved-away, show the file's new location during
> notification.
Hi Stefan. I've just come across this bit of code and I think we need
to move it to somewhere else. It's inside a diff editor which is used
for general repos-repos diffing as well as for providing the source of a
merge. The merge code gives this editor a (supposed) WC target path
parameter:
svn_client__get_diff_editor(target=target_abspath ...)
and the editor uses that "target" parameter as the prefix on each path
that it sends to the diff callbacks, and on each path that it sends to
the notification callback. But that's the only thing it does with it.
Until now.
When "svn diff" uses the same editor, it passes an empty string as the
"target" parameter because there is no WC path involved in a repos-repos
diff.
I was just cleaning up this editor, stripping out this "target" path
completely, so that it would simply pass relative paths out to the diff
callbacks and to the notification functions. The merge code knows what
WC path this diff relates to, and so can add the prefix itself inside
its callbacks. That will keep the knowledge about the WC completely
separate from the repos-diff editor, which is a good thing.
We are at the point now where we will want to follow a local move and
apply a sub-tree of the incoming diff to the new path in the merge
target WC, not the path that is currently being constructed by prefixing
the supposed "WC target" onto the repository-side relpath. So getting
this info out of the diff editor will help to make everything clearer
for following local moves as well.
Do you follow? Can you help me to find the right way to implement this
notification adjustment in the merge code outside this editor?
- Julian
> * subversion/libsvn_client/merge.c
> (merge_file_changed): If the file being merged into was
> moved away locally, merge incoming changes to the file
> at the new location.
>
> * subversion/tests/cmdline/tree_conflict_tests.py
> (merge_file_mod_onto_not_file): Don't expect a tree conflict
> for locally moved files.
>
> Modified:
> subversion/trunk/subversion/libsvn_client/merge.c
> subversion/trunk/subversion/libsvn_client/repos_diff.c
> subversion/trunk/subversion/tests/cmdline/tree_conflict_tests.py
>
> Modified: subversion/trunk/subversion/libsvn_client/merge.c
> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_client/merge.c?rev=1161219&r1=1161218&r2=1161219&view=diff
> ==============================================================================
> --- subversion/trunk/subversion/libsvn_client/merge.c (original)
> +++ subversion/trunk/subversion/libsvn_client/merge.c Wed Aug 24 18:26:19 2011
> @@ -1460,8 +1460,9 @@ merge_file_changed(svn_wc_notify_state_t
> way svn_wc_merge4() can do the merge. */
> if (wc_kind != svn_node_file || is_deleted)
> {
> - svn_boolean_t moved_away;
> + const char *moved_to_abspath;
> svn_wc_conflict_reason_t reason;
> + svn_error_t *err;
>
> /* Maybe the node is excluded via depth filtering? */
>
> @@ -1491,23 +1492,39 @@ merge_file_changed(svn_wc_notify_state_t
> /* This is use case 4 described in the paper attached to issue
> * #2282. See also notes/tree-conflicts/detection.txt
> */
> - SVN_ERR(check_moved_away(&moved_away, merge_b->ctx->wc_ctx,
> - mine_abspath, scratch_pool));
> - if (moved_away)
> - reason = svn_wc_conflict_reason_moved_away;
> - else if (is_deleted)
> - reason = svn_wc_conflict_reason_deleted;
> + err = svn_wc__node_was_moved_away(&moved_to_abspath, NULL,
> + merge_b->ctx->wc_ctx, mine_abspath,
> + scratch_pool, scratch_pool);
> + if (err)
> + {
> + if (err->apr_err == SVN_ERR_WC_PATH_NOT_FOUND)
> + svn_error_clear(err);
> + else
> + return svn_error_trace(err);
> + }
> +
> + if (moved_to_abspath)
> + {
> + /* File has been moved away locally -- apply incoming
> + * changes at the new location. */
> + mine_abspath = moved_to_abspath;
> + }
> else
> - reason = svn_wc_conflict_reason_missing;
> - SVN_ERR(tree_conflict(merge_b, mine_abspath, svn_node_file,
> - svn_wc_conflict_action_edit, reason));
> - if (tree_conflicted)
> - *tree_conflicted = TRUE;
> - if (content_state)
> - *content_state = svn_wc_notify_state_missing;
> - if (prop_state)
> - *prop_state = svn_wc_notify_state_missing;
> - return SVN_NO_ERROR;
> + {
> + if (is_deleted)
> + reason = svn_wc_conflict_reason_deleted;
> + else
> + reason = svn_wc_conflict_reason_missing;
> + SVN_ERR(tree_conflict(merge_b, mine_abspath, svn_node_file,
> + svn_wc_conflict_action_edit, reason));
> + if (tree_conflicted)
> + *tree_conflicted = TRUE;
> + if (content_state)
> + *content_state = svn_wc_notify_state_missing;
> + if (prop_state)
> + *prop_state = svn_wc_notify_state_missing;
> + return SVN_NO_ERROR;
> + }
> }
>
> /* ### TODO: Thwart attempts to merge into a path that has
>
> Modified: subversion/trunk/subversion/libsvn_client/repos_diff.c
> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_client/repos_diff.c?rev=1161219&r1=1161218&r2=1161219&view=diff
> ==============================================================================
> --- subversion/trunk/subversion/libsvn_client/repos_diff.c (original)
> +++ subversion/trunk/subversion/libsvn_client/repos_diff.c Wed Aug 24 18:26:19 2011
> @@ -1052,6 +1052,7 @@ close_file(void *file_baton,
> svn_wc_notify_t *notify;
> svn_wc_notify_action_t action;
> svn_node_kind_t kind = svn_node_file;
> + const char *moved_to_abspath = NULL;
>
> /* Find out if a pending delete notification for this path is
> * still around. */
> @@ -1087,9 +1088,30 @@ close_file(void *file_baton,
> else if (b->added)
> action = svn_wc_notify_update_add;
> else
> - action = svn_wc_notify_update_update;
> + {
> + svn_error_t *err;
> +
> + action = svn_wc_notify_update_update;
> +
> + /* If the file was moved-away, use its new path in the
> + * notification.
> + * ### This is redundant. The file_changed() callback should
> + * ### pass the moved-to path back up here. */
> + err = svn_wc__node_was_moved_away(&moved_to_abspath, NULL,
> + eb->wc_ctx, b->wcpath,
> + scratch_pool, scratch_pool);
> + if (err)
> + {
> + if (err->apr_err == SVN_ERR_WC_PATH_NOT_FOUND)
> + svn_error_clear(err);
> + else
> + return svn_error_trace(err);
> + }
> + }
>
> - notify = svn_wc_create_notify(b->wcpath, action, scratch_pool);
> + notify = svn_wc_create_notify(moved_to_abspath ? moved_to_abspath
> + : b->wcpath,
> + action, scratch_pool);
> notify->kind = kind;
> notify->content_state = content_state;
> notify->prop_state = prop_state;
>
> Modified: subversion/trunk/subversion/tests/cmdline/tree_conflict_tests.py
> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/tests/cmdline/tree_conflict_tests.py?rev=1161219&r1=1161218&r2=1161219&view=diff
> ==============================================================================
> --- subversion/trunk/subversion/tests/cmdline/tree_conflict_tests.py (original)
> +++ subversion/trunk/subversion/tests/cmdline/tree_conflict_tests.py Wed Aug 24 18:26:19 2011
> @@ -657,7 +657,7 @@ def merge_file_mod_onto_not_file(sbox):
> "merge file: modify onto not-file"
> sbox2 = sbox.clone_dependent()
> test_tc_merge(sbox, f_mods, br_scen = f_dels + f_moves + f_rpl_d)
> - test_tc_merge(sbox2, f_mods, wc_scen = f_dels + f_moves)
> + test_tc_merge(sbox2, f_mods, wc_scen = f_dels)
> # Note: See UC4 in notes/tree-conflicts/use-cases.txt.
>
> def merge_file_del_onto_not_same(sbox):
>
>
Re: svn commit: r1161219 - Auto-resolve 'local move vs. incoming
edit'
Posted by Julian Foad <ju...@wandisco.com>.
On Mon, 2011-08-29 at 13:27 +0200, Stefan Sperling wrote:
> On Fri, Aug 26, 2011 at 05:27:16PM +0100, Julian Foad wrote:
> > I (Julian Foad) wrote:
> > > I was just cleaning up this editor, stripping out this "target" path
> > > completely, so that it would simply pass relative paths out to the diff
> > > callbacks and to the notification functions. The merge code knows what
> > > WC path this diff relates to, and so can add the prefix itself inside
> > > its callbacks. That will keep the knowledge about the WC completely
> > > separate from the repos-diff editor, which is a good thing.
> > [...]
> >
> > Attached is my patch in progress to strip WC path info from the
> > repos-repos diff editor.
>
> I agree with your layering concerns.
> There is one point I am not sure about. You haven't explained
> how you want to address the following:
>
> > @@ -1110,30 +1095,9 @@ close_file(void *file_baton,
[...]
> > - notify = svn_wc_create_notify(moved_to_abspath ? moved_to_abspath
> > - : b->wcpath,
> > - action, scratch_pool);
> > + notify = svn_wc_create_notify(b->path, action, scratch_pool);
>
> You cannot just use b->path here. There is a chicken-and-egg problem.
>
> If the node was moved locally, this layer doesn't know about the new
> location of the node, unless it runs svn_wc__node_was_moved_away()
>
> I agree that calling it here is bad layering.
> But I think notifications should show the new paths.
>
> b->path is the old location of the node.
Agreed, so far.
> The layer which modifies the WC already knows about the new path
> but it does not perform notification.
> As I wrote in the comment your patch removes, it would be good to pass
> the proper path up to layer which performs the notifications.
As we discussed in IRC just now, this isn't so hard. merge.c is in
control of modifying the WC and also in control of notifications. It
tells the repos-diff editor to send notifications to
merge.c:notification_receiver(). So it can do the transformation from
repos path to WC moved-to path there.
> This would imply a change in the editor interface.
Doesn't look like that's needed. I'll proceed.
- Julian
> (It is becoming very apparent now that editor v1 was not designed
> with moves in mind...)
Re: svn commit: r1161219 - Auto-resolve 'local move vs. incoming
edit'
Posted by Stefan Sperling <st...@elego.de>.
On Fri, Aug 26, 2011 at 05:27:16PM +0100, Julian Foad wrote:
> I (Julian Foad) wrote:
> > I was just cleaning up this editor, stripping out this "target" path
> > completely, so that it would simply pass relative paths out to the diff
> > callbacks and to the notification functions. The merge code knows what
> > WC path this diff relates to, and so can add the prefix itself inside
> > its callbacks. That will keep the knowledge about the WC completely
> > separate from the repos-diff editor, which is a good thing.
> [...]
>
> Attached is my patch in progress to strip WC path info from the
> repos-repos diff editor.
I agree with your layering concerns.
There is one point I am not sure about. You haven't explained
how you want to address the following:
> @@ -1110,30 +1095,9 @@ close_file(void *file_baton,
> else if (b->added)
> action = svn_wc_notify_update_add;
> else
> - {
> - svn_error_t *err;
> -
> - action = svn_wc_notify_update_update;
> -
> - /* If the file was moved-away, use its new path in the
> - * notification.
> - * ### This is redundant. The file_changed() callback should
> - * ### pass the moved-to path back up here. */
> - err = svn_wc__node_was_moved_away(&moved_to_abspath, NULL,
> - eb->wc_ctx, b->wcpath,
> - scratch_pool, scratch_pool);
> - if (err)
> - {
> - if (err->apr_err == SVN_ERR_WC_PATH_NOT_FOUND)
> - svn_error_clear(err);
> - else
> - return svn_error_trace(err);
> - }
> - }
> + action = svn_wc_notify_update_update;
>
> - notify = svn_wc_create_notify(moved_to_abspath ? moved_to_abspath
> - : b->wcpath,
> - action, scratch_pool);
> + notify = svn_wc_create_notify(b->path, action, scratch_pool);
You cannot just use b->path here. There is a chicken-and-egg problem.
If the node was moved locally, this layer doesn't know about the new
location of the node, unless it runs svn_wc__node_was_moved_away()
I agree that calling it here is bad layering.
But I think notifications should show the new paths.
b->path is the old location of the node.
The layer which modifies the WC already knows about the new path
but it does not perform notification.
As I wrote in the comment your patch removes, it would be good to pass
the proper path up to layer which performs the notifications.
This would imply a change in the editor interface.
(It is becoming very apparent now that editor v1 was not designed
with moves in mind...)
Re: svn commit: r1161219 - Auto-resolve 'local move vs. incoming
edit'
Posted by Julian Foad <ju...@wandisco.com>.
I (Julian Foad) wrote:
> I was just cleaning up this editor, stripping out this "target" path
> completely, so that it would simply pass relative paths out to the diff
> callbacks and to the notification functions. The merge code knows what
> WC path this diff relates to, and so can add the prefix itself inside
> its callbacks. That will keep the knowledge about the WC completely
> separate from the repos-diff editor, which is a good thing.
[...]
Attached is my patch in progress to strip WC path info from the
repos-repos diff editor.
- Julian