subversion-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Paul Burba <ptbu...@gmail.com>
Subject Re: svn commit: r1488183 - /subversion/trunk/subversion/libsvn_client/merge.c
Date Tue, 04 Jun 2013 16:46:32 GMT
On Fri, May 31, 2013 at 8:02 AM,  <ivan@apache.org> wrote:
> Author: ivan
> Date: Fri May 31 12:02:32 2013
> New Revision: 1488183
>
> URL: http://svn.apache.org/r1488183
> Log:
> Validate target working copy for mixed revisions, local modifications and
> switched subtrees before contacting server. Mixed revision working copy is
> very often case and it's better to notify user earlier before spending
> significant amount of time for merge info retrieval from server.
>
> * subversion/libsvn_client/merge.c
>   (client_find_automatic_merge): Check target WC for mixed revisions,
>    local mods and switched substrees.
>
> Modified:
>     subversion/trunk/subversion/libsvn_client/merge.c
>
> Modified: subversion/trunk/subversion/libsvn_client/merge.c
> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_client/merge.c?rev=1488183&r1=1488182&r2=1488183&view=diff
> ==============================================================================
> --- subversion/trunk/subversion/libsvn_client/merge.c (original)
> +++ subversion/trunk/subversion/libsvn_client/merge.c Fri May 31 12:02:32 2013
> @@ -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:

[[[
Index: /svn/src-branch-1.8.x-2/subversion/libsvn_client/merge.c
===================================================================
--- /svn/src-branch-1.8.x-2/subversion/libsvn_client/merge.c
(revision 1489460)
+++ /svn/src-branch-1.8.x-2/subversion/libsvn_client/merge.c    (working copy)
@@ -12409,14 +12415,19 @@
   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.  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));

   /* Open RA sessions to the source and target trees. */
]]]

--
Paul T. Burba
CollabNet, Inc. -- www.collab.net -- Enterprise Cloud Development
Skype: ptburba

>                           ctx, result_pool, scratch_pool));
>
>    /* Open RA sessions to the source and target trees. */
>
>

Mime
View raw message