[Lua API] enable nil parameters in vis:pipe() for File and Range

[jorbakk]

This enables launching a subprocess from a Lua plugin that doesn't act as a filter. It is useful for running interactive programs like nnn as a file picker where the picked file name is provided in a temporary file and not piped back to vis, see https://github.com/captaingroove/vis-nnn.

[mcepl]

What’s the relationship of this PR to (now hopefully finally being reviewed) #675 ?

[jorbakk]

Opposed to #675 this PR just uses the standard blocking pipe call of vis and not an asynchronous version like in #675. This PR just adds the capability to not wait for output of the subprocess if nil is provided as input for the pipe.

In the example plugin, 'nnn' is called but vis waits for 'nnn' to finish (blocking) and then retrieves the file name picked by 'nnn' from a temporary file, which is the desired behaviour for a file picker. As a sidenote: the file name could also be piped to stdout but then 'nnn' behaves slightly different which is not wanted in this context.

[rnpnr]

@mcepl I don't really think this PR has any relation to that one. This just lets you more easily use vis:pipe() to collect the output of a program from the Lua side.

Previously you would need to do something like vis:pipe(vis.win.file, {start = 0, finish = 0}, cmd) which I did for example in the original version of the vis-lint plugin. Now you can just do vis:pipe(nil, nil, cmd) which I think is much more intuitive.

[mcepl]

It has been merged to the devel branch of https://git.sr.ht/~mcepl/vis and I will test it further, so far it seems to build cleanly.

[mcepl]

Also, update of https://martanne.github.io/vis/doc/index.html#Vis:pipe is missing.

[jorbakk]

I extended the patch based on the suggestion from @ninewise, so now it should support all three relevant cases for providing arguments to vis:pipe(). The fourth possible combination doesn't make sense from my point of view in the context of vis (any other views on that?):

1. vis:pipe(cmd) --- no input, don't collect output; equivalent to range = {EPOS, EPOS}, could not be expressed in the lua API before
2. vis:pipe(nil, nil, cmd) --- no input, collect output; can also be expressed as vis:pipe(vis.win.file, {start = 0, finish = 0}, cmd) which is what @rnpnr mentioned
3. vis:pipe(file, range, cmd) --- provide input, collect output
4. Provide input, don't collect output --- can't think of a reasonable command in the context of vis

[jorbakk]

Also, update of https://martanne.github.io/vis/doc/index.html#Vis:pipe is missing.

It seems like .html files are in .gitignore, so updates from make luadoc are not pushed. Is there a different procedure for generating the docs for the repo when the doc strings are changed?

[mcepl]

This covers vis:pipe(nil, nil, command) signature, but would it be possible also to provide the vis:pipe(nil, input-string, command) one?

[rnpnr]

Also, update of https://martanne.github.io/vis/doc/index.html#Vis:pipe is missing.

It seems like .html files are in .gitignore, so updates from make luadoc are not pushed. Is there a different procedure for generating the docs for the repo when the doc strings are changed?

They are generated by the luadoc Github action when a commit touching the lua code is added. You just need to update the comment above pipe_func() like you did.

[mcepl]

Probably expanding Doxygen strings would be enough. Otherwise, I played with documentation a little bit and it is not nice https://paste.sr.ht/~mcepl/e9495fd806e2b3bcbc076d0559b3aa2d90c42e41

[rnpnr]

1. vis:pipe(cmd) --- no input, don't collect output; equivalent to range = {EPOS, EPOS}, could not be expressed in the lua API before

This could be done before: vis:command("!cmd"). I'm just having a hard time understanding why this is useful for vis:pipe(cmd) to do. cmd doesn't have to output anything for you to use vis:pipe() and you don't need to check the output if you don't need it.

I think I'm against vis:pipe(cmd) being a special case. It should just run cmd and still allow you to use the output and error strings if you need them.

[jorbakk]

