patch, 2 partsallow git-svn fetching to work using serf

This patch allows git-svn to fetch successfully using the serf library when given an https?: url to fetch from.

Unfortunately some svn servers do not seem to be configured well for use with the serf library. This can cause fetching to take longer compared to the neon library or actually cause timeouts during the fetch. When timeouts occur git-svn can be safely restarted to fetch more revisions.

A new temp_is_locked function has been added to Git.pm to facilitate using the minimal number of temp files possible when using serf.

The problem that occurs when running git-svn fetch using the serf library is that the previously used temp file is not always unlocked before the next temp file needs to be used.

To work around this problem, a new temp name is used if the temp name that would otherwise be chosen is currently locked.

This patch may not be all that is required, but at least it is a starting point.

Daniel Shahaf has suggested also setting "servers:global:http-bulk- updates=on".

Kyle J. McKay (2):
  Git.pm: add new temp_is_locked function
  git-svn: allow git-svn fetching to work using serf

perl/Git.pm | 17 ++++++++++++++++- perl/Git/SVN/Fetcher.pm | 6 ++++-- 2 files changed, 20 insertions(+), 3 deletions(-)

On 7/5/2013 8:41 PM, Kyle McKay wrote:

Thanks, Kyle. I confirm this is working for my problem cases as well.

I have a patch that does this, but since turning on bulk updates has a possible performance penalty, I prefer your approach.

Please let me know if anyone wants to see the patch.

David Rothenberger wrote:

I assume that's because http-bulk-updates defeats caching. If so, makes sense.

Please forgive my ignorance: is there a bug filed about ra_serf's misbehavior here? Is it eventually going to be fixed and this is just a workaround, or is the growth in temp file use something we'd live with permanently?

Thanks, Jonathan

On Jul 6, 2013, at 17:28, Jonathan Nieder wrote:

Apparently it will not be fixed:

Begin forwarded message:

The summary of [3] which is marked RESOLVED,FIXED is "Add errata / release note noise around ra_serf's editor drive violations".

Kyle

Kyle McKay wrote:

[...]

[...]

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?

For example, does the workaround mentioned by danielsh work? Does using ra_neon instead of ra_serf avoid trouble as well? Is there a simple explanation of why violating the depth-first constraint would lead to multiple blob (i.e., file, not directory) deltas being opened in a row without an intervening close?

Jonathan

On Jul 6, 2013, at 18:37, Jonathan Nieder wrote:

Using ra_neon seams to eliminate the problem. Using ra_neon has always been the default until svn 1.8 which drops ra_neon support entirely and always uses ra_serf for https?: urls.

The workaround mentioned by danielsh won't work if the server has configured SVNAllowBulkUpdates Off because that will force use of skelta mode no matter what the client does. However, since ra_neon only ever has a single connection to the server it probably doesn't matter.

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.

Kyle

(cc-ing subversion's users@ list for advice) Kyle McKay wrote:

[...]

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?

Or do you just mean that the traversal order is based on the order in which results are received? That would be fine, as long as after each apply_textdelta call, close_file is called before the next apply_textdelta.

Jonathan

On 7/6/2013 5:28 PM, Jonathan Nieder wrote:

I believe that "bulk updates" means that serf makes one request for a lot of information and receives it all in one big response. In "skelta" mode, serf makes a single request for a single piece of information. The serf authors feel this can lead to improved overall throughput because they can pipeline these requests and have multiple connections open at the same time.

The downside, though, is that serf will do multiple open_file calls in parallel as it descends down sibling directories.

I did do a trace of "git svn fetch" and observed this non-depth-first traversal. It certainly causes the failure we've observed.

I believe serf is doing the following for a number of files in parallel:
 1. open_file
 2. apply_textdelta
 3. change_file_prop, change_file_prop, ...
 4. close_file

(cc-ing users@ as requested by danielsh) David Rothenberger wrote:

Ah, that makes more sense. It is not about traversal order but about processing multiple non-directory files in parallel, and step (3) potentially involving a large number of property changes means that it can make sense not to take a lock.

Perhaps the reference documentation could warn about this?

On the git-svn side, it looks like we have enough information to make a more complete commit message or in-code comment so the reason for multiple git_blob tempfiles is not forgotten. Thanks for your patient explanations.

Jonathan