patch, 6 partscleanups for git-send-email

The following are some code cleanups for git-send-email.perl. They're based on suggestions by perlcritc.

Bill Pemberton (6):
  Remove return undef from validate_patch
  Remove function prototypes from git-send-email.perl
  Remove return undef from ask()
  Add explict return to end of subroutines
  Remove mix of high and low-precedence booleans
  Remove bareword filehandles in git-send-email.perl

 git-send-email.perl |   63 ++++++++++++++++++++++++++------------------------
 1 files changed, 33 insertions(+), 30 deletions(-)

Returning undef is rarely the correct way to return a failure. Replace it with return 0

Signed-off-by: Bill Pemberton <wfp5p@virginia.edu>
---
 git-send-email.perl |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

Use of function prototypes is considered bad practice in perl. The ones used here didn't accomplish anything anyhow, so they've been removed.

Signed-off-by: Bill Pemberton <wfp5p@virginia.edu>
---
 git-send-email.perl |   11 ++++-------
 1 files changed, 4 insertions(+), 7 deletions(-)

Returning undef is rarely the correct way to return a failure. Replace it with bare return

Signed-off-by: Bill Pemberton <wfp5p@virginia.edu>
---
 git-send-email.perl |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

In perl a subroutine that ends without an explicit return will return the value of the last expression evalutated. This can lead to unexpected return values.

Signed-off-by: Bill Pemberton <wfp5p@virginia.edu>
---
 git-send-email.perl |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

Booleans such as &&, ||, ! have higher precedence than and, or, not. They should not be mixed.

Signed-off-by: Bill Pemberton <wfp5p@virginia.edu>
---
 git-send-email.perl |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

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

Signed-off-by: Bill Pemberton <wfp5p@virginia.edu>
---
 git-send-email.perl |   36 ++++++++++++++++++------------------
 1 files changed, 18 insertions(+), 18 deletions(-)

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

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:

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)

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

  print $C <<EOT;

will not work?

-Peff

Le Sunday 03 May 2009 22:58:26 Jeff King, vous avez écrit :

And why not go the full way and using IO::File?

my $fh = new IO::File;

$fh->open("/the/file", O_RDONLY|...)

On Sun, 3 May 2009 23:34:03 +0200, Francis Galiegue <fge@one2team.com> wrote:

Because that would be travelling back in time. The most efficient and preferred way is three-arg lexical:

    open my $fh, "<", $filename or die "$filename: $!";
    while (<$fh>) {
        # ...
        }
    close $fh or die "$filename: $!";

give or take quoting style, some spaces and/or indents

Why use a module for something that is neatly buit in?

Le lundi 04 mai 2009, vous avez écrit : [...]

I don't see how using IO::File is going back in time at all. It's a standard perl module, even in 5.10.

Because it reads better? YMMV, of course. I prefer using IO::File because perl has too many keywords for its own good :p

On Mon, 4 May 2009 08:53:44 +0200, Francis Galiegue <fge@one2team.com> wrote:

It was written to be able to have lexical handles and pass them around. With the above implementation you don't need to read a 600+ line module to have every file action go through methods that are not needed at all, and you also do not have to read the documentation for the deviating syntax. I'd rather stick to simple, easy and standard.

Because it is more efficient? And yes, it reads better, because it is used in a zillion places already.

In the syntax I wrote down are now new keywords at all.

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

But round brackets have higher precedence than both '&&' and 'and', right? If so, why thoses changes?

While those particular statements may be correct, mixing the low precedence logical operators with the high precedence ones is likely to introduce bugs down the road. See the book "Perl Best Practices" for more information on this.

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

I am not opposed to this, but it is really not helping anything. These functions aren't meant to return any value, and their return values are not used. In C, they would simply be declared "void", but there is no way (AFAIK) to do that in perl.

But for some of them, I wonder if you would do better instead of returning _nothing_ to return something that might make a little bit of sense. For example, make_message_id munges a global variable. A useful refactoring might be to have it return the value of the global variable, and then perhaps even the global could go away, which _would_ be a real improvement.

-Peff

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

I'm not sure this makes sense. The function is meant to be (and is always) called in a scalar context, so "return" and "return undef" produce exactly the same results. I find the latter much more clear, because it is explicit about what the function is doing: return the user's desire; if that fails, return the default; if no default, return undef.

Returning undef can be a problem for programs which are meant to be used in list context; you generally want to return the empty list in that case (not a list with a single undef in it). So that is the reason for the advice you are quoting.

I am not sure it is worth making this function behave properly in a list context; it is not a library function, so we can see that all of the existing callers use it in a scalar context (and we would have to define sensible semantics for a list context). But even if we _did_ want to do that, your patch is only the first step. You would have to also fix the other returns which can return undef instead of an empty list. So there is not much point to your patch as it stands.

