Re: [PATCH 6/6] Remove bareword filehandles in git-send-email.perl

From: Jeff King <peff@peff.net>
Date: May 3, 2009, 20:58 UTC
Message-ID: <20090503205826.GF20468@coredump.intra.peff.net>
In-Reply-To: <1241010743-7020-7-git-send-email-wfp5p@virginia.edu>

On Wed, Apr 29, 2009 at 09:12:23AM -0400, Bill Pemberton wrote:

> The script was using bareword filehandles.  This is considered a bad
> practice so they have been changed to indirect filehandles.

I think this is a real improvement; using indirect filehandles mean they get scoped properly, which can avoid errors (especially forgetting to close() them, which happens automagically when they go out of scope). Assuming, of course, that the scoping added by your change is correct, and doesn't close a handle during a loop that we may have wanted to keep open (I didn't check carefully).

But in the patch itself:

> -	open(C,">",$compose_filename)
> +	open my $C,'>',$compose_filename

There are actually two things happening here:

  1. s/C/my $C/, which I think is good

  2. losing the parentheses around open(). This is a style issue, but I
     think we usually prefer the parenthesized form of most perl
     builtins (and certainly in the absence of other information, it
     should be left as-is).

And the style thing that probably _should_ be changed is the spacing: we typically have a space between function arguments. So:

  open(my $C, '>', $compose_file)

> -	print C <<EOT;
> +	print {$C} <<EOT;

Are the braces really necessary here? Is there a version of perl on which

  print $C <<EOT;

will not work?

-Peff