patch, 16 partsgit-send-email cleanups

As threatened here's a series that cleans up some of the glaring warts in git-send-email.

Ævar Arnfjörð Bjarmason (16):
  send-email: use lexical filehandle for opendir
  send-email: use lexical filehandles for $compose
  send-email: use lexical filehandles during sending
  send-email: get_patch_subject doesn't need a prototype
  send-email: file_declares_8bit_cte doesn't need a prototype
  send-email: unique_email_list doesn't need a prototype
  send-email: cleanup_compose_files doesn't need a prototype
  send-email: use \E***\Q instead of \*\*\*
  send-email: sanitize_address use $foo, not "$foo"
  send-email: sanitize_address use qq["foo"], not "\"foo\""
  send-email: use (?:) instead of () if no  match variables are needed
  send-email: is_rfc2047_quoted use qr// regexes
  send-email: extract_valid_address use qr// regexes and /o
  send-email: send_message die on $!, not $?
  send-email: make_message_id use "require" instead of "use"
  send-email: use Perl idioms in while loop

 git-send-email.perl   |   80 +++++++++++++++++++++++-------------------------
 t/t9001-send-email.sh |    2 +-
 2 files changed, 39 insertions(+), 43 deletions(-)

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 git-send-email.perl |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 git-send-email.perl |   22 +++++++++++-----------
 1 files changed, 11 insertions(+), 11 deletions(-)

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 git-send-email.perl |   14 +++++++-------
 1 files changed, 7 insertions(+), 7 deletions(-)

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 git-send-email.perl |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 git-send-email.perl |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 git-send-email.perl |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 git-send-email.perl |    4 +---
 1 files changed, 1 insertions(+), 3 deletions(-)

Change the regex introduced in a03bc5b to use the \E...\Q escape syntax instead of using backslashes. It's more readable like this, and easier to grep for.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 git-send-email.perl |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

There's no reason to explicitly stringify a variable in Perl unless it's an overloaded object and you want to call overload::StrVal, otherwise it's just creating a new scalar redundantly.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 git-send-email.perl |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

Perl provides an alternate quote syntax which can make using "" inside interpolated strings easier to read.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 git-send-email.perl |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 git-send-email.perl |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 git-send-email.perl |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

Change the regex fragment in extract_valid_address to use the qr// syntax for compiled regexes, and when they're used add a /o flag so they're only compiled once for the lifetime of the program.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 git-send-email.perl |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

On Thu, Sep 30, 2010 at 01:43:06PM +0000, Ævar Arnfjörð Bjarmason wrote:

Hmm. But these are lexical variables, so won't we recompile them each time we enter the subroutine? I don't think it affects correctness, as this "/o":

means that we will compile and use the value from the first time we run the function.

But we are unnecessarily compiling the sub-regexes each time. Not that this is probably a performance critical piece of code, but your "/o" is doing very little, and this is exactly the sort perl wankery that I find interesting.

Sadly, there is no real perl equivalent of C static local variables, which is what you really want. Usually I would do:

  {
    my $local_part_regexp = qr/.../;
    sub extract_valid_address {
      ...
    }
  }

but beware of the execution order. That works well in a module, where the module code is executed before anybody calls the function. But it breaks in something like this:

  foo();
  {
    my $foo_static_local = 5;
    sub foo {
      print "$foo_static_local\n";
    }
  }

I think you could get by with:

  {
    my $local_part_regexp;
    sub extract_valid_address {
      $local_part_regexp ||= qr/.../;
      ...
    }
  }

-Peff

On Thu, Sep 30, 2010 at 16:19, Jeff King <peff@peff.net> wrote:

IIRC different perl versions treat this differently. In more recent versions doing:

    perl -Mre=debug -E 'sub x { my $x = qr/foo/; my $y = qr/bar/;
/$x$y/ } x($_) for 1..2'

Will only compile all of those regexes once, since perl can see that they're constant. So the /o does nothing.

We might want to keep it for self-documentation purposes to indicate that the $local_part_regexp would never change, but it's probably best to drop that /o altogether.

Perl has static variables like that, just not in the ancient version we're pinned to:

    sub foo {
        state $foo = qr/$_[0]/
    }

On Thu, Sep 30, 2010 at 04:33:59PM +0000, Ævar Arnfjörð Bjarmason wrote:

Ah, right. I am used to using qr// with variable input (since that is the intended use), but of course perl is smart enough to just need to compile qr/constant/ once.

Nice. I didn't think it would be smart enough to handle the layer of indirection, but obviously it does. Go perl.

Yeah, I would just drop the /o. It will in all probability never change, but it it _did_, I think having the /o just makes it buggy.

Nice, I obviously have not kept up on my perl-changelog-reading. ;) But yeah, it's not in 5.8. It's a moot point anyway, since as you demonstrated above, perl is already doing the optimization.

-Peff

Jeff King <peff@peff.net> writes:

Well, isn't the _sole_ point of using qr// to optimize by avoiding recompilation? If this is not a performance critical section of the code, what is the point of this change?

