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

Access to jump history from commands #1277

Closed
j-xella opened this issue May 31, 2023 · 43 comments · Fixed by #1384
Closed

Access to jump history from commands #1277

j-xella opened this issue May 31, 2023 · 43 comments · Fixed by #1384

Comments

@j-xella
Copy link

j-xella commented May 31, 2023

Is it possible to access jump history, similar to what jumps command shows, from ${{ lf commands }} ?

I am trying to write a function that would feed the jump history to fzf, and then go to the selected folder. However, I am currently stuck with trying to get this list somehow...

There is a whole range of lf commands that produce multiple lines of output (jumps, maps). Ideally, it would be nice to have some generic way to query lf with lf --remote command, so that the output of such commands would be printed to stdout by lf --remote. For example, lf --remote "pipe $id jumps" | grep ....

@j-xella
Copy link
Author

j-xella commented May 31, 2023

Another option could be to make the list commands accept a file name argument which, if provided, would make them dump the output to that file instead of piping it via less.

@ilyagr
Copy link
Collaborator

ilyagr commented Jun 1, 2023

I've been thinking about doing something like this on and off.

I think the solution of adding an argument to maps, jumps, etc is reasonable, but I'd prefer to ideally also solve the problem of configuring maps to pipe to a program other than $PAGER. (In my case, PAGER is less -FR which is good outside lf, but is problematic when jumps take up less than one screen.)

My favorite approach at the moment is the following, but I'm wondering if people think it's too complicated.

  • Create a pipe_state_to_cmd command that could be used as pipe_state_to_cmd jumps less -R.
  • Replace the built-in maps, jumps and the like with default mappings: cmd jumps pipe_state_to_cmd maps "$PAGER" and so on. The advantage of this is that the user can remap cmd jumps pipe_state_to_cmd jumps less -R

We could also have a pipe_state_to_file command (in which case the above command could be simply pipe_state) or this could be achieved with shell commands. I'm not 100% shell commands would be sufficient on Windows.

If that's too complicated, the two problems could also be tackled separately.

@joelim-work
Copy link
Collaborator

This is something I have also thought about. However, the reason why this kind of feature is currently not implemented is because the lf server uses a very simple protocol where it forwards requests to the corresponding instance, but does not wait for any response.

If we were to implement this, I would prefer to keep the lf -remote "pipe $id jumps" semantics though - it's very convenient as you can just include it directly in a ${{ }} shell command, and also pipe the result into another program or redirect it to a file, etc.

I think it would be possible to enhance the current protocol to allow for request/response communication, for example lf -remote would send req $id jumps (perhaps it would need to provide its own ID so the server knows who to forward the response back to), and the main instance would respond with resp $remote_id <data>, which would be sent back to the remote instance via the server.

I am slightly worried that it would add too much complexity to the current protocol though, might need some more time to consider.

@joelim-work
Copy link
Collaborator

Actually never mind, I realised that if you make lf -remote "pipe $id jumps" synchronous and run it inside a ${{ }} shell command, the fact that lf -remote blocks will actually prevent event loop from resolving the jumps request. Running it inside an asynchronous &{{ }} shell command also won't work if you want to display the results.

I think a synchronous lf -remote won't work then, and perhaps we can provide an alternate pipe command that can take arguments indicating what to output and where to pipe it to.

@j-xella
Copy link
Author

j-xella commented Jun 1, 2023

My favorite approach at the moment is the following, but I'm wondering if people think it's too complicated.

  • Create a pipe_state_to_cmd command that could be used as pipe_state_to_cmd jumps less -R.
  • Replace the built-in maps, jumps and the like with default mappings: cmd jumps pipe_state_to_cmd maps "$PAGER" and so on. The advantage of this is that the user can remap cmd jumps pipe_state_to_cmd jumps less -R

Just to be clear, that would be local internal lf commands, correct? And running them from the shell via lf -remote "send ..." would have no effect until the shell script is finished? So, (while not completely impossible) difficult to use from inside shell scripts?

Also, from what I understand, lf commands can have arbitrary number of arguments, so in pipe_state_to_cmd cmd arg1 arg2 less -R it is not easy to understand where arguments end and the cmd to pipe to begins. I would say, it would have to be

  • either
    • pipe_state_to_cmd "cmd arg1 arg2" less -R
    • pipe_state_to_file "cmd arg1 arg2" file_name
  • or something like
    • pipe_state cmd arg1 arg2 | less -R
    • pipe_state cmd arg1 arg2 > filename

@ilyagr
Copy link
Collaborator

ilyagr commented Jun 2, 2023

Just to be clear, that would be local internal lf commands, correct?

Correct.

And running them from the shell via lf -remote "send ..." would have no effect until the shell script is finished? So, (while not completely impossible) difficult to use from inside shell scripts?

The command would be executed in parallel with your shell script. You're right that this makes such features inconvenient to use from shell scripts. For example, if we had a pipe_state_to_file command, after sending it to lf via lf -remote you'd have to make the script wait until the file appears and is done being written to. Or, you could craft an lf command that does pipe_state_to_file and then runs $touch another_file to indicate that it is done (and have the script wait for the second file to appear).

A third option would be to split your script in two and use pipe_state_to_cmd instead to pipe the output to the second half of the script.

I'm not sure how much better we could do without making the lf server protocol more complicated.

Following the general direction of @joelim-work's thoughts, we could consider making the server protocol more complicated. I haven't looked into whether that's reasonable or feasible. For example, perhaps there could be a way to ask lf's server to send a command to some instance of lf and wait for it to finish before exiting (something like lf -remote "send-and-wait ..."). I think that would help, but I'm not sure what the cost of such a feature would be.

Also, from what I understand, lf commands can have arbitrary number of arguments

My original idea was to have a limited number of arguments that could be passed to pipe_state_to_file; something like "maps", "cmaps", "jumps", "cmds", and that's it. They are not commands, just arguments.

Alternatively, we could have built-in commands named internal-maps, internal-cmaps, etc., that could be called as internal-maps > file or internal-maps | file. Then, we'd have the default alias cmd maps internal-maps that the user could replace with something like cmd maps internal-maps | less -R.

This alternative idea could be confusing in a similar way, since the first argument to the command has to be | or >. This is made to look like shell syntax, but it is not passed to the shell at all.

Perhaps a better yet idea on the syntax will come to somebody's mind.

@joelim-work
Copy link
Collaborator

Following the general direction of @joelim-work's thoughts, we could consider making the server protocol more complicated. I haven't looked into whether that's reasonable or feasible. For example, perhaps there could be a way to ask lf's server to send a command to some instance of lf and wait for it to finish before exiting (something like lf -remote "send-and-wait ..."). I think that would help, but I'm not sure what the cost of such a feature would be.

To be clear, I pretty much gave up on the ${{ lf -remote "send-and-wait <command>" }} approach, because the shell script needs to wait for the command to be executed in order to continue with the result, but actually the command won't execute until the shell script finishes as they both run on the main event loop.