This could be done before: vis:command("!cmd"). I'm just having a hard time understanding why this is useful for vis:pipe(cmd) to do. cmd doesn't have to output anything for you to use vis:pipe() and you don't need to check the output if you don't need it.

I think I'm against vis:pipe(cmd) being a special case. It should just run cmd and still allow you to use the output and error strings if you need them.

True, it only makes sense in combination with #1105 when cmd is run with the additional (and optional) fullscreen option. Without both #1104 and #1105, you can't use curses commands like nnn from vis. But if we stick to shell commands without curses, you are right.

However, I think, this would be an unnecessary limitation. Vis has a very unique way to compose processes in the terminal where no separate window and terminal emulator layer is necessary and shell commands as well as curses programs can be handled cleanly. Without #1105, curses commands trash the terminal.

I really enjoy the solution with using nnn from vis as nnn is very powerful and simple and doesn't require a full blown editor plugin to provide a decent file manager for e.g. browsing directories and quickly selecting files, using bookmarked directories, renaming files on-the-fly and so on. And you can use the same program for file/directory browsing and handling from the command line. I think, this is generally a very common task and this solution improves the workflow greatly, at least for me. This is just one use case and there might of course be others.

[jorbakk]

This covers vis:pipe(nil, nil, command) signature, but would it be possible also to provide the vis:pipe(nil, input-string, command) one?

Yes, I think it would be possible but would require to add another parameter to vis_pipe() in vis.c in order to pass the string for piping into command. However, the same result could be achieved with: vis:pipe(nil, nil, "echo '" .. input_string .. "' | " .. command) Would this be an option for this use case?

[rnpnr]

True, it only makes sense in combination with #1105 when cmd is run with the additional (and optional) fullscreen option. ... Without #1105, curses commands trash the terminal.

Ok this is making more sense to me now. But for your use case vis:command("!nnn [options]")should work right? (I only briefly looked at your vis-nnn plugin.) From what I can tell right now it only trashes the terminal after exiting vis. Then a patch similar to #1105 will fix that issue (I'm just not sure #1105 is the correct way of going about it).

With regards to @mcepl's request I think that should be a separate patch. Or maybe its not even needed as your example suggests.

[mcepl]

This covers vis:pipe(nil, nil, command) signature, but would it be possible also to provide the vis:pipe(nil, input-string, command) one?

Yes, I think it would be possible but would require to add another parameter to vis_pipe() in vis.c in order to pass the string for piping into command. However, the same result could be achieved with: vis:pipe(nil, nil, "echo '" .. input_string .. "' | " .. command) Would this be an option for this use case?

See the function run_shell_cmd in https://git.sr.ht/~mcepl/vis-par/tree/master/item/init.lua#L50 … with better understanding of vis:pipe I have right, I could probably make it slightly less complicated (and, of course, patches are more than welcome), but I think this whole juggling should be done by vis (or vis provided Lua functions) itself. Why only Python people should have actually working subprocess.Pipe function (yes, I am a Python packager by day job)?

[jorbakk]

Ok this is making more sense to me now. But for your use case vis:command("!nnn [options]")should work right? (I only briefly looked at your vis-nnn plugin.) From what I can tell right now it only trashes the terminal after exiting vis. Then a patch similar to #1105 will fix that issue (I'm just not sure #1105 is the correct way of going about it).

Yes, you're right, it works but trashes the terminal after exiting vis, which makes it rather unusable IMHO. If there's any better way to achieve this than with #1105, I would be happy! But otherwise, I don't think #1105 is too intrusive, but your mileage may vary.

[jorbakk]

See the function run_shell_cmd in https://git.sr.ht/~mcepl/vis-par/tree/master/item/init.lua#L50 … with better understanding of vis:pipe I have right, I could probably make it slightly less complicated (and, of course, patches are more than welcome), but I think this whole juggling should be done by vis (or vis provided Lua functions) itself. Why only Python people should have actually working subprocess.Pipe function (yes, I am a Python packager by day job)?

