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

From: Allen Hubbe <allenbh@gmail.com>
Date: May 26, 2015, 19:41 UTC
Message-ID: <CAJ80sasp6kNgbJJw-2TzZnPPDVgYdAwwsdh=hNH4xxu1TBtiyA@mail.gmail.com>
In-Reply-To: <CAPig+cTaiZ_PVaGk6n_bsEqqTJEYEMSCWcnC0=MiN2Bf7L4sWw@mail.gmail.com>

On Tue, May 26, 2015 at 3:10 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:

Show 67 quoted lines

> On Saturday, May 23, 2015, Allen Hubbe <allenbh@gmail.com> wrote:
>> 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...
>
>> 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.
>
>>  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*#/;

I noticed this too after sending the patch, and I have already changed the comment to mention blank lines or comment lines.

Splitting the regex would be more simple, but the regex is already quite simple as it is.

Show 5 quoted lines

>
> 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.

These used to be `if (foo) { somthing; next; }` while this version was work in progress, which I changed to elsif with the intention of removing the next. Thanks for catching the inconsistency. I will remove the next.

Show 20 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].

That's a keen observation. I think it would work simply to use a non-greedy +? in the first capture group.

Show 17 quoted lines

>
> [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.
>
>> +                       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 ];