This [PATCH 13/16] and also [PATCH 12/16] rewrite strings using qr// but the patterns thus compiled are used exactly once before the control leaves the scope of the variables, so...

It is a different story if the patch instead introduced module-level global variables to hold a pre-compiled regexp objects, but that is not what we are seeing here.

On Thu, Sep 30, 2010 at 17:25, Junio C Hamano <gitster@pobox.com> wrote:

The point is to use Perl's data-types for what they're supposed to be used for. In perl you *can* write:

    my $two = "1" + "1";

To get 2, but that's silly. You should just use integers instead of string coercion:

    my $two = 1 + 1;

Similarly you *can* put regexes in strings, but perl has a first-class datatype for it, so you should do:

    my $rx = qr/foo/;

Not:

    my $rx = "foo";

So it's just a change to use a better style. There are some tangible advantages to using qr// over qq// (also known as ""), e.g. Emacs and Vim will know to highlight the string as a regexp, and when the $rx is interpolated Perl won't need to recompile it because it can just add a pointer to the existing regex in the new pattern.

But the main reason here is to not write code that looks like it targets Perl 5.5, but 5.8.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---

Here's v2 which omits the confusing /o addition and just uses the more correct qr// syntax over qq//.

 git-send-email.perl |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

If close fails we want to emit errno, not the return code of whatever happened to be the child process run.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 git-send-email.perl |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

Change the use of Sys::Hostname from a "use" to a "require". The former happens in an implicit BEGIN block and is thus immune from the if block it's contained in, so it's always loaded.

This should speed up the invocation of git-send-email by a few milliseconds.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 git-send-email.perl |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

Change `while(<$fh>) { my $c = $_' to `while(my $c = <$fh>) {', and use `chomp $c' instead of `$c =~ s/\n$//g;', the two are equivalent in this case.

I've also changed the --cccmd test so that we test for the stripping of whitespace at the beginning of the lines returned from the --cccmd. I think we probably shouldn't do this, but it was there already so I haven't changed the behavior.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 git-send-email.perl   |    5 ++---
 t/t9001-send-email.sh |    2 +-
 2 files changed, 3 insertions(+), 4 deletions(-)

On Sep 30, 2010, at 9:42 AM, Ævar Arnfjörð Bjarmason wrote:

None of these subroutines strictly need the prototype, but it does allow Perl to warn us if we send incorrect arguments.  Why remove them?  Are they causing problems somewhere?

~~ Brian

On Thu, Sep 30, 2010 at 10:30:31AM -0400, Brian Gernhardt wrote:

They don't necessarily do what you want:

  perl -e 'sub want_scalar($) { print "got $_[0]\n" }
           want_scalar("ok");
           my @a = qw(totally broken);
           want_scalar(@a);
  '

I get:

  got ok
  got 2

And using "sub want_list(@)" basically does nothing at all (you can pass nothing, a scalar, or a list).

For more details, read:

 http://www.perlmonks.org/?node_id=861966

(If you're impatient, skip to the section "Problems with Regular Prototypes").

-Peff

On Sep 30, 2010, at 10:52 AM, Jeff King wrote:

I'm not surprised by that at all, but I've spent too much time in scripting languages recently.

Huh.  Somewhere along the way I had picked up using prototypes as a solution to some problem and got used to them.  Interesting to see the full discussion why you shouldn't.  I guess I'm reading too much Ruby and Perl6 and didn't think about how that would work with Perl5.

~~ Brian

On Thu, Sep 30, 2010 at 14:30, Brian Gernhardt <brian@gernhardtsoftware.com> wrote:

As Jeff pointed out prototypes are troublesome. If you want to be warned about too many arguments a better way is:

    sub foo {
        warn sprintf "You gave me %d arguments", scalar @_ if @_ != 1;

Or something like that, but there's no reason to do that for these subs in particular. There are 32 subroutines in git-send-email.perl, these weren't in any way more special than the rest.

They probably had prototypes in the first place because they were added by someone who was under the mistaken impression that Perl prototypes were remotely similar to C-like prototypes, they're not.

The purpose of Perl prototypes is to rewrite the *caller* code, so that e.g. if you have:

    sub blah ($$) { ... }

Perl will rewrite this call:

    blah @foo, @bar;

As:

    blah(scalar(@foo), scalar(@bar))

While a blah without prototypes would just be:

    blah(@foo, @bar);

Using prototypes superfluously like this makes it harder to read the code, because you end up checking every call site for every subroutine call that uses prototypes to see if rewriting the argument list like this is producing some unexpected logic error.

On Thu, Sep 30, 2010 at 9:42 AM, Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:

These all look pretty uncontroversial and sane by me, though I don't think you touched any of the lines I contributed to git-send-email. If anything, perhaps this series is a bit too granular -- I might combine the commits that get rid of the function prototypes into one for example. Otherwise, thanks for the cleanup.

Acked-by: Jay Soffian <jaysoffian@gmail.com>

j.

On Thu, Sep 30, 2010 at 01:42:53PM +0000, Ævar Arnfjörð Bjarmason wrote:

With the exception of my comments on the "/o" regexes, these all look sane to me.

-Peff