A simple way to demonstrate this is to run $lf -remote "send $id INVALID"; sleep 5 with logging enabled:

  1. Synchronous shell commands (with a $ prefix) are run on the main loop, and therefore prevent other commands from being executed
  2. lf -remote sends the command INVALID to the main instance and terminates instantly.
  3. The command is received by the main instance in a separate thread. This thread has only one responsibility which is to read data from the server, however it doesn't actually process the data and instead forwards it to the main thread to be evaluated.
  4. Meanwhile the shell command sleeps for 5 seconds, and during this time the main thread is blocked from processing the INVALID command.
  5. Finally the shell command terminates, allowing the main thread to continue and (fail to) process the INVALID command, which will be recorded in the log file.

Usually the above sequence of steps is not a problem, because lf -remote is only used at the end of a shell script to send some updates.


Regarding internal commands, I was actually thinking of having a syntax similar to map or cmd like this:

pipe jumps ${{
    grep foobar | less -R
}}

This allows the user to decide on the type of shell command, and also doesn't add too much complexity to the current parsing rules. But I have not tested this yet so I'm not sure how feasible it is.

@joelim-work
Copy link
Collaborator

joelim-work commented Jun 2, 2023

I was working on a proof of concept for this, which defines a new command type pipeExpr:

type mapExpr struct {
	keys string
	expr expr
}

type cmdExpr struct {
	name string
	expr expr
}

type execExpr struct {
	prefix string
	value  string
}

// NEWLY ADDED
type pipeExpr struct {
	data string
	exec execExpr
}

Below is the diff, which contains code that was written quickly, and could possibly be refined further.

Click to show diff
diff --git a/app.go b/app.go
index f05a7a3..60836cd 100644
--- a/app.go
+++ b/app.go
@@ -500,32 +500,36 @@ func (app *app) runCmdSync(cmd *exec.Cmd, pause_after bool) {
 	app.nav.renew()
 }
 
 // This function is used to run a shell command. Modes are as follows:
 //
 //	Prefix  Wait  Async  Stdin  Stdout  Stderr  UI action
 //	$       No    No     Yes    Yes     Yes     Pause and then resume
 //	%       No    No     Yes    Yes     Yes     Statline for input/output
 //	!       Yes   No     Yes    Yes     Yes     Pause and then resume
 //	&       No    Yes    No     No      No      Do nothing
-func (app *app) runShell(s string, args []string, prefix string) {
+func (app *app) runShell(s string, args []string, prefix string, stdin io.Reader) {
 	app.nav.exportFiles()
 	app.ui.exportSizes()
 	exportOpts()
 
 	cmd := shellCommand(s, args)
 
 	var out io.Reader
 	var err error
 	switch prefix {
 	case "$", "!":
-		cmd.Stdin = os.Stdin
+		if stdin != nil {
+			cmd.Stdin = stdin
+		} else {
+			cmd.Stdin = os.Stdin
+		}
 		cmd.Stdout = os.Stdout
 		cmd.Stderr = os.Stderr
 
 		app.runCmdSync(cmd, prefix == "!")
 		return
 	}
 
 	// We are running the command asynchroniously
 	if prefix == "%" {
 		if app.ui.cmdPrefix == ">" {
@@ -537,20 +541,24 @@ func (app *app) runShell(s string, args []string, prefix string) {
 		}
 		app.cmdIn = stdin
 		stdout, err := cmd.StdoutPipe()
 		if err != nil {
 			log.Printf("reading stdout: %s", err)
 		}
 		out = stdout
 		cmd.Stderr = cmd.Stdout
 	}
 
+	if prefix == "&" && stdin != nil {
+		cmd.Stdin = stdin
+	}
+
 	shellSetPG(cmd)
 	if err = cmd.Start(); err != nil {
 		app.ui.echoerrf("running shell: %s", err)
 	}
 
 	// Asynchronous shell invocations return immediately without waiting for the
 	// command to finish, so there is no point refreshing the preview if nothing
 	// has changed yet.
 	volatile := prefix != "&"
 	app.ui.loadFile(app, volatile)
diff --git a/eval.go b/eval.go
index 0ddf107..a449fc0 100644
--- a/eval.go
+++ b/eval.go
@@ -2161,38 +2161,38 @@ func (e *callExpr) eval(app *app, args []string) {
 			for p.parse() {
 				p.expr.eval(app, nil)
 			}
 			if p.err != nil {
 				app.ui.echoerrf("%s", p.err)
 			}
 		case "$":
 			log.Printf("shell: %s", s)
 			app.ui.cmdPrefix = ""
 			app.cmdHistory = append(app.cmdHistory, cmdItem{"$", s})
-			app.runShell(s, nil, "$")
+			app.runShell(s, nil, "$", nil)
 		case "%":
 			log.Printf("shell-pipe: %s", s)
 			app.cmdHistory = append(app.cmdHistory, cmdItem{"%", s})
-			app.runShell(s, nil, "%")
+			app.runShell(s, nil, "%", nil)
 		case ">":
 			io.WriteString(app.cmdIn, s+"\n")
 			app.cmdOutBuf = nil
 		case "!":
 			log.Printf("shell-wait: %s", s)
 			app.ui.cmdPrefix = ""
 			app.cmdHistory = append(app.cmdHistory, cmdItem{"!", s})
-			app.runShell(s, nil, "!")
+			app.runShell(s, nil, "!", nil)
 		case "&":
 			log.Printf("shell-async: %s", s)
 			app.ui.cmdPrefix = ""
 			app.cmdHistory = append(app.cmdHistory, cmdItem{"&", s})
-			app.runShell(s, nil, "&")
+			app.runShell(s, nil, "&", nil)
 		case "/":
 			dir := app.nav.currDir()
 			old := dir.ind
 			if gOpts.incsearch {
 				dir.ind = app.nav.searchInd
 				dir.pos = app.nav.searchPos
 			}
 			log.Printf("search: %s", s)
 			app.ui.cmdPrefix = ""
 			app.nav.search = s
@@ -2538,32 +2538,46 @@ func (e *callExpr) eval(app *app, args []string) {
 			return
 		}
 		cmd.eval(app, e.args)
 	}
 }
 
 func (e *execExpr) eval(app *app, args []string) {
 	switch e.prefix {
 	case "$":
 		log.Printf("shell: %s -- %s", e, args)
-		app.runShell(e.value, args, e.prefix)
+		app.runShell(e.value, args, e.prefix, nil)
 	case "%":
 		log.Printf("shell-pipe: %s -- %s", e, args)
-		app.runShell(e.value, args, e.prefix)
+		app.runShell(e.value, args, e.prefix, nil)
 	case "!":
 		log.Printf("shell-wait: %s -- %s", e, args)
-		app.runShell(e.value, args, e.prefix)
+		app.runShell(e.value, args, e.prefix, nil)
 	case "&":
 		log.Printf("shell-async: %s -- %s", e, args)
-		app.runShell(e.value, args, e.prefix)
+		app.runShell(e.value, args, e.prefix, nil)
 	default:
 		log.Printf("evaluating unknown execution prefix: %q", e.prefix)
 	}
 }
 
+func (e *pipeExpr) eval(app *app, args []string) {
+	var stdin io.Reader
+	switch e.data {
+	case "maps":
+		stdin = listBinds(gOpts.keys)
+	case "cmaps":
+		stdin = listBinds(gOpts.cmdkeys)
+	case "jumps":
+		stdin = listJumps(app.nav.jumpList, app.nav.jumpListInd)
+	}
+
+	app.runShell(e.exec.value, args, e.exec.prefix, stdin)
+}
+
 func (e *listExpr) eval(app *app, args []string) {
 	for i := 0; i < e.count; i++ {
 		for _, expr := range e.exprs {
 			expr.eval(app, nil)
 		}
 	}
 }
diff --git a/parse.go b/parse.go
index e6abeb0..4340457 100644
--- a/parse.go
+++ b/parse.go
@@ -105,20 +105,29 @@ func (e *execExpr) String() string {
 		}
 
 		break
 	}
 
 	buf.WriteString(" }}")
 
 	return buf.String()
 }
 
