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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reply via email to