Agreed, a more convenient solution would be nicer. Thinking twice, there might be a way to handle it in vis-lua.c:pipe_func() without changing vis.c:vis_pipe(). I had a look at how vim handles that (as you mentioned the system() call) and there the string is written to a file first. We could create a temporary file with vis.c:file_new_internal() and then proceed as with a File/Range. I agree with @rnpnr that this might be better handled in a third patch to vis:pipe() and am willing to give it a try, later. But first, we might sort out if any of the previous two patches to vis:pipe() will be accepted and then go from there.

[rnpnr]

This one can be merged without vis:pipe(cmd) being a special case if you want to make that fix and squash the commits.

Then we can work on #1105. If we find there that we need some special case for vis:pipe() it can be reintroduced. I suspect that it is not needed and the curses fix can be done in a better way.

[jorbakk]

Okay, I made the fix and set nil, nil as an alias for win.file, {start = 0, finish = 0} for the Range and File params ... not sure though if I got the squashing right. For indicating an invalid range in the Lua API such that vis is not collecting output, I now use win.file, {start = 1, finish = 0}.

[rnpnr]

For indicating an invalid range in the Lua API

I still don't understand why you need to do that. If you are trying to run a command where you are not collecting the output or error streams, as the original change indicated, then you should just use vis:command("! " .. cmd) which boils down to calling vis_pipe() with an invalid range.

not sure though if I got the squashing right

Not quite but I can fix it. I also meant to keep vis:pipe(cmd) as a valid call but just treat it the same as vis:pipe(nil, nil, cmd). I believe the following diff is correct:

--- a/vis-lua.c
+++ b/vis-lua.c
@@ -1325,10 +1325,11 @@ static int exit_func(lua_State *L) {
  * Pipe file range to external process and collect output.
  *
  * The editor core will be blocked while the external process is running.
+ * File and Range can be omitted or nil to indicate empty input.
  *
  * @function pipe
- * @tparam File file the file to which the range applies
- * @tparam Range range the range to pipe
+ * @tparam[opt] File file the file to which the range applies
+ * @tparam[opt] Range range the range to pipe
  * @tparam string command the command to execute
  * @treturn int code the exit status of the executed command
  * @treturn string stdout the data written to stdout
@@ -1336,10 +1337,17 @@ static int exit_func(lua_State *L) {
  */
 static int pipe_func(lua_State *L) {
    Vis *vis = obj_ref_check(L, 1, "vis");
-   File *file = obj_ref_check(L, 2, VIS_LUA_TYPE_FILE);
-   Filerange range = getrange(L, 3);
-   const char *cmd = luaL_checkstring(L, 4);
+   int cmd_idx = 4;
    char *out = NULL, *err = NULL;
+   File *file = vis->win->file;
+   Filerange range = text_range_new(0, 0);
+   if (lua_gettop(L) <= 3) {
+       cmd_idx = 2;
+   } else if (!(lua_isnil(L, 2) && lua_isnil(L, 3))) {
+       file = obj_ref_check(L, 2, VIS_LUA_TYPE_FILE);
+       range = getrange(L, 3);
+   }
+   const char *cmd = luaL_checkstring(L, cmd_idx);
    int status = vis_pipe_collect(vis, file, &range, (const char*[]){ cmd, NULL }, &out, &err);
    lua_pushinteger(L, status);
    if (out)

[jorbakk]

I still don't understand why you need to do that. If you are trying to run a command where you are not collecting the output or error streams, as the original change indicated, then you should just use vis:command("! " .. cmd) which boils down to calling vis_pipe() with an invalid range.

... no worries, it only makes sense in combination with having the fullscreen parameter in vis:pipe(). Sorry for the confusion.

Not quite but I can fix it. I also meant to keep vis:pipe(cmd) as a valid call but just treat it the same as vis:pipe(nil, nil, cmd). I believe the following diff is correct:

Yes, looks good to me. Thanks for fixing!

[rnpnr]

Merged, thanks for your help addressing the issues!