Re: [PATCH v4 2/2] git-svn: allow git-svn fetching to work using serf

From: Kyle J. McKay <mackyle@gmail.com>
Date: Jul 18, 2013, 23:31 UTC
Message-ID: <53FD2913-2A8E-42E1-9C76-D0AEBD76E0DA@gmail.com>
In-Reply-To: <20130718192943.GT14690@google.com>

On Jul 18, 2013, at 12:29, Jonathan Nieder wrote:

Show 14 quoted lines

> (cc-ing Eric Wong, who maintains git-svn and knows both it and
> the libsvn perl bindings much better than I do)
> Kyle J. McKay wrote:
>
>> David Rothenberger <daveroth@acm.org> has determined the cause to
>> be that ra_serf does not drive the delta editor in a depth-first
>> manner [...]. Instead, the calls come in this order:
>
> Thanks.
>
> Sorry to nitpick, but the problem is not depth-first versus
> breadth-first versus random.  Blaming the traversal order makes this
> completely confusing.  The actual problem is that the driver asks us
> to keep multiple files open at a time.

On Sun, 07 Jul 2013 18:00:40 GMT, Branko Čibej wrote:

Show 31 quoted lines

> On 07.07.2013 19:40, Jonathan Nieder wrote:
>> (cc-ing subversion's users@ list for advice)
>> Kyle McKay wrote:
>>> On Jul 6, 2013, at 18:37, Jonathan Nieder wrote:
>>
>>>
>>>> Kyle McKay wrote:
>>>>> Begin forwarded message:
>>>>>> [2] http://subversion.tigris.org/issues/show_bug.cgi?id=2932
>>>>>
>>>> Ah, thanks for the context.
>>>>
>>>> It's still not clear to me how we know that ra_serf driving the  
>>>> editor
>>>> in a non depth-first manner is the problem here.  Has that  
>>>> explanation
>>>> been confirmed somehow?
>>> [...]
>>> Since ra_serf makes multiple connections to the server (hard-coded
>>> to 4 prior to svn 1.8, defaults to 4 in svn 1.8 but can be set to
>>> between 1 and 8) it makes sense there would be multiple active calls
>>> to apply_textdelta if processing is done as results are received on
>>> the multiple connections.
>> Ah, that's worrisome.  Do I understand you correctly that to work  
>> with
>> ra_serf in skelta mode, callers need to make their apply_textdelta
>> callback thread-safe?
>
> No; the editor drive is single-threaded, but the order of the  
> operations
> isn't strictly depth-first.

Brane also describes this as a non-depth-first traversal order which is the root of the problem. If the order of operations were strictly depth-first, the previous file would end up being closed before the next one's opened.

> The approach taken in this patch would be racy if the driver calls us
> multiple times concurrently (since temp_acquire can fail).  I believe
> it doesn't but haven't checked.

Brane says the editor drive is single-threaded so that doesn't seem like a problem.