Change BITCOUNT 'end' as optional like BITPOS

[LiiNen]

This change is the thing I suggested to redis when it was BSD, and is not just migration - this is of course more advanced

Issue

There is weird difference in syntax between BITPOS and BITCOUNT:

BITPOS key bit [start [end [BYTE | BIT]]]
BITCOUNT key [start end [BYTE | BIT]]

I think this might cause confusion in terms of usability. It was not just a syntax typo error, and really works differently. The results below are with unstable build:

> get TEST:ABCD
"ABCD"
> BITPOS TEST:ABCD 1 0 -1
(integer) 1
> BITCOUNT TEST:ABCD 0 -1
(integer) 9
> BITPOS TEST:ABCD 1 0
(integer) 1
> BITCOUNT TEST:ABCD 0
(error) ERR syntax error

What did I fix

simply changes logic, to accept BITCOUNT also without 'end' - 'end' become optional, like BITPOS

> GET TEST:ABCD
"ABCD"
> BITPOS TEST:ABCD 1 0 -1
(integer) 1
> BITCOUNT TEST:ABCD 0 -1
(integer) 9
> BITPOS TEST:ABCD 1 0
(integer) 1
> BITCOUNT TEST:ABCD 0
(integer) 9

Of course, I also fixed syntax hint:

# ASIS 
> BITCOUNT key [start end [BYTE|BIT]]
# TOBE
> BITCOUNT key [start [end [BYTE|BIT]]]

Moreover ...

I hadn't noticed that there was very small dead code in these command logic, when I wrote PR to redis. I found it now, when write code again, so I wrote it in valkey.

/* asis unstable */

/* bitcountCommand() */
if (!strcasecmp(c->argv[4]->ptr,"bit")) isbit = 1;
// ...
if (c->argc < 4) {
    if (isbit) end = (totlen<<3) + 7;
    else end = totlen-1;
}

/* bitposCommand() */
if (!strcasecmp(c->argv[5]->ptr,"bit")) isbit = 1;
// ...
if (c->argc < 5) {
    if (isbit) end = (totlen<<3) + 7;
    else end = totlen-1;
}

Bit variable (actually int) "isbit" is only being set as 1, when 'BIT' is declared. But we were checking whether 'isbit' is true or false in this 'if' phrase, even if isbit could never be 1, because argc is always less than 4 (or 5 in bitpos).

I think this minor fixes will make valkey command operation more consistent. Of course, this PR contains just changing args from "required" to "optional", so it will never hurt previous users.

Thanks,

[LiiNen]

updated it and run COMMAND DOCS for checking it works properly

Note. github commit with 'commit suggestion' won't let me signoff ;-; didnt know that

[LiiNen]

rebase to avoid spellcheck issue: https://github.com/valkey-io/valkey/pull/139

[LiiNen]

seems like issues in testfile. I'll check it

edited) I have missed that bitcount now accept without end. Removed it in tcl.

assert_error {ERR *syntax*} {r bitcount s 0}

p.s. I have changed it when I made PR to redis, but just missed it in here by human(my) fault 😅

[hwware]

You are right. Just ignore my suggestion code changes and keep current json file. For this PR, just add more several edge cases and I will approve it, Thanks

[hwware]

But I doubt, maybe for BITPOS and BITCOUNT, the command format:
BITPOS key bit start BYTE | BIT BITCOUNT key start BYTE | BIT should be acceptable as well in the future because end is optional

Let us discuss this later

[LiiNen]

I agree. If there is no need to consider backward compatibility, then me also want to change: using BYTE|BIT without end. This might also affect aof, so this have to be discussed later 😢

In case of testcase, I'll work on it asap when i have time :)

[codecov[bot]]

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

:exclamation: No coverage uploaded for pull request base (unstable@83507d7). Click here to learn what that means.

Additional details and impacted files

@@             Coverage Diff             @@
##             unstable     #118   +/-   ##
===========================================
  Coverage            ?   68.37%           
===========================================
  Files               ?      108           
  Lines               ?    61563           
  Branches            ?        0           
===========================================
  Hits                ?    42092           
  Misses              ?    19471           
  Partials            ?        0           

Files	Coverage Δ
src/bitops.c	93.32% <100.00%> (ø)
src/commands.def	100.00% <ø> (ø)