Merge branch 'use-arbitrary-arrays-in-getopt-parsing' into develop

* use-arbitrary-arrays-in-getopt-parsing:
  Error if argument to --timeout is in unexpected format
  Add script to find orphaned test reference files
  Fix command line reconstruction, no-arg options should not include an arg in reconstruction
  error if unidentified information found in option list
  Change Getopt option processing to use a custom list

Changes

@@ -884,3 +884,12 @@
 * 20191028 Fix bug setting filename in attachments when '@' prefix used
 * 20191028 Add deprecation warning system.  Warnings in code and a table
            of deprecated functionality and removal dates in documentation.
+* 20191111 Switch Getopt processing to an arbitrary list instead of  changing
+           ARGV for each call.
+* 20191111 Check to see if there are any entries left in the option list
+           after each stage of option processing and error if so.
+* 20191111 Rework the command line reconstruction so that options that
+           don't take an argument do not include one in the reconstuction.
+* 20191111 Unexpected argument format for --timeout now errors and exits
+           instead of silently using default timeout.
+* 20191111 Document that --timeout argument can use 'h' as a modifier.

doc/base.pod

@@ -111,6 +111,8 @@ A message's body is the portion of its DATA section following the first blank li
 
 To prevent potential confusion in this document a flag to Swaks is always referred to as an "option".  If the option takes additional data, that additional data is referred to as an argument to the option.  For example, "--from [email protected]" might be provided to Swaks on the command line, with "--from" being the option and "[email protected]" being --from's argument.
 
+Options arguments are the only way to provide information to Swaks.  If Swaks finds data during option processing that is neither an option nor an option's argument, it will error and exit.  For instance, if "--no-data-fixup 1" were found on the command line, this would result in an error because --no-data-fixup does not take an argument and therefore Swaks would no know what to do with '1'.
+
 Options can be given to Swaks in three ways.  They can be specified in a configuration file, in environment variables, and on the command line.  Depending on the specific option and whether an argument is given to it, Swaks may prompt the user for the argument.
 
 When Swaks evaluates its options, it first looks for a configuration file (either in a default location or specified with --config).  Then it evaluates any options in environment variables.  Finally, it evaluates command line options.  At each round of processing, any options set earlier will be overridden.  Additionally, any option can be prefixed with "no-" to cause Swaks to forget that the variable had previously been set (either in an earlier round, or earlier in the same round).  This capability is necessary because many options treat defined-but-no-argument differently than not-defined.
@@ -131,8 +133,11 @@ A set of "portable" defaults can also be created by adding options to the end of
 
 If configuration files have not been explicitly turned off, the __END__ config is always read.  Only one other configuration file will ever be used per single invocation of Swaks, even if multiple configuration files are specified.  If the __END__ config and another config are to be read, the __END__ config will be processed first.  Specifying the --config option with no argument turns off the processing of both the __END__ config and any actual config files.
 