+type pipeExpr struct {
+	data string
+	exec execExpr
+}
+
+func (e *pipeExpr) String() string {
+	return fmt.Sprintf("%s | %s", e.data, e.exec.String())
+}
+
 type listExpr struct {
 	exprs []expr
 	count int
 }
 
 func (e *listExpr) String() string {
 	var buf bytes.Buffer
 
 	buf.WriteString(":{{ ")
 
@@ -211,20 +220,38 @@ func (p *parser) parseExpr() expr {
 			name := s.tok
 
 			s.scan()
 			if s.typ != tokenSemicolon {
 				expr = p.parseExpr()
 			} else {
 				s.scan()
 			}
 
 			result = &cmdExpr{name, expr}
+		case "pipe":
+			var expr expr
+
+			s.scan()
+			data := s.tok
+
+			s.scan()
+			if s.typ != tokenSemicolon {
+				expr = p.parseExpr()
+			} else {
+				s.scan()
+			}
+
+			if exec, ok := expr.(*execExpr); ok {
+				result = &pipeExpr{data, *exec}
+			} else {
+				p.err = fmt.Errorf("expected shell command '%s'", expr.String())
+			}
 		default:
 			name := s.tok
 
 			var args []string
 			for s.scan() && s.typ != tokenSemicolon {
 				args = append(args, s.tok)
 			}
 
 			s.scan()
 

And now it's possible to write configurations like below. I don't know if there's any real use case for other shell prefixes (i.e. !, % and &), but I suppose it's nice to have support for it.

cmd ctrlmaps pipe maps ${{
    grep '^<c-' | less
}}

@j-xella
Copy link
Author

j-xella commented Jun 2, 2023

cmd ctrlmaps pipe maps ${{
    grep '^<c-' | less
}}

I wonder, if this approach goes live, it would be a good idea to redefine existing list commands to something like this:

  • Before
  • jumps

  • After
  • jumps -> list_jumps (or similar name), printing its contents to the bottom statline, like shell-pipe or echo

  • cmd jumps pipe list_jumps $ $PAGER

@joelim-work
Copy link
Collaborator

jumps -> list_jumps (or similar name), printing its contents to the bottom statline, like shell-pipe or echo

I'm not sure how useful printing to the bottom statline via shell-pipe would be, since it can only display a single line (the last line printed), but I guess it's possible to implement this so that shell-pipe reads from the specified pipe contents if provided, and defaults to standard input if not provided.

Also the identifier after pipe is just a name indicating what is to be sent as input, it doesn't matter if it shares the name with other things like commands.

cmd jumps pipe list_jumps $ $PAGER

Yes, that's the idea of the proposed syntax - this defines a command called jumps which pipes the jump history into a shell command ($PAGER in this case). I think it would be better to provide this mapping by default (along with maps and cmaps) so that users wouldn't have to configure it.

@joelim-work
Copy link
Collaborator

One example application is that I have successfully found a way to use pipe maps to delete every keybinding as discussed in #1161:

cmd clearmaps pipe maps ${{
    awk 'NR >= 2 { printf "map '\''%s'\''; ", $1 }' |
    sed "s/'''/\"'\"/" |
    xargs -0 -I{} lf -remote "send $id :{}"
}}

Although I think it would be better to have a builtin command for this. In any case I think users will come up with applications for this pipe that are beyond the scope of my imagination.

@ilyagr
Copy link
Collaborator

ilyagr commented Jun 3, 2023

To be clear, I pretty much gave up on the ${{ lf -remote "send-and-wait " }} approach, because the shell script needs to wait for the command to be executed in order to continue with the result, but actually the command won't execute until the shell script finishes as they both run on the main event loop.

Ah, I didn't initially understand is. Your point is that if lf executes a shell script in the $ or ! mode, and that script executed lf -remote "send $id some_command" to ask the same instance of lf to execute something, some_command would only be executed after the script completes because the event loop is blocked until then.

So, @j-xella was actually correct when they said

running them from the shell via lf -remote "send ..." would have no effect until the shell script is finished?


I'm still looking over the other interesting things that were said.

@gokcehan
Copy link
Owner

gokcehan commented Jun 4, 2023

I don't have much specifics to add but all this discussion suggests me that maybe we should have preferred the environment variable approach discussed in #918 instead. Wouldn't that have made this possible with something like the following:

lf -remote "send $id cd $(printf $lf_jumps | ... | fzf | ...)"

I can't say I liked reading through the solutions proposed here as it felt too complicated for my taste in general. So far, I think our general way of passing information to shell commands was to use environment variables. We are now talking about other ways for a similar purpose. So if this is the direction we decide to take, I think it should be more strongly justified somehow.

@joelim-work
Copy link
Collaborator

The use of environment variables is simple, and consistent with passing other data like options and selected files. The problem is that environment variables don't work with large amounts of data due to their size limits as pointed out in #918. I do acknowledge that the pipe suggestion I had can be complex, but I don't know if there's a better way to define what gets send to stdin if we choose that route.

While I think environment variables are fine for exporting maps and cmaps, the jump list can grow very big (it wasn't difficult for me to trigger the error running shell: fork/exec /usr/sbin/sh: argument list too long by using cd repeatedly). I think if we can cap the size of the jump list then the use of environment variables would be OK.

@joelim-work
Copy link
Collaborator

I can't remember if I have said this before or not, but while I support the idea of having lf being able to provide information like maps etc., I do find it somewhat restricting that it is limited to being displayed in a pager. For instance I have found that there's a bug in Windows where the pager quits immediately if there isn't enough text fill a page (behaviour of the more pager) because runPagerOn unconditionally uses a $ command.

@ilyagr
Copy link
Collaborator

ilyagr commented Jun 5, 2023

This is perhaps a minor issue, on Linux at least, but I'm not very comfortable with environment variables containing newlines. It's an area of shell peculiarities I never learned. I'm not immediately sure which of the following will work:

echo $lf_maps   | less
echo "$lf_maps" | less
printf "%s\n" "$lf_maps" | less
IFS="\n" echo $lf_maps | less  # lf's default `maps` command should not depend on the value of $IFS though
export IFS="\n"; echo "$lf_maps" | less

On Windows, this could be outright impossible. Do either of you know if it's doable? Windows also has a low limit on the length of a single cmd.exe command, and I'm not sure whether that limit is applied before or after expanding the environment variables.

If we find a way to deal with this on Windows, or decide to not do this on Windows, I think this will be fine.


For the issue of jumplist being long, I suppose we could just cut it off at some number of entries. Or are the limits too small for this to work, @joelim-work?


Other than that, my main objection to the environment variables approach is unsightliness in the output to set, but I'm beginning to think that maybe that's not too bad.

The set from sh on my Debian bullseye machine (it has dash) already has trouble because of ANSI escape sequences in the variables lf exports like $lf_errorfmt. It's my own fault, by the way, and I stand by it :).

The set from bash on my machine includes a bunch of shell functions that are very long. It also displays newlines as "\n" making the output more compact.

The set from fish truncates all long variables.

I don't have cmd.exe available at the moment to try its set.

@j-xella
Copy link
Author

j-xella commented Jun 5, 2023

If environment variables are too restrictive, why not

  1. Create a temporary folder
  2. Populate it with many files, containing as much info as possible (maps.list, jumps.list, options.list, ...). The file names and contents would be standardised.
  3. Pass the path to that temporary folder to the cmd script in an environment variable
  4. Delete the folder and its contents once the cmd execution is finished.

A bonus of this approach would be that each cmd script would have its own temporary folder to store files, on any OS lf is running.

@j-xella
Copy link
Author

j-xella commented Jun 5, 2023

Also the identifier after pipe is just a name indicating what is to be sent as input, it doesn't matter if it shares the name with other things like commands.

This is the part I misunderstood. I thought that "pipe" would accept ANY lf command that produces output, and redirect it to a script via stdin. Then it seemed natural to me to redirect the output that otherwise would appear on status line.

@joelim-work
Copy link
Collaborator

Regarding environment variables, I'm not really familiar with the intricacies such as support for newline characters. TBH I have always seen environment variables as something you would use for providing configuration values, not as a means of passing arbitrary data for once-off purposes.

Temp files were also discussed as a possible solution in #918. For some reason (especially in #918 (comment)), this idea was rejected, but I think it is better than using environment variables. Actually I think I like it better than my jumps proposal because the data can be accessed at any time during the execution of the script, and instead of being piped to the first command.

I should probably also mention that some of the examples in the wiki Tips use temporary files like ~/.local/share/lf/files to access the list of files being cut/copied, and I wouldn't be surprised if there are users who do the same for the marks/tags/history files as well.

Creating a separate directory to isolate each instance of lf certainly makes a lot of sense, maybe it's possible to use the $id as part of the name.

@ilyagr
Copy link
Collaborator

ilyagr commented Jun 5, 2023

One more minor thought about environment variables: one way to avoid the issues I described with Windows etc would be to create a special option to the lf binary along the lines of

lf -print-env var

It would simply print the value of var to stdout and exit. This would avoid any differences between shells and should work on Windows.

The cost is that this makes lf do work that the shell should do which, conceptually, is not its job. We'd have to decide if this situation is important enough to make an exception to that principle.

@j-xella
Copy link
Author

j-xella commented Jun 5, 2023

TBH I have always seen environment variables as something you would use for providing configuration values, not as a means of passing arbitrary data for once-off purposes.

That. +

Not to mention that slicing and dicing of environment variables is not always convenient.

Not to mention that environment variables are all in the same namespace, so the more variables you rely upon, the higher the chance of a clash with some other systems.

It can not be taken for granted that they are "limitless" in size. For example, on Windows, the whole environment block of a process is just 32K. That is all variable names + all values + separators in-between. Is this what lf is relying upon to pass the amounts of data of potentially unlimited size?

IMO, isn't that a strong justification enough to consider other ways of passing data to subcommands?

@gokcehan
Copy link
Owner

gokcehan commented Jun 6, 2023

Sorry for disappearing after my previous comment. Let me drop a quick reply so I won't stall the discussion further.

I understand that environment variables can be limiting at times. I also agree they are strange when used to hold information consisting of multiple lines. I guess temporary file approach would be the most reasonable way for further development then.

By the way, I wasn't aware environment variable limits can be so low. I wonder if this could potentially be a problem for selection as well (e.g. go into a crowded folder and then invert so $fs and $fx can exceed the limit?).

So far, I think there were two main reasons to use files (i.e. selection, marks, tags, history), shared state and persistence. Jump list discussed here would be different in that regard. Having a separate temporary directory for each instance sounds reasonable. I guess this would conceptually resemble /proc filesystem in linux to expose information.

By the way, maybe it is worth mentioning, selection was once stored in server memory, but then it was changed to a file in ec330c1 as part of #177 for easier scripting.

However, one thing I might add here is that maybe the use of such temporary files/directories should be controlled with an option. For example, we had a request for an option to disable history for privacy reasons before in #853 . There might also be some slight performance concerns if these files are to be created/modified for each command. Even if there isn't a noticable performance slowdown, I would personally like to be able to disable this myself as I find it strange for some reason to create multiple files/directories for each command.

@joelim-work
Copy link
Collaborator

I wonder if this could potentially be a problem for selection as well (e.g. go into a crowded folder and then invert so $fs and $fx can exceed the limit?).

It is potential problem, for me touch foobar_{1..100000} and then selecting everything already exceeds the limit, and makes it impossible to start any kind of shell command. But I think it's not worth changing since it's too large of a breaking change, and the scenario is unlikely to happen during normal usage.

Having a separate temporary directory for each instance sounds reasonable. I guess this would conceptually resemble /proc filesystem in linux to expose information.

Yes, the /proc filesystem was the inspiration behind that idea, so the user can type something like cat /tmp/lf/$id/jumps. I'm still not 100% sure on the exact implementation, for instance whether the files should be cleaned up after the command finishes, or after lf exits. Alternatively we could just go with the original suggestion and use os.MkdirTemp to create a random directory and export it as an environment variable so the user can type something like cat $lf_data/jumps.

However, one thing I might add here is that maybe the use of such temporary files/directories should be controlled with an option. For example, we had a request for an option to disable history for privacy reasons before in #853 . There might also be some slight performance concerns if these files are to be created/modified for each command. Even if there isn't a noticable performance slowdown, I would personally like to be able to disable this myself as I find it strange for some reason to create multiple files/directories for each command.

We can add an option like exportdata/datafiles (feel free to suggest a better name) to enable this behaviour, and perhaps we can make it disabled by default so users have to opt-in. Regarding performance, I did some very basic testing on my setup and I didn't notice any slowdown. Also I agree that it is a bit strange to export data into multiple files each and every time you run a shell command, but I don't think there's any good solution since you can't anticipate what data the user is interested in, well not unless you want to revisit the pipe stdin approach.

@j-xella
Copy link
Author

j-xella commented Jun 7, 2023

Yes, the /proc filesystem was the inspiration behind that idea, so the user can type something like cat /tmp/lf/$id/jumps. I'm still not 100% sure on the exact implementation, for instance whether the files should be cleaned up after the command finishes, or after lf exits. Alternatively we could just go with the original suggestion and use os.MkdirTemp to create a random directory and export it as an environment variable so the user can type something like cat $lf_data/jumps.

I would advocate for os.MkdirTemp . There were many discussions why it is the best approach, but basically it is more secure and lower chance of a folder name clash.

Second, I would advocate for recreating/deleting temp folder for each ${{ command }} run, rather than keeping one folder for the duration of lf process. Here are my reasons:

  1. Maybe it is a bit slower, but speed is not the issue here - we are responding to user actions
  2. As the folders are quickly deleted, general amount of used disk space at any time will be lower
  3. It is simpler to implement - lf just creates the files once, no need to maintain them in good state
  4. Convenience for user - as long as he is using the temp folder, he does not have to care about tidying up.
  5. Isolation - if a user command messes with the files in the temp folder, it will have no lasting effect

Also, just a suggestion: there are currently 4 ways to invoke the shell: $, !, % and &. Another way to control whether temp folder is created or not could be with modified shell invocations. For example, ${{ command }} works as now, but $>{{ command }} also creates the temporary folder.

@joelim-work
Copy link
Collaborator

OK I'm convinced having a separate temp directory for shell invocation is better, thanks for the feedback. I also agree that the implementation would be simpler as the lifetime is scoped to just the shell invocation itself, so that there is no need to manage the state for the duration of the lf instance running.

Apart from exposing a config setting, I can't really think of any better way to control whether the temp directory is created or not without making changes to the command parsing logic. I have written a very rough implementation below:

Click to show diff
diff --git a/eval_test.go b/eval_test.go
index 48cb445..5671759 100644
--- a/eval_test.go
+++ b/eval_test.go
@@ -283,27 +283,45 @@ var gEvalTests = []struct {
 
 	{
 		"map r :push :rename<space> ; set hidden",
 		[]string{"map", "r", ":", "push", ":rename<space>", ";", "set", "hidden", "\n", "\n"},
 		[]expr{&mapExpr{"r", &listExpr{[]expr{&callExpr{"push", []string{":rename<space>"}, 1}, &setExpr{"hidden", ""}}, 1}}},
 	},
 
 	{
 		"map u $du -h . | less",
 		[]string{"map", "u", "$", "du -h . | less", "\n"},
-		[]expr{&mapExpr{"u", &execExpr{"$", "du -h . | less"}}},
+		[]expr{&mapExpr{"u", &execExpr{"$", false, "du -h . | less"}}},
 	},
 
 	{
 		"cmd usage $du -h $1 | less",
 		[]string{"cmd", "usage", "$", "du -h $1 | less", "\n"},
-		[]expr{&cmdExpr{"usage", &execExpr{"$", "du -h $1 | less"}}},
+		[]expr{&cmdExpr{"usage", &execExpr{"$", false, "du -h $1 | less"}}},
+	},
+
+	{
+		`cmd maps $> $PAGER "$lf_data/maps"`,
+		[]string{"cmd", "maps", "$", ">", `$PAGER "$lf_data/maps"`, "\n"},
+		[]expr{&cmdExpr{"maps", &execExpr{"$", true, `$PAGER "$lf_data/maps"`}}},
+	},
+
+	{
+		`cmd maps $>{{
+			$PAGER "$lf_data/maps"
+		}}`,
+		[]string{"cmd", "maps", "$", ">", "{{", `
+			$PAGER "$lf_data/maps"
+		`, "}}", "\n"},
+		[]expr{&cmdExpr{"maps", &execExpr{"$", true, `
+			$PAGER "$lf_data/maps"
+		`}}},
 	},
 
 	{
 		"map u usage /",
 		[]string{"map", "u", "usage", "/", "\n"},
 		[]expr{&mapExpr{"u", &callExpr{"usage", []string{"/"}, 1}}},
 	},
 
 	{
 		"map ss :set sortby size; set info size",
diff --git a/parse.go b/parse.go
index e6abeb0..9aeac0d 100644
--- a/parse.go
+++ b/parse.go
@@ -74,20 +74,21 @@ func (e *cmdExpr) String() string { return fmt.Sprintf("cmd %s %s", e.name, e.ex
 type callExpr struct {
 	name  string
 	args  []string
 	count int
 }
 
 func (e *callExpr) String() string { return fmt.Sprintf("%s -- %s", e.name, e.args) }
 
 type execExpr struct {
 	prefix string
+	export bool
 	value  string
 }
 
 func (e *execExpr) String() string {
 	var buf bytes.Buffer
 
 	buf.WriteString(e.prefix)
 	buf.WriteString("{{ ")
 
 	lines := strings.Split(e.value, "\n")
@@ -260,34 +261,40 @@ func (p *parser) parseExpr() expr {
 			}
 		}
 
 		s.scan()
 
 		result = &listExpr{exprs, 1}
 	case tokenPrefix:
 		var expr string
 
 		prefix := s.tok
+		export := false
 
 		s.scan()
+		if s.typ == tokenRAngle {
+			export = true
+			s.scan()
+		}
+
 		if s.typ == tokenLBraces {
 			s.scan()
 			expr = s.tok
 			s.scan()
 		} else {
 			expr = s.tok
 		}
 
 		s.scan()
 		s.scan()
 
-		result = &execExpr{prefix, expr}
+		result = &execExpr{prefix, export, expr}
 	default:
 		p.err = fmt.Errorf("unexpected token: %s", s.tok)
 	}
 
 	return result
 }
 
 func (p *parser) parse() bool {
 	if p.expr = p.parseExpr(); p.expr == nil {
 		return false
diff --git a/scan.go b/scan.go
index beb2b5c..808d644 100644
--- a/scan.go
+++ b/scan.go
@@ -11,20 +11,21 @@ type tokenType byte
 const (
 	tokenEOF tokenType = iota
 	// no explicit keyword type
 	tokenIdent     // e.g. set, ratios, 1:2:3
 	tokenColon     // :
 	tokenPrefix    // $, %, !, &
 	tokenLBraces   // {{
 	tokenRBraces   // }}
 	tokenCommand   // in between a prefix to \n or between {{ and }}
 	tokenSemicolon // ;
+	tokenRAngle    // >
 	// comments are stripped
 )
 
 type scanner struct {
 	buf []byte    // input buffer
 	off int       // current offset in buf
 	chr byte      // current character in buf
 	sem bool      // insert semicolon
 	nln bool      // insert newline
 	eof bool      // buffer ended
@@ -151,20 +152,27 @@ scan:
 					s.cmd = false
 					return true
 				}
 			}
 		}
 
 		s.typ = tokenEOF
 		s.tok = "EOF"
 		return false
 	case s.cmd:
+		if !s.eof && s.chr == '>' {
+			s.next()
+			s.typ = tokenRAngle
+			s.tok = ">"
+			return true
+		}
+
 		for !s.eof && isSpace(s.chr) {
 			s.next()
 		}
 
 		if !s.eof && s.chr == '{' {
 			if s.peek() == '{' {
 				s.next()
 				s.next()
 				s.typ = tokenLBraces
 				s.tok = "{{"

The concept is similar to the pipes approach, where you define some kind of indicator in the command (> after the shell prefix) to control if any data is made available for the shell process to consume, and therefore changes need to be made to the parsing logic.

@gokcehan How open would you be to extending the command parsing logic to allow for specifying data to be exported to shell invocations? I understand the parsing logic is supposed to be quite stable already, and that you might not want to make any changes to it, but to me it makes sense to toggle the behaviour here as opposed to a config setting because the lifetime of the data is tied to the shell invocation.

@ilyagr
Copy link
Collaborator

ilyagr commented Jun 8, 2023

Re @joelim-work's patch in the previous comment, why couldn't we just check if the first letter of a command is > in runShell?

I find the !> syntax a little cryptic, but the only other idea I have at the moment is to have something intentionally ugly and long like __lf_export_state__ instead of >.

I haven't had time to think in-depth about these issues, but I personally don't have a problem with exporting data to files, especially if it's opt-in per command. I never really understood the downsides of using temporary files mentioned in #918 (comment). I just assumed we shouldn't use them after that.

Instead of some clever directory structure, +1 on just using os.MkdirTemp and recreating things on each invocation as suggested in #1277 (comment). I think there should be a way to ask it to include "lf_$pid" in the name before or after the random string, but that'd be just for the convenience of the user. Another project I contribute to does that all the time, and it seems to work fine.

@joelim-work
Copy link
Collaborator

Re @joelim-work's patch in the previous comment, why couldn't we just check if the first letter of a command is > in runShell?

Correct me if I'm wrong, but I don't think that will work for multiline shell commands, because the command in the execExpr object contains only the contents in between the {{ ... }} brackets, and the > appears outside of that.

I find the !> syntax a little cryptic, but the only other idea I have at the moment is to have something intentionally ugly and long like __lf_export_state__ instead of >.

I think it's fine as long as it's documented. The problem with a long ugly string is that it makes it tedious to type in the command line, and asking users to create a push mapping to get around that is probably asking too much.

Instead of some clever directory structure, +1 on just using os.MkdirTemp and recreating things on each invocation as suggested in #1277 (comment). I think there should be a way to ask it to include "lf_$pid" in the name before or after the random string, but that'd be just for the convenience of the user. Another project I contribute to does that all the time, and it seems to work fine.

I guess we could add the ID into the temp directory pattern, but I don't know how much it matters - the idea is that upon creating the temp directory, it's location will be made available by an environment variable, which I have named lf_data (as usual, feel free to suggest better names), so you can just use that variable to find all the actual files.

@ilyagr
Copy link
Collaborator

ilyagr commented Jun 8, 2023

because the command in the execExpr object contains only the contents in between the {{ ... }} brackets

Good point. OTOH, we could have {{> ... }}.

The problem with a long ugly string is that it makes it tedious to type in the command line.

I was thinking people would mostly use this as part of a mapping or a cmd or in their config file. If there are usecases in the command-line, it could indeed be tedious.

I guess we could add the ID into the temp directory pattern

This is for users' convenience only. If they care about the differences in jumps between different instances of lf, they can check they are seeing the jumps from the right one by looking at the directory name for the file. This would probably mostly come up if someone is debugging a script that's not working correctly.

@j-xella
Copy link
Author

j-xella commented Jun 8, 2023

This is for users' convenience only. If they care about the differences in jumps between different instances of lf, they can check they are seeing the jumps from the right one by looking at the directory name for the file. This would probably mostly come up if someone is debugging a script that's not working correctly.

Ehm ... how will it help user, if the temp folder is deleted right after cmd is finished? I would be afraid to touch the temporary folders outside of the cmd execution, TBH. For debugging, a user would have to add something like this to his cmd anyway: cp -r $lf_data /temp/lf_cmd_$id

@j-xella
Copy link
Author

j-xella commented Jun 8, 2023

I find the !> syntax a little cryptic, but the only other idea I have at the moment is to have something intentionally ugly and long like __lf_export_state__ instead of >.

Is this more cryptic, than, say, using % ...? Just saying :) Obviously, that is another thing for the user to learn. FYI, I chose > because it is the stream redirection character that is usually used in shells to dump output to files...

@ilyagr
Copy link
Collaborator

ilyagr commented Jun 8, 2023

Ehm ... how will it help user, if the temp folder is deleted right after cmd is finished?

If you are debugging a script, you can add echo $lf_data into it. If you are using less, it can show you the filename.

In any case, this is a very minor issue, just a thought I found neat. We can skip it for now and then I can look into it after the initial PR. Let's not waste too much of our attention on it.

Is this more cryptic, than, say, using % ...? FYI, I chose > because it is the stream redirection character that is usually used in shells to dump output to files...

They are both cryptic, but the brevity of % is much more useful than the brevity of >. Also, an infrequently used cryptic symbol is more confusing than one that's used all the time (and therefore everybody learns).

If we are going with one cryptic symbol, I agree that > is a pretty good choice; I can't think of a better one.

@gokcehan
Copy link
Owner

gokcehan commented Jun 8, 2023

I never really understood the downsides of using temporary files mentioned in #918 (comment). I just assumed we shouldn't use them after that.

@ilyagr I think it mainly boils down to personal preferences but we also had some minor problems reported in the past. For example, temporary files can remain if you quit the program in an unusual way (e.g. #65, #292, #300). It is also not always straightforward to decide where to create temporary files (e.g. #722, #726). It may not be possible to use a volatile location that cleans up after a restart (e.g. Windows). It may not be possible to use a memory filesystem like tmpfs so you may end up with frequent disk writes (e.g. Windows). Note, creation of temporary files can fail in unexpected ways (e.g. os.TempDir states "The directory is neither guaranteed to exist nor have accessible permissions"). There are also privacy and security concerns when you make use of files (e.g. #722, #726, #853). This is why I had finally decided to remove temporary mechanism for log files (e.g. #739).

@gokcehan How open would you be to extending the command parsing logic to allow for specifying data to be exported to shell invocations? I understand the parsing logic is supposed to be quite stable already, and that you might not want to make any changes to it, but to me it makes sense to toggle the behaviour here as opposed to a config setting because the lifetime of the data is tied to the shell invocation.

@joelim-work I think I'm still not convinced that this feature is useful enough to introduce a new syntax. Jumplist already has a builtin way for interaction that should work for most users (e.g. jumps, jump-next, jump-prev). We are now talking about extending the configuration syntax to be able to skip this builtin mechanism altogether. As you know, every new addition comes with a cost, so I think we should be able to draw a line somewhere when a new addition requires a significant amount of complexity. I don't think it would be nice to end up as an experimental platform that people can test their various ideas.

Can you give more details why a configuration option is not a good solution for this? As a non-user of this feature, I would like to be able to disable this myself, but if you make use of this feature in some of your commands, why would you want to be able to disable this for others? So far, I mostly felt alone in this discussion for being a critic of temporary files.

@joelim-work
Copy link
Collaborator

@joelim-work I think I'm still not convinced that this feature is useful enough to introduce a new syntax. Jumplist already has a builtin way for interaction that should work for most users (e.g. jumps, jump-next, jump-prev). We are now talking about extending the configuration syntax to be able to skip this builtin mechanism altogether.

First off, I would like to say that the idea of having lf export its internal data for custom usage represents a significant and powerful addition for user configuration. This idea is not just about being able to use fzf on the jump list as described in the initial post, but is intended to support other use cases. For instance. this feature can completely replace the runPagerOn function which is currently being used for the maps/cmaps/jumps/cmds commands, while also allowing the user to harness the data for other purposes. Another use case is providing the list of files in the cut/copy buffer without exposing ~/.local/share/lf/files, which is supposed to be (?) for internal usage, as discussed in #1152 (comment).

As you know, every new addition comes with a cost, so I think we should be able to draw a line somewhere when a new addition requires a significant amount of complexity. I don't think it would be nice to end up as an experimental platform that people can test their various ideas.

I understand your concern here - lf contains a number of features (and the corresponding complexity in the code) that I personally have no interest in, but also cannot remove because other people use them, and I'm quite sure that the situation is the same for you. This is the reason why up until now I have only posted sample diffs instead of submitting an actual PR. In particular I'm not willing to work on a proper implementation for this without the agreement of others, due to the potential amount of rework that could be required.

Can you give more details why a configuration option is not a good solution for this? As a non-user of this feature, I would like to be able to disable this myself, but if you make use of this feature in some of your commands, why would you want to be able to disable this for others?

Just as you have said in #1277 (comment), to me it also feels strange to create these files each and every time you run a shell command. A configuration option will disable the feature globally, but can't distinguish between shell commands that require data to be exported and shell commands that don't (e.g. the default open command). Even then, you could argue that there is still some waste since multiple files are being created each time and it is unlikely that you would need all of them for any one command (e.g. the fzf jump list scenario only requires the jump list to be exported), and the suggested $> syntax doesn't handle this.

That being said, I think a configuration option is still acceptable, and method of controlling whether to export data or not can be revisited later.

So far, I mostly felt alone in this discussion for being a critic of temporary files.

I think the more accurate view is that everyone else saw temporary files as a better solution compared to using environment variables 🙂. I agree that temporary files have their drawbacks (unreliable cleanup, privacy concerns, unnecessary exports, etc.) and for this reason I prefer to pipe the data to stdin, but unfortunately I don't think this approach is possible unless you're willing to make some kind of change to the command parsing rules.

@gokcehan
Copy link
Owner

gokcehan commented Jun 9, 2023

@joelim-work I have given this more thoughts and I think I'm finally convinced a configuration syntax addition is necessary for this, so I would accept a patch if you decide to work on this.

First off, I would like to say that the idea of having lf export its internal data for custom usage represents a significant and powerful addition for user configuration. This idea is not just about being able to use fzf on the jump list as described in the initial post, but is intended to support other use cases. For instance. this feature can completely replace the runPagerOn function which is currently being used for the maps/cmaps/jumps/cmds commands, while also allowing the user to harness the data for other purposes. Another use case is providing the list of files in the cut/copy buffer without exposing ~/.local/share/lf/files, which is supposed to be (?) for internal usage, as discussed in #1152 (comment).

I'm glad you mentioned the other use cases because I wasn't aware you also have those in mind. If we want to convert maps/cmaps/jumps/cmds to make use of the temporary file mechanism discussed here, a configuration option to control the creation of temporary files may not be appropriate since it would break these builtin commands when disabled. Regarding the cut/copy file, I still don't see how it relates to this proposal, unless I'm missing something.

Temp files were also discussed as a possible solution in #918. For some reason (especially in #918 (comment)), this idea was rejected, but I think it is better than using environment variables. Actually I think I like it better than my jumps proposal because the data can be accessed at any time during the execution of the script, and instead of being piped to the first command.

...

I think the more accurate view is that everyone else saw temporary files as a better solution compared to using environment variables 🙂. I agree that temporary files have their drawbacks (unreliable cleanup, privacy concerns, unnecessary exports, etc.) and for this reason I prefer to pipe the data to stdin, but unfortunately I don't think this approach is possible unless you're willing to make some kind of change to the command parsing rules.

Now that we have the configuration syntax addition on the table, I'm at equal distances to the temporary file proposal and data pipe proposal. And with a high-level overview without going into much detail, I currently feel like data pipe approach might actually be better and cleaner than temporary files, though I will leave it for your judgement and the consensus of discussion here.

Finally, I should note that unfortunately I will disappear for a couple of weeks for vacation in the upcoming days and I probably won't be responsive. Feel free to further discuss and/or send patches in the meantime.

@joelim-work
Copy link
Collaborator

@gokcehan Thanks for your understanding and additional feedback, hope you have a nice relaxing break 🌴.


So after thinking about this some more, I think I very much prefer the pipe stdin approach that I originally suggested in #1277 (comment), because it has none of the drawbacks associated with creating temporary files, such as unreliable cleanup, privacy concerns, and unnecessary exports.

The only downside I can think of is that you can't pipe multiple types of data (e.g. maps and cmaps to the same shell process), but those situations are quite rare, and can still be solved by grouping commands (i.e. have sub-commands to write the data to temporary files, another sub-command to read from them, and then group everything into a single wrapper command).

I have written an implementation for this, which I might consider submitting as a draft PR later on. In addition to implementing the pipe command, the code also replaces the runPagerOn function as it is now rendered obsolete. I have also found that it may be useful to support piping the command history (the history file is only saved when quitting lf, so it isn't as useful), but this can be added later.

A sample pipe command which solves the initial issue looks like this:

map f pipe jumps ${{
    dir=$(awk -F'\t' 'NR > 1 { print $NF }' | sort -u | fzf)
    if [ -d "$dir" ]; then
        lf -remote "send $id cd \"$dir\""
    fi
}}

I have chosen this syntax so that pipe is similar in shape to other commands like map and cmd, and the only real difference is that for pipe, the expression object has to be of type execExpr because it doesn't make sense to pipe data into any other type of expression. In addition, there are no changes to scan.go since no new types of tokens are introduced, and only the parsing logic in parse.go has been changed to add support for this new pipe command.


Everyone is welcome to review the branch and provide feedback.

@j-xella
Copy link
Author

j-xella commented Jun 12, 2023

Thank you for your work. When creating this ticket, I only needed jumps, and it is covered now. But I am wondering if the following types of data could be added as well, so that the access is complete:

  • history - as you mentioned earlier
  • options - yes, they are exported as environment variables. However, it would be nice to have a separate way to get them, the way that does not suffer from environment limitations of a particular shell/OS.
  • fs / fx variables - same reasons as with options. Additionally, currently file selections are separated with a configurable separator (filesep). This is all nice and flexible, but makes it a pain splitting the string into individual files, if we want it work with any possible filesep. I take it that the output produced by pipe will be always line-oriented, i.e. lines split with \n, same as in marks or tags files? That would make writing reliable scripts much easier.

@j-xella
Copy link
Author

j-xella commented Jun 12, 2023

A question: if I invoke a series of commands:

  1. lf --remote "send $id pipe maps $ cat > file1"
  2. lf --remote "send $id pipe jumps $ cat > file2"
  3. lf --remote "send $id do_something_with_file1_and_file2"

I understand that they will be put to a queue of sorts and executed later, after the current script is finished. However, is the order of execution guaranteed, i.e. will they be executed in the order I sent them, or it may be any arbitrary order?

@joelim-work
Copy link
Collaborator

Thank you for your work. When creating this ticket, I only needed jumps, and it is covered now. But I am wondering if the following types of data could be added as well, so that the access is complete:

  • history - as you mentioned earlier
  • options - yes, they are exported as environment variables. However, it would be nice to have a separate way to get them, the way that does not suffer from environment limitations of a particular shell/OS.
  • fs / fx variables - same reasons as with options. Additionally, currently file selections are separated with a configurable separator (filesep). This is all nice and flexible, but makes it a pain splitting the string into individual files, if we want it work with any possible filesep. I take it that the output produced by pipe will be always line-oriented, i.e. lines split with \n, same as in marks or tags files? That would make writing reliable scripts much easier.

I prefer to not introduce too much change in a single PR, so I will wait until the general functionality is merged before looking at supporting any more types of data. I definitely plan to add support for the command history, but I guess we would need a strong case for options and fs/fx, as it will be confusing for users if we provide a second way to access these from the shell. AFAIK most users don't seem to have problems working with the existing environment variables, but I could be wrong about this.

On a slightly related note, there is also a request for exporting the list of displayed files in #1084, it seemed to me like it would be better to just create another environment variable (e.g. fa) as suggested in the issue, but perhaps this approach is more appropriate.

A question: if I invoke a series of commands:

  1. lf --remote "send $id pipe maps $ cat > file1"
  2. lf --remote "send $id pipe jumps $ cat > file2"
  3. lf --remote "send $id do_something_with_file1_and_file2"

I understand that they will be put to a queue of sorts and executed later, after the current script is finished. However, is the order of execution guaranteed, i.e. will they be executed in the order I sent them, or it may be any arbitrary order?

I would assume the order is guaranteed - what happens is that lf has a separate dedicated thread for reading commands from the socket, which it then puts into a Go channel (essentially a thread-safe queue) and is finally read by the main thread for processing.

BTW it is possible to group builtin commands and shell commands sequentially using :{{ }}:

cmd allmaps :{{
    pipe maps $cat > /tmp/maps
    pipe cmaps $cat > /tmp/cmaps
    $cat /tmp/maps /tmp/cmaps | less
}}

@j-xella
Copy link
Author

j-xella commented Jun 12, 2023

we would need a strong case for options and fs/fx, as it will be confusing for users if we provide a second way to access these from the shell.

Speaking strictly from my personal experience here, but I would find it confusing if there were 2 ways that produce similar, but not the same results. Then I would be guessing all the time, which one is better. But if the documentation says that both ways return the same data, I would just use what I am most convenient with.

AFAIK most users don't seem to have problems working with the existing environment variables, but I could be wrong about this.

True, because usually all of the following applies:

  • lf users are on Linux
  • Users use modern shells like bash/zsh
  • Users don't select many files at a time

But, honestly, using environment variables to pass potentially unlimited amounts of data seems like environment abuse to me... Again, purely my personal opinion, but I think that lf would be better off if fs/fx/lf_{option} data would be handled outside of process environment. And providing an alternative way is the first step to that.

On a slightly related note, there is also a request for exporting the list of displayed files in #1084, it seemed to me like it would be better to just create another environment variable (e.g. fa) as suggested in the issue, but perhaps this approach is more appropriate.

Totally agree.

@ilyagr
Copy link
Collaborator

ilyagr commented Jun 12, 2023

My 2 cents: I like the syntax @joelim-work proposed in #1277 (comment). I glanced at the implementation, and it seemed pretty clean and restrained in terms of how much complexity it adds.

For this approach, the caveat is the threading issue that @joelim-work and @j-xella discuss in #1277 (comment) and that we mentioned before. I'm fine with that; it doesn't interfere with what I'd use this syntax for and each of the approaches we discussed has caveats.

My one minor worry is that the command will result in more people running into the threading issue (which would happen when you use lf -remote "send pipe $id ..." from a script that's itself called with $ from lf) and becoming confused. I'm not sure how much more common that will become and we could try to address this in the documentation.

@joelim-work
Copy link
Collaborator

But, honestly, using environment variables to pass potentially unlimited amounts of data seems like environment abuse to me... Again, purely my personal opinion, but I think that lf would be better off if fs/fx/lf_{option} data would be handled outside of process environment. And providing an alternative way is the first step to that.

I suppose I don't mind exporting these as well at some point later in time. But even if it happens, I think it's unlikely the original environment variables will be removed because it would be a large breaking change for users, which means you are still stuck with same problem of potentially having a large environment table. I guess it would be possible to deprecate its use and eventually remove them, but I feel uncomfortable to make this decision by myself.

My one minor worry is that the command will result in more people running into the threading issue (which would happen when you use lf -remote "send pipe $id ..." from a script that's itself called with $ from lf) and becoming confused. I'm not sure how much more common that will become and we could try to address this in the documentation.

I do not know how common these kinds of issues will be. For the record I tried the example given in #1277 (comment) and it worked fine:

cmd test ${{
    lf -remote "send $id pipe maps $ cat > /tmp/maps"
    lf -remote "send $id pipe cmaps $ cat > /tmp/cmaps"
    lf -remote "send $id $ cat /tmp/maps /tmp/cmaps | less"
}}

It doesn't work if you use a builtin command after lf -remote:

cmd test2 ${{
    lf -remote "send $id pipe maps $ cat > /tmp/maps"
    less /tmp/maps
}}

Which makes sense because lf -remote is asynchronous by nature (i.e. it returns after sending a message, and doesn't wait for the actual command to be executed). This isn't really an issue with pipe though, maybe we can make it clear in the documentation that lf -remote doesn't actually wait for the command to be executed, and is generally only useful for performing some kind of final action at the end.

@joelim-work
Copy link
Collaborator

joelim-work commented Jun 17, 2023

I have spent some more time experimenting, and after a lot of consideration, I would like to (once again) provide another alternative approach, which is to have lf -remote output the necessary information. - this is in fact the approach suggested in the original post:

There is a whole range of lf commands that produce multiple lines of output (jumps, maps). Ideally, it would be nice to have some generic way to query lf with lf --remote command, so that the output of such commands would be printed to stdout by lf --remote. For example, lf --remote "pipe $id jumps" | grep ....

The reason I didn't consider this approach very much earlier because after some initial investigation I found that lf wasn't able to serve such a request while running a synchronous shell command, see #1277 (comment). However this time I decided to try storing all of the information in the server, rather than having the server delegate the request to lf, and it appears to work rather well:

  • The approach isn't new, in the past a similar technique was used for having the server manage the cut/copy buffer, see ec330c1.
  • This is effectively the same strategy as using temporary files, except that the data is stored inside the server memory instead of actual files, which means there are none of the issues associated with temporary files such as unreliable cleanup and privacy concerns.
  • It is possible to query multiple pieces of data in the same shell command, unlike the pipes approach.
  • The syntax for writing shell commands is more natural, as the data now comes from an explicit lf -remote call rather than consuming the data via standard input.
  • There are no changes to lf command syntax, although additional server commands are (re-)introduced to allow for storing and querying data.

I have pushed up another branch for anyone interested to look at, feedback is of course welcome.

The only downside that I can think of so far is that the default commands don't work well on Windows - in general lf -remote "send ..." is problematic on Windows because of the way arguments with spaces are handled. I'm not sure how much of a concern it is though, since there is currently a bug which prevents cmaps, cmds and jumps from working on Windows anyway.
No longer an issue, will be fixed in #1309.

For the record, selecting a directory based on the jump list now looks like this:

map f ${{
    extract_dirs() {
        # remove the header line and print the last field
        awk -F'\t' 'NR > 1 { print $NF }'
    }
    dir=$(lf -remote "query $id jumps" | extract_dirs | sort -u | fzf)
    if [ -d "$dir" ]; then
        lf -remote "send $id cd \"$dir\""
    fi
}}

I have also moved #1306 back to a draft, as I wish to explore this new approach further. My apologies for continuously switching back and forth between ideas, but this feature greatly enhances the scripting power available in lf, and I think it should be considered carefully before being accepted because it will be very difficult to change later on.

@j-xella
Copy link
Author

j-xella commented Jun 19, 2023

First of all, thanks for all the work.

IMO, from the user perspective, this seems like a better approach, making commands more intuitive. Also, this way it seems to be easier to reuse multiple queries in one command.

I just hope that it does not put too much of additional load on the server, making the operation of lf more unstable.

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