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: #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]

Could you please add several special cases which include larger value of start, such as start = 10 or 50, Thanks

[LiiNen]

I'll add some new test cases in near days. thxs

[LiiNen]

Added more testcases; 843c84a

1. Larger value of start as you said
2. Larger negative value of start.
3. Also added large negative value in BITCOUNT with start, end
4. fix [count_bits ""] -> 0, when start > end if it is in the range of string (line 147)

[LiiNen]

note: you said larger value as 10 or 50 for example, but the value 1000 is already used from others, so I also use 1000 :)

[LiiNen]

@hwware gently ping, just wondered if you didn't receive above comments alarm.
i hope it didn't bother you.

[LiiNen]

end is in optional block, so this is useless I think.
I saw syntax again and below is what I have changed.

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

If end is also optional, then the syntax should be like below:

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

I didn't think it before, and it seems this is also maybe reasonable - allowing using just 'start' and '[BYTE | BIT]'.
But in this case, it is harmful for backwards compatibility, and maybe critical to client libraries.

ex. BITCOUNT key start BYTE will be think as BITCOUNT key start end in client libraries, and will be return error because end is just string 'BYTE'.

It will be a quiet issue. Is this what you meant?

[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]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 70.19%. Comparing base (e4ead94) to head (153625c).

Additional details and impacted files

@@             Coverage Diff              @@
##           unstable     #118      +/-   ##
============================================
- Coverage     70.22%   70.19%   -0.04%     
============================================
  Files           109      109              
  Lines         59880    59879       -1     
============================================
- Hits          42051    42030      -21     
- Misses        17829    17849      +20     

Files	Coverage Δ
src/bitops.c	94.03% <100.00%> (+0.29%)	⬆️
src/commands.def	100.00% <ø> (ø)

... and 8 files with indirect coverage changes

[madolson]

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

We can solve this problem in the future.

Discussed in separate meeting, directionally LGTM, just need someone to review the code and merge.

[LiiNen]

seems like conflicts came out due to modifying language format (#323)
I'll resolve it in my branch and commit once more

[LiiNen]

update done.

while fixing conflict, found small typo against lint, so it also have been fixed.

/* line 798 in src/bitops.c */
// before fix
if (c-> argc == 3 

// after fix ( remove space between '>' and 'a' )
if (c->argc == 3 

[srgsanky]

There is a clang-format issue from this commit found while trying to submit another commit - https://github.com/valkey-io/valkey/actions/runs/9279101016/job/25531813775. Can you fix it?

diff --git a/src/bitops.c b/src/bitops.c
index 7a385812d..2094bb0ea 100644
--- a/src/bitops.c
+++ b/src/bitops.c
@@ -808,10 +808,9 @@ void bitcountCommand(client *c) {
             }
         }
         if (c->argc >= 4) {
-            if (getLongLongFromObjectOrReply(c,c->argv[3],&end,NULL) != C_OK)
-                return;
+            if (getLongLongFromObjectOrReply(c, c->argv[3], &end, NULL) != C_OK) return;
         }
-
+
         /* Lookup, check for type. */
         o = lookupKeyRead(c->db, c->argv[1]);
         if (checkType(c, o, OBJ_STRING)) return;
@@ -821,7 +820,7 @@ void bitcountCommand(client *c) {
         /* Make sure we will not overflow */
         serverAssert(totlen <= LLONG_MAX >> 3);

-        if (c->argc < 4) end = totlen-1;
+        if (c->argc < 4) end = totlen - 1;

         /* Convert negative indexes */
         if (start < 0 && end < 0 && start > end) {

[LiiNen]

@srgsanky sorry I dont know why but i missed it.
but this PR already closed (merged) today, so I write a simple change as PR #570
I'll left this also in your PR. thanks that let me know about it.