On Tue, May 26, 2015 at 3:41 PM, Allen Hubbe <allenbh@xxxxxxxxx> wrote:
> On Tue, May 26, 2015 at 3:10 PM, Eric Sunshine <sunshine@xxxxxxxxxxxxxx> wrote:
>> On Saturday, May 23, 2015, Allen Hubbe <allenbh@xxxxxxxxx> 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*(.+?)$/
--
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