All of lore.kernel.org
 help / color / mirror / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: Petr Baudis <pasky@ucw.cz>
Cc: Thomas Rast <trast@inf.ethz.ch>,
	git@vger.kernel.org, Eric Wong <normalperson@yhbt.net>,
	Marcin Owsiany <marcin@owsiany.pl>
Subject: Re: [PATCH v2 1/2] perl: redirect stderr to /dev/null instead of closing
Date: Fri, 05 Apr 2013 11:57:19 -0700	[thread overview]
Message-ID: <7vsj34byb4.fsf@alter.siamese.dyndns.org> (raw)
In-Reply-To: <20130405144828.GX6137@machine.or.cz> (Petr Baudis's message of "Fri, 5 Apr 2013 16:48:28 +0200")

Petr Baudis <pasky@ucw.cz> writes:

>> >  		} elsif ($pid == 0) {
>> > -			if (defined $opts{STDERR}) {
>> > -				close STDERR;
>> > -			}
>> >  			if ($opts{STDERR}) {
>> >  				open (STDERR, '>&', $opts{STDERR})
>> >					or die "dup failed: $!";
>> 
>> Indeed.  Thanks for pointing that out.
>
>   I'm sorry, I don't follow. Doesn't this just break the STDERR option
> altogether as we will try to dup2() over an already open file
> descriptor? We do need to close STDERR if we are going to reopen it,
> I think.

When $opts{STDERR} is 2, what the three lines the proposed patch
removes did is actively wrong, because you dup2 the fd you just
closed.

When $opts{STDERR} is 1, it seems to do the right thing with or
without the "close STDERR" in front.  Isn't this because the usual
"open($fd, <<<anything>>>) closes $fd as necessary" applies to this
case as well?

So, I am not sure what you are viewing as a problem.  Puzzled...

  reply	other threads:[~2013-04-06 16:53 UTC|newest]

Thread overview: 11+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2013-04-03 22:26 [PATCH] perl: redirect stderr to /dev/null instead of closing Thomas Rast
2013-04-04  1:16 ` Eric Wong
2013-04-04 20:41   ` [PATCH v2 1/2] " Thomas Rast
2013-04-04 21:14     ` Eric Wong
2013-04-05 14:48     ` Petr Baudis
2013-04-05 18:57       ` Junio C Hamano [this message]
2013-04-05 23:34         ` Petr Baudis
2013-04-06  8:07           ` Thomas Rast
2013-04-06 10:34             ` Petr Baudis
2013-04-04 20:41   ` [PATCH v2 2/2] t9700: do not close STDERR Thomas Rast
2013-04-04 21:11     ` Jonathan Nieder

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=7vsj34byb4.fsf@alter.siamese.dyndns.org \
    --to=gitster@pobox.com \
    --cc=git@vger.kernel.org \
    --cc=marcin@owsiany.pl \
    --cc=normalperson@yhbt.net \
    --cc=pasky@ucw.cz \
    --cc=trast@inf.ethz.ch \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.