On a side note, while looking at this function, I wonder if that "return undef" is correct after all. We get there only if the user has failed to give valid input 10 times, so presumably it is a sanity check to prevent runaway input errors (and I am cc'ing Jay, who added the function not too long ago). Should we be respecting the default here, as we do when we get EOF? Although I tend to think if the user is repeatedly giving us bogus input that we should not just proceed, but should probably die. Because otherwise we are guessing at what they might have wanted.

-Peff

On Sun, May 3, 2009 at 4:26 PM, Jeff King <peff@peff.net> wrote:

Correct, that is why it is there.

The original motivation was a user who was running send-email from cron and it was looping forever. That case is now actually handled before the loop, and all other normal cases are handled inside the loop.

So the only thing that can cause the loop to exit (AFAIK) is when $valid_re is passed in and the user provides invalid input 10x.

Well, it returns undef, at which point it's up to the caller to figure out what to do. You'll notice the one caller which passes in $valid_re dies:

die "Send this email reply required" unless defined $_;

Letting the caller decide what to do provides more flexibility.

j.

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

I think this one is sensible.

-Peff

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

No, it's the right way to return failure here. The function returns either an error string, prefixed with the problematic line number, or undef. So 'undef' is working as a sentinel value here, not as part of a boolean.

That being said, the _calling_ code is a bit sloppy in checking "$error" instead of "defined($error)". It is not an actual bug because the beginning of the string is always a line number >= 1, so it always triggers as desired. However, it would probably be more clear to write it like this:

---

Bill Pemberton <wfp5p@virginia.edu> writes:

Perl styles are highly personal.

While I admit that the changes these patches bring in match my personal taste more or less exactly [*1*], there was somebody (sorry I lost track) who volunteered to take the maintenance responsibility of that program over to clean up or even rewrite it, so I would rather have that person have a say in the overall styles of the (rewritten) program.

[Footnote]

*1* ...except for the "and/or vs &&/||" bits, even though I prefer the latter myself solely because I am old fashioned.

I think it is simply silly to say "precedence of ! and and/or does not mix". "!" and "&&" have different precedence and rewriting (A and !B) into (A && !B) would not make things any better nor worse. After all, nobody would have problems with "$a + $b * $c" even though + and * have different precedence.

Oh, I also do not agree with "always explicitly return". If the change and explanation were limited to the subs whose return values are _used_, I would agree with the change, though.

So are C styles, but the kernel and git doesn't allow all sorts of mixed styles. My changes are also not just coding style, they have actual meaning in perl.

My changes come directly from the book "Perl Best Practices". Just as you do things like "don't allow assignment in conditionals" in C, even though it's legal. There are good reasons to do these things in perl to prevent bugs down the road.

Again, it prevents bugs. People use "and" vs "&&" as the same thing, when they are not. The have different precedence in perl.

For example, 

next if not $finished || $x < 5; next if !$finished || $x < 5;

do not mean the same thing.

It's not that ! and && have different precedence. It's that "not" and ! have different precedence. Using your math example, it would be like having an operator named plus that had a higher precedence than "*". Now if you wrote "$a plus $b * $c" it would have different result than "$a + $b * $c".

Again, it prevents potential bugs down the road. Currently those functions return something. While they are not used, the something they return can be interpreted by developers as an intentional return value and that property may get used. If some other developer changes the original function in some way that the implicit return becomes something else, it'll create a bug. If a subroutine isn't supposed to return a meaningful value, it should do it explicitly.

wfp5p@viridian.itc.Virginia.EDU (Bill Pemberton) writes:

Earlier I did guide the community not to use "more advanced" (aka "obscure") Perl features so that people not so familiar with Perl can still tweak scripts without breaking them; the tricks in your patches that "prevent potential bugs" are in line with that, and that is why I said my personal taste more or less agrees with your patch already.

But the line between "more advanced and tricky" and "if you are coding in Perl you should know your language" is not so black and white as you seem to think. I'd rather defer that decision to whoever is taking send-email over.

On Wed, Apr 29, 2009 at 03:48:51PM -0400, Bill Pemberton wrote:

I agree with you except that the chapter 4.16 from the Perl Best Practices book does not apply here. FMPOV, we don't really mix booleans because the precedence is explicitly given by the parentheses.

[ Notice _how_ the author raises the ambiguity to explain his point in
  the book: he uses parentheses. ]

True. But the lines we are talking about are different. We have:

	next if ($finished or $x < 5);

If we add a "not"/"!" or append a "&&"/"and" - or whatever -, we do know what will be evaluated easily:

	next if !($finished or $x < 5);

looks rather different from

	next if (!$finished or $x < 5);

Nicolas Sebrecht wrote:

I'm rather clueless when it comes to perl coding, but I know what I don't know, so I've got enough sense to look these things up whenever I have to hack some perl. I hope others do the same.

Personally, I've found that never using 'or', 'not' or 'and', and overparenthesize when I'm uncertain seems to work rather nicely, even though perl gurus would probably shunt my code as overly explicit.