my comment after the discussion with Yossi said that we'll also undo the breaking change that we were aware of.
maybe we can reconsider (since this PR has a compromise of keeping the fix for values that start with --).
but maybe we can go even further and even be backwards compatible with an empty save list (considering it could be a common problem).

i.e. after finding --save we can look into the next argument, and if it starts with --, we can set handled_last_config_arg to 1.
it would be ugly, but so is the desire to be backwards compatible with bugs, so a big comment explaining it could un-uglify that hack.

WDYT?

redis-server --save --loglevel verbose, ok, i can do it with --save

for the record
src/redis-server redis.conf --save did not work in 6.2, although it can start the server, but it did not reset the save config. i.e put --save in the last won't reset the save params

7.0, in this case, argc == 0, in setConfigSaveOption we won't reset the save params, the behavior match 6.2 (the commit i made did not change it)
should we take care of this case? make this case work or just leave it as it was

@oranagra i want to split the "save" changes out of this PR (allow passing config name and value in the sama arg)
in 6.2.7, redis.conf (save 3600 1)

• src/redis-server redis.conf --save --loglevel verbose, did not reset the save, we should fix it
• src/redis-server redis.conf --save "" --loglevel verbose, will reset the save
• src/redis-server redis.conf --loglevel verbose --save, did not reset the save, we should fix it
• src/redis-server redis.conf --loglevel verbose --save "", will reset the save

in current unstable, redis.conf (save 3600 1)

• src/redis-server redis.conf --save "" --loglevel verbose, will reset the save
• src/redis-server redis.conf --loglevel verbose --save "", will reset the save
• src/redis-server redis.conf --save --loglevel verbose, (Invalid save parameters, we know it, we break it in 10660, but the behavior in 6.2 we can see it is also a bug..., so we mention the breaking change in 10660 said it work same as --save "" was actually wrong.)
• src/redis-server redis.conf --loglevel verbose --save, (put --save in the last, server can start, but will not reset the save)

in this current PR redis.conf (save 3600 1):

• src/redis-server redis.conf --save "" --loglevel verbose, will reset the save
• src/redis-server redis.conf --loglevel verbose --save "", will reset the save
• src/redis-server redis.conf --save --loglevel verbose, will reset the save, one of the purpose for this PR
• src/redis-server redis.conf --loglevel verbose --save, will not reset the save, i think we should also fix it

ok, i see what i did differently, indeed i used the default config file (in which the save lines are in comment), and saw that the empty save command line argument resets the default save that redis ships with (even without a config file).

so there was a difference in 6.2 between the first ever save directive redis sees, be it a config file or a CLI argument (which resets the default that redis ships with), and an additional empty save after already processing some save directives (which does nothing).

in 6.2 the first ever save directive did a reset, and any additional empty save later will not do anything.
and in 7.0.0, any empty save directive will do a reset.
at this point, i don't think we should change 6.2 behavior at all (not even fixing bugs in that area), and i like to keep things in 7 as they are too in that respect (how an empty save behaves, IIRC it was an intended behavior change).

i would also like to note that the default redis.conf that ships with 6.2 did have non-commentated save lines in in it, and some users may be using a new version of redis with an old config file.

the only things that i think we should change are:

1. different behavior for a save with missing value when it's the final CLI arg vs the non final one (they should both either error, or do a reset)
2. consider unbreaking the breaking change we did in redis-server command line arguments support take one bulk string with spaces for MULTI_ARG configs parsing. And allow options value to use the -- prefix #10660 (the one we were aware of) using the ugly trick we implemented in this PR.

ok, i got it, so in this PR, the changes around the save are:

• src/redis-server redis.conf --save --loglevel verbose, will reset the save, one of the purpose for this PR, unbreaking the breaking change we did in 10660
• src/redis-server redis.conf --loglevel verbose --save, it will not reset the save in 6.2, but in 7.0, we will make it reset the save

yes, it means we still have a behavior change vs 6.2 (even with default config), but i'm hoping no one is affected (since in 6.2 adding --save last had no effect, so i hope anyone who used it would have noticed).
in 7.0 i don't want to have a different behavior between empty save as last arg and non-last, that looks like a bug.

i'm trying to validate that against 6.2 binary.
had to modify or delete quite a few tests from introspection.tcl to make it work on 6.2, due to known differences.
but still, i see that out of the 5 tests in redis-server command line arguments - save with empty input
the first two produce the wrong result (save config isn't empty), and last one fails to load (which i guess it's ok).
can you look into it. (i did remove the config "default.conf" part to let it start without a config file)

i did remove the config "default.conf" part to let it start without a config file)

i am guessing, althought we remove the config "default.conf" part, we still using a baseconfig (which happend is the default.conf) , so we are not start without a config file

i remove the config default.conf part, in 6.2.7, and i print the $cmd (the output seems odd, it sholud be "900 1 300 10 60 10000" ...)

src/redis-server ./tests/tmp/redis.conf.26623.4 --save --loglevel verbose
[err]: redis-server command line arguments - save with empty input in tests/unit/introspection.tcl
Expected 'save {}' to match 'save {60 10000}' (context: type eval line 2 cmd {assert_match [r config get save] {

i am guessing, althought we remove the config "default.conf" part, we still using a baseconfig (which happend is the default.conf) , so we are not start without a config file

ohh, right. i missed that.
so other than the fact the 5th test in that group errors in 6.2, which is ok by me, i think we're good.

@redis/core-team please approve this un-breaking change. making these previously (unintentionally) working patterns and official thing.