-In a configuration file lines beginning with a hash (#) are ignored.  All other lines are assumed to be an option to Swaks, with the leading dash or dashes optional.  Everything after a option line's first space is assumed to be the option's argument and is not shell processed.  Therefore, quoting is usually unneeded and will be included literally in the argument.  Here is an example of the contents of a configuration file:
+In a configuration file lines beginning with a hash (#) are ignored.  All other lines are assumed to be an option to Swaks, with the leading dash or dashes optional.  Everything after a option line's first space is assumed to be the option's argument and is not shell processed.  Therefore, quoting is usually unneeded and will be included literally in the argument.
+
+There is a subtle difference between providing an option with no argument and providing an option with an empty argument.  If an option line does not have a space, the entire line is treated as an option and there is no argument.  If the line ends in a single space, it will be processed as an option with an empty argument.  So, "apt" will be treated as "--apt", but "apt " will be treated as "--apt ''".
 
+Here is an example of the contents of a configuration file:
 
     # always use this sender, no matter server or logged in user
     --from [email protected]
@@ -326,7 +331,7 @@ This option is similar to --drop-after, but instead of dropping the connection a
 
 =item --timeout [time]
 
-Use argument as the SMTP transaction timeout, or prompt user if no argument given.  Argument can either be a pure digit, which will be interpreted as seconds, or can have a specifier s or m (5s = 5 seconds, 3m = 180 seconds).  As a special case, 0 means don't timeout the transactions.  Default value is 30s.
+Use argument as the SMTP transaction timeout, or prompt user if no argument given.  Argument can either be a pure digit, which will be interpreted as seconds, or can have a specifier s, m, or h (5s = 5 seconds, 3m = 180 seconds, 1h = 3600 seconds).  As a special case, 0 means don't timeout the transactions.  Default value is 30s.
 
 =item --protocol [protocol]
 

swaks

@@ -1611,8 +1611,9 @@ sub test_support {
 sub time_to_seconds {
 	my $t = shift;
 
-	if ($t !~ /^(\d+)([hms])?/i) {
-		return(30); # error condition - just use default value
+	if ($t !~ /^(\d+)([hms])?$/i) {
+		ptrans(12, 'Unknown timeout format \'' . $t . '\'');
+		exit(1);
 	} else {
 		my $r = $1;
 		my $u = lc($2);
@@ -2118,17 +2119,12 @@ sub get_option_struct {
 }
 
 # returns %O, the large raw option hash
+# This sub is a jumping point.  We will construct an argv based on the different ways that options can be specified
+# and call GetOptions multiple times.  We are essentially "layering" options.  First we load from a config file (if
+# exists/specified), then from any environment variables, then the actual command line.
 sub load_args {
-	# this sub is a jumping point.  What we will do is repeatedly massage
-	# @ARGV and call GetOptions multiple times.  We are essentially "layering"
-	# options.  First we load from a config file (if exists/specified), then
-	# from any environment variables, then the actual command line.
-
-	# First, save a copy of the real @ARGV, because that's actually what
-	# gets processed last
-	my @real_ARGV = @ARGV;
-	my %ARGS      = ();
-	@ARGV         = ();
+	my %ARGS      = (); # this is the structure that gets returned
+	my @fakeARGV  = ();
 
 	# we load our options processing hash here.  We abstract it back from the
 	# native getopt-format because we need to be able to intercept "no-" options
@@ -2163,8 +2159,6 @@ sub load_args {
 		$ARGS{cfgs}{$e->{okey}} = $e;
 	}
 
-
-
 	# we want to process config files first.  There's a default config file in
 	# ~/.swaksrc, but it is possible for the user to override this with the
 	# --config options.  So, find the one and only file we will use here.
@@ -2191,12 +2185,12 @@ sub load_args {
 	}
 	# lastly go (backwards) through original command line looking for config file,
 	# choosing the first one found (meaning last one specified)
-	for (my $i = scalar(@real_ARGV) - 1; $i >= 0; $i--) {
-		if ($real_ARGV[$i] =~ /^-?-config$/) {
-			if ($i == scalar(@real_ARGV) - 1 || $real_ARGV[$i+1] =~ /^-/) {
+	for (my $i = scalar(@ARGV) - 1; $i >= 0; $i--) {
+		if ($ARGV[$i] =~ /^-?-config$/) {
+			if ($i == scalar(@ARGV) - 1 || $ARGV[$i+1] =~ /^-/) {
 				$skip_config = 1;
 			} else {
-				$config_file       = $real_ARGV[$i+1];
+				$config_file       = $ARGV[$i+1];
 				$config_is_default = 0;
 				$skip_config       = 0;
 			}
@@ -2210,13 +2204,14 @@ sub load_args {
 		my @configs = ('&DATA');
 		push(@configs, $config_file) if ($config_file);
 		foreach my $configf (@configs) {
+			my @fakeARGV = ();
 			if (open(C, '<' . $configf)) {
 				# "#" in col 0 is a comment
 				while (defined(my $m = <C>)) {
 					next if ($m =~ m|^#|);
 					chomp($m);
 					$m = '--' . $m if ($m !~ /^-/);
-					push(@ARGV, split(/\s/, $m, 2));
+					push(@fakeARGV, split(/\s/, $m, 2));
 				}
 				close(C);
 			} elsif (!$config_is_default && $configf eq $config_file) {
@@ -2225,71 +2220,90 @@ sub load_args {
 				exit(1);
 			}
 
-			# OK, all that work to load @ARGV with values from the config file.  Now
-			# we just need to process it.  (don't call if nothing set in @ARGV)
-			fetch_args(\%ARGS, $option_list) if (scalar(@ARGV));
+			# OK, all that work to load @fakeARGV with values from the config file.  Now
+			# we just need to process it.  (don't call if nothing set in @fakeARGV)
+			fetch_args(\%ARGS, $option_list, \@fakeARGV) if (scalar(@fakeARGV));
+			check_opt_processing(\@fakeARGV, 'Config file ' . $configf);
 		}
 	}
 
 	# OK, %ARGS contains all the settings from the config file.  Now do it again
 	# with SWAKS_OPT_* environment variables
-	@ARGV = ();
+	@fakeARGV = ();
 	foreach my $v (sort keys %ENV) {
 		if ($v =~ m|^SWAKS_OPT_(.*)$|) {
 			my $tv = $1; $tv =~ s|_|-|g;
-			push(@ARGV, '--' . $tv);
-			push(@ARGV, $ENV{$v}) if (length($ENV{$v}));
+			push(@fakeARGV, '--' . $tv);
+			push(@fakeARGV, $ENV{$v}) if (length($ENV{$v}));
 		}
 	}
-	fetch_args(\%ARGS, $option_list) if (scalar(@ARGV));
+	fetch_args(\%ARGS, $option_list, \@fakeARGV) if (scalar(@fakeARGV));
+	check_opt_processing(\@fakeARGV, 'environment');
 
 	# and now, after all of that, process the actual cmdline args
-	@ARGV = @real_ARGV;
-	fetch_args(\%ARGS, $option_list) if (scalar(@ARGV));
+	fetch_args(\%ARGS, $option_list, \@ARGV) if (scalar(@ARGV));
+	check_opt_processing(\@ARGV, 'command line');
 
 	return(\%ARGS);
 }
 
+# if there's anything left in the fake argv after Getopts processed it, it's an error.  There's nothing
+# that can be passed in to swaks that isn't an option or an argument to an option, all of which Getopt
+# should consume.  So if there's anything left, the user did something weird.  Just let them know and
+# error instead of letting them think their ignored stuff is working.
+sub check_opt_processing {
+	my $argv_local  = shift;
+	my $option_type = shift;
+
+	if (scalar(@$argv_local)) {
+		ptrans(12, 'Data left in option list when processing  ' . $option_type . ' (' .
+		           join(', ', map { "'$_'" } (@$argv_local)) .
+		           ').  Exiting');
+		exit(1);
+	}
+}
+
 sub fetch_args {
-	my $r = shift;
-	my $l = shift;
+	my $r          = shift;
+	my $l          = shift;
+	my $argv_local = shift;
 
 	my %to_delete = ();
 
 	# need to rewrite header-HEADER opts before std option parsing
 	# also see if there are any --no- options that need to be processed
 	RUNOPTS:
-	for (my $i = 0; $i < scalar(@ARGV); $i++) {
+	for (my $i = 0; $i < scalar(@$argv_local); $i++) {
 		# before doing any option processing, massage from the optional '--option=arg' format into '--option arg' format.
-		if ($ARGV[$i] =~ /^(-[^=]+)=(.*)$/) {
-			$ARGV[$i] = $1;
-			splice(@ARGV, $i+1, 0, $2);
+		if ($argv_local->[$i] =~ /^(-[^=]+)=(.*)$/) {
+			$argv_local->[$i] = $1;
+			splice(@$argv_local, $i+1, 0, $2);
 		}
 
 		# -g is not really necessary.  It is now deprecated. During the deprecation window, make
 		# it a straight-up alias to `--data -`.  If has already appeared, just ignore -g.  If
 		# --data has not appeared, change -g into `--data -`.
-		if ($ARGV[$i] =~ /^-?-g$/) {
+		if ($argv_local->[$i] =~ /^-?-g$/) {
 			deprecate('The -g option is deprecated and will be removed. Please use \'--data -\' instead.');
-			if (scalar(grep(/^-?-data$/, @ARGV)) || check_arg('mail_data', $r)) {
+			if (scalar(grep(/^-?-data$/, @$argv_local)) || check_arg('mail_data', $r)) {
 				# if --data appears in the current stream or has already appeared in a previous stream, ignore -g
-				splice(@ARGV, $i, 1); # remove the current index from @ARGV
+				splice(@$argv_local, $i, 1); # remove the current index from @$argv_local
 				redo(RUNOPTS);        # since there's now a new value at $i, redo this iteration of the loop
 			}
 			else {
 				# if we haven't seen --data yet, change -g into `--data -`
-				splice(@ARGV, $i, 1, '--data', '-');
+				splice(@$argv_local, $i, 1, '--data', '-');
 			}
 		}
 
-		if ($ARGV[$i] =~ /^-?-h(?:eader)?-([^:]+):?$/) {
+		if ($argv_local->[$i] =~ /^-?-h(?:eader)?-([^:]+):?$/) {
 			# rewrite '--header-Foo bar' into '--header "Foo: bar"'
-			$ARGV[$i]   = "--header";
-			$ARGV[$i+1] = $1 . ': ' . $ARGV[$i+1];
+			$argv_local->[$i]   = "--header";
+			$argv_local->[$i+1] = $1 . ': ' . $argv_local->[$i+1];
 		}
-		elsif ($ARGV[$i] =~ /^-?-no-h(?:eader)?-/) {
+		elsif ($argv_local->[$i] =~ /^-?-no-h(?:eader)?-/) {
 			# rewrite '--no-header-Foo' into '--no-header'
-			$ARGV[$i]   = "--no-header";
+			$argv_local->[$i]   = "--no-header";
 		}
 	}
 
@@ -2323,7 +2337,7 @@ sub fetch_args {
 	}
 
 	Getopt::Long::Configure("no_ignore_case");
-	GetOptions(%options) || exit(1);
+	Getopt::Long::GetOptionsFromArray($argv_local, %options) || exit(1);
 }
 
 sub store_option {
@@ -2392,7 +2406,7 @@ sub reconstruct_options {
 			my $lopt          = $o->{cfgs}{$okey}{opts}[0];
 
 			push(@c, '--'.$lopt);
-			if (length($optStruct->{arg})) {
+			if (length($optStruct->{arg}) && !($o->{cfgs}{$okey}{cfgs} & OP_ARG_NONE)) {
 				if ($okey eq 'auth_pass') {
 					push(@c, shquote('%RAW_PASSWORD_STRING%'));
 				}

testing/regressions/_exec-output-dump/0004.test

@@ -1,3 +1,3 @@
 auto: REMOVE_FILE,CREATE_FILE,MUNGE,COMPARE_FILE %TESTID%.stdout %TESTID%.stderr
 
-test action: CMD_CAPTURE %SWAKS% --dump --to [email protected] --server ser.ver --helo host1.nodns.test.swaks.net --from [email protected] --quit-after 'mail' --auth-user 'user' --auth-password 'pass' --tls '1' --hide-send '1' --dump --xclient-addr '192.168.0.4' --xclient-optional '1' --proxy-version '1' --proxy-family 'TCP4' --proxy-source '192.168.0.5' --proxy-source-port '111' --proxy-dest '1.1.1.1' --proxy-dest-port '26'
+test action: CMD_CAPTURE %SWAKS% --dump --to [email protected] --server ser.ver --helo host1.nodns.test.swaks.net --from [email protected] --quit-after 'mail' --auth-user 'user' --auth-password 'pass' --tls --hide-send --dump --xclient-addr '192.168.0.4' --xclient-optional --proxy-version '1' --proxy-family 'TCP4' --proxy-source '192.168.0.5' --proxy-source-port '111' --proxy-dest '1.1.1.1' --proxy-dest-port '26'

testing/regressions/_exec-output-dump/out-ref/0004.stdout

@@ -15,7 +15,7 @@
 
 App Info:
   X-Mailer = swaks v99999999.9 jetmore.org/john/code/swaks/
-  Cmd Line = %SWAKS_COMMAND% --from '[email protected]' --to '[email protected]' --helo 'host1.nodns.test.swaks.net' --server 'ser.ver' --quit-after 'mail' --auth-user 'user' --auth-password 'pass' --tls '1' --hide-send '1' --dump --xclient-addr '192.168.0.4' --xclient-optional '1' --proxy-version '1' --proxy-family 'TCP4' --proxy-source '192.168.0.5' --proxy-source-port '111' --proxy-dest '1.1.1.1' --proxy-dest-port '26'
+  Cmd Line = %SWAKS_COMMAND% --from '[email protected]' --to '[email protected]' --helo 'host1.nodns.test.swaks.net' --server 'ser.ver' --quit-after 'mail' --auth-user 'user' --auth-password 'pass' --tls --hide-send --dump --xclient-addr '192.168.0.4' --xclient-optional --proxy-version '1' --proxy-family 'TCP4' --proxy-source '192.168.0.5' --proxy-source-port '111' --proxy-dest '1.1.1.1' --proxy-dest-port '26'
 
 Output Info:
   show_time_lapse    = FALSE

testing/regressions/_exec-output-dump/out-ref/0554.stdout

@@ -1,6 +1,6 @@
 App Info:
   X-Mailer = swaks v99999999.9 jetmore.org/john/code/swaks/
-  Cmd Line = %SWAKS_COMMAND% --from '[email protected]' --to '[email protected]' --helo 'host1.nodns.test.swaks.net' --server 'ser.ver' --dump-as-body 'app,auth' --dump-as-body-shows-password '1' --auth-user 'TEST_USER' --auth-password 'TEST_PASS' --auth-hide-password 'CUSTOM_PASSWORD_REPLACEMENT' --dump 'app,auth,data'
+  Cmd Line = %SWAKS_COMMAND% --from '[email protected]' --to '[email protected]' --helo 'host1.nodns.test.swaks.net' --server 'ser.ver' --dump-as-body 'app,auth' --dump-as-body-shows-password --auth-user 'TEST_USER' --auth-password 'TEST_PASS' --auth-hide-password 'CUSTOM_PASSWORD_REPLACEMENT' --dump 'app,auth,data'
 
 Authentication Info:
   auth           = required
@@ -28,7 +28,7 @@ X-Mailer: swaks v99999999.9 jetmore.org/john/code/swaks/
 
 App Info:
   X-Mailer = swaks v99999999.9 jetmore.org/john/code/swaks/
-  Cmd Line = %SWAKS_COMMAND% --from '[email protected]' --to '[email protected]' --helo 'host1.nodns.test.swaks.net' --server 'ser.ver' --dump-as-body 'app,auth' --dump-as-body-shows-password '1' --auth-user 'TEST_USER' --auth-password 'TEST_PASS' --auth-hide-password 'CUSTOM_PASSWORD_REPLACEMENT' --dump 'app,auth,data'
+  Cmd Line = %SWAKS_COMMAND% --from '[email protected]' --to '[email protected]' --helo 'host1.nodns.test.swaks.net' --server 'ser.ver' --dump-as-body 'app,auth' --dump-as-body-shows-password --auth-user 'TEST_USER' --auth-password 'TEST_PASS' --auth-hide-password 'CUSTOM_PASSWORD_REPLACEMENT' --dump 'app,auth,data'
 
 Authentication Info:
   auth           = required

testing/regressions/_options-auth/00713.test

@@ -4,6 +4,6 @@ auto: REMOVE_FILE,CREATE_FILE,MUNGE,COMPARE_FILE %TESTID%.stdout %TESTID%.stderr
 
 title: apt, config, no-option
 
-pre action: MERGE %OUTDIR%/swaksrc-%TESTID% string:'apt \nno-apt'
+pre action: MERGE %OUTDIR%/swaksrc-%TESTID% string:'apt\nno-apt'
 test action: CMD_CAPTURE %SWAKS% --dump AUTH --to [email protected] --from [email protected] --helo hserver --server "ser.ver" --au USER --ap PASS \
     --config %OUTDIR%/swaksrc-%TESTID%

testing/regressions/_options-auth/00763.test

@@ -4,6 +4,6 @@ auto: REMOVE_FILE,CREATE_FILE,MUNGE,COMPARE_FILE %TESTID%.stdout %TESTID%.stderr
 
 title: auth-plaintext, config, no-option
 
-pre action: MERGE %OUTDIR%/swaksrc-%TESTID% string:'auth-plaintext \nno-auth-plaintext'
+pre action: MERGE %OUTDIR%/swaksrc-%TESTID% string:'auth-plaintext\nno-auth-plaintext'
 test action: CMD_CAPTURE %SWAKS% --dump AUTH --to [email protected] --from [email protected] --helo hserver --server "ser.ver" --au USER --ap PASS \
     --config %OUTDIR%/swaksrc-%TESTID%

testing/regressions/_options-data/00203.test

@@ -5,4 +5,4 @@ auto: REMOVE_FILE,CREATE_FILE,MUNGE,COMPARE_FILE %TESTID%.stdout %TESTID%.stderr
 title: dabsp, command line, no-option
 
 test action: CMD_CAPTURE %SWAKS% --dump DATA --to [email protected] --from [email protected] --helo hserver --server "ser.ver" --au auth_user --ap auth_pass --dab AUTH \
-    --dabsp '' --no-dabsp
+    --dabsp --no-dabsp

testing/regressions/_options-data/00213.test

@@ -4,6 +4,6 @@ auto: REMOVE_FILE,CREATE_FILE,MUNGE,COMPARE_FILE %TESTID%.stdout %TESTID%.stderr
 
 title: dabsp, config, no-option
 
-pre action: MERGE %OUTDIR%/swaksrc-%TESTID% string:'dabsp \nno-dabsp'
+pre action: MERGE %OUTDIR%/swaksrc-%TESTID% string:'dabsp\nno-dabsp'
 test action: CMD_CAPTURE %SWAKS% --dump DATA --to [email protected] --from [email protected] --helo hserver --server "ser.ver" --au auth_user --ap auth_pass --dab AUTH \
     --config %OUTDIR%/swaksrc-%TESTID%

testing/regressions/_options-data/00253.test

@@ -5,4 +5,4 @@ auto: REMOVE_FILE,CREATE_FILE,MUNGE,COMPARE_FILE %TESTID%.stdout %TESTID%.stderr
 title: dump-as-body-shows-password, command line, no-option
 
 test action: CMD_CAPTURE %SWAKS% --dump DATA --to [email protected] --from [email protected] --helo hserver --server "ser.ver" --au auth_user --ap auth_pass --dab AUTH \
-    --dump-as-body-shows-password '' --no-dump-as-body-shows-password
+    --dump-as-body-shows-password --no-dump-as-body-shows-password

testing/regressions/_options-data/00263.test

@@ -4,6 +4,6 @@ auto: REMOVE_FILE,CREATE_FILE,MUNGE,COMPARE_FILE %TESTID%.stdout %TESTID%.stderr
 
 title: dump-as-body-shows-password, config, no-option
 
-pre action: MERGE %OUTDIR%/swaksrc-%TESTID% string:'dump-as-body-shows-password \nno-dump-as-body-shows-password'
+pre action: MERGE %OUTDIR%/swaksrc-%TESTID% string:'dump-as-body-shows-password\nno-dump-as-body-shows-password'
 test action: CMD_CAPTURE %SWAKS% --dump DATA --to [email protected] --from [email protected] --helo hserver --server "ser.ver" --au auth_user --ap auth_pass --dab AUTH \
     --config %OUTDIR%/swaksrc-%TESTID%