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

vis_pipe*: take a Text object, vis-lua: add pipe_buffer #1177

Closed
wants to merge 2 commits into from

Conversation

git-bruh
Copy link
Contributor

@git-bruh git-bruh commented Apr 6, 2024

This adds a pipe_buffer helper to the lua api to allow piping arbritary buffers to an external command, as the current way of constructing a printf command is quite hacky and is prone to shell injection

As a workaround, I'd written a wrapper like this but it messes up the scroll position, so made this PR instead:

vis.pipe_buffer = function(vis, buffer, cmd, fullscreen)
	vis:command("open")
	vis.win.file:insert(0, buffer)

	local status, stdout, stderr = vis:pipe(vis.win.file, {start = 0, finish = vis.win.file.size}, cmd, fullscreen)

	vis.win:close(true)

	return status, stdout, stderr
end

@git-bruh git-bruh mentioned this pull request Apr 6, 2024
@rnpnr
Copy link
Collaborator

rnpnr commented Apr 21, 2024

I'm not sure about this one. Can you provide some real example of where printf is creating a problem?

(Ignore the failing macOS CI, github broke something.)

@git-bruh
Copy link
Contributor Author

I'm not sure about this one. Can you provide some real example of where printf is creating a problem?

(Ignore the failing macOS CI, github broke something.)

In my case I had to display a list in the format of file:line: contents with a fuzzy search program, and it broke in certain cases as file contents can have embedded quotes as well that might not be terminated, so the constructed command could look something like this, causing the command to fail

printf '
./file1:0:line zero
./file1:1:'unterminated quote
'

@rnpnr
Copy link
Collaborator

rnpnr commented Apr 21, 2024

Is there reason why you don't pipe the output of the fuzzy finder to the next stage in one go? Why do you need to save the intermediate output?

@git-bruh
Copy link
Contributor Author

The contents I shared are generated by my lua code, I'm using it for displaying all the lines for the goto references in LSP. I get the file and line numbers from the LSP server, and then construct a buffer in lua that is piped to a fuzzy finder

@fischerling
Copy link
Contributor

I wanted something like this too, but for different reasons.

Sometimes you do not have a window or a file to use vis:pipe with.

For example during the creation of the vis-lockfile plugin it would have been beneficial to be able to prompt for a user selection during a FILE_OPEN event (we solved it by subscribing to both FILE_OPEN and WIN_OPEN).
During the first FILE_OPEN event there does not exist a window or a file yet.

@rnpnr
Copy link
Collaborator

rnpnr commented May 13, 2024

I'm OK with the idea but I'm not a fan of needing to go through at least 4 different dynamic allocations (text, piece, change, at least 1 in text_insert(), etc.) just to pass a string to a shell program. Maybe there is a better way?

@git-bruh
Copy link
Contributor Author

Yeah we can definitely come up with more efficient ways of doing this, but for the PR just wanted to re-use as much of the existing code as possible

@mcepl
Copy link
Contributor

mcepl commented Sep 7, 2024

@git-bruh Hi, what is the status of this PR, please? Your last comment seems to indicate that there will be some new version of this PR coming.

@git-bruh
Copy link
Contributor Author

git-bruh commented Sep 8, 2024

Hey sorry, I won't be having time soon to work on this PR. In the comment I meant we could discuss and implement a more efficient version, but I didn't get time to follow up on it

@fischerling
Copy link
Contributor

I played with the idea and came up with #1204.
Any suggestions are welcome :)

@rnpnr
Copy link
Collaborator

rnpnr commented Sep 13, 2024

Superseded by #1204

@rnpnr rnpnr closed this Sep 13, 2024
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.

4 participants