Re: [PATCH 17/18] Place the open() call inside the do{} struct and prevent failing close

From: Célestin Matte <celestin.matte@ensimag.fr>
Date: Jun 6, 2013, 21:30 UTC
Message-ID: <51B0FF75.9070506@ensimag.fr>
In-Reply-To: <7vhahbx7r7.fsf@alter.siamese.dyndns.org>

Le 06/06/2013 23:13, Junio C Hamano a écrit :

> Confused.  Which part of this patch moves open inside a do{} block?
> This was last touched by [9/18] but it doesn't do any such thing,
> either.

I must have failed the rebase, as the first part of the commit moved to [14/18] because it modifies a part of it:

Show 16 quoted lines

>@@ -344,10 +344,10 @@ sub get_mw_pages {
> #        $out = run_git("command args", "raw"); # don't interpret
>output as UTF-8.
> sub run_git {
> 	my $args = shift;
>-	my $encoding = (shift || "encoding(UTF-8)");
>-	open(my $git, "-|:$encoding", "git " . $args)
>-	    or die "Unable to open: $!\n";
>-	my $res = do {
>+	my $encoding = (shift || 'encoding(UTF-8)');
>+	my $res = do {
>+		open(my $git, "-|:$encoding", "git ${args}")
>+		    or die "Unable to fork: $!\n";
> 		local $/ = undef;
> 		<$git>
> 	};

I'm not sure how I should correct this. I'll have a look if this commit actually is useful.

Show 14 quoted lines

> Upon leaving this subroutine, $git filehandle will go out of scope,
> so in that sense, close may not be necessary, but that does not
> match what the proposed log message claims what the patch does.
> 
> Also, this patch does not remove "or die" 9/18 added, even though
> the proposed log message claims that with autodie it is no longer
> necessary.
> 
> I am not convinced that using autodie globally, without vetting the
> calls the original code make, is a good idea in the first place.
> How does this change interact with existing calls to open, close,
> etc. that check the return value from them, now these calls throw
> exception and will not give a chance for the existing error handling
> codepath to intervene?

So using autodie may not be a good idea.
But the problem is that in the current state, open() return values are
checked, but print ones are not, although it should be. So, either:
- we use autodie and remove checking of existing return values, or
- we don't use autodie and add checking of return value of print calls
- or I'm missing some point :)

-- 
Célestin Matte