@oranagra This one is small and looks like a bug
PTAL when you have time. Thanks

@zuiderkwast It's a bit embarrassing..., I don't know how to judge empty array in single test

In TCL? I don't know either. :-) But there must be some way, maybe using lower level protocol or a redis-cli test case(?)

In other test cases, they just check that it's an empty string, such as here:

$ git grep -A5 -n 'yields an empty array'
tests/unit/type/list.tcl:961:    test {LRANGE with start > end yields an empty array for backward compatibility} {
tests/unit/type/list.tcl-962-        create_list mylist "1 2 3"
tests/unit/type/list.tcl-963-        assert_equal {} [r lrange mylist 1 0]
tests/unit/type/list.tcl-964-        assert_equal {} [r lrange mylist -1 -2]
tests/unit/type/list.tcl-965-    }
tests/unit/type/list.tcl-966-

@zuiderkwast Yes. The original code test case is also check the empty string. Not an empty array

regarding the tests, TCL doesn't really have any types other than strings, so there's not much we can do.
our options would be:

1. keep the current test (turn a blind eye)
2. change the code in redis.tcl to return a {nil} string in that cases, and update all the (22) tests it breaks to explicitly check for {nil} instead of {}
3. create a new infrastructure (special mode in redis.tcl?) that allows executing command and getting the raw RESP response.

considering this a breaking change in a command that was recently introduced, in theory there's a question of whether to fix it and also backport the fix to 6.2.
i suppose it should share the same fate as #9089 which had nearly identical concerns.
@yossigo let me know if you think this should be a major decision, or just silently follow the footsteps of #9089

For the test, I think we should take 2 or 3. Or at least one way to check.
null or empty array or emtpy string or any others should be different

2 we can change the whole point, including changing the previous ones and maybe simpler
3 we can only need to change what is currently changed but it seems more difficult to me

I think i will try method 2 first and see how the changes are...

I suggest 4: Use redis-cli and check that the output is "(empty array)". Very similar to this: https://github.com/redis/redis/blob/unstable/tests/integration/redis-cli.tcl#L170

integration/redis-cli.tcl might not be the right place to test these corner cases. With a little refactoring (move some helper functions such as open_cli into tests/support/cli.tcl) we can use cli also in other test suites with very little effort.

[Edit] But on the other hand, one purpose is to check that redis-cli outputs "(empty array)" for empty arrays, so therefore we can put the test cases in the redis-cli test suite. :-)

i don't think we should abuse redis-cli for these tests (they don't intend to test redis-cli).
i think that adding a raw mode to redis.tcl isn't that complicated (already have a partially working POC).
but i also think that maybe it's not that bad idea to do both options 3 and 4.

option 3 is useful so that it's easy to use and will also fail tests that actually expect an empty array (so we don't wanna change the test), if for some reason redis one day responds with null (we'll be able to catch a regression).

and i think that option 4 is an important tool to have in our toolbox, to be able to read RESP directly from the tests if we want.

i was considering to post my POC here and let you take it to completion, but if we wanna do both 3 and 4 anyway, that raw mode can come in a different PR.

the only question is if there's any reason not to do 3?
on one hand it does introduce a mass of changes (at least 22 tests), but on the other hand, as i noted above, it can some day spot a regression, and it's also easier to use than raw mode, so maybe we'll find it more useful..

Raw mode (3) or cli (4) both do the job AFAICT. If 3 is easy enough to do, then no need for 4.

(The only option I don't love is option 2 since {nil} in TCL is a list of one element. Also it still doesn't allow distinguish between empty string and null. And in the docs, nil is used for null, not for empty arrays.)

@zuiderkwast do you mean that the string we choose:
i.e.

 proc ::redis::redis_multi_bulk_read {id fd} {
     set count [redis_read_line $fd]
-    if {$count == -1} return {}
+    if {$count == -1} return {nil}

can be confused with a normal response containing the string "nil" (i.e. addReplyBulkCBuffer(c, "nil", 3))?
i don't think there's a real chance the tests will need to make that distinction.
same applies for a list of one element containing the value "nil".

regarding the terms "nil" or "null" and consistency with the docs, i don't think it matters, we can take either one as long as the tests are consistent with their infra.

the only concern i have is if this kind of change causes more damage than good. and if we apply similar logic in other places (for constancy), it may cause more damage than good.
do you think so?

can be confused with a normal response containing the string "nil" (i.e. addReplyBulkCBuffer(c, "nil", 3))?
i don't think there's a real chance the tests will need to make that distinction.
same applies for a list of one element containing the value "nil".

That's the ambiguity, yes, but I don't think there's a real risk either. But even redis-cli prints "(nil)" for null, so I think "nil" for an empty array is a bad choice. If we do 2 ("corner case mode"), how about returning special strings for these corner cases:

• "empty-array"
• "empty-string"
• "nil" (or "null", or even "null-string" and "null-array")

maybe we can try being consistent with the codebase:

    shared.null[2] = createObject(OBJ_STRING,sdsnew("$-1\r\n"));
    shared.null[3] = createObject(OBJ_STRING,sdsnew("_\r\n"));

    shared.nullarray[2] = createObject(OBJ_STRING,sdsnew("*-1\r\n"));
    shared.nullarray[3] = createObject(OBJ_STRING,sdsnew("_\r\n"));

and also make it clear that this is a special hack string, and make it easy to grep for it and find the other mentions of it.
so how about ::null:: and ::nullarray::?

Here's what i came up with: #9193

ohh, i missed one... (waiting to get feedback from Yossi, hope to merge soon)