subversion-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Julian Foad <julianf...@btopenworld.com>
Subject Re: svn commit: r1488183 - /subversion/trunk/subversion/libsvn_client/merge.c
Date Tue, 04 Jun 2013 21:50:25 GMT
Paul Burba wrote:
> On Fri, May 31, 2013 at 8:02 AM,  <ivan@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


Mime
View raw message