{"thread":{"id":"34366","subject":"[PATCH v3 0/2] allow git-svn fetching to work using serf","startedAt":"2013-07-07T04:20:47Z","lastAt":"2013-07-18T19:35:52Z","messageCount":7,"participants":["Kyle J. McKay","Junio C Hamano","David Rothenberger","Jonathan Nieder"],"isPatch":true,"patchVersion":3,"patchTotal":2},"messages":[{"id":"222699","messageId":"1373170849-9150-1-git-send-email-mackyle@gmail.com","threadId":"34366","inReplyTo":null,"subject":"[PATCH v3 0/2] allow git-svn fetching to work using serf","fromName":"Kyle J. McKay","fromEmail":"mackyle@gmail.com","sentAt":"2013-07-07T04:20:47Z","receivedAt":"2013-07-07T04:20:47Z","isPatch":true,"sender":{"key":"mackyle@gmail.com","avatar":"https://avatars.githubusercontent.com/u/813346?v=4"},"body":"From: \"Kyle J. McKay\" \n\nThis patch allows git-svn to fetch successfully using the\nserf library when given an https?: url to fetch from.\n\nUnfortunately some svn servers do not seem to be configured\nwell for use with the serf library. This can cause fetching\nto take longer compared to the neon library or actually\ncause timeouts during the fetch. When timeouts occur\ngit-svn can be safely restarted to fetch more revisions.\n\nA new temp_is_locked function has been added to Git.pm\nto facilitate using the minimal number of temp files\npossible when using serf.\n\nThe problem that occurs when running git-svn fetch using\nthe serf library is that the previously used temp file\nis not always unlocked before the next temp file needs\nto be used.\n\nTo work around this problem, a new temp name is used\nif the temp name that would otherwise be chosen is\ncurrently locked.\n\nVersion v2 of the patch introduced a bug when changing the _temp_cache\nfunction to use the new temp_is_locked function at the suggestion of a\nreviewer. That has now been resolved.\n\nKyle J. McKay (2):\n Git.pm: add new temp_is_locked function\n git-svn: allow git-svn fetching to work using serf\n\n perl/Git.pm | 33 +++++++++++++++++++++++++++++++--\n perl/Git/SVN/Fetcher.pm | 6 ++++--\n 2 files changed, 35 insertions(+), 4 deletions(-)\n\n-- \n1.8.3\n"},{"id":"222701","messageId":"1373170849-9150-2-git-send-email-mackyle@gmail.com","threadId":"34366","inReplyTo":"1373170849-9150-1-git-send-email-mackyle@gmail.com","subject":"[PATCH v3 1/2] Git.pm: add new temp_is_locked function","fromName":"Kyle J. McKay","fromEmail":"mackyle@gmail.com","sentAt":"2013-07-07T04:20:48Z","receivedAt":"2013-07-07T04:20:48Z","isPatch":true,"sender":{"key":"mackyle@gmail.com","avatar":"https://avatars.githubusercontent.com/u/813346?v=4"},"body":"From: \"Kyle J. McKay\" \n\nThe temp_is_locked function can be used to determine whether\nor not a given name previously passed to temp_acquire is\ncurrently locked.\n\nSigned-off-by: Kyle J. McKay \n---\n perl/Git.pm | 33 +++++++++++++++++++++++++++++++--\n 1 file changed, 31 insertions(+), 2 deletions(-)\n\ndiff --git a/perl/Git.pm b/perl/Git.pm\nindex 7a252ef..0ba15b9 100644\n--- a/perl/Git.pm\n+++ b/perl/Git.pm\n@@ -61,7 +61,7 @@ require Exporter;\n remote_refs prompt\n get_tz_offset\n credential credential_read credential_write\n- temp_acquire temp_release temp_reset temp_path);\n+ temp_acquire temp_is_locked temp_release temp_reset temp_path);\n \n \n =head1 DESCRIPTION\n@@ -1206,6 +1206,35 @@ sub temp_acquire {\n \t$temp_fd;\n }\n \n+=item temp_is_locked ( NAME )\n+\n+Returns true if the internal lock created by a previous C\n+call with C is still in effect.\n+\n+When temp_acquire is called on a C, it internally locks the temporary\n+file mapped to C. That lock will not be released until C\n+is called with either the original C or the L that was\n+returned from the original call to temp_acquire.\n+\n+Subsequent attempts to call C with the same C will fail\n+unless there has been an intervening C call for that C\n+(or its corresponding L that was returned by the original\n+C call).\n+\n+If true is returned by C for a C, an attempt to\n+C the same C will cause an error unless\n+C is first called on that C (or its corresponding\n+L that was returned by the original C call).\n+\n+=cut\n+\n+sub temp_is_locked {\n+\tmy ($self, $name) = _maybe_self(@_);\n+\tmy $temp_fd = \\$TEMP_FILEMAP{$name};\n+\n+\tdefined $$temp_fd && $$temp_fd->opened && $TEMP_FILES{$$temp_fd}{locked};\n+}\n+\n =item temp_release ( NAME )\n \n =item temp_release ( FILEHANDLE )\n@@ -1248,7 +1277,7 @@ sub _temp_cache {\n \n \tmy $temp_fd = \\$TEMP_FILEMAP{$name};\n \tif (defined $$temp_fd and $$temp_fd->opened) {\n-\t\tif ($TEMP_FILES{$$temp_fd}{locked}) {\n+\t\tif (temp_is_locked($name)) {\n \t\t\tthrow Error::Simple(\"Temp file with moniker '\" .\n \t\t\t\t$name . \"' already in use\");\n \t\t}\n-- \n1.8.3\n"},{"id":"222700","messageId":"1373170849-9150-3-git-send-email-mackyle@gmail.com","threadId":"34366","inReplyTo":"1373170849-9150-1-git-send-email-mackyle@gmail.com","subject":"[PATCH v3 2/2] git-svn: allow git-svn fetching to work using serf","fromName":"Kyle J. McKay","fromEmail":"mackyle@gmail.com","sentAt":"2013-07-07T04:20:49Z","receivedAt":"2013-07-07T04:20:49Z","isPatch":true,"sender":{"key":"mackyle@gmail.com","avatar":"https://avatars.githubusercontent.com/u/813346?v=4"},"body":"From: \"Kyle J. McKay\" \n\nWhen attempting to git-svn fetch files from an svn https?: url using\nthe serf library (the only choice starting with svn 1.8) the following\nerrors can occur:\n\nTemp file with moniker 'svn_delta' already in use at Git.pm line 1250\nTemp file with moniker 'git_blob' already in use at Git.pm line 1250\n\nDavid Rothenberger has determined the cause to\nbe that ra_serf does not drive the delta editor in a depth-first\nmanner [...]. Instead, the calls come in this order:\n\n1. open_root\n2. open_directory\n3. add_file\n4. apply_textdelta\n5. add_file\n6. apply_textdelta\n\nWhen using the ra_serf access method, git-svn can end up needing\nto create several temp files before the first one is closed.\n\nThis change causes a new temp file moniker to be generated if the\none that would otherwise have been used is currently locked.\n\nSigned-off-by: Kyle J. McKay \n---\n perl/Git/SVN/Fetcher.pm | 6 ++++--\n 1 file changed, 4 insertions(+), 2 deletions(-)\n\ndiff --git a/perl/Git/SVN/Fetcher.pm b/perl/Git/SVN/Fetcher.pm\nindex bd17418..10edb27 100644\n--- a/perl/Git/SVN/Fetcher.pm\n+++ b/perl/Git/SVN/Fetcher.pm\n@@ -315,11 +315,13 @@ sub change_file_prop {\n sub apply_textdelta {\n \tmy ($self, $fb, $exp) = @_;\n \treturn undef if $self->is_path_ignored($fb->{path});\n-\tmy $fh = $::_repository->temp_acquire('svn_delta');\n+\tmy $suffix = 0;\n+\t++$suffix while $::_repository->temp_is_locked(\"svn_delta_${$}_$suffix\");\n+\tmy $fh = $::_repository->temp_acquire(\"svn_delta_${$}_$suffix\");\n \t# $fh gets auto-closed() by SVN::TxDelta::apply(),\n \t# (but $base does not,) so dup() it for reading in close_file\n \topen my $dup, '<&', $fh or croak $!;\n-\tmy $base = $::_repository->temp_acquire('git_blob');\n+\tmy $base = $::_repository->temp_acquire(\"git_blob_${$}_$suffix\");\n \n \tif ($fb->{blob}) {\n \t\tmy ($base_is_link, $size);\n-- \n1.8.3\n"},{"id":"222843","messageId":"7vip0l10ow.fsf@alter.siamese.dyndns.org","threadId":"34366","inReplyTo":"1373170849-9150-1-git-send-email-mackyle@gmail.com","subject":"Re: [PATCH v3 0/2] allow git-svn fetching to work using serf","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-07-08T16:22:23Z","receivedAt":"2013-07-08T16:22:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Kyle J. McKay\" writes:\n\n> From: \"Kyle J. McKay\" \n>\n> This patch allows git-svn to fetch successfully using the\n> serf library when given an https?: url to fetch from.\n>\n> Unfortunately some svn servers do not seem to be configured\n> well for use with the serf library. This can cause fetching\n> to take longer compared to the neon library or actually\n> cause timeouts during the fetch. When timeouts occur\n> git-svn can be safely restarted to fetch more revisions.\n>\n> A new temp_is_locked function has been added to Git.pm\n> to facilitate using the minimal number of temp files\n> possible when using serf.\n>\n> The problem that occurs when running git-svn fetch using\n> the serf library is that the previously used temp file\n> is not always unlocked before the next temp file needs\n> to be used.\n>\n> To work around this problem, a new temp name is used\n> if the temp name that would otherwise be chosen is\n> currently locked.\n>\n> Version v2 of the patch introduced a bug when changing the _temp_cache\n> function to use the new temp_is_locked function at the suggestion of a\n> reviewer. That has now been resolved.\n\nThanks; I've queued this version to 'pu' at least tentatively.\n\nIs everybody who discussed the issue happy with the direction of\nthis patch?\n"},{"id":"223677","messageId":"loom.20130718T202918-857@post.gmane.org","threadId":"34366","inReplyTo":"1373170849-9150-2-git-send-email-mackyle@gmail.com","subject":"Re: [PATCH v3 1/2] Git.pm: add new temp_is_locked function","fromName":"David Rothenberger","fromEmail":"daveroth@acm.org","sentAt":"2013-07-18T18:34:48Z","receivedAt":"2013-07-18T18:34:48Z","isPatch":true,"sender":{"key":"daveroth@acm.org","avatar":null},"body":"Kyle J. McKay gmail.com> writes:\n\n> +sub temp_is_locked {\n> +\tmy ($self, $name) = _maybe_self( _);\n> +\tmy $temp_fd = \\$TEMP_FILEMAP{$name};\n> +\n> +\tdefined $$temp_fd && $$temp_fd->opened && $TEMP_FILES{$$temp_fd}{locked};\n> +}\n> +\n> =item temp_release ( NAME )\n> \n> =item temp_release ( FILEHANDLE )\n> -1248,7 +1277,7 sub _temp_cache {\n> \n> \tmy $temp_fd = \\$TEMP_FILEMAP{$name};\n> \tif (defined $$temp_fd and $$temp_fd->opened) {\n> -\t\tif ($TEMP_FILES{$$temp_fd}{locked}) {\n> +\t\tif (temp_is_locked($name)) {\n> \t\t\tthrow Error::Simple(\"Temp file with moniker '\" .\n> \t\t\t\t$name . \"' already in use\");\n> \t\t}\n\nThere's a problem with this use of temp_is_locked. There is an else\nclause right after this:\n\n\t} else {\n\t\tif (defined $$temp_fd) {\n\t\t\t# then we're here because of a closed handle.\n\nPrior to the patch, the comment is correct, but after the patch, the\nif block may also be entered if the file is open but locked. This is\nbecause temp_is_locked checks that the temp file is defined, open,\nand locked.\n\nThis issue leads to lots of \n\n Temp file 'svn_delta_3360_0' was closed. Opening replacement. \n\nmessages for me.\n\nReverting the change in _temp_cache solves the problem for me.\nAdding an \" && !$$temp_fd->opened\" clause to the if statement also\nworks, but this is less efficient.\n"},{"id":"223680","messageId":"842D3E63-9E6B-45F0-A7F6-03082C4D067F@gmail.com","threadId":"34366","inReplyTo":"loom.20130718T202918-857@post.gmane.org","subject":"Re: [PATCH v3 1/2] Git.pm: add new temp_is_locked function","fromName":"Kyle J. McKay","fromEmail":"mackyle@gmail.com","sentAt":"2013-07-18T19:14:47Z","receivedAt":"2013-07-18T19:14:47Z","isPatch":true,"sender":{"key":"mackyle@gmail.com","avatar":"https://avatars.githubusercontent.com/u/813346?v=4"},"body":"On Jul 18, 2013, at 11:34, David Rothenberger wrote:\n> Kyle J. McKay gmail.com> writes:\n>\n>> +sub temp_is_locked {\n>> +\tmy ($self, $name) = _maybe_self( _);\n>> +\tmy $temp_fd = \\$TEMP_FILEMAP{$name};\n>> +\n>> +\tdefined $$temp_fd && $$temp_fd->opened && $TEMP_FILES{$$temp_fd} \n>> {locked};\n>> +}\n>> +\n>> =item temp_release ( NAME )\n>>\n>> =item temp_release ( FILEHANDLE )\n>> -1248,7 +1277,7 sub _temp_cache {\n>>\n>> \tmy $temp_fd = \\$TEMP_FILEMAP{$name};\n>> \tif (defined $$temp_fd and $$temp_fd->opened) {\n>> -\t\tif ($TEMP_FILES{$$temp_fd}{locked}) {\n>> +\t\tif (temp_is_locked($name)) {\n>> \t\t\tthrow Error::Simple(\"Temp file with moniker '\" .\n>> \t\t\t\t$name . \"' already in use\");\n>> \t\t}\n>\n> There's a problem with this use of temp_is_locked. There is an else\n> clause right after this:\n>\n> \t} else {\n> \t\tif (defined $$temp_fd) {\n> \t\t\t# then we're here because of a closed handle.\n>\n> Prior to the patch, the comment is correct, but after the patch, the\n> if block may also be entered if the file is open but locked. This is\n> because temp_is_locked checks that the temp file is defined, open,\n> and locked.\n>\n> This issue leads to lots of\n>\n> Temp file 'svn_delta_3360_0' was closed. Opening replacement.\n>\n> messages for me.\n>\n> Reverting the change in _temp_cache solves the problem for me.\n> Adding an \" && !$$temp_fd->opened\" clause to the if statement also\n> works, but this is less efficient.\n\nThat change was made as a result of this feedback:\n\nOn Jul 6, 2013, at 17:11, Jonathan Nieder wrote:\n> Hi,\n>\n> Kyle McKay wrote:\n>\n>> The temp_is_locked function can be used to determine whether\n>> or not a given name previously passed to temp_acquire is\n>> currently locked.\n> [...]\n>> +=item temp_is_locked ( NAME )\n>> +\n>> +Returns true if the file mapped to C is currently locked.\n>> +\n>> +If true is returned, an attempt to C the same\n>\n[snip]\n>\n> Looking more closely, it looks like this is factoring out the idiom\n> for checking if a name is already in use from the _temp_cache\n> function. Would it make sense for _temp_cache to call this helper?\n\nSo I think the answer is it does not make sense for _temp_cache to \ncall this helper.\n\nWill release a v4 in just a moment with that single change reverted.\n"},{"id":"223686","messageId":"20130718193552.GU14690@google.com","threadId":"34366","inReplyTo":"842D3E63-9E6B-45F0-A7F6-03082C4D067F@gmail.com","subject":"Re: [PATCH v3 1/2] Git.pm: add new temp_is_locked function","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-07-18T19:35:52Z","receivedAt":"2013-07-18T19:35:52Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Kyle J. McKay wrote:\n\n> That change was made as a result of this feedback:\n>\n> On Jul 6, 2013, at 17:11, Jonathan Nieder wrote:\n>> Kyle McKay wrote:\n>>\n>>> The temp_is_locked function can be used to determine whether\n>>> or not a given name previously passed to temp_acquire is\n>>> currently locked.\n>> [...]\n>>> +=item temp_is_locked ( NAME )\n>>> +\n>>> +Returns true if the file mapped to C is currently locked.\n>>> +\n>>> +If true is returned, an attempt to C the same\n>>\n> [snip]\n>\n>> Looking more closely, it looks like this is factoring out the idiom\n>> for checking if a name is already in use from the _temp_cache\n>> function. Would it make sense for _temp_cache to call this helper?\n>\n> So I think the answer is it does not make sense for _temp_cache to\n> call this helper.\n\nThanks for looking into it.\n\nSorry for the confusion. The point of my question was an example of a\nway to make sure the internal API stays easy to understand. But it\nseems to have backfired, and this is a small enough isolated change\nthat I think it's okay to say \"let's clean it up later\".\n\n> Will release a v4 in just a moment with that single change reverted.\n\nThanks.\n"}]}