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...)