threads / patch / 17451

patchDon't try to reclose in command_close_bidi_pipe

Subject: [PATCH] Don't try to reclose in command_close_bidi_pipe

## tl;dr

4 messages between Jan 30, 2009 and Jan 31, 2009. Diffs are folded; open one to read it.

replies: 3people: 2as markdown or json

Yuval Kogman· Jan 30, 2009, 04:59 UTC · lore

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(-)

Show changes to perl/Git.pm +1 −0

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· Jan 30, 2009, 14:35 UTC · re: Yuval Kogman · lore

Re: [PATCH] Don't try to reclose in command_close_bidi_pipe

Hi,

On Fri, Jan 30, 2009 at 12:59 PM, Yuval Kogman <nothingmuch@woobling.org> 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?

Show 13 quoted lines

> ---
>  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· Jan 30, 2009, 15:06 UTC · re: Tay Ray Chuan · lore

Re: [PATCH] Don't try to reclose in command_close_bidi_pipe

Resending, forgot to CC

2009/1/30 Tay Ray Chuan <rctay89@gmail.com>

> 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· Jan 31, 2009, 00:03 UTC · re: Yuval Kogman · lore

Re: [PATCH] Don't try to reclose in command_close_bidi_pipe

Hi,

On Fri, Jan 30, 2009 at 11:06 PM, Yuval Kogman <nothingmuch@woobling.org> wrote:

Show 11 quoted lines

> 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