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:
>@@ -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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html