On Tue, May 26, 2015 at 3:10 PM, Eric Sunshine <sunshine@xxxxxxxxxxxxxx> wrote:
> On Saturday, May 23, 2015, Allen Hubbe <allenbh@xxxxxxxxx> 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@xxxxxxxxx>
>> ---
>>
>> 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.
>
> 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.
>
>> + # 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.
>
> [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 ];
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html