refactor!: change suda#system to a list-based api

[aarondill]

This eliminates the need for shell-escaping, and removes possibility of accidental shell-injection.

This is technically a breaking change, since suda#system is globally exposed; however this is not a documented public api, so the chance of breakage is minimal.

Note that this *changes the behavior of g:suda#executeable! It is no longer shell-processed, but rather treated as a path/name to an executable and processed by the os in that way. Should we split suda#executable on spaces to preserve the old behavior? Should we allow suda#executable to (optionally) be a list to allow passing arguments?

Summary by CodeRabbit

• Refactor
  • Enhanced the internal handling of command execution and options in the suda functionality for improved performance and flexibility.

[coderabbitai[bot]]

Walkthrough

The recent changes in the suda.vim plugin involve restructuring the command execution functions. The suda#system function has been split into suda#systemlist and a new suda#system function for different output handling. Command construction within these functions has been updated, along with adjustments to input and output processing in related functions.

Changes

File(s)	Summary
autoload/suda.vim	Refactored suda#system to suda#systemlist, introduced new suda#system, updated command construction, adjusted input/output processing.

🐇✨
Among lines of code, a rabbit did play,
Refactoring functions to light the way.
With a tweak here and a split there,
Commands now flow with a newfound flair.
"In the realm of Vim," the rabbit did say,
"May your plugins shine, in the light of day!"
🌟🐰

---

Recent Review Details

**Configuration used: CodeRabbit UI**

Commits

Files that changed from the base of the PR and between a583f3a7e764daee6f119887c879f8b048fc80fa and bebd7b3affe0ccdbc25d1c8503a61e9041e481eb.

Files selected for processing (1)

* autoload/suda.vim (6 hunks)

Additional comments not posted (4)

autoload/suda.vim (4)

`7-14`: Consider revising the handling of `suda#executable` to support a list of executables for better flexibility and clarity, as discussed in the TODO comments. --- `36-69`: > :memo: **NOTE** > This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [20-55] Ensure that the handling of password input and command execution in `suda#systemlist` adheres to security best practices, particularly in avoiding potential exposure of sensitive information.

Verification successful

--- The handling of password input and command execution in `suda#systemlist` adheres to security best practices. The password is securely collected using `inputsecret` and the command construction in `s:get_command` does not expose sensitive information. The use of `--` to separate sudo options from the command further ensures that parameters are treated securely.

Scripts Executed

The following scripts were executed for the analysis: --- Script: ```shell #!/bin/bash # Verify that password handling is secure and does not expose sensitive information in logs or error messages. rg --type vimscript 'inputsecret' --context 5 ``` Length of output: 479 --- --- Script: ```shell #!/bin/bash # Check the implementation of s:get_command to ensure it handles passwords securely rg --type vimscript 'function! s:get_command' --context 10 ``` Length of output: 1064

--- `86-100`: > :memo: **NOTE** > This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [78-97] Evaluate the feasibility of using `readfile()` in `suda#read` for performance improvement and simplification, as suggested in the TODO comment. Would you like me to help implement this change or should I open a GitHub issue to track this task? --- `143-154`: > :memo: **NOTE** > This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [130-151] Consider using `writefile()` in `suda#write` to enhance performance and reduce complexity, as suggested in the TODO comment. Would you like me to help implement this change or should I open a GitHub issue to track this task?

--- Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

Share

