Re: [PATCH] Added get sendmail from .mailrc

From: Jeff King <peff@peff.net>
Date: Jan 28, 2014, 01:15 UTC
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: $!";

Show 7 quoted lines

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