Re: [PATCH v5 1/1] send-email: Add sendmail email aliases format

From: Eric Sunshine <sunshine@sunshineco.com>
Date: May 26, 2015, 19:10 UTC
Message-ID: <CAPig+cTaiZ_PVaGk6n_bsEqqTJEYEMSCWcnC0=MiN2Bf7L4sWw@mail.gmail.com>
In-Reply-To: <49e9a95b52aa61ed4f37edf1dfa178186acb4a29.1432367540.git.allenbh@gmail.com>

On Saturday, May 23, 2015, Allen Hubbe <allenbh@gmail.com> wrote:

Show 29 quoted lines

> Note that this only adds support for a limited subset of the sendmail
> format.  The format is is as follows.
>
>         <alias>: <address|alias>[, <address|alias>...]
>
> Aliases are specified one per line, and must start on the first column of the
> line.  Blank lines are ignored.  If the first non whitespace character
> on a line is a '#' symbol, then the whole line is considered a comment,
> and is ignored.
> [...]
> Signed-off-by: Allen Hubbe <allenbh@gmail.com>
> ---
>
> Notes:
>     This v5 renames the parser 'sendmail' again, from 'simple'.
>     Therefore, the subject line is changed again, too.
>
>     Previous subject line: send-email: Add simple email aliases format
>
>     The format is restricted to a subset of sendmail.  When the subset
>     diverges from sendmail, the parser warns about the line that diverges,
>     and ignores the line.  The supported format is described in the
>     documentation, as well as the behavior when an unsupported format
>     construct is detected.
>
>     A badly constructed sentence was corrected in the documentation.
>
>     The test case was changed to use a here document, and the unsupported
>     comment after an alias was removed from the test case alias file input.

Thanks. This round looks much nicer. A few minor comments below...

Show 10 quoted lines

> diff --git a/git-send-email.perl b/git-send-email.perl
> index e1e9b1460ced..ffea50094a48 100755
> --- a/git-send-email.perl
> +++ b/git-send-email.perl
> @@ -487,6 +487,8 @@ sub split_addrs {
>  }
>
>  my %aliases;
> +
> +

Unnecessary whitespace change sneaked in.

Show 10 quoted lines

>  my %parse_alias = (
>         # multiline formats can be supported in the future
>         mutt => sub { my $fh = shift; while (<$fh>) {
> @@ -516,6 +518,33 @@ my %parse_alias = (
>                           }
>                       } },
>
> +       sendmail => sub { my $fh = shift; while (<$fh>) {
> +               # ignore comment lines
> +               if (/^\s*(?:#.*)?$/) { }

This confused me at first because the comment talks only about "comment lines", for which a simpler /^\s*#/ would suffice. The regex, however, actually matches blank lines and comment lines (both of which get skipped). Either the comment should be fixed or the regex could be split into two much simpler ones. The splitting into simpler regex's has the benefit of being easier to comprehend at a glance. For instance:

    next if /^\s*$/;
    next if /^\s*#/;

Speaking of 'next', its use here is inconsistent. Due to use of the if/elsif/else chain, 'next' is not needed at all, yet it is used for some cases but not others. To be consistent, either use it everywhere or nowhere.

Show 14 quoted lines

> +               # warn on lines that contain quotes
> +               elsif (/"/) {
> +                       print STDERR "sendmail alias with quotes is not supported: $_\n";
> +                       next;
> +               }
> +
> +               # warn on lines that continue
> +               elsif (/^\s|\\$/) {
> +                       print STDERR "sendmail continuation line is not supported: $_\n";
> +                       next;
> +               }
> +
> +               # recognize lines that look like an alias
> +               elsif (/^(\S+)\s*:\s*(.+?)$/) {

Observation: Given "foo:bar:baz", this regex will take "foo:bar" as
the key, and "baz" as the value, which is probably not what was
intended, however, it likely doesn't matter much in this case since
colon isn't legal in an email address[1].

[1]: However, I could have sworn that colon was legal in some type of email address years ago, but I can no longer remember which type it was. UUCP used '!' in email addresses, so that wasn't it.

Show 12 quoted lines

> +                       my ($alias, $addr) = ($1, $2);
> +                       $aliases{$alias} = [ split_addrs($addr) ];
> +               }
> +
> +               # warn on lines that are not recognized
> +               else {
> +                       print STDERR "sendmail line is not recognized: $_\n";
> +               }}},
> +
>         gnus => sub { my $fh = shift; while (<$fh>) {
>                 if (/\(define-mail-alias\s+"(\S+?)"\s+"(\S+?)"\)/) {
>                         $aliases{$1} = [ $2 ];