I don't think I wrote it right but let us discuss it in here.

i also thought about something like:

int variable_key_specs_num = 0;
for (int i = 0; i < cmd->key_specs_num; i++) {
    if (cmd->key_specs[i].flags & CMD_KEY_VARIABLE_FLAGS &&
        cmd->key_specs[i].begin_search_type == KSPEC_BS_INDEX &&
        cmd->key_specs[i].find_keys_type == KSPEC_FK_RANGE)
    {
        variable_key_specs_num++;
    }
}
if (cmd->key_specs_num != 0 && cmd->key_specs_num == variable_key_specs_num)
    movablekeys = 0;

i agree we need to completely re-write this function.
i don't like the branch on getkeys_proc, one side looks just at the VARIABLE_FLAGS flag, and the other does not.

since we define movablekeys as a way to determine if the legacy range [first, last, step] doesn't find all keys, then i think we need a completely different approach.

the right approach should be to check if the legacy range covers all key-specs, and if none of the key-specs have the INCOMPLETE flag.
this way, we don't need to look at getkeys_proc of VARIABLE_FLAG at all.
probably with the exception of modules, who may still not be using key-specs.

maybe the easiest way to do that is to unify populateCommandLegacyRangeSpec and populateCommandMovableKeys into one function.

then, let's compare the result to the output of 6.2.

p.s. in order to properly serve both old and new modules, we must probably keep relying CMD_MODULE_GETKEYS, but do that only for modules that don't declare key-specs. for ones that do, we need to take the same approach we take with native redis commands.

For the record, i compared the list of commands tagged with movablekeys between 7.0 and 6.2.
other than these two fixed in this PR, there are a bunch of other commands tagged, but they're all new 7.0 commands, which are using the numkeys approach, so they don't have a legacy range spec.

The other difference was that STRALGO and MEMORY are no longer listed, one was removed (LSC does have a legacy range spec), and the other has a specific sub-command with a legacy range spec, i'm not sure how compatible is that with old clients.

i.e. in the past memory was listed as:

1) 1) "memory"
   2) (integer) -2
   3) 1) readonly
      2) random
      3) movablekeys
   4) (integer) 0
   5) (integer) 0
   6) (integer) 0

and not it is listed as

1)  1) "memory"
    2) (integer) -2
    3) (empty array)
    4) (integer) 0
    5) (integer) 0
    6) (integer) 0
...
...
       3)  1) "memory|usage"
           2) (integer) -3
           3) 1) readonly
           4) (integer) 2
           5) (integer) 2
           6) (integer) 1

@chayim can you check how this affects redis-py (not sure if we have anything to improve)

i pushed suggested approach and some tests.
i think i need more tests for modules on the legacy renage spec and movablekeys in various conditions.
not sure. i'll look again later

@oranagra thanks for picking it up, i read the code, LGTM. and i did some comment cleanup (hope you don't mind)
and i had a question, i don't see prev_lastkey being set anywhere (both in the old code and the new code)?
so (prev_lastkey && prev_lastkey != c->key_specs[i].bs.index.pos-1) is never being true.

so i added a prev_lastkey = lastkey; base on my understanding
and i ran the code, (prev_lastkey && prev_lastkey != c->key_specs[i].bs.index.pos-1) is still never being true (redis native commands), so maybe it is use for module

seems like that bug with prev_lastkey didn't have an impact in redis (maybe it did for some modules).
i.e. there's no redis native command with two range specs that are not consecutive (so that if you mentioned is still always false).