- [X](https://twitter.com/intent/tweet?text=I%20just%20used%20%40coderabbitai%20for%20my%20code%20review%2C%20and%20it%27s%20fantastic%21%20It%27s%20free%20for%20OSS%20and%20offers%20a%20free%20trial%20for%20the%20proprietary%20code.%20Check%20it%20out%3A&url=https%3A//coderabbit.ai) - [Mastodon](https://mastodon.social/share?text=I%20just%20used%20%40coderabbitai%20for%20my%20code%20review%2C%20and%20it%27s%20fantastic%21%20It%27s%20free%20for%20OSS%20and%20offers%20a%20free%20trial%20for%20the%20proprietary%20code.%20Check%20it%20out%3A%20https%3A%2F%2Fcoderabbit.ai) - [Reddit](https://www.reddit.com/submit?title=Great%20tool%20for%20code%20review%20-%20CodeRabbit&text=I%20just%20used%20CodeRabbit%20for%20my%20code%20review%2C%20and%20it%27s%20fantastic%21%20It%27s%20free%20for%20OSS%20and%20offers%20a%20free%20trial%20for%20proprietary%20code.%20Check%20it%20out%3A%20https%3A//coderabbit.ai) - [LinkedIn](https://www.linkedin.com/sharing/share-offsite/?url=https%3A%2F%2Fcoderabbit.ai&mini=true&title=Great%20tool%20for%20code%20review%20-%20CodeRabbit&summary=I%20just%20used%20CodeRabbit%20for%20my%20code%20review%2C%20and%20it%27s%20fantastic%21%20It%27s%20free%20for%20OSS%20and%20offers%20a%20free%20trial%20for%20proprietary%20code)

Tips

### Chat There are 3 ways to chat with [CodeRabbit](https://coderabbit.ai): - Review comments: Directly reply to a review comment made by CodeRabbit. Example: - `I pushed a fix in commit .` - `Generate unit testing code for this file.` - `Open a follow-up GitHub issue for this discussion.` - Files and specific lines of code (under the "Files changed" tab): Tag `@coderabbitai` in a new review comment at the desired location with your query. Examples: - `@coderabbitai generate unit testing code for this file.` - `@coderabbitai modularize this function.` - PR comments: Tag `@coderabbitai` in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples: - `@coderabbitai generate interesting stats about this repository and render them as a table.` - `@coderabbitai show all the console.log statements in this repository.` - `@coderabbitai read src/utils.ts and generate unit testing code.` - `@coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.` Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. ### CodeRabbit Commands (invoked as PR comments) - `@coderabbitai pause` to pause the reviews on a PR. - `@coderabbitai resume` to resume the paused reviews. - `@coderabbitai review` to trigger a review. This is useful when automatic reviews are disabled for the repository. - `@coderabbitai resolve` resolve all the CodeRabbit review comments. - `@coderabbitai help` to get help. Additionally, you can add `@coderabbitai ignore` anywhere in the PR description to prevent this PR from being reviewed. ### CodeRabbit Configration File (`.coderabbit.yaml`) - You can programmatically configure CodeRabbit by adding a `.coderabbit.yaml` file to the root of your repository. - Please see the [configuration documentation](https://docs.coderabbit.ai/guides/configure-coderabbit) for more information. - If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: `# yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json` ### Documentation and Community - Visit our [Documentation](https://coderabbit.ai/docs) for detailed information on how to use CodeRabbit. - Join our [Discord Community](https://discord.com/invite/GsXnASn26c) to get help, request features, and share feedback. - Follow us on [X/Twitter](https://twitter.com/coderabbitai) for updates and announcements.

[aarondill]

@lambdalisue what do you think about this?

[lambdalisue]

Sorry for late. Well, probably we should add suda#systemlist function and use it internally.

[aarondill]

So you're saying have both suda#system and suda#systemlist? I was thinking about this, but went with the current approach to reduce code duplication. since the two functions do nearly the same thing, except calling system and systemlist respectively. if you think it would increase clarity, I could implement separate functions as wrappers around the current suda#system implementation.

[lambdalisue]

if you think it would increase clarity, I could implement separate functions as wrappers around the current suda#system implementation.

Yes please. We may remove suda#system later but not now 👍

[aarondill]

@lambdalisue to be clear, I was thinking about the following simple implementation

" name tbd
function s:_system(cmd, list, ...)
... " the current implementation
endfunction

function suda#system(cmd, ...)
return s:_system(cmd, 0, ...)
endfunction

function suda#systemlist(cmd, ...)
return s:_system(cmd, 1, ...)
endfunction

is something like this acceptable?

[lambdalisue]

I'm sorry. I meant something like

" arguments must like systemlist()
function! suda#systemlist(cmd, ...) abort
  " ...
endfunction

" arguments must like sysmte()
function! suda#system(cmd, ...) abort
  " ...
  return suda#systemlist(...)
endfunction

So that we can simply remove suda#system later.

[aarondill]

that wouldn't work the way you're thinking. suda#system mimics the vim system function. system and systemlist have the same arguments (cmd as a string or list, and input as optional string/list/buffer id) they differ in return types. system returns a string representing the command's output, while systemlist returns a list representing the command's output (split by newlines, preserving nulls as embedded newlines)

this is why I went the approach that i did, the input arguments are the same, but the return type varies depending on if it calls system or systemlist

[lambdalisue]

Apologies. I completely forgot that I had previously made the exact same mistake a long time ago. Nevertheless, in this scenario, could we consider the following approach?

function! suda#systemlist(cmd, ...) abort
  " ...
endfunction

function! suda#system(cmd, ...) abort
  let result = call('suda#systemlist', [a:cmd] + a:000)
  return join(result, "\n")
endfunction

Alternatively, if you find suda#system() to be too redundant, we can omit it and perhaps introduce suda#systemlist() as part of a new major release.

[aarondill]

@lambdalisue This aproach also fails because the output of systemlist translates NULs into NLs. If we only had to spawn the process once, we could have a helper which returns the argument list, but we spawn the process once, check for failure and potentially spawn it again, meaning we would have to pass in a funcref anyways. I don't see a good way to implement it without passing a funcref to system/systemlist to a helper, reimplementing system's processing of the output (which is already provided by VimScript :pensive:), or a lot of code duplication.

systemlist({expr} [, {input}]) systemlist()

Same as system(), but returns a List with lines (parts of output separated by NL) with NULs transformed into NLs. Output is the same as readfile() will output with {binary} argument set to "b", except that there is no extra empty item when the result ends in a NL. Note that on MS-Windows you may get trailing CR characters.

[lambdalisue]

@aarondill Well, nvm for NULs while Vim's system() doesn't support anyway. Using join(systemlist(...), "\n") should be enough I think.

[aarondill]

@lambdalisue that approach won't work unless we remove the NULs before joining (since they're represented by newlines).

it seems that the default system turns NULs into \u0001 so they don't break strings. should we do this too, or just remove them from the string?

[aarondill]

To make the result more system-independent, the shell output is filtered to replace with for Macintosh, and with for DOS-like systems. To avoid the string being truncated at a NUL, all NUL characters are replaced with SOH (0x01).

[lambdalisue]

it seems that the default system turns NULs into \u0001 so they don't break strings. should we do this too, or just remove them from the string?

Let's follow Vim's behavior 👍

[aarondill]

@lambdalisue I've implemented this change in a583f3a7e764daee6f119887c879f8b048fc80fa

[lambdalisue]

LGTM