Fix --save command line regression in redis 7.0.0 (#10690)

Unintentional change in #9644 (since RC1) meant that an empty `--save ""` config
from command line, wouldn't have clear any setting from the config file

Added tests to cover that, and improved test infra to take additional
command line args for redis-server

src/config.c

@@ -2595,8 +2595,10 @@ static int setConfigSaveOption(standardConfig *config, sds *argv, int argc, cons
     int j;
 
     /* Special case: treat single arg "" as zero args indicating empty save configuration */
-    if (argc == 1 && !strcasecmp(argv[0],""))
+    if (argc == 1 && !strcasecmp(argv[0],"")) {
+        resetServerSaveParams();
         argc = 0;
+    }
 
     /* Perform sanity check before setting the new config:
     * - Even number of args

tests/assets/default.conf

@@ -13,9 +13,8 @@ databases 16
 latency-monitor-threshold 1
 repl-diskless-sync-delay 0
 
+# Note the infrastructure in server.tcl uses a dict, we can't provide several save directives
 save 900 1
-save 300 10
-save 60 10000
 
 rdbcompression yes
 dbfilename dump.rdb

tests/support/server.tcl

@@ -262,16 +262,22 @@ proc create_server_config_file {filename config} {
     close $fp
 }
 
-proc spawn_server {config_file stdout stderr} {
+proc spawn_server {config_file stdout stderr args} {
+    set cmd [list src/redis-server $config_file]
+    set args {*}$args
+    if {[llength $args] > 0} {
+        lappend cmd {*}$args
+    }
+
     if {$::valgrind} {
-        set pid [exec valgrind --track-origins=yes --trace-children=yes --suppressions=[pwd]/src/valgrind.sup --show-reachable=no --show-possibly-lost=no --leak-check=full src/redis-server $config_file >> $stdout 2>> $stderr &]
+        set pid [exec valgrind --track-origins=yes --trace-children=yes --suppressions=[pwd]/src/valgrind.sup --show-reachable=no --show-possibly-lost=no --leak-check=full {*}$cmd >> $stdout 2>> $stderr &]
     } elseif ($::stack_logging) {
-        set pid [exec /usr/bin/env MallocStackLogging=1 MallocLogFile=/tmp/malloc_log.txt src/redis-server $config_file >> $stdout 2>> $stderr &]
+        set pid [exec /usr/bin/env MallocStackLogging=1 MallocLogFile=/tmp/malloc_log.txt {*}$cmd >> $stdout 2>> $stderr &]
     } else {
         # ASAN_OPTIONS environment variable is for address sanitizer. If a test
         # tries to allocate huge memory area and expects allocator to return
         # NULL, address sanitizer throws an error without this setting.
-        set pid [exec /usr/bin/env ASAN_OPTIONS=allocator_may_return_null=1 src/redis-server $config_file >> $stdout 2>> $stderr &]
+        set pid [exec /usr/bin/env ASAN_OPTIONS=allocator_may_return_null=1 {*}$cmd >> $stdout 2>> $stderr &]
     }
 
     if {$::wait_server} {
@@ -398,6 +404,7 @@ proc start_server {options {code undefined}} {
     set overrides {}
     set omit {}
     set tags {}
+    set args {}
     set keep_persistence false
 
     # parse options
@@ -409,6 +416,9 @@ proc start_server {options {code undefined}} {
             "overrides" {
                 set overrides $value
             }
+            "args" {
+                set args $value
+            }
             "omit" {
                 set omit $value
             }
@@ -518,7 +528,7 @@ proc start_server {options {code undefined}} {
 
         send_data_packet $::test_server_fd "server-spawning" "port $port"
 
-        set pid [spawn_server $config_file $stdout $stderr]
+        set pid [spawn_server $config_file $stdout $stderr $args]
 
         # check that the server actually started
         set port_busy [wait_server_started $config_file $stdout $pid]
@@ -721,7 +731,7 @@ proc restart_server {level wait_ready rotate_logs {reconnect 1} {shutdown sigter
 
     set config_file [dict get $srv "config_file"]
 
-    set pid [spawn_server $config_file $stdout $stderr]
+    set pid [spawn_server $config_file $stdout $stderr {}]
 
     # check that the server actually started
     wait_server_started $config_file $stdout $pid

tests/unit/introspection.tcl

@@ -166,11 +166,26 @@ start_server {tags {"introspection"}} {
             assert_match [r config get save] {save {3600 1 300 100 60 10000}}
         }
 
-        # First "save" keyword overrides defaults
+        # First "save" keyword overrides hard coded defaults
         start_server {config "minimal.conf" overrides {save {100 100}}} {
             # Defaults
             assert_match [r config get save] {save {100 100}}
         }
+
+        # First "save" keyword in default config file
+        start_server {config "default.conf"} {
+            assert_match [r config get save] {save {900 1}}
+        }
+
+        # First "save" keyword appends default from config file
+        start_server {config "default.conf" args {--save 100 100}} {
+            assert_match [r config get save] {save {900 1 100 100}}
+        }
+
+        # Empty "save" keyword resets all
+        start_server {config "default.conf" args {--save {}}} {
+            assert_match [r config get save] {save {}}
+        }
     } {} {external:skip}
 
     test {CONFIG sanity} {