# [PATCH] Don't try to reclose in command_close_bidi_pipe 4 messages from 2009-01-30 to 2009-01-31. Participants: Yuval Kogman, Tay Ray Chuan. Thread: https://gitlist.dev/t/17451 ## Yuval Kogman, 2009-01-30 04:59 Subject: [PATCH] Don't try to reclose in command_close_bidi_pipe Message-ID: <1233291570-23295-1-git-send-email-nothingmuch@woobling.org> URL: https://gitlist.dev/e/1233291570-23295-1-git-send-email-nothingmuch%40woobling.org ``` Some commands require their standard input to be closed (like git-commit-tree). This patch changes command_close_bidi_pipe so no longer tries to close already closed handles, resulting in an error. --- perl/Git.pm | 1 + 1 files changed, 1 insertions(+), 0 deletions(-) diff --git a/perl/Git.pm b/perl/Git.pm index 7d7f2b1..283bba8 100644 --- a/perl/Git.pm +++ b/perl/Git.pm @@ -422,6 +422,7 @@ sub command_close_bidi_pipe { local $?; my ($pid, $in, $out, $ctx) = @_; foreach my $fh ($in, $out) { + next unless defined(fileno($fh)); unless (close $fh) { if ($!) { carp "error closing pipe: $!"; -- 1.6.1 ``` ## Tay Ray Chuan, 2009-01-30 14:35 Subject: Re: [PATCH] Don't try to reclose in command_close_bidi_pipe Message-ID: URL: https://gitlist.dev/e/be6fef0d0901300635v4e6009f1n7da405ebc4c8dd68%40mail.gmail.com In-Reply-To: <1233291570-23295-1-git-send-email-nothingmuch@woobling.org> ``` Hi, On Fri, Jan 30, 2009 at 12:59 PM, Yuval Kogman wrote: > Some commands require their standard input to be closed (like > git-commit-tree). This patch changes command_close_bidi_pipe so no > longer tries to close already closed handles, resulting in an error. >From this message, you seem to intend this as a fix. Can you tell us how one might go about to reproduce this issue? > --- > perl/Git.pm | 1 + > 1 files changed, 1 insertions(+), 0 deletions(-) > > diff --git a/perl/Git.pm b/perl/Git.pm > index 7d7f2b1..283bba8 100644 > --- a/perl/Git.pm > +++ b/perl/Git.pm > @@ -422,6 +422,7 @@ sub command_close_bidi_pipe { > local $?; > my ($pid, $in, $out, $ctx) = @_; > foreach my $fh ($in, $out) { > + next unless defined(fileno($fh)); Quoting the perldoc for fileno at http://perldoc.perl.org/functions/fileno.html "(Filehandles connected to memory objects via new features of open may return undefined even though they are open.)" Is "unless defined(fileno($fh))" a reliable way to check if the handle is closed? -- Cheers, Ray Chuan ``` ## Yuval Kogman, 2009-01-30 15:06 Subject: Re: [PATCH] Don't try to reclose in command_close_bidi_pipe Message-ID: URL: https://gitlist.dev/e/a891e1bd0901300706h4fdffe24m78c418f8c1ae7cd8%40mail.gmail.com In-Reply-To: ``` Resending, forgot to CC 2009/1/30 Tay Ray Chuan > From this message, you seem to intend this as a fix. Can you tell us > how one might go about to reproduce this issue? Here is an example: http://github.com/yanick/git-cpan-patch/blob/ed67d3f86f371764935fd0da3e7f08536c95b606/git-cpan-import#L190 Since git-commit-tree requires the message to be written before it can write the sha1 of the new commit object, the handle has to be closed already, which makes command_close_bidi_pipe die with an error. The workaround in the linked code reopens a fake handle so that close() will not error. For what it's worth, I've been poking at t/t9700/test.pl but since it's pretty sparse I figured I should contribute this fix independently. > Is "unless defined(fileno($fh))" a reliable way to check if the handle > is closed? Yes, since the handle is expected to be the result of a pipe open. The documentation refers to this case: open my $fh, "<", \$buffer; in which case the filehandle $fh has no associated file descriptor. fileno() is not a universal check, but it is applicable in this situation. ``` ## Tay Ray Chuan, 2009-01-31 00:03 Subject: Re: [PATCH] Don't try to reclose in command_close_bidi_pipe Message-ID: URL: https://gitlist.dev/e/be6fef0d0901301603m452b1579q509bdc997ecad7c4%40mail.gmail.com In-Reply-To: ``` Hi, On Fri, Jan 30, 2009 at 11:06 PM, Yuval Kogman wrote: > Here is an example: > http://github.com/yanick/git-cpan-patch/blob/ed67d3f86f371764935fd0da3e7f08536c95b606/git-cpan-import#L190 > > Since git-commit-tree requires the message to be written before it can > write the sha1 of the new commit object, the handle has to be closed > already, which makes command_close_bidi_pipe die with an error. > The workaround in the linked code reopens a fake handle so that > close() will not error. > For what it's worth, I've been poking at t/t9700/test.pl but since > it's pretty sparse I figured I should contribute this fix > independently. Your usage seems to a very special case. You're closing the handle, and then invoking the method "command_close_bidi_pipe", whose job is to close the pipe handles passed to it (among other things). So, it doesn't seem very good to make "command_close_bidi_pipe" to take into account that you closed your handle independently. Maybe you could write a pair of new functions for the commit-tree command in the Git package, something like the "_open_hash_and_insert_object_if_needed" and "hash_and_insert_object" pair. -- Cheers, Ray Chuan ```