redis-cli: make returning error exit code possible when command execution fails

[hwware]

This PR fixes #8127, by providing -e option, user can control whether return error code if the command execution fails. @oranagra please take a look and let me know if i miss anything, thanks

Example:

./redis-cli config set requirepass 1234
OK
./redis-cli -e flushdb
NOAUTH Authentication required.
echo $?
1

[oranagra]

thanks @hwware.

the -e option certainly makes sense (to terminate the execution as soon as one error happens).

but i think it might also be useful to have the return code represent a presence of an error even if we don't abort the execution (i.e. in case the user executed a pipeline of commands and one failed).
if we have that, maybe it should even be the default behavior, and then, for the case of a simple single command it's all the same (so users will benefit from that without the need of adding any cli flag).

@redis/core-team please approve the new flag, and share your opinion about the above.

[itamarhaber]

Yep, also very related to #3475

[yossigo]

@oranagra I agree it can be useful but I don't think we should change the default behavior now as it may have a big negative impact on compatibility.

While at it - we should probably also consider the different high level error cases and map them to error codes.

[yossigo]

@hwware In addition to aborting with an error code, this also introduces other changes as the error is now sent to stderr rather than stdout and it does not conform to the cliFormatReply formatting. Is that intentional? We should consider maintaining compatibility here.

[hwware]

hi @yossigo , thanks for your comments, yes this is intentional, since when cli abort with error code, the stderr will always been set based on other parts of code, for example when context error happens and we have to exit with an error: https://github.com/redis/redis/blob/unstable/src/redis-cli.c#L873 . the cliFormatReply based on my understanding is formatting reply for "no error", even though this function contains a error case we still don't consider the execution fails and the exit code is still 0.

[oranagra]

@yossigo by different high level errors, you mean an error to establish connection or authentication error (which means the command wasn't at all executed), should be distinguished from an error response from the command?
or did you also mean to distinguish between -OOM, -WRONGTYPE, and such?

i understand why my idea of changing the default behavior is wrong, i just think it's a shame that for a single command the default is to return 0 even though it failed.
anyway, let's assume we don't make it the default, do you think we should add such a thing (run the pipeline of commands to completion but reflect and error if one failed)? in a later PR or now?

[hwware]

Hi @oranagra , I agree with @yossigo 's points regarding the default behaviour when command execution fails. Since this may cause big compatability issue if user already starting using current version. But we can centainy provide another flag controlling the return and it won't break the backward compatability (maybe we have too many flags IMHO and you don't think we should do this) Or we can think about this later, maybe we need to refactor the cli code for error handling part..

[madolson]

LGTM