{"thread":{"id":"34475","subject":"[PATCH v4 1/2] Git.pm: add new temp_is_locked function","startedAt":"2013-07-18T19:15:27Z","lastAt":"2013-07-19T00:16:44Z","messageCount":8,"participants":["Kyle J. McKay","Jonathan Nieder","Junio C Hamano","Eric Wong"],"isPatch":true,"patchVersion":4,"patchTotal":2},"messages":[{"id":"223682","messageId":"8a6adfae744011599e410da7a46808b@f74d39fa044aa309eaea14b9f57fe79","threadId":"34475","inReplyTo":null,"subject":"[PATCH v4 0/2] allow git-svn fetching to work using serf","fromName":"Kyle J. McKay","fromEmail":"mackyle@gmail.com","sentAt":"2013-07-18T19:15:27Z","receivedAt":"2013-07-18T19:15:27Z","isPatch":true,"sender":{"key":"mackyle@gmail.com","avatar":"https://avatars.githubusercontent.com/u/813346?v=4"},"body":"This 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\nVersions v2-v3 of the patch introduced a bug when attempting\nto change the _temp_cache function to use the new\ntemp_is_locked function at the suggestion of a reviewer.\n\nThis version reverts that as the logic in _temp_cache isn't\nreally conducive to that change because of the tests it makes\nand the change was causing problems.\n\nThis is the single change that is reverted in Git.pm \ncompared to v3 of the patch:\n\ndiff --git a/perl/Git.pm b/perl/Git.pm\nindex 0ba15b9..204fdc6 100644\n--- a/perl/Git.pm\n+++ b/perl/Git.pm\n@@ -1277,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_is_locked($name)) {\n+\t\tif ($TEMP_FILES{$$temp_fd}{locked}) {\n \t\t\tthrow Error::Simple(\"Temp file with moniker '\" .\n \t\t\t\t$name . \"' already in use\");\n \t\t}\n\n\nOther than that single change, this patch series is identical to v3,\nand in particular the 0002 perl/Git/SVN/Fetcher.pm change is identical\nto that of v3.\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 | 31 ++++++++++++++++++++++++++++++-\n perl/Git/SVN/Fetcher.pm | 6 ++++--\n 2 files changed, 34 insertions(+), 3 deletions(-)\n\n-- \n1.8.3\n"},{"id":"223681","messageId":"59bf58623f9a0e344af1d2055673b54@f74d39fa044aa309eaea14b9f57fe79","threadId":"34475","inReplyTo":"8a6adfae744011599e410da7a46808b@f74d39fa044aa309eaea14b9f57fe79","subject":"[PATCH v4 1/2] Git.pm: add new temp_is_locked function","fromName":"Kyle J. McKay","fromEmail":"mackyle@gmail.com","sentAt":"2013-07-18T19:15:28Z","receivedAt":"2013-07-18T19:15:28Z","isPatch":true,"sender":{"key":"mackyle@gmail.com","avatar":"https://avatars.githubusercontent.com/u/813346?v=4"},"body":"The 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 | 31 ++++++++++++++++++++++++++++++-\n 1 file changed, 30 insertions(+), 1 deletion(-)\n\ndiff --git a/perl/Git.pm b/perl/Git.pm\nindex 7a252ef..204fdc6 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-- \n1.8.3\n"},{"id":"223683","messageId":"ac83ba2812aee248275a0760bb39a00@f74d39fa044aa309eaea14b9f57fe79","threadId":"34475","inReplyTo":"8a6adfae744011599e410da7a46808b@f74d39fa044aa309eaea14b9f57fe79","subject":"[PATCH v4 2/2] git-svn: allow git-svn fetching to work using serf","fromName":"Kyle J. McKay","fromEmail":"mackyle@gmail.com","sentAt":"2013-07-18T19:15:29Z","receivedAt":"2013-07-18T19:15:29Z","isPatch":true,"sender":{"key":"mackyle@gmail.com","avatar":"https://avatars.githubusercontent.com/u/813346?v=4"},"body":"When 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":"223684","messageId":"20130718192943.GT14690@google.com","threadId":"34475","inReplyTo":"ac83ba2812aee248275a0760bb39a00@f74d39fa044aa309eaea14b9f57fe79","subject":"Re: [PATCH v4 2/2] git-svn: allow git-svn fetching to work using serf","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-07-18T19:29:43Z","receivedAt":"2013-07-18T19:29:43Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"(cc-ing Eric Wong, who maintains git-svn and knows both it and\nthe libsvn perl bindings much better than I do)\nKyle J. McKay wrote:\n\n> David Rothenberger has determined the cause to\n> be that ra_serf does not drive the delta editor in a depth-first\n> manner [...]. Instead, the calls come in this order:\n\nThanks.\n\nSorry to nitpick, but the problem is not depth-first versus\nbreadth-first versus random. Blaming the traversal order makes this\ncompletely confusing. The actual problem is that the driver asks us\nto keep multiple files open at a time.\n\nThe approach taken in this patch would be racy if the driver calls us\nmultiple times concurrently (since temp_acquire can fail). I believe\nit doesn't but haven't checked.\n\nThe approach is generally good. I wanted to propose some clearer\ndocumentation for temp_is_locked() but didn't end up finding a moment\nfor it, so... meh. I'll be happy to help get the details right if\nsomeone else finds time for that (hint, hint).\n\nHope that helps,\nJonathan\n"},{"id":"223722","messageId":"53FD2913-2A8E-42E1-9C76-D0AEBD76E0DA@gmail.com","threadId":"34475","inReplyTo":"20130718192943.GT14690@google.com","subject":"Re: [PATCH v4 2/2] git-svn: allow git-svn fetching to work using serf","fromName":"Kyle J. McKay","fromEmail":"mackyle@gmail.com","sentAt":"2013-07-18T23:31:52Z","receivedAt":"2013-07-18T23:31:52Z","isPatch":true,"sender":{"key":"mackyle@gmail.com","avatar":"https://avatars.githubusercontent.com/u/813346?v=4"},"body":"On Jul 18, 2013, at 12:29, Jonathan Nieder wrote:\n> (cc-ing Eric Wong, who maintains git-svn and knows both it and\n> the libsvn perl bindings much better than I do)\n> Kyle J. McKay wrote:\n>\n>> David Rothenberger has determined the cause to\n>> be that ra_serf does not drive the delta editor in a depth-first\n>> manner [...]. Instead, the calls come in this order:\n>\n> Thanks.\n>\n> Sorry to nitpick, but the problem is not depth-first versus\n> breadth-first versus random. Blaming the traversal order makes this\n> completely confusing. The actual problem is that the driver asks us\n> to keep multiple files open at a time.\n\nOn Sun, 07 Jul 2013 18:00:40 GMT, Branko Čibej wrote:\n> On 07.07.2013 19:40, Jonathan Nieder wrote:\n>> (cc-ing subversion's users@ list for advice)\n>> Kyle McKay wrote:\n>>> On Jul 6, 2013, at 18:37, Jonathan Nieder wrote:\n>>\n>>>\n>>>> Kyle McKay wrote:\n>>>>> Begin forwarded message:\n>>>>>> [2] http://subversion.tigris.org/issues/show_bug.cgi?id=2932\n>>>>>\n>>>> Ah, thanks for the context.\n>>>>\n>>>> It's still not clear to me how we know that ra_serf driving the \n>>>> editor\n>>>> in a non depth-first manner is the problem here. Has that \n>>>> explanation\n>>>> been confirmed somehow?\n>>> [...]\n>>> Since ra_serf makes multiple connections to the server (hard-coded\n>>> to 4 prior to svn 1.8, defaults to 4 in svn 1.8 but can be set to\n>>> between 1 and 8) it makes sense there would be multiple active calls\n>>> to apply_textdelta if processing is done as results are received on\n>>> the multiple connections.\n>> Ah, that's worrisome. Do I understand you correctly that to work \n>> with\n>> ra_serf in skelta mode, callers need to make their apply_textdelta\n>> callback thread-safe?\n>\n> No; the editor drive is single-threaded, but the order of the \n> operations\n> isn't strictly depth-first.\n\n\nBrane also describes this as a non-depth-first traversal order which \nis the root of the problem. If the order of operations were strictly \ndepth-first, the previous file would end up being closed before the \nnext one's opened.\n\n> The approach taken in this patch would be racy if the driver calls us\n> multiple times concurrently (since temp_acquire can fail). I believe\n> it doesn't but haven't checked.\n\nBrane says the editor drive is single-threaded so that doesn't seem \nlike a problem.\n"},{"id":"223724","messageId":"7v8v131lye.fsf@alter.siamese.dyndns.org","threadId":"34475","inReplyTo":"8a6adfae744011599e410da7a46808b@f74d39fa044aa309eaea14b9f57fe79","subject":"Re: [PATCH v4 0/2] allow git-svn fetching to work using serf","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-07-18T23:34:49Z","receivedAt":"2013-07-18T23:34:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Kyle J. McKay\" writes:\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> Versions v2-v3 of the patch introduced a bug when attempting\n> to change the _temp_cache function to use the new\n> temp_is_locked function at the suggestion of a reviewer.\n\nErr, excuse me, is this meant to _replace_ what is already in 'next'\nfor the past 6 days?\n\nCould you feed an incremental update, highlighting where the update\nimproves compared to the previous attempt in the log message?\n"},{"id":"223725","messageId":"20130718234047.GA17346@dcvr.yhbt.net","threadId":"34475","inReplyTo":"20130718192943.GT14690@google.com","subject":"Re: [PATCH v4 2/2] git-svn: allow git-svn fetching to work using serf","fromName":"Eric Wong","fromEmail":"normalperson@yhbt.net","sentAt":"2013-07-18T23:40:47Z","receivedAt":"2013-07-18T23:40:47Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"Jonathan Nieder wrote:\n> (cc-ing Eric Wong, who maintains git-svn and knows both it and\n> the libsvn perl bindings much better than I do)\n\nI doubt that's the case anymore. I've hardly looked at SVN in many\nyears, now.\n\nAnyways, if the patches:\n\n1) do not introduce regressions\n2) fixes real problems\n\nI'm inclined to think they're OK...\n\nI'm sorry I can't help more, I had access to better drugs back then.\n"},{"id":"223728","messageId":"ACCFF6B2-010B-465A-A011-E56BCE64C119@gmail.com","threadId":"34475","inReplyTo":"7v8v131lye.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v4 0/2] allow git-svn fetching to work using serf","fromName":"Kyle J. McKay","fromEmail":"mackyle@gmail.com","sentAt":"2013-07-19T00:16:44Z","receivedAt":"2013-07-19T00:16:44Z","isPatch":true,"sender":{"key":"mackyle@gmail.com","avatar":"https://avatars.githubusercontent.com/u/813346?v=4"},"body":"On Jul 18, 2013, at 16:34, Junio C Hamano wrote:\n> \"Kyle J. McKay\" writes:\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>> Versions v2-v3 of the patch introduced a bug when attempting\n>> to change the _temp_cache function to use the new\n>> temp_is_locked function at the suggestion of a reviewer.\n>\n> Err, excuse me, is this meant to _replace_ what is already in 'next'\n> for the past 6 days?\n>\n> Could you feed an incremental update, highlighting where the update\n> improves compared to the previous attempt in the log message?\n\nOK. I have sent out an \"incremental update against 'next' branch'\" \npatch based on how a set of recent incremental updates to 'next' was \nsent to the list on 2013-07-04. If that is not what you had in mind, \nI apologize in advance.\n"}]}