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

[Eric Sunshine <sunshine@sunshineco.com>]

On Tue, May 26, 2015 at 3:41 PM, Allen Hubbe <allenbh@gmail.com> wrote:
> On Tue, May 26, 2015 at 3:10 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>> On Saturday, May 23, 2015, Allen Hubbe <allenbh@gmail.com> wrote:
>>> 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
>>> @@ -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.

To be clear, the reason that I brought up the idea of splitting the
regex was that /^\s*$/ and /^\s*#/ are very common idioms which people
can and do recognize and understand at-a-glance without having to
spend time deciphering them. On the other hand, /^\s*(?:#.*)?$/
doesn't lend itself to that sort of instant comprehension; it requires
a certain amount of mental effort to understand.

Anyhow, it's just an idea put forth in case you or someone favors it;
not an outright request for a change.

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

Yes, that would work. Alternately: /^([^\s:]+)\s*:\s*(.+?)$/