{"thread":{"id":"38655","subject":"Re: git svn import failure : write .git/Git_svn_hash_BmjclS: Bad file descriptor","startedAt":"2015-02-26T09:09:09Z","lastAt":"2015-03-23T19:56:43Z","messageCount":6,"participants":["Nico Schlömer","Kyle J. McKay","Eric Wong","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"256672","messageId":"CAK6Z60fhyY3wPG6hx7YJ4y7wZsWb=33JhqejmpVBgZLGdB1XMA@mail.gmail.com","threadId":"38655","inReplyTo":"CAK6Z60ciheWOUGOv1sYcA==B2WR1Rs_eMU+9a=R3FBwc_37CyQ@mail.gmail.com","subject":"Re: git svn import failure : write .git/Git_svn_hash_BmjclS: Bad file descriptor","fromName":"Nico Schlömer","fromEmail":"nico.schloemer@gmail.com","sentAt":"2015-02-26T09:09:09Z","receivedAt":"2015-02-26T09:09:09Z","isPatch":false,"sender":{"key":"nico.schloemer@gmail.com","avatar":"https://gravatar.com/avatar/b0a6998b0c9bcd0641b4d07789be6f64e21fce87df71a94b952611c7994b9878?d=mp&s=160"},"body":"All,\n\nI applied Kyle's latest patch\n\n> diff --git a/perl/Git/SVN/Ra.pm b/perl/Git/SVN/Ra.pm\n> index 622535e2..96888228 100644\n> --- a/perl/Git/SVN/Ra.pm\n> +++ b/perl/Git/SVN/Ra.pm\n> @@ -391,6 +391,7 @@ sub longest_common_path {\n> sub gs_fetch_loop_common {\n>         my ($self, $base, $head, $gsv, $globs) = @_;\n>         return if ($base > $head);\n> +       $::_repository->_open_cat_blob_if_needed;\n>         my $gpool = SVN::Pool->new_default;\n>         my $ra_url = $self->url;\n>         my $reload_ra = sub {\n\nalone and this fixes the bug for me. Thanks a lot, Kyle!\n\nCheers,\nNico\n\nOn Thu, Feb 26, 2015 at 12:37 AM, Nico Schlömer\n<nico.schloemer@gmail.com> wrote:\n> Kyle,\n>\n> you are absolutely correct, I used the wrong Git installation in my last\n> email. With both your and Eric's patches applied, I'm getting\n> ```\n> [...]\n> r100 = e2a9b5baa2cebb18591ecb04ff350410d52f36de (refs/remotes/git-svn)\n> Unexpected result returned from git cat-file at\n> /home/nschloe/share/perl/5.18.2/Git/SVN/Fetcher.pm line 344.\n> Failed to read object 619f9d1d857fb287d06a70c9dac6b8b534d0de6a at\n> /home/nschloe/share/perl/5.18.2/Git/SVN/Fetcher.pm line 345, <GEN16> line\n> 757.\n>\n> error closing pipe: Bad file descriptor at\n> /home/nschloe/libexec/git-core/git-svn line 0.\n> error closing pipe: Bad file descriptor at\n> /home/nschloe/libexec/git-core/git-svn line 0.\n> ```\n> where line 344 is\n> ```\n> $size = $::_repository->cat_blob($fb->{blob}, $base);\n> ```\n> Adding the line in `perl/Git/SVN/Ra.pm` as per your suggestion does not\n> change this.\n>\n> Cheers,\n> Nico\n>\n>\n> On Wed, Feb 25, 2015 at 9:34 PM Kyle J. McKay <mackyle@gmail.com> wrote:\n>>\n>> On Feb 25, 2015, at 07:07, Nico Schlömer wrote:\n>>\n>> > Thanks Kyle for the patch! I applied it and ran\n>> > ```\n>> > git svn clone https://geuz.org/svn/gmsh/trunk\n>> > ```\n>> > Unfortunately, I'm still getting\n>> > ```\n>> > [...]\n>> > r100 = e2a9b5baa2cebb18591ecb04ff350410d52f36de (refs/remotes/git-svn)\n>> > error closing pipe: Bad file descriptor at /usr/share/perl5/Git/SVN/\n>> > Fetcher.pm line 335.\n>> > error closing pipe: Bad file descriptor at /usr/share/perl5/Git/SVN/\n>> > Fetcher.pm line 335.\n>> > out pipe went bad at /usr/share/perl5/Git.pm line 955.\n>> > ```\n>>\n>> Are you certain you're running with the patch?  I ask because earlier\n>> you sent this:\n>>\n>> On Jan 31, 2015, at 04:51, Nico Schlömer wrote:\n>>\n>> > I tried the patch and I still get\n>> > ```\n>> > [...]\n>> > r100 = e2a9b5baa2cebb18591ecb04ff350410d52f36de (refs/remotes/git-svn)\n>> > Unexpected result returned from git cat-file at\n>> > /home/nschloe/share/perl/5.18.2/Git/SVN/Fetcher.pm line 335.\n>> > Failed to read object 619f9d1d857fb287d06a70c9dac6b8b534d0de6a at\n>> > /home/nschloe/share/perl/5.18.2/Git/SVN/Fetcher.pm line 336, <GEN16>\n>> > line 757.\n>> >\n>> > error closing pipe: Bad file descriptor at\n>> > /home/nschloe/libexec/git-core/git-svn line 0.\n>> > error closing pipe: Bad file descriptor at\n>> > /home/nschloe/libexec/git-core/git-svn line 0.\n>> > ```\n>> > when\n>> > ```\n>> > git svn clone https://geuz.org/svn/gmsh/trunk\n>> > ```\n>>\n>> And as the patch below applies to Fetcher.pm at line 322 and inserts 8\n>> lines, I do not see how you could be getting the same error message at\n>> line 335 if the patch was present.  Even if you manually applied it by\n>> only inserting the two lines of code, the line numbers would be at\n>> least 337, not 335 as reported above.  I also notice the path to\n>> Fetcher.pm is different from your earlier report.\n>>\n>> That said, the fact that it happens right after r100 (which is when\n>> SVN::Pool::clear is getting called) suggests there's another file\n>> handle getting caught up in the SVN::Pool somehow.  (When I try to\n>> clone gmsh, I got to r4885 before a server disconnection.)\n>>\n>> It could be as simple as the open2 call FileHandle result in later\n>> perl versions ends up in the SVN::Pool whereas in earlier Perl and/or\n>> svn versions it does not.\n>>\n>> What happens if you add this change on top of the Fetcher.pm change:\n>>\n>> diff --git a/perl/Git/SVN/Ra.pm b/perl/Git/SVN/Ra.pm\n>> index 622535e2..96888228 100644\n>> --- a/perl/Git/SVN/Ra.pm\n>> +++ b/perl/Git/SVN/Ra.pm\n>> @@ -391,6 +391,7 @@ sub longest_common_path {\n>>   sub gs_fetch_loop_common {\n>>         my ($self, $base, $head, $gsv, $globs) = @_;\n>>         return if ($base > $head);\n>> +       $::_repository->_open_cat_blob_if_needed;\n>>         my $gpool = SVN::Pool->new_default;\n>>         my $ra_url = $self->url;\n>>         my $reload_ra = sub {\n>>\n>> -Kyle\n>>\n>> > Cheers,\n>> > Nico\n>> >\n>> > On Wed, Feb 25, 2015 at 11:20 AM Kyle J. McKay <mackyle@gmail.com>\n>> > wrote:\n>> > I believe I have been able to track down this problem and implement a\n>> > fix.  Please report back if this patch fixes the problem for you.\n>> >\n>> > -Kyle\n>> >\n>> > -- 8< --\n>> > Subject: [PATCH] Git::SVN::Fetcher: avoid premature 'svn_hash' temp\n>> > file closure\n>> >\n>> > Since b19138b (git-svn: Make it incrementally faster by minimizing\n>> > temp\n>> > files, v1.6.0), git-svn has been using the Git.pm temp_acquire and\n>> > temp_release mechanism to avoid unnecessary temp file churn and\n>> > provide\n>> > a speed boost.\n>> >\n>> > However, that change introduced a call to temp_acquire inside the\n>> > Git::SVN::Fetcher::close_file function for an 'svn_hash' temp file.\n>> > Because an SVN::Pool is active at the time this function is called, if\n>> > the Git::temp_acquire function ends up actually creating a new\n>> > FileHandle for the temp file (which it will the first time it's called\n>> > with the name 'svn_hash') that FileHandle will end up in the SVN::Pool\n>> > and should that pool have SVN::Pool::clear called on it that\n>> > FileHandle\n>> > will be closed out from under Git::temp_acquire.\n>> >\n>> > Since the only call site to Git::temp_acquire with the name 'svn_hash'\n>> > is inside the close_file function, if an 'svn_hash' temp file is ever\n>> > created its FileHandle is guaranteed to be created in the active\n>> > SVN::Pool.\n>> >\n>> > This has not been a problem in the past because the SVN::Pool was not\n>> > being cleared.  However, since dfa72fdb (git-svn: reload RA every\n>> > log-window-size, v2.2.0) the pool has been getting cleared\n>> > periodically\n>> > at which point the FileHandle for the 'svn_hash' temp file gets\n>> > closed.\n>> > Any subsequent calls to Git::temp_acquire for 'svn_hash', however,\n>> > succeed without creating/opening a new temporary file since it still\n>> > has\n>> > the now invalid FileHandle in its cache.  Callers that then attempt to\n>> > use that FileHandle fail with an error.\n>> >\n>> > We avoid this problem by making sure the 'svn_hash' temp file is\n>> > created\n>> > in the same place the 'svn_delta_...' and 'git_blob_...' temp files\n>> > are\n>> > (and then temp_release'd) so that it can be safely used inside the\n>> > close_file function without having its FileHandle end up in an\n>> > SVN::Pool\n>> > that gets cleared.\n>> >\n>> > Signed-off-by: Kyle J. McKay <mackyle@gmail.com>\n>> > ---\n>> >  perl/Git/SVN/Fetcher.pm | 8 ++++++++\n>> >  1 file changed, 8 insertions(+)\n>> >\n>> > diff --git a/perl/Git/SVN/Fetcher.pm b/perl/Git/SVN/Fetcher.pm\n>> > index 10edb277..613055a3 100644\n>> > --- a/perl/Git/SVN/Fetcher.pm\n>> > +++ b/perl/Git/SVN/Fetcher.pm\n>> > @@ -322,6 +322,14 @@ sub apply_textdelta {\n>> >         # (but $base does not,) so dup() it for reading in close_file\n>> >         open my $dup, '<&', $fh or croak $!;\n>> >         my $base = $::_repository->temp_acquire(\"git_blob_${$}_\n>> > $suffix\");\n>> > +       # close_file may call temp_acquire on 'svn_hash', but\n>> > because of the\n>> > +       # call chain, if the temp_acquire call from close_file ends\n>> > up being the\n>> > +       # call that first creates the 'svn_hash' temp file, then the\n>> > FileHandle\n>> > +       # that's created as a result will end up in an SVN::Pool\n>> > that we clear\n>> > +       # in SVN::Ra::gs_fetch_loop_common.  Avoid that by making\n>> > sure the\n>> > +       # 'svn_hash' FileHandle is already created before close_file\n>> > is called.\n>> > +       my $tmp_fh = $::_repository->temp_acquire('svn_hash');\n>> > +       $::_repository->temp_release($tmp_fh, 1);\n>> >\n>> >         if ($fb->{blob}) {\n>> >                 my ($base_is_link, $size);\n>> > --\n>>\n>\n"},{"id":"256688","messageId":"cbcb926c1b7edd31548a15ea4966a35@74d39fa044aa309eaea14b9f57fe79c","threadId":"38655","inReplyTo":"CAK6Z60fhyY3wPG6hx7YJ4y7wZsWb=33JhqejmpVBgZLGdB1XMA@mail.gmail.com","subject":"Re: git svn import failure : write .git/Git_svn_hash_BmjclS: Bad file descriptor","fromName":"Kyle J. McKay","fromEmail":"mackyle@gmail.com","sentAt":"2015-02-26T13:49:34Z","receivedAt":"2015-02-26T13:49:34Z","isPatch":false,"sender":{"key":"mackyle@gmail.com","avatar":"https://avatars.githubusercontent.com/u/813346?v=4"},"body":"On Feb 26, 2015, at 01:09, Nico Schl=F6mer wrote:\n> All,\n>\n> I applied Kyle's latest patch\n>\n>> diff --git a/perl/Git/SVN/Ra.pm b/perl/Git/SVN/Ra.pm\n>> index 622535e2..96888228 100644\n>> --- a/perl/Git/SVN/Ra.pm\n>> +++ b/perl/Git/SVN/Ra.pm\n>> @@ -391,6 +391,7 @@ sub longest_common_path {\n>> sub gs_fetch_loop_common {\n>>        my ($self, $base, $head, $gsv, $globs) =3D @_;\n>>        return if ($base > $head);\n>> +       $::_repository->_open_cat_blob_if_needed;\n>>        my $gpool =3D SVN::Pool->new_default;\n>>        my $ra_url =3D $self->url;\n>>        my $reload_ra =3D sub {\n>\n> alone and this fixes the bug for me. Thanks a lot, Kyle!\n\nThe updated patch with the additional fix is below.\n\nThere are two symptoms it addresses:\n\n1) the 'Git_svn_hash_... bad file descriptor' failure\n\n2) the 'out pipe went bad' failure\n\nWhile (1) could probably also be addressed by Eric's alternate\n\"destroy all tempfiles when reloading RA\" patch, that would require re-\nopening all the temp files every 100 revisions (or whatever the log\nwindow size is changed to).\n\nI noticed that with the default log window size of 100 revisions, each\ntime the connection was reset at the 100-revision boundary (to reduce\nthe overall memory usage) it seemed to take approx. 3 seconds to start\nup again processing revisions on that gmsh repository.  If you're\ncloning 30000 revisions, that adds 15 minutes to the total clone time\nalready.  Admittedly opening new temp files will be a lot faster and\nhardly noticeable, but doing that 300 times over the course of 30000\nrevisions will probably add at least a little extra delay and since\nblowing away all the temp files does not seem to be necessary, why\nincur the extra delay?\n\nAlso the \"destroy all tempfiles when reloading RA\" patch isn't going\nto fix the cat_blob 'out pipe went bad' problem since that has nothing\nto do with the temp files so another change will still be required for\nthat.\n\n-Kyle\n\n-- 8< --\nSubject: [PATCH v2] Git::SVN::*: avoid premature FileHandle closure\n\nSince b19138b (git-svn: Make it incrementally faster by minimizing temp\nfiles, v1.6.0), git-svn has been using the Git.pm temp_acquire and\ntemp_release mechanism to avoid unnecessary temp file churn and provide\na speed boost.\n\nHowever, that change introduced a call to temp_acquire inside the\nGit::SVN::Fetcher::close_file function for an 'svn_hash' temp file.\nBecause an SVN::Pool is active at the time this function is called, if\nthe Git::temp_acquire function ends up actually creating a new\nFileHandle for the temp file (which it will the first time it's called\nwith the name 'svn_hash') that FileHandle will end up in the SVN::Pool\nand should that pool have SVN::Pool::clear called on it that FileHandle\nwill be closed out from under Git::temp_acquire.\n\nSince the only call site to Git::temp_acquire with the name 'svn_hash'\nis inside the close_file function, if an 'svn_hash' temp file is ever\ncreated its FileHandle is guaranteed to be created in the active\nSVN::Pool.\n\nThis has not been a problem in the past because the SVN::Pool was not\nbeing cleared.  However, since dfa72fdb (git-svn: reload RA every\nlog-window-size, v2.2.0) the pool has been getting cleared periodically\nat which point the FileHandle for the 'svn_hash' temp file gets closed.\nAny subsequent calls to Git::temp_acquire for 'svn_hash', however,\nsucceed without creating/opening a new temporary file since it still has\nthe now invalid FileHandle in its cache.  Callers that then attempt to\nuse that FileHandle fail with an error.\n\nWe avoid this problem by making sure the 'svn_hash' temp file is created\nin the same place the 'svn_delta_...' and 'git_blob_...' temp files are\n(and then temp_release'd) so that it can be safely used inside the\nclose_file function without having its FileHandle end up in an SVN::Pool\nthat gets cleared.\n\nAdditionally the Git.pm cat_blob function creates a bidirectional pipe\nFileHandle using the IPC::Open2::open2 function.  If that handle is\ncreated too late, it also gets caught up in the SVN::Pool and incorrectly\nclosed by the SVN::Pool::clear call.  But this only seems to happen with\nmore recent versions of Perl and svn.\n\nTo avoid this problem we add an explicit call to _open_cat_blob_if_needed\nbefore the first call to SVN::Pool->new_default to make sure the open2\nhandle does not end up in the SVN::Pool.\n\nSigned-off-by: Kyle J. McKay <mackyle@gmail.com>\n---\n perl/Git/SVN/Fetcher.pm | 8 ++++++++\n perl/Git/SVN/Ra.pm      | 3 +++\n 2 files changed, 11 insertions(+)\n\ndiff --git a/perl/Git/SVN/Fetcher.pm b/perl/Git/SVN/Fetcher.pm\nindex 10edb277..613055a3 100644\n--- a/perl/Git/SVN/Fetcher.pm\n+++ b/perl/Git/SVN/Fetcher.pm\n@@ -322,6 +322,14 @@ sub apply_textdelta {\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_${$}_$suffix\");\n+\t# close_file may call temp_acquire on 'svn_hash', but because of the\n+\t# call chain, if the temp_acquire call from close_file ends up being the\n+\t# call that first creates the 'svn_hash' temp file, then the FileHandle\n+\t# that's created as a result will end up in an SVN::Pool that we clear\n+\t# in SVN::Ra::gs_fetch_loop_common.  Avoid that by making sure the\n+\t# 'svn_hash' FileHandle is already created before close_file is called.\n+\tmy $tmp_fh = $::_repository->temp_acquire('svn_hash');\n+\t$::_repository->temp_release($tmp_fh, 1);\n \n \tif ($fb->{blob}) {\n \t\tmy ($base_is_link, $size);\ndiff --git a/perl/Git/SVN/Ra.pm b/perl/Git/SVN/Ra.pm\nindex 622535e2..32e2f19e 100644\n--- a/perl/Git/SVN/Ra.pm\n+++ b/perl/Git/SVN/Ra.pm\n@@ -391,6 +391,9 @@ sub longest_common_path {\n sub gs_fetch_loop_common {\n \tmy ($self, $base, $head, $gsv, $globs) = @_;\n \treturn if ($base > $head);\n+\t# Make sure the cat_blob open2 FileHandle is created before calling\n+\t# SVN::Pool::new_default so that it does not incorrectly end up in the pool.\n+\t$::_repository->_open_cat_blob_if_needed;\n \tmy $gpool = SVN::Pool->new_default;\n \tmy $ra_url = $self->url;\n \tmy $reload_ra = sub {\n--\n"},{"id":"256710","messageId":"20150226212721.GA2930@dcvr.yhbt.net","threadId":"38655","inReplyTo":"cbcb926c1b7edd31548a15ea4966a35@74d39fa044aa309eaea14b9f57fe79c","subject":"Re: git svn import failure : write .git/Git_svn_hash_BmjclS: Bad file descriptor","fromName":"Eric Wong","fromEmail":"normalperson@yhbt.net","sentAt":"2015-02-26T21:27:21Z","receivedAt":"2015-02-26T21:27:21Z","isPatch":false,"sender":{"key":"e@80x24.org","avatar":null},"body":"\"Kyle J. McKay\" <mackyle@gmail.com> wrote:\n> The updated patch with the additional fix is below.\n\nThanks, signed-off and pushed to master on git://bogomips.org/git-svn\nI've dropped my \"destroy all tempfiles...\" patch.\n"},{"id":"258380","messageId":"xmqqa8z3a5rm.fsf@gitster.dls.corp.google.com","threadId":"38655","inReplyTo":"20150226212721.GA2930@dcvr.yhbt.net","subject":"Re: git svn import failure : write .git/Git_svn_hash_BmjclS: Bad file descriptor","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-03-23T19:11:09Z","receivedAt":"2015-03-23T19:11:09Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Wong <normalperson@yhbt.net> writes:\n\n> \"Kyle J. McKay\" <mackyle@gmail.com> wrote:\n>> The updated patch with the additional fix is below.\n>\n> Thanks, signed-off and pushed to master on git://bogomips.org/git-svn\n> I've dropped my \"destroy all tempfiles...\" patch.\n\nI think I missed this exchange.  I see these two patches:\n\n    e0b4cad Git::SVN::*: avoid premature FileHandle closure\n    ce1b57b git-svn: fix localtime=true on non-glibc environments\n\non the 'master' branch of your git://git.bogomips.org/git-svn.git/\nrepository.  Is this a good time to pull from you, and should I\nexpect more during this cycle (I am guessing that the answers would\nbe yes and no, but just to make sure...)?\n\nThanks.\n"},{"id":"258381","messageId":"20150323193634.GA3454@dcvr.yhbt.net","threadId":"38655","inReplyTo":"xmqqa8z3a5rm.fsf@gitster.dls.corp.google.com","subject":"Re: git svn import failure : write .git/Git_svn_hash_BmjclS: Bad file descriptor","fromName":"Eric Wong","fromEmail":"normalperson@yhbt.net","sentAt":"2015-03-23T19:36:34Z","receivedAt":"2015-03-23T19:36:34Z","isPatch":false,"sender":{"key":"e@80x24.org","avatar":null},"body":"Junio C Hamano <gitster@pobox.com> wrote:\n> Eric Wong <normalperson@yhbt.net> writes:\n> > \"Kyle J. McKay\" <mackyle@gmail.com> wrote:\n> >> The updated patch with the additional fix is below.\n> >\n> > Thanks, signed-off and pushed to master on git://bogomips.org/git-svn\n> > I've dropped my \"destroy all tempfiles...\" patch.\n> \n> I think I missed this exchange.  I see these two patches:\n> \n>     e0b4cad Git::SVN::*: avoid premature FileHandle closure\n>     ce1b57b git-svn: fix localtime=true on non-glibc environments\n> \n> on the 'master' branch of your git://git.bogomips.org/git-svn.git/\n> repository.  Is this a good time to pull from you, and should I\n> expect more during this cycle (I am guessing that the answers would\n> be yes and no, but just to make sure...)?\n\nCorrect, it's a good time to pull and I don't expect to have more time\nto work on new features (lazy load) for a bit.  I was under the\nimpression you already pulled:\n\nhttp://mid.gmane.org/xmqq61aol44q.fsf@gitster.dls.corp.google.com\n"},{"id":"258382","messageId":"xmqq619ra3no.fsf@gitster.dls.corp.google.com","threadId":"38655","inReplyTo":"20150323193634.GA3454@dcvr.yhbt.net","subject":"Re: git svn import failure : write .git/Git_svn_hash_BmjclS: Bad file descriptor","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-03-23T19:56:43Z","receivedAt":"2015-03-23T19:56:43Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Wong <normalperson@yhbt.net> writes:\n\n> Correct, it's a good time to pull and I don't expect to have more time\n> to work on new features (lazy load) for a bit.  I was under the\n> impression you already pulled:\n>\n> http://mid.gmane.org/xmqq61aol44q.fsf@gitster.dls.corp.google.com\n\nIndeed I did.  I was fooled by my own rebase X-<.\n\nThanks, then I am fully up to date with you.\n"}]}