Paul Burba wrote: > On Fri, May 31, 2013 at 8:02 AM, <i...@apache.org> wrote: >> URL: http://svn.apache.org/r1488183 >> >> Modified: subversion/trunk/subversion/libsvn_client/merge.c >> ============================================================================== >> --- 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. > >> 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));
I had similar thoughts. This is what I have in my WC at the moment: [[[ - /* "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(). */ + /* "Open" the target WC. + * + * Check the target WC for mixed-rev, local mods and/or switched subtrees + * (if requested), so that we exit fast and notify the user before taking + * potentially a long time contacting the server. Note that rejecting a + * merge because of a mixed-rev WC is very common. + * + * After we find out what kind of merge is required, we will check the WC + * again in do_automatic_merge_locked(). That is mainy because, if a + * reintegrate-like merge is required, we need to disallow all three + * conditions at that time. Also, we have not yet locked the WC so + * it could potentially have changed by then (though in that case the + * calculated merge may no longer be correct, and we really should fix + * that problem by holding a WC write lock throughout this procedure). + * + * TODO: Take out a single WC write lock around both 'find_...()' and + * 'do_...()'. Don't repeat parts of 'opening' the WC again in + * 'do_...()' that we've already done here. Don't check for + * mixed-rev (etc.) again in 'do_...()' if we already did it here. + */ ]]] That indicates my thoughts on the matter. - Julian
