Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

valkey-cli: ensure output ends with a newline if missing when printing reply #1782

Open
wants to merge 4 commits into
base: unstable
Choose a base branch
from

Conversation

xbasel
Copy link
Member

@xbasel xbasel commented Feb 25, 2025

Example:

   127.0.0.1:6379> multi
   OK
   127.0.0.1:6379(TX)> client info
   QUEUED127.0.0.1:6379(TX)>

Fixes: #1778

reply

Example:
   127.0.0.1:6379> multi
   OK
   127.0.0.1:6379(TX)> client info
   QUEUED127.0.0.1:6379(TX)>

Signed-off-by: xbasel <[email protected]>
@xbasel xbasel marked this pull request as draft February 25, 2025 21:37
@madolson
Copy link
Member

I would guess it's more likely related to this code path,

if (!strcasecmp(command, "info") || !strcasecmp(command, "lolwut") ||
, which explicitly changes the output mode to raw if it's an info command. Probably need to check for multi there.

Copy link

codecov bot commented Feb 26, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 71.01%. Comparing base (221ff44) to head (8d2b037).
Report is 4 commits behind head on unstable.

Additional details and impacted files
@@             Coverage Diff              @@
##           unstable    #1782      +/-   ##
============================================
- Coverage     71.18%   71.01%   -0.17%     
============================================
  Files           123      123              
  Lines         65619    65642      +23     
============================================
- Hits          46712    46618      -94     
- Misses        18907    19024     +117     
Files with missing lines Coverage Δ
src/valkey-cli.c 54.39% <100.00%> (-1.69%) ⬇️

... and 12 files with indirect coverage changes

@xbasel xbasel marked this pull request as ready for review February 26, 2025 09:49
(argc == 2 && !strcasecmp(command, "latency") && !strcasecmp(argv[1], "doctor")) ||
/* Format PROXY INFO command for Cluster Proxy:
* https://github.com/artix75/redis-cluster-proxy */
(argc >= 2 && !strcasecmp(command, "proxy") && !strcasecmp(argv[1], "info")))) {
output_raw = 1;
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we correct the output_raw in here? something like this:

if (config.in_multi) output_raw = 0; /* multi must in xxx mode. */

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you suggesting this for readability, or to address a problem?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume readability, as it has the same result. I will say I would prefer we add a comment somewhere to include why multi does this. My hot take was going to be let's just do:

if (!config.in_multi) output_raw = 1; /* In a multi block, commands will return status instead of verbatim. */

I don't feel all that strongly though.

Copy link
Member Author

@xbasel xbasel Feb 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You meant:

if (config.in_multi) output_raw = 0; /* In a multi block, commands will return status instead of verbatim. */

?

I want to skip the big if.
See 61559bf

This function could be more readable; I can refactor it in a separate PR.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I meant my suggestion inside the block of text, so it minimizes the number of lines changed. I'm OK with this though.

(argc == 2 && !strcasecmp(command, "latency") && !strcasecmp(argv[1], "doctor")) ||
/* Format PROXY INFO command for Cluster Proxy:
* https://github.com/artix75/redis-cluster-proxy */
(argc >= 2 && !strcasecmp(command, "proxy") && !strcasecmp(argv[1], "info")))) {
output_raw = 1;
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I meant my suggestion inside the block of text, so it minimizes the number of lines changed. I'm OK with this though.

Signed-off-by: Madelyn Olson <[email protected]>
Copy link
Member

@enjoy-binbin enjoy-binbin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i am ok with this, but i still prefer the smaller diff way. The logic is pretty simple, we can argue that we correct it after the big if block since it is in multi.

Not a blocker to me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] Incorrect output formatting in valkey-cli when queuing "client info" in a transaction
3 participants