On Wed, Jun 5, 2013 at 1:50 AM, Julian Foad <julianf...@btopenworld.com> wrote: > Paul Burba wrote: >> On Fri, May 31, 2013 at 8:02 AM, <i...@apache.org> wrote: >>> URL: http://svn.apache.org/r1488183 >>> >>> --- subversion/trunk/subversion/libsvn_client/merge.c (original) >>> +++ subversion/trunk/subversion/libsvn_client/merge.c Fri May 31 12:02:32 >>> @@ -12415,14 +12415,15 @@ client_find_automatic_merge(automatic_me >>> source_and_target_t *s_t = apr_palloc(result_pool, sizeof(*s_t)); >>> automatic_merge_t *merge = apr_palloc(result_pool, sizeof(*merge)); >>> >>> - /* "Open" the target WC. We're not going to check the target WC for >>> - * mixed-rev, local mods or switched subtrees yet. After we find out >>> - * what kind of merge is required, then if a reintegrate-like merge is >>> - * required we'll do the stricter checks, in >>> do_automatic_merge_locked(). */ >>> + /* "Open" the target WC. Check the target WC for mixed-rev, local mods >>> and >>> + * switched subtrees yet to faster exit and notify user before contacting >>> + * with server. After we find out what kind of merge is required, then >>> if >>> + * a reintegrate-like merge is required we'll do the stricter checks, in >>> + * do_automatic_merge_locked(). */ >> >> Hi Ivan, >> >> (Julian - ccing you as you originally added this bit of code that Ivan >> tweaked here) >> >> The comment is a bit misleading. We don't *unconditionally* check the >> target WC for mixed-rev, local mods, and/or switched subtrees, it >> depends on what the caller asks for. Yes, currently the only two >> callers of client_find_automatic_merge() unconditionally pass >> ALLOW_LOCAL_MODS=TRUE and ALLOW_SWITCHED_SUBTREES=TRUE, so the comment >> here is effectively true, but might not be in the future. >> I agree that comment a little bit confusing. Sorry. I'm fine with Juilan comment fix.
>>> SVN_ERR(open_target_wc(&s_t->target, target_abspath, >>> - TRUE /*allow_mixed_rev*/, >>> - TRUE /*allow_local_mods*/, >>> - TRUE /*allow_switched_subtrees*/, >>> + allow_mixed_rev, >>> + allow_local_mods, >>> + allow_switched_subtrees, >> >> Since you were worried about performance with mixed-rev WCs, I wonder >> if this is better: >> >> [[[ >> - /* "Open" the target WC. We're not going to check the target WC for >> - * mixed-rev, local mods or switched subtrees yet. After we find out >> - * what kind of merge is required, then if a reintegrate-like merge is >> - * required we'll do the stricter checks, in do_automatic_merge_locked(). >> */ >> + /* "Open" the target WC. We're not going to check the target WC for local >> + mods or switched subtrees yet, but since the check for mixed-revisions >> + is cheap, easily violated, and required by most merges, check for that >> + now, before we go through the potentially lengthy process of contacting >> + the server -- It's bad form to make the user wait, only to learn that >> + they cannot merge due to the state of their WC: "Uh, why didn't you >> tell >> + me that at the start!" Later, if we determine that a reintegrate-like >> + merge is called for, then we'll do the local mod and switched subtree >> + checks in do_automatic_merge_locked(). */ >> SVN_ERR(open_target_wc(&s_t->target, target_abspath, >> - TRUE /*allow_mixed_rev*/, >> - TRUE /*allow_local_mods*/, >> - TRUE /*allow_switched_subtrees*/, >> + allow_mixed_rev, >> + TRUE, /* allow_local_mods */ >> + TRUE, /* allow_switched_subtrees */ >> ctx, result_pool, scratch_pool)); Working copy with local mods is also typical case IMHO. Is local mods check expensive? I'm fine to delay local mods and switched subtrees checks, but I think it's premature optimization: local operations generally fast, while remote is slow. -- Ivan Zhakov VisualSVN Team
