git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Petr Baudis <pasky@ucw.cz>
To: Junio C Hamano <gitster@pobox.com>
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: Sat, 6 Apr 2013 01:34:51 +0200	[thread overview]
Message-ID: <20130405233450.GA6137@machine.or.cz> (raw)
In-Reply-To: <7vsj34byb4.fsf@alter.siamese.dyndns.org>

On Fri, Apr 05, 2013 at 11:57:19AM -0700, Junio C Hamano wrote:
> Petr Baudis <pasky@ucw.cz> writes:
> >> > -			if (defined $opts{STDERR}) {
> >> > -				close STDERR;
> >> > -			}
> >> >  			if ($opts{STDERR}) {
> >> >  				open (STDERR, '>&', $opts{STDERR})
> >
> >   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.

Indeed, though $opts{STDERR} == 2 is something weird to do, it is a case
to consider.

> 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?

I never actually tried that and was always happy to go with perldoc
maxim

	To (re)open "STDOUT" or "STDERR" as an in-memory file, close it first:
	           close STDOUT;
	           open(STDOUT, ">", \$variable)
	               or die "Can't open STDOUT: $!";

but my assumption that this generalizes to other kinds of open was
apparently invalid; an example further down the page proves me wrong
completely, moreover.

  The thing is, I was confused about dup2() all along as my old UNIX
masters taught me that I must close() the original descriptor first
and since that's what's commonly done anyway, I never thought to
double-check. Now I did and I learned something new, thanks!

I guess Acked-by: Petr Baudis <pasky@ucw.cz> then. :-)

-- 
				Petr "Pasky" Baudis
	For every complex problem there is an answer that is clear,
	simple, and wrong.  -- H. L. Mencken

  reply	other threads:[~2013-04-06 16:57 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
2013-04-05 23:34         ` Petr Baudis [this message]
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=20130405233450.GA6137@machine.or.cz \
    --to=pasky@ucw.cz \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=marcin@owsiany.pl \
    --cc=normalperson@yhbt.net \
    --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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).