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...)
Received on 2011-08-29 13:28:14 CEST