Paul Burba wrote:
> On Fri, May 31, 2013 at 8:02 AM, <ivan_at_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
Received on 2013-06-04 23:51:18 CEST