threads / patch / 34475

v4, 2 partsGit.pm: add new temp_is_locked function

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

## tl;dr

8 messages between Jul 18, 2013 and Jul 19, 2013. Diffs are folded; open one to read it.

replies: 7people: 4as markdown or json

Kyle J. McKay· Jul 18, 2013, 19:15 UTC · lore

[PATCH v4 0/2] allow 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.

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:

Show changes to perl/Git.pm +1 −1

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

Kyle J. McKay· Jul 18, 2013, 19:15 UTC · re: Kyle J. McKay · lore

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

Show changes to perl/Git.pm +30 −1

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

Kyle J. McKay· Jul 18, 2013, 19:15 UTC · re: Kyle J. McKay · lore

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

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

Show changes to perl/Git/SVN/Fetcher.pm +4 −2

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

Jonathan Nieder· Jul 18, 2013, 19:29 UTC · re: Kyle J. McKay · lore

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

(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

Kyle J. McKay· Jul 18, 2013, 23:31 UTC · re: Jonathan Nieder · lore

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

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.

Eric Wong· Jul 18, 2013, 23:40 UTC · re: Jonathan Nieder · lore

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

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.

Junio C Hamano· Jul 18, 2013, 23:34 UTC · re: Kyle J. McKay · lore

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

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

Show 6 quoted lines

> 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?

Kyle J. McKay· Jul 19, 2013, 00:16 UTC · re: Junio C Hamano · lore

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

On Jul 18, 2013, at 16:34, Junio C Hamano wrote:

Show 14 quoted lines

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