fix: quote program and args in debug message from cmd::cmd(..)#9777
Conversation
Greptile SummaryThis PR improves the
Confidence Score: 5/5Safe to merge — the change is limited to a single debug log line with no effect on program execution or error handling. The only modification is how the debug string is formatted before being passed to No files require special attention. Important Files Changed
Reviews (3): Last reviewed commit: "fix: quote `program` and `args` in debug..." | Re-trigger Greptile |
There was a problem hiding this comment.
Code Review
This pull request refactors the construction of the display_command string in src/cmd.rs to use an iterator chain for debug logging. However, the current implementation will fail to compile due to a type mismatch between &PathBuf and &OsString in the chain method. Feedback also highlights that the string is constructed even when debug logging is disabled and that the quoting mechanism does not correctly handle arguments containing single quotes.
| let display_command = std::iter::once(&program) | ||
| .chain(&args) | ||
| .map(|s| format!("'{}'", s.to_string_lossy())) | ||
| .collect::<Vec<_>>() | ||
| .join(" "); |
There was a problem hiding this comment.
This code will fail to compile due to a type mismatch in the chain call. std::iter::once(&program) yields &PathBuf, while &args (as an iterator) yields &OsString. The chain method requires both iterators to have the exact same item type. You can fix this by explicitly converting both to &OsStr using .as_os_str().
Additionally, note that this implementation constructs the display_command string regardless of whether the debug log level is enabled, which is slightly inefficient. Also, the quoting is naive and doesn't handle arguments containing single quotes (e.g., it's a file becomes 'it's a file', which is invalid shell syntax).
| let display_command = std::iter::once(&program) | |
| .chain(&args) | |
| .map(|s| format!("'{}'", s.to_string_lossy())) | |
| .collect::<Vec<_>>() | |
| .join(" "); | |
| let display_command = std::iter::once(program.as_os_str()) | |
| .chain(args.iter().map(|s| s.as_os_str())) | |
| .map(|s| format!("'{}'", s.to_string_lossy())) | |
| .collect::<Vec<_>>() | |
| .join(" "); |
There was a problem hiding this comment.
This code will fail to compile ...
It compiles (and runs) just fine.
Additionally, note that this implementation constructs the display_command string regardless of whether the debug log level is enabled, which is slightly inefficient.
Yes, but the original code did the same. So I did not see a reason to change that.
Also, the quoting is naive and doesn't handle arguments containing single quotes (e.g., it's a file becomes 'it's a file', which is invalid shell syntax).
Fixed by using shell_escape::escape(..) instead of format!(..).
29ce57c to
7783d96
Compare
# Problem While trying to understand an issue with the value of VISUAL in `mise -v tasks edit <task>` (see jdx#9731, already addressed by PR jdx#9752), I found the `debug!` message generated by `cmd::cmd(..)` to be less helpful than it could be. With `VISUAL='cat -n'`, the following information got logged: ``` log ... DEBUG $ cat -n /home/kt/work/oss/mise/tasks.toml Error: 0: No such file or directory (os error 2) ... ``` Here, `cat -n` was interpreted as the program to execute. However, the `DEBUG ...` line, doesn't show that. # Solution Call `shell_escape::escape(..)` on `program` and `args` when generating the `debug!` message. This changes the logged information to: ```log ... DEBUG $ 'cat -n' /home/kt/work/oss/mise/tasks.toml Error: 0: No such file or directory (os error 2) ... ``` This hopefully helps to understand the reason for the following error message (from `duct::cmd(..)`). --- Note: The error reported by `duct::cmd(..)`: ```log Error: 0: No such file or directory (os error 2) ``` also leaves room for improvement. This was already discussed in [issue 109 (Include the command that failed in the error)](oconnor663/duct.rs#109) from 2023 in the *duct.rs repository*. A fix for this issue would potentially solve the problem addressed by this commit/PR. But so far, nobody stepped up to provide that fix.
7783d96 to
7d52e92
Compare
Problem
While trying to understand an issue with the value of VISUAL in
mise -v tasks edit <task>(see #9731, already addressed by PR #9752), I found thedebug!message generated bycmd::cmd(..)to be less helpful than it could be. WithVISUAL='cat -n', the following information got logged:Here,
cat -nwas interpreted as the program to execute. However, theDEBUG ...line, doesn't show that.Solution
Call
shell_escape::escape(..)onprogramandargswhen generating thedebug!message. This changes the logged information to:This hopefully helps to understand the reason for the following error message from
duct::cmd(..)).Note: The error reported by
duct::cmd(..):also leaves room for improvement. This was already discussed in issue 109: Include the command that failed in the error from 2023 in the duct.rs repository. A fix for this issue would potentially solve the problem addressed by this commit/PR. But so far, nobody stepped up to provide that fix.