{"thread":{"id":"34362","subject":"[PATCH 2/2] git-svn: allow git-svn fetching to work using serf","startedAt":"2013-07-06T03:44:07Z","lastAt":"2013-07-07T18:27:20Z","messageCount":8,"participants":["Kyle McKay","Jonathan Nieder","Daniel Shahaf","David Rothenberger"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"222664","messageId":"ABDE5FFA-C19F-44BF-A360-3FD5D74F2B28@gmail.com","threadId":"34362","inReplyTo":null,"subject":"[PATCH 2/2] git-svn: allow git-svn fetching to work using serf","fromName":"Kyle McKay","fromEmail":"mackyle@gmail.com","sentAt":"2013-07-06T03:44:07Z","receivedAt":"2013-07-06T03:44:07Z","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 <daveroth@acm.org> 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\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 <mackyle@gmail.com>\n---\nperl/Git/SVN/Fetcher.pm | 6 ++++--\n1 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 {\nsub 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_${$}_ \n$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":"222678","messageId":"20130707002430.GE30132@google.com","threadId":"34362","inReplyTo":"ABDE5FFA-C19F-44BF-A360-3FD5D74F2B28@gmail.com","subject":"Re: [PATCH 2/2] git-svn: allow git-svn fetching to work using serf","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-07-07T00:24:30Z","receivedAt":"2013-07-07T00:24:30Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"(cc-ing Eric Wong, who wrote this code)\nHi,\n\nKyle McKay wrote:\n\n> Temp file with moniker 'svn_delta' already in use at Git.pm line 1250\n> Temp file with moniker 'git_blob' already in use at Git.pm line 1250\n>\n> David Rothenberger <daveroth@acm.org> 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> --- 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\nThanks for your work tracking this down.\n\nI'm a bit confused.  Are you saying that apply_textdelta gets called\nmultiple times in a row without an intervening close_file?\n\nPuzzled,\nJonathan\n"},{"id":"222686","messageId":"8CACBE8F-8672-43AB-882E-4ADA05B4D822@gmail.com","threadId":"34362","inReplyTo":"20130707002430.GE30132@google.com","subject":"Re: [PATCH 2/2] git-svn: allow git-svn fetching to work using serf","fromName":"Kyle McKay","fromEmail":"mackyle@gmail.com","sentAt":"2013-07-07T02:13:42Z","receivedAt":"2013-07-07T02:13:42Z","isPatch":true,"sender":{"key":"mackyle@gmail.com","avatar":"https://avatars.githubusercontent.com/u/813346?v=4"},"body":"On Jul 6, 2013, at 17:24, Jonathan Nieder wrote:\n> (cc-ing Eric Wong, who wrote this code)\n> Hi,\n>\n> Kyle McKay wrote:\n>\n>> Temp file with moniker 'svn_delta' already in use at Git.pm line 1250\n>> Temp file with moniker 'git_blob' already in use at Git.pm line 1250\n>>\n>> David Rothenberger <daveroth@acm.org> 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>> --- 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_${$}_ \n>> $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> Thanks for your work tracking this down.\n>\n> I'm a bit confused.  Are you saying that apply_textdelta gets called\n> multiple times in a row without an intervening close_file?\n\nUnless bulk updates are disabled when using the serf access method  \n(the only one available with svn 1.8) for https?: urls,  \napply_textdelta does indeed get called multiple times in a row without  \nan intervening temp_release.\n\nTwo temp files are created for each apply_textdelta ('svn_delta...'  \nand 'git_blob...').  In my tests it seems that most of the time the  \ntwo temp files are enough, but occasionally as many as six will be  \nopen at the same time.\n\nI suspect this maximum number is related to the maximum number of  \nsimultaneous connections the serf access method will use which  \ndefaults to 4.  Therefore I would expect to see as many as 8 temp  \nfiles (4 each for 'svn_delta...' and 'git_blob...'), but I have only  \nbeen able to trigger creation of 6 temp files so far.\n\nKyle\n"},{"id":"222688","messageId":"20130707022332.GD4193@google.com","threadId":"34362","inReplyTo":"8CACBE8F-8672-43AB-882E-4ADA05B4D822@gmail.com","subject":"Re: [PATCH 2/2] git-svn: allow git-svn fetching to work using serf","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-07-07T02:23:32Z","receivedAt":"2013-07-07T02:23:32Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Kyle McKay wrote:\n\n> Unless bulk updates are disabled when using the serf access method\n> (the only one available with svn 1.8) for https?: urls,\n> apply_textdelta does indeed get called multiple times in a row\n> without an intervening temp_release.\n\nYou mean \"Unless bulk updates are enabled\" and \"without an intervening\nclose_file\", right?\n\nUnlike the non-depth-first thing, that sounds basically broken ---\nwhat would be stopping subversion from calling the editor's close\nmethod when done with each file?  I can't see much reason unless it is\ncalling apply_textdelta multiple times in parallel --- is it doing\nthat, and if so is git-svn able to cope with that?\n\nThis sounds like something that should be fixed in ra_serf.\n\nBut if the number of overlapping open text nodes is bounded by a low\nnumber, the workaround of using multiple temp files sounds ok as a way\nof dealing with unfixed versions of Subversion.\n\nJonathan\n"},{"id":"222690","messageId":"3871C226-16AE-4E25-8AD3-007EDAB0E25F@gmail.com","threadId":"34362","inReplyTo":"20130707022332.GD4193@google.com","subject":"Re: [PATCH 2/2] git-svn: allow git-svn fetching to work using serf","fromName":"Kyle McKay","fromEmail":"mackyle@gmail.com","sentAt":"2013-07-07T02:46:40Z","receivedAt":"2013-07-07T02:46:40Z","isPatch":true,"sender":{"key":"mackyle@gmail.com","avatar":"https://avatars.githubusercontent.com/u/813346?v=4"},"body":"On Jul 6, 2013, at 19:23, Jonathan Nieder wrote:\n> Kyle McKay wrote:\n>\n>> Unless bulk updates are disabled when using the serf access method\n>> (the only one available with svn 1.8) for https?: urls,\n>> apply_textdelta does indeed get called multiple times in a row\n>> without an intervening temp_release.\n>\n> You mean \"Unless bulk updates are enabled\" and \"without an intervening\n> close_file\", right?\n\nThe problem seems to be skelta mode although it may just be the fact  \nthat ra_serf has multiple connections outstanding and since ra_neon  \nonly ever has one it can't happen over ra_neon.\n\nIf the server disables bulk updates (SVNAllowBulkUpdates Off) all  \nclients are forced to use skelta mode, even ra_neon clients.\n\n> This sounds like something that should be fixed in ra_serf.\n\nYes, but apparently it will not be.\n\n> But if the number of overlapping open text nodes is bounded by a low\n> number, the workaround of using multiple temp files sounds ok as a way\n> of dealing with unfixed versions of Subversion.\n\nI believe it will never exceed twice ('svn_delta...' and  \n'git_blob...') the maximum number of serf connections allowed.  Four  \nby default (hard-coded prior to svn 1.8).  Limited to between 1 and 8  \non svn 1.8.  Actually it looks like from my testing that it won't ever  \nexceed twice the (max number of serf connections - 1).\n\nKyle\n"},{"id":"222747","messageId":"20130707133957.GA3648@lp-shahaf.local","threadId":"34362","inReplyTo":"3871C226-16AE-4E25-8AD3-007EDAB0E25F@gmail.com","subject":"Re: [PATCH 2/2] git-svn: allow git-svn fetching to work using serf","fromName":"Daniel Shahaf","fromEmail":"danielsh@apache.org","sentAt":"2013-07-07T13:39:57Z","receivedAt":"2013-07-07T13:39:57Z","isPatch":true,"sender":{"key":"danielsh@apache.org","avatar":null},"body":"Kyle McKay wrote on Sat, Jul 06, 2013 at 19:46:40 -0700:\n> On Jul 6, 2013, at 19:23, Jonathan Nieder wrote:\n>> Kyle McKay wrote:\n>>\n>>> Unless bulk updates are disabled when using the serf access method\n>>> (the only one available with svn 1.8) for https?: urls,\n>>> apply_textdelta does indeed get called multiple times in a row\n>>> without an intervening temp_release.\n>>\n>> You mean \"Unless bulk updates are enabled\" and \"without an intervening\n>> close_file\", right?\n>\n> The problem seems to be skelta mode although it may just be the fact  \n> that ra_serf has multiple connections outstanding and since ra_neon only \n> ever has one it can't happen over ra_neon.\n>\n> If the server disables bulk updates (SVNAllowBulkUpdates Off) all  \n> clients are forced to use skelta mode, even ra_neon clients.\n\nAs Brane and I have pointed out, git-svn can instruct libsvn_* to use\nbulk updates regardless of the server version, by setting\nSVN_CONFIG_OPTION_HTTP_BULK_UPDATES (new in 1.8).\n\nIf you have questions about that, though, please address them to\nusers@subversion.apache.org (the proper list for API usage questions),\nnot to me personally.\n\nCheers,\n\nDaniel\n"},{"id":"222748","messageId":"51D994DC.9050500@acm.org","threadId":"34362","inReplyTo":"20130707133957.GA3648@lp-shahaf.local","subject":"Re: [PATCH 2/2] git-svn: allow git-svn fetching to work using serf","fromName":"David Rothenberger","fromEmail":"daveroth@acm.org","sentAt":"2013-07-07T16:18:36Z","receivedAt":"2013-07-07T16:18:36Z","isPatch":true,"sender":{"key":"daveroth@acm.org","avatar":null},"body":"On 7/7/2013 6:39 AM, Daniel Shahaf wrote:\n> Kyle McKay wrote on Sat, Jul 06, 2013 at 19:46:40 -0700:\n>> On Jul 6, 2013, at 19:23, Jonathan Nieder wrote:\n>>> Kyle McKay wrote:\n>>>\n>>>> Unless bulk updates are disabled when using the serf access method\n>>>> (the only one available with svn 1.8) for https?: urls,\n>>>> apply_textdelta does indeed get called multiple times in a row\n>>>> without an intervening temp_release.\n>>>\n>>> You mean \"Unless bulk updates are enabled\" and \"without an intervening\n>>> close_file\", right?\n>>\n>> The problem seems to be skelta mode although it may just be the fact  \n>> that ra_serf has multiple connections outstanding and since ra_neon only \n>> ever has one it can't happen over ra_neon.\n>>\n>> If the server disables bulk updates (SVNAllowBulkUpdates Off) all  \n>> clients are forced to use skelta mode, even ra_neon clients.\n> \n> As Brane and I have pointed out, git-svn can instruct libsvn_* to use\n> bulk updates regardless of the server version, by setting\n> SVN_CONFIG_OPTION_HTTP_BULK_UPDATES (new in 1.8).\n\nAccording to the table in the release notes [1], Skelta mode will be\nused if the 1.7 or 1.8 server sets SVNAllowBulkUpdates to Off,\nregardless of what the client sets in the configuration.\n\nIs that not true?\n\n[1] https://subversion.apache.org/docs/release-notes/1.8.html#neon-deleted\n\n-- \nDavid Rothenberger  ----  daveroth@acm.org\n"},{"id":"222766","messageId":"053E9C47-31D9-4BD8-A417-4CEC371B1A07@gmail.com","threadId":"34362","inReplyTo":"20130707133957.GA3648@lp-shahaf.local","subject":"Re: [PATCH 2/2] git-svn: allow git-svn fetching to work using serf","fromName":"Kyle McKay","fromEmail":"mackyle@gmail.com","sentAt":"2013-07-07T18:27:20Z","receivedAt":"2013-07-07T18:27:20Z","isPatch":true,"sender":{"key":"mackyle@gmail.com","avatar":"https://avatars.githubusercontent.com/u/813346?v=4"},"body":"I forwarded the \"SVNAllowBulkUpdates Off\" question to the users@subversion.apache.org \n  list and here's the reply:\n\nOn Jul 7, 2013, at 11:11, Lieven Govaerts wrote:\n> On Sun, Jul 7, 2013 at 4:48 PM, Kyle McKay <mackyle@gmail.com> wrote:\n>> On Jul 7, 2013, at 06:39, Daniel Shahaf wrote:\n>>>\n>>> Kyle McKay wrote on Sat, Jul 06, 2013 at 19:46:40 -0700:\n>>>>\n>>>> On Jul 6, 2013, at 19:23, Jonathan Nieder wrote:\n>>>>>\n>>>>> Kyle McKay wrote:\n>>>>>\n>>>>>> Unless bulk updates are disabled when using the serf access  \n>>>>>> method\n>>>>>> (the only one available with svn 1.8) for https?: urls,\n>>>>>> apply_textdelta does indeed get called multiple times in a row\n>>>>>> without an intervening temp_release.\n>>>>>\n>>>>>\n>>>>> You mean \"Unless bulk updates are enabled\" and \"without an  \n>>>>> intervening\n>>>>> close_file\", right?\n>>>>\n>>>>\n>>>> The problem seems to be skelta mode although it may just be the  \n>>>> fact\n>>>> that ra_serf has multiple connections outstanding and since  \n>>>> ra_neon only\n>>>> ever has one it can't happen over ra_neon.\n>>>>\n>>>> If the server disables bulk updates (SVNAllowBulkUpdates Off) all\n>>>> clients are forced to use skelta mode, even ra_neon clients.\n>>>\n>>>\n>>> As Brane and I have pointed out, git-svn can instruct libsvn_* to  \n>>> use\n>>> bulk updates regardless of the server version, by setting\n>>> SVN_CONFIG_OPTION_HTTP_BULK_UPDATES (new in 1.8).\n>>>\n>>> If you have questions about that, though, please address them to\n>>> users@subversion.apache.org (the proper list for API usage  \n>>> questions),\n>>> not to me personally.\n>>\n>>\n>> According to the table at\n>> <http://subversion.apache.org/docs/release-notes/1.8.html#serf-skelta-default \n>> >,\n>> if the server sets SVNAllowBulkUpdates Off, the client will be  \n>> forced to use\n>> skelta no matter what the client setting is.\n>\n> Indeed, the server admin has the final say in which mode is actually\n> used. SVNAllowBulkUpdates Off is only advised if the server admin\n> wants a log line per accessed resource. I doubt it's used a lot, but\n> the option is there.\n>\n>>\n>> Is that table incorrect?\n>\n> No, that table is correct.\n>\n> Lieven\n\nSo the final say so on whether or not bulk updates are allowed is on  \nthe server side which means git-svn really needs to handle skelta mode  \non the client side properly when using ra-serf to guarantee  \nfunctionality with all subversion server configurations.\n\nKyle\n"}]}