From: Jeff King <peff@peff.net>
Date: Tue, 28 Jan 2014 01:15:12 GMT
Subject: Re: [PATCH] Added get sendmail from .mailrc
Message-ID: <20140128011512.GB17945@sigill.intra.peff.net>
In-Reply-To: <1390643210-5748-1-git-send-email-brilliantov@inbox.ru>

On Sat, Jan 25, 2014 at 01:46:50PM +0400, Brilliantov Kirill Vladimirovich wrote:

> +	if (!defined $smtp_server) {
> +		my $mailrc = File::HomeDir->my_home . "/.mailrc";

The new module dependency has been discussed elsewhere in the thread.

> +		if (-e $mailrc) {
> +			open FILE, $mailrc or die "Failed open $mailrc: $!";

Please use the three-argument of 'open', and use a regular scalar
instead of a glob. Both are safer, and we assume a modern enough perl to
support both. I.e.:

  open(my $file, '<', $mailrc)
          or die "failed to open $mailrc: $!";

> +			while (<FILE>) {
> +			    chomp;
> +			    if (/set sendmail=.*/) {
> +				my @data = split '"';
> +				$smtp_server = $data[1];
> +				last;
> +			    }

Your split is a rather unusual way to do the parsing, and it took me a
minute to figure it out. It might be more obvious as:

  if (/set sendmail="(.*)"/) {
          $smtp_server = $1;
          last;
  }

I do not know anything about the mailrc format, nor does it seem to be
well documented. Are the double-quotes required? If not, then the above
regex can easily make them optional. I also wonder if any whitespace is
allowed. E.g., this might be more forgiving:

  /set sendmail\s*=\s*"?(.*?)"?/

but I am just guessing at what the format allows.

-Peff