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

suda#read throws uncaught exception when opening suda:///not-existing.txt #18

Open
doronbehar opened this issue Jun 3, 2019 · 4 comments

Comments

@doronbehar
Copy link
Contributor

Hey,

I noticed that if I launch nvim suda:///not-existing.txt and /not-existing.txt doesn't exist, I get an exception:

Error detected while processing function suda#BufReadCmd[5]..suda#read:
line   28:
E605: Exception not caught:

Here's where this happens, for reference:

https://github.com/lambdalisue/suda.vim/blob/b867074b30f623ccb81a0bb8d1b74a6d0fafd160/autoload/suda.vim#L70-L77

cat fails because the file doesn't exist. Technically this error is harmless because if the file doesn't exist the buffer should be empty anyway. Perhaps this 'bug' shouldn't even be fixed?

We could ignore any errors but that won't be reliable if e.g the password was incorrect. As I'm thinking about it, this part of suda should be implemented with extreme care because if no error is thrown, a user might think he opened an empty file, add something according to some manual, save it (without errors) and end up with a totally different file with all the previous content lost.

According to my tests, even a wrong password ends up with an empty buffer and the same exception error is printed. Maybe suda should clearly state to the user what command failed and that the buffer's contents don't correspond to the real contents? Unless of course the file doesn't exist.. Anyway, I wonder what is your opinion on the subject.

Thanks.

@lambdalisue
Copy link
Owner

It sounds really complicated 🤕

Well in the usual case, I think we can use getftype() to check if the file is not readable or does not exist (not 100% sure). The function would return values even the user does not have permission to read the file.

What I'm not sure is the situation when a user does not have permission to read the parent directory. I'm not sure if getftype() returns value in that case.

Anyway, I agree to NOT silence the error. We need deep investigation before applying fixes.

@doronbehar
Copy link
Contributor Author

What I'm not sure is the situation when a user does not have permission to read the parent directory. I'm not sure if getftype() returns value in that case.

It returns an empty string in this case.

I've been thinking about the UX of the 'wrong password' vs 'other legitimate reasons for the buffer to appear empty' and I've had an idea:

When a suda:// buffer is opened, we should first check if the parent directory is readable and then check if we can be sure the file exists. In case we know it's there, and that cat command failed, the user should be prompted to try to read the command again and again until cat doesn't fail. Only if he wishes to quit this loop he'll be represented with a warning saying that the buffer's contents probably don't fit the real contents of the file.

In case the parent directory of the requested file doesn't exist, suda should still suggest to read the file with sudo cat again and again in the same manner as above (with a "would you like to try read it again y/n" prompt) until there's no v:shell_error. However, in this case, the warning message should be a little bit more calmed since we can't be sure of that there is such file or not.

Perhaps this kind of behavior is overwhelming but I think it'll be most liable.

@lambdalisue
Copy link
Owner

I think there are following two distinct issues

  1. suda.vim should repeat the command until a correct password is given like normal sudo command on suda#system
  2. suda.vim should NOT read a content from non existing file on suda#BufReadCmd because now suda://xxxxx is used for non existing files as well

So I'd love to

  1. Fix 1st issue
  2. Add a way to find if a file exists or not (maybe by sudo ls or whatever)
  3. Use above and skip suda#read on suda#BufReadCmd when the target does not exist

@doronbehar
Copy link
Contributor Author

Oh right, I guess it'll be better to fix this in steps but there are a few details I think that should be discussed as for the implementation, per issue:

  1. Perhaps this repetition could be implemented in a :term? Certainly this may be too verbose but I think it has the advantage that the user can see the error message clearly and exactly what command produced it, all in the :term. In case this sudo cat command's exit is false,the user could decide according to the stderr of it whether it ought to try and run that command again.
  2. Using sudo ls sounds good, In addition to the above suggestion, perhaps it will be better to use [[ -f <file> ]] instead?

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

No branches or pull requests

2 participants