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

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

[Kyle J. McKay]

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.

Versions v2-v3 of the patch introduced a bug when attempting
to change the _temp_cache function to use the new
temp_is_locked function at the suggestion of a reviewer.

This version reverts that as the logic in _temp_cache isn't
really conducive to that change because of the tests it makes
and the change was causing problems.

This is the single change that is reverted in Git.pm 
compared to v3 of the patch:

diff --git a/perl/Git.pm b/perl/Git.pm
index 0ba15b9..204fdc6 100644
--- a/perl/Git.pm
+++ b/perl/Git.pm
@@ -1277,7 +1277,7 @@ sub _temp_cache {
 
 	my $temp_fd = \$TEMP_FILEMAP{$name};
 	if (defined $$temp_fd and $$temp_fd->opened) {
-		if (temp_is_locked($name)) {
+		if ($TEMP_FILES{$$temp_fd}{locked}) {
 			throw Error::Simple("Temp file with moniker '" .
 				$name . "' already in use");
 		}


Other than that single change, this patch series is identical to v3,
and in particular the 0002 perl/Git/SVN/Fetcher.pm change is identical
to that of v3.

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             | 31 ++++++++++++++++++++++++++++++-
 perl/Git/SVN/Fetcher.pm |  6 ++++--
 2 files changed, 34 insertions(+), 3 deletions(-)

-- 
1.8.3

[PATCH v4 1/2] Git.pm: add new temp_is_locked function

[Kyle J. McKay]

The temp_is_locked function can be used to determine whether
or not a given name previously passed to temp_acquire is
currently locked.

Signed-off-by: Kyle J. McKay <mackyle@gmail.com>
---
 perl/Git.pm | 31 ++++++++++++++++++++++++++++++-
 1 file changed, 30 insertions(+), 1 deletion(-)

diff --git a/perl/Git.pm b/perl/Git.pm
index 7a252ef..204fdc6 100644
--- a/perl/Git.pm
+++ b/perl/Git.pm
@@ -61,7 +61,7 @@ require Exporter;
                 remote_refs prompt
                 get_tz_offset
                 credential credential_read credential_write
-                temp_acquire temp_release temp_reset temp_path);
+                temp_acquire temp_is_locked temp_release temp_reset temp_path);
 
 
 =head1 DESCRIPTION
@@ -1206,6 +1206,35 @@ sub temp_acquire {
 	$temp_fd;
 }
 
+=item temp_is_locked ( NAME )
+
+Returns true if the internal lock created by a previous C<temp_acquire()>
+call with C<NAME> is still in effect.
+
+When temp_acquire is called on a C<NAME>, it internally locks the temporary
+file mapped to C<NAME>.  That lock will not be released until C<temp_release()>
+is called with either the original C<NAME> or the L<File::Handle> that was
+returned from the original call to temp_acquire.
+
+Subsequent attempts to call C<temp_acquire()> with the same C<NAME> will fail
+unless there has been an intervening C<temp_release()> call for that C<NAME>
+(or its corresponding L<File::Handle> that was returned by the original
+C<temp_acquire()> call).
+
+If true is returned by C<temp_is_locked()> for a C<NAME>, an attempt to
+C<temp_acquire()> the same C<NAME> will cause an error unless
+C<temp_release> is first called on that C<NAME> (or its corresponding
+L<File::Handle> that was returned by the original C<temp_acquire()> call).
+
+=cut
+
+sub temp_is_locked {
+	my ($self, $name) = _maybe_self(@_);
+	my $temp_fd = \$TEMP_FILEMAP{$name};
+
+	defined $$temp_fd && $$temp_fd->opened && $TEMP_FILES{$$temp_fd}{locked};
+}
+
 =item temp_release ( NAME )
 
 =item temp_release ( FILEHANDLE )
-- 
1.8.3

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

[Kyle J. McKay]

When attempting to git-svn fetch files from an svn https?: url using
the serf library (the only choice starting with svn 1.8) the following
errors can occur:

Temp file with moniker 'svn_delta' already in use at Git.pm line 1250
Temp file with moniker 'git_blob' already in use at Git.pm line 1250

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:

1. open_root
2. open_directory
3. add_file
4. apply_textdelta
5. add_file
6. apply_textdelta

When using the ra_serf access method, git-svn can end up needing
to create several temp files before the first one is closed.

This change causes a new temp file moniker to be generated if the
one that would otherwise have been used is currently locked.

Signed-off-by: Kyle J. McKay <mackyle@gmail.com>
---
 perl/Git/SVN/Fetcher.pm | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/perl/Git/SVN/Fetcher.pm b/perl/Git/SVN/Fetcher.pm
index bd17418..10edb27 100644
--- a/perl/Git/SVN/Fetcher.pm
+++ b/perl/Git/SVN/Fetcher.pm
@@ -315,11 +315,13 @@ sub change_file_prop {
 sub apply_textdelta {
 	my ($self, $fb, $exp) = @_;
 	return undef if $self->is_path_ignored($fb->{path});
-	my $fh = $::_repository->temp_acquire('svn_delta');
+	my $suffix = 0;
+	++$suffix while $::_repository->temp_is_locked("svn_delta_${$}_$suffix");
+	my $fh = $::_repository->temp_acquire("svn_delta_${$}_$suffix");
 	# $fh gets auto-closed() by SVN::TxDelta::apply(),
 	# (but $base does not,) so dup() it for reading in close_file
 	open my $dup, '<&', $fh or croak $!;
-	my $base = $::_repository->temp_acquire('git_blob');
+	my $base = $::_repository->temp_acquire("git_blob_${$}_$suffix");
 
 	if ($fb->{blob}) {
 		my ($base_is_link, $size);
-- 
1.8.3

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

[Jonathan Nieder]

(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.

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.

The approach is generally good.  I wanted to propose some clearer
documentation for temp_is_locked() but didn't end up finding a moment
for it, so... meh.  I'll be happy to help get the details right if
someone else finds time for that (hint, hint).

Hope that helps,
Jonathan

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

[Kyle J. McKay]

On Jul 18, 2013, at 12:29, Jonathan Nieder wrote:
> (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:
> 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.

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

[Junio C Hamano]

"Kyle J. McKay" <mackyle@gmail.com> writes:

> This patch allows git-svn to fetch successfully using the
> serf library when given an https?: url to fetch from.
> ...
> Versions v2-v3 of the patch introduced a bug when attempting
> to change the _temp_cache function to use the new
> temp_is_locked function at the suggestion of a reviewer.

Err, excuse me, is this meant to _replace_ what is already in 'next'
for the past 6 days?

Could you feed an incremental update, highlighting where the update
improves compared to the previous attempt in the log message?

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

[Eric Wong]

Jonathan Nieder <jrnieder@gmail.com> wrote:
> (cc-ing Eric Wong, who maintains git-svn and knows both it and
> the libsvn perl bindings much better than I do)

I doubt that's the case anymore.  I've hardly looked at SVN in many
years, now.

Anyways, if the patches:

1) do not introduce regressions
2) fixes real problems

I'm inclined to think they're OK...

I'm sorry I can't help more, I had access to better drugs back then.

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

[Kyle J. McKay]

On Jul 18, 2013, at 16:34, Junio C Hamano wrote:
> "Kyle J. McKay" <mackyle@gmail.com> writes:
>
>> This patch allows git-svn to fetch successfully using the
>> serf library when given an https?: url to fetch from.
>> ...
>> Versions v2-v3 of the patch introduced a bug when attempting
>> to change the _temp_cache function to use the new
>> temp_is_locked function at the suggestion of a reviewer.
>
> Err, excuse me, is this meant to _replace_ what is already in 'next'
> for the past 6 days?
>
> Could you feed an incremental update, highlighting where the update
> improves compared to the previous attempt in the log message?

OK.  I have sent out an "incremental update against 'next' branch'"  
patch based on how a set of recent incremental updates to 'next' was  
sent to the list on 2013-07-04.  If that is not what you had in mind,  
I apologize in advance.