Re: [PATCH 17/18] Place the open() call inside the do{} struct and prevent failing close
From: Célestin Matte <hidden>
Date: 2016-06-15 22:57:34
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:
quoted hunk ↗ jump to hunk
@@ -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.
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