Mailing-List: contact dev-help@subversion.apache.org; run by ezmlm
Precedence: bulk
Received-SPF: pass (nike.apache.org: domain of ivan@visualsvn.com designates
 209.85.128.44 as permitted sender)
MIME-Version: 1.0
Sender: ivan@visualsvn.com
In-Reply-To: <1370382625.31924.YahooMailNeo@web186101.mail.ir2.yahoo.com>
References: <20130531120233.29E442388847@eris.apache.org>
 <CAO16vNS21X6ETKM4R42uFLpGYnaRn9wtKo88TYLBKumYWfE8rA@mail.gmail.com>
 <1370382625.31924.YahooMailNeo@web186101.mail.ir2.yahoo.com>
From: Ivan Zhakov <ivan@apache.org>
Date: Wed, 5 Jun 2013 18:12:52 +0400
Message-ID: 
 <CABw-3YdjYxwdiOUVznh9AVcQ2pzs0=rRiPQB9KSw1ADfugRckw@mail.gmail.com>
Subject: Re: svn commit: r1488183 -
 /subversion/trunk/subversion/libsvn_client/merge.c
To: Julian Foad <julianfoad@btopenworld.com>
Cc: Paul Burba <ptburba@gmail.com>,
 Subversion Development <dev@subversion.apache.org>
Content-Type: text/plain; charset=UTF-8

On Wed, Jun 5, 2013 at 1:50 AM, Julian Foad <julianfoad@btopenworld.com> wrote:
> Paul Burba wrote:
>> On Fri, May 31, 2013 at 8:02 AM,  <ivan@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