-
Notifications
You must be signed in to change notification settings - Fork 328
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
lfcd improvement proposal #1393
Comments
I'm worried this would get messed up if the user calls any program that prints to stdout before quitting |
@nicolasdumitru Thank you for the proposal. @ilyagr is right in that it can get messy when shell commands are used. Infact, this idea has been discussed before in #140 and #472. If you want to see this in action, you can try one of the workarounds mentioned in these discussions (e.g. |
@ilyagr valid concern. Do you think that forcing |
No, it wouldn't. The user might need the output from the commands run from inside Try running |
This is probably a crazy idea, but would this be possible if the shell commands were connected directly to the TTY instead of diff --git a/app.go b/app.go
index fc9bd11..3057c00 100644
--- a/app.go
+++ b/app.go
@@ -327,20 +327,6 @@ func (app *app) loop() {
app.nav.dirPreviewChan <- nil
log.Print("bye!")
-
- if gLastDirPath != "" {
- f, err := os.Create(gLastDirPath)
- if err != nil {
- log.Printf("opening last dir file: %s", err)
- }
- defer f.Close()
-
- _, err = f.WriteString(app.nav.currDir().path)
- if err != nil {
- log.Printf("writing last dir file: %s", err)
- }
- }
-
return
case n := <-app.nav.copyBytesChan:
app.nav.copyBytes += n
@@ -518,9 +504,15 @@ func (app *app) runShell(s string, args []string, prefix string) {
var err error
switch prefix {
case "$", "!":
- cmd.Stdin = os.Stdin
- cmd.Stdout = os.Stdout
- cmd.Stderr = os.Stderr
+ tty, err := os.OpenFile("/dev/tty", os.O_RDWR, 0)
+ if err != nil {
+ log.Printf("getting tty: %s", err)
+ return
+ }
+
+ cmd.Stdin = tty
+ cmd.Stdout = tty
+ cmd.Stderr = tty
app.runCmdSync(cmd, prefix == "!")
return
diff --git a/client.go b/client.go
index 0fab7d5..7fbe32f 100644
--- a/client.go
+++ b/client.go
@@ -61,6 +61,10 @@ func run() {
app.loop()
app.ui.screen.Fini()
+
+ if gLastDirPath != "" {
+ fmt.Println(app.nav.currDir().path)
+ }
}
func readExpr() <-chan expr { Even if it does work, I still have some concerns about this idea in general, as it involves breaking changes and involves a fair amount of risk when considering other platforms like Windows (which will require a different implementation). I imagine that @gokcehan might have tried this already, perhaps there's a good reason this wasn't done. |
@ilyagr indeed, not a nice experience.
|
Pretty much how you described above: That being said, the patch above is just a quick and dirty POC and takes a number of shortcuts. I haven't looked into it much further as I still have reservations about changing the design (i.e. having the shell commands bypass |
I don't think @joelim-work 's idea is crazy, but I'm not sure what bugs might lurk if we do that. I'm especially wary of issues on unusual OSes. We'd at least need a separate codepath for Windows, unless Go stdlib somehow provides a |
@joelim-work Thank you for coming up with a solution. I have not experimented with using Historically, we have had issues before when there was a change in tcell from A slightly safer alternative might be to direct all command output to stderr with something like following:
Although, I'm not sure about this change either. There are just too many different kinds of commands that users can come up with, so it is difficult to tell if such a change would break anything. All in all, I'm still not convinced that this change is worth the risks involved. I don't think we get any new functionality with this change. I can see the appeal of a simple command like I don't want to disregard this suggestion altogether, but if we go down this route, I would personally like to see this change tested extensively, preferably on all platforms that we support. |
@gokcehan I had not thought of piping the shell output to The problem of obtaining a handle to the TTY for reading/writing in a portable manner is a serious concern to me. I would prefer to not handle it directly inside the I agree that the benefit this feature provides is not really enough to warrant the risks involved. The problems described above seem to be theoretical in nature as opposed to actual issues that a user might encounter in practice. From my opinion, the sample That being said, I can see the appeal of using |
@gokcehan @ilyagr @joelim-work what about setting a shell/enviroment variable (with a unique name that can be configured in lfrc) that holds the path to the last directory that was open in lf? I don't know how Window's shell works (and consequently if it could work on Windows), but lfcd is already different for Windows and this solution would work on every POSIX-compliant system and would circumvent stdout and the problems it poses. lfcd just becomes: |
@nicolasdumitru Using a pre-defined environment variable as the argument for That being said, after looking into this problem some more, I have found a library tty, err := tty.Open()
if err != nil {
log.Printf("getting tty: %s", err)
}
cmd.Stdin = tty.Input()
cmd.Stdout = tty.Output()
cmd.Stderr = tty.Output() I have done some very brief testing on both Linux and Windows and it appears to work fine. There is still some risk involved given that running shell commands is a core feature of |
@joelim-work it doesn't necessarily require prior configuration. The shell variable can have a default name (a very descriptive one that is very unlikely to coincide with what is already on the system). And the shell variable's name could just be a one-liner in (default) lfrc (basically set a particular string and then lf names the shell variable like that). |
@nicolasdumitru I still have some trouble understanding, could you please provide a more detailed description about how this is supposed to work? If the user doesn't specify a path for this file, then we would have to provide one by default, and there isn't really a sane value for that, which is one of the reasons why Also, I'm not sure if this is relevant to the discussion, but AFAIK any environment variables set by |
@joelim-work let me put it this way:
This does not take into account providing the optional argument, but you get the idea. |
@nicolasdumitru If I'm not mistaken, that requires changing the value of an environment variable that belongs to the parent process (i.e. shell), which is not allowed due to security considerations as @joelim-work mentioned. To further iterate on the stderr approach, maybe it could only be done when the new command line options are used. So let's say we added two command line options discussed here, |
@gokcehan Although I see the TTY approach as the ideal end goal (especially after discovering
Only 1) is required to satisfy the user requirements, while 2) and 3) are nice to have but also optional and not necessary to implement at all. I am happy to work on this, though as a side note I was thinking of merging the other PRs I have outstanding (#1384 and #1386) soon just so I don't have so many changes sitting in progress, if you have no objections. P.S. I was thinking of calling the options |
@joelim-work I was expecting more discussion about the stderr approach. That is why I had not written anything about go-tty yet. I think we can really benefit from more discussion before the execution. First of all, I did not mean to replace the old flags with the new ones, but instead I meant adding two new flags. Otherwise, this would be a breaking change that could effect the majority of our users. I don't think anything discussed in this thread is worth such a major breaking change. I suggested this because you mentioned the use case for redirecting stderr to a file in your previous comment. I assume you meant redirecting stderr to a file at all times so one should be able to see stack traces for crashes that are hard to reproduce consistently. Currently it is possible to do this with something like the following:
With the new stderr proposal, this should work exactly as before. However, this would not work well if one tries to use the new flag with an err file like the following:
The assumption here is that if someone is okay to create a temporary err file, then they wouldn't mind creating a temporary file for directory change either. I should note that the current behavior is not perfect for the err file either since it also redirects command errors to the err file. If we want to properly support the err file, an alternative approach might be to use either stdout or stderr at all times like the following:
But then this is technically a breaking change, so we should consider more carefully, though I don't expect many problems with this either. In any case, I still see such changes much safer than anything that make use of tty. go-tty library seems nice, but it is still an additional library which has its own lifecyle with issues and updates, so I think it is better to avoid it if possible. Looking through its current and past issues, I get the impression that we might need to be extra careful about signal handling when using this library. With these in mind, is there any disadvantage of the stderr approach that I'm missing? Why do you still see the tty approach as the ideal end goal? What does it bring to the table that stderr approach is missing? |
@gokcehan Thanks for your feedback, and also my apologies for causing any misunderstandings and confusion with the PR. I probably should clarify a few things:
I admit that I jumped the gun here and made the assumption that you wanted to replace the existing flags. I wasn't sure because providing two sets of flags could be confusing for users, however I am happy to add the old flags back in if you wish. Does this mean that going forward, you intend to maintain both sets of flags, or are you planning to eventually deprecate the old flags, or perhaps just leave the decision for later? Also if both sets of flags are being maintained, I would assume that you prefer to leave the various
While the cmd.Stdin = os.Stdin
if gLastDir || gSelection {
cmd.Stdout = os.Stderr
} else {
cmd.Stdout = os.Stdout
}
cmd.Stderr = os.Stderr
The reason I see writing to the TTY as the ideal end goal is because there is no need to worry about this kind of conditional stream mapping, and that shell commands would still work regardless of whether the user redirects Having said all of that, regarding #1397, did you have any other issues apart from replacing the old flags, and is there anything else you would like to discuss? |
@joelim-work Thank you for your understanding. I hope you forgive me for dragging this discussion a little further. I just think it is important to find the right trade-off here that enables all use cases if possible and causes the least amount of disruption. If err redirection is not on the table anymore, I should remind again that the original stderr approach (i.e. send all command output to stderr) might be a better option. This has the advantage of getting rid of conditional behavior, which I'm not a fan of either. When we don't need conditional behavior, additional flags may also not be necessary, since I should say a few words about breaking changes in general. I use many open-source tools myself, and I rarely follow the development of these tools myself. This is why I assume there are |
@gokcehan Thanks once again for your input. In my mind I had not considered using I have submitted a new draft PR #1399 which incorporates your suggestions. There are no changes to command line options or any other breaking changes, which simplifies a lot of things. As for the |
@nicolasdumitru I guess we can reopen this ticket. For now I haven't made any changes to the I do agree that the the syntax |
I think we could mark "-last-dir-path" as deprecated and keep it (either forever or for a few versions). I'd then name the other flag to "-print-last-dir" or "-echo-last-dir". |
I'm also wondering if |
I do not mind adding new flags but if so I would prefer to deprecate the old ones, since to me it doesn't make sense to expose two similar sets of flags (which could be confusing for new users, as they might wonder which one is better to use). I also noticed that a newline character is missing at the end for both the |
@joelim-work if you were asking me whether it causes a practical issue, no, it doesn't cause me an issue in my current situation, but it may cause people running e.g BDSs or other unixes some problems (I don't know exactly what os does or doesn't have /dev/stdout); idk, maybe one day I'll try one of those. Also, if it's POSIX and as simple as possible, you don't have to worry about lfcd (os, other programs' implementations etc). |
I think marking one of the flags as "deprecated" in the help text and the docs is good enough. I think we definitely should keep the old flags for a version or two, just in case the new ones have unexpected bugs on some systems. After that, I don't have a strong preference. I personally would probably keep them unless it actually causes confusion, but it'll probably be clearer what's best at that time.
Yes, I don't think we should change those flags. I'm suggesting maybe inserting a newline if we make a new flag. I don't insist on it if people think that slight difference in behavior is confusing. |
OK, given the way this conversation is going, I will consider doing the following:
However I would like some feedback from @gokcehan before taking any action. |
I agree it is a good idea to add new flags for stdout, especially considering Windows, but I'm still not convinced that we should deprecate the old flags. What exactly does deprecation mean in this context? If we are going to keep them forever, then what is the point of deprecation? If we plan to remove them at some point, then how long should we wait? One simple heuristic to determine a deprecation duration might be to wait as long as the lifetime of the feature. So if a feature was implemented 1 month/release ago, we should at least deprecate it for 1 month/release. Clearly, this would not work for these flags since they are almost 7 years old at this point. Ubuntu pushes LTS releases every 2 years. Would that be an appropriate duration for this deprecation? Otherwise, system upgrades might result in a broken setup without any deprecation warning. Do we really want to plan a 2 year deprecation ahead so that we will have a "perfect" software after all this time? I also don't see the point of removing the old flags as they provide two different functionalities in a way. There are many programs which has an I commonly see the argument about unix philosophy in these proposals. As I said previously, there are many terminal applications including the text editors in the original unix that do not write to stdout. Most compilers in unix systems do not read from stdin and print to stdout. There are many other unix tools that skip the use of stdin, stdout, or both. The use of stdin/stdout/stderr as a design principle is mostly useful for tools that are meant to be used in pipes to process and filter data, though not every tool is meant to be used in such a manner. Technically speaking, we don't even consider any examples with pipes in this discussion. Note, we also don't use stdin in Regarding the POSIX compatibility, we try to remain compatible in our shell examples, though this compatibility does not include our command line options. POSIX standard has an utility convention (https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap12.html) that defines options as single characters with a preceding dash character. We only have long form options in In general, we should keep in mind that @nicolasdumitru Regarding the portability of All in all, I'm in favor of getting rid of our attitudes of perfectionism, unix philosophy, POSIX compatibility, or whatever it is that drives us to make a breaking change and keep the old flags existing as before without any deprecation. I don't think it is too difficult to maintain the extra code. I don't remember any major issues with these flags before and I don't expect anything much in the future. If an issue comes up regarding these flags, I'm ready to take the blame and responsibility of fixing the issue. If I'm not around, then feel free to remove these flags. Having said that, I don't mind adding new flags for stdout, and it is probably a good idea to do so. If new flags are added, it could also be nice to use them in our I currently have no opinions about the names for the new flags, and the existence of a trailing newline in the output. |
I don't have much more to add to this discussion. TBH I did not expect that I would have to reopen this issue after implementing #1399. Regarding the various topics discussed in this issue (e.g. deprecation strategies, POSIX compatibility, potential use of In any case I think we are all in agreement to add new flags, which I don't mind working on. In addition, I will update |
Goal
Simplifying
lfcd
and making it more POSIX compliant.Issue
Currently,
lfcd
uses temporary files that it creates withmktemp
in order to store the path of the directory that was last open in lf and into it. This poses multiple problems:mktemp
is not POSIX. This means thatlfcd
is also not POSIX (here, by POSIX I mean it only requires utilities/commands found in the POSIX specification and the obviouslf
). Whilemktemp
is pretty widely available as a GNU and BSD utility, it could be a problem on some POSIX compliant operating systems (e.g. non-GNU Linux distros).Solution
Add a flag/option to lf that is similar to
-last-dir-path
, but instead of writing to a file, outputs to stdout. Why?\cd $(\lf -new-flag-to-print-path-to-stdout "$@")
The inspiration for this is
fzf
. By default, fzf prints the path to what you search for to stdout. Consequently, a user can do something likevim $(fzf)
to edit a file that they don't know the exact path to orcd $(dirname $(fzf))
to do some fuzzy searching for a file and cd into the directory of said file. If you are interested and would like to try something similar without having to write a single line of code, try the above mentionedcd $(dirname $(fzf))
and search for a file or a directory where you have files.The text was updated successfully, but these errors were encountered: