Non-zero exit status in case of failure

[zarkone]

Mimics the value of exit status from GNU ls in case of failure:

Exit status: 0 if OK, 1 if minor problems (e.g., cannot access subdirectory), 2 if serious trouble (e.g., cannot access command-line argument).

Closes #502

---

TODO

• [ ] Use cargo fmt
• [ ] Add necessary tests
• [ ] Add changelog entry

[zarkone]

Hello!

I've noticed this "good first issue" label and decided give it a try. I haven't covered all the cases (including tests) -- metadata fails are not handled -- but decided to push it as is to ask if it is a good approach to return status like this.

So looking forward for some guidance in terms of what needs to be done and how.

Thanks!

[zwpaper]

Hi @zarkone, thanks for the implementation! you have almost done the work.

maybe some updates like below could make further exit code cases easier to implement:

1. create a struct, ExitStatus for example, containing the code and a vector of errors
2. add a macro or fn like the print_error! to add error code and error to the ExitStatus
3. print all the errors in the main and exit with the code

what's your idea? @meain

[zarkone]

@zwpaper thanks for your feedback! Please check this recent commit https://github.com/Peltoche/lsd/pull/536/commits/3bca4047097b487b8b6675a5e7405ce56b1a71be if I understood your approach correct. If it is the case, I'll proceed with mod.rs and tests.

[zarkone]

BTW, I've noticed that Meta::recurse_into from mod.rs never returns an Error.. I think because of this Err branch from recurse in fetch never gets executed if you test it with 400 subdir and --tree.

Am I missing something?

[meain]

@zwpaper I don't think we should collect errors into a vec and print them in main. From what I can see, gnu ls just seems to print the errors as it goes through the directories. I think we can stick to that itself(especially useful in case of tree or other recursive items). Other than that, lets have the error returned as you mentioned in an ExitStatus enum, but that can be a simple enum just for clarity and convert that to the proper exit codes in main.

[zarkone]

@meain this is a good observation! And even more -- GNU ls doesn't collect/buffer anything at all -- it prints all the info right away. lsd, on the contrary, uses buffered approach.

It actually brings quite a significant delay if you compare ls and lsd on a folder with big-enough amount of file / recursion depth: ls will start printing right away while lsd will have a noticeable silent delay before printing.

With the current buffered approach of lsd, in case of any access errors, you'll see this errors first -- before printing any kind of fileinfo. And then, after delay of buffering, actual fileinfo will be printed. Erorrs and fileinfo output will be separated from eachother, which is not the case for GNU ls.

To summarize, I think lsd should either buffer everything or not buffer anything at all, to be consistent and not to produce confusing output (which I described in a paragraph above).

[meain]

I think in lsd we are printing the errors as we go while printing the actual output only at the end. But if we were to collect the errors and print it out at the end, the only change is that instead of printing the errors at the top, we will be print errors at the bottom which does not help much.

Also if we were to do something like:

ls existing-file1 non-existing-file existing-file2

gnu ls throws the error at top anyways.

ls: cannot access 'non-existing-file': No such file or directory
existing-file1  existing-file2

I don't personally see any use for collecting the errors into a vec as of now.

[zarkone]

@meain sounds good, thanks for the feedback!

How about the other points? About error propagation from mod.rs and the implementation in general?

[zwpaper]

as I tested in ubuntu 18.04 we can see GNU ls cached the error msg and show together like this:

Edited: Oops, I got the point now, the GNU ls cached the files result and print it at the last, making the non-cached error msg printed at the top.

test case, return 0:

lstest # ls -l
total 24
drwxr-xr-x 2 root root 4096 Jul 27 10:58 2
lrwxrwxrwx 1 root root    1 Jul 27 11:03 3 -> 3
drwxr-xr-x 2 root root 4096 Jul 27 10:58 4
drwxr-xr-x 2 root root 4096 Jul 27 10:58 6
drwxr-xr-x 2 root root 4096 Jul 27 10:58 a
lrwxrwxrwx 1 root root    9 Jul 27 11:05 b -> non-exist
drwxr-xr-x 2 root root 4096 Jul 27 10:58 c
drwxr-xr-x 2 root root 4096 Jul 27 10:58 e

symbol link dereference error, return 1:

lstest # ls -l -L
ls: cannot access '3': Too many levels of symbolic links
ls: cannot access 'b': No such file or directory
total 24
drwxr-xr-x 2 root root 4096 Jul 27 10:58 2
l????????? ? ?    ?       ?            ? 3
drwxr-xr-x 2 root root 4096 Jul 27 10:58 4
drwxr-xr-x 2 root root 4096 Jul 27 10:58 6
drwxr-xr-x 2 root root 4096 Jul 27 10:58 a
l????????? ? ?    ?       ?            ? b
drwxr-xr-x 2 root root 4096 Jul 27 10:58 c
drwxr-xr-x 2 root root 4096 Jul 27 10:58 e

symbol link dereference error, and non existed files, return 1:

lstest # ls -l -L . 5 d
ls: cannot access '5': No such file or directory
ls: cannot access 'd': No such file or directory
.:
ls: cannot access '3': Too many levels of symbolic links
ls: cannot access 'b': No such file or directory
total 24
drwxr-xr-x 2 root root 4096 Jul 27 10:58 2
l????????? ? ?    ?       ?            ? 3
drwxr-xr-x 2 root root 4096 Jul 27 10:58 4
drwxr-xr-x 2 root root 4096 Jul 27 10:58 6
drwxr-xr-x 2 root root 4096 Jul 27 10:58 a
l????????? ? ?    ?       ?            ? b
drwxr-xr-x 2 root root 4096 Jul 27 10:58 c
drwxr-xr-x 2 root root 4096 Jul 27 10:58 e

the performance part, the GNU ls should also cache the result, it has to because it would rearrange the result for sorting and aligning, right? @zarkone

[zwpaper]

I also noticed that GNU ls would print errors before each folder result:

lstest # ls -l -L -R .
.:
ls: cannot access '3': Too many levels of symbolic links
ls: cannot access 'b': No such file or directory
total 24
drwxr-xr-x 2 root root 4096 Jul 27 11:24 2
l????????? ? ?    ?       ?            ? 3
drwxr-xr-x 2 root root 4096 Jul 27 10:58 4
drwxr-xr-x 2 root root 4096 Jul 27 10:58 6
drwxr-xr-x 2 root root 4096 Jul 27 10:58 a
l????????? ? ?    ?       ?            ? b
drwxr-xr-x 2 root root 4096 Jul 27 10:58 c
drwxr-xr-x 2 root root 4096 Jul 27 10:58 e

./2:
ls: cannot access './2/4': Not a directory
total 0
-rw------- 1 root root 0 Jul 27 10:58 2
l????????? ? ?    ?    ?            ? 4

[meain]

@zwpaper Yeah, that does make sense. Since we will not be sorting across multiple directories, I don't think we have buffer all the output, but rather just per folder. But let us not get that done in this PR. We could keep that as a separate PR.

@zarkone Yeah, you are right. As of now we are not returning any errors. Mostly it is just print the error and continue, but we can change that to return the ExitCode enum.

---

One other thing that I noticed when trying out what @zarkone mentioned is that tree does not recurse into the directory if the perm is 400, but lsd as of now does. I believe we are currently emulating the behaviour of ls -R for tree which actually does recurse into the dirs.

Setup:

$ mkdir -p a/{b,c}
$ touch a/b/one a/c/two
$ chmod 400 a/b

tree (no errors here)

$ tree
.
└── a
    ├── b
    └── c
        └── two

3 directories, 1 file

lsd --tree

$ ll --tree
lsd: ./a/b/one: Permission denied (os error 13).

drwxr-xr-x  96B 42 seconds ago .
drwxr-xr-x 128B 42 seconds ago └── a
dr--------  96B now                ├── b
drwxr-xr-x  96B 35 seconds ago     └── c
.rw-r--r--   0B now                    └── two

lsd -R

$ ll -R a
lsd: a/b/one: Permission denied (os error 13).

dr-------- 96B 10 minutes ago b
drwxr-xr-x 96B 11 minutes ago c

a/b:

a/c:
.rw-r--r-- 0B 10 minutes ago two

[zwpaper]

ok, it make sense to me now printing the error message without caching it.

as the tree and 400 part, I would stand for the current for the option printing error.

1. ls -R would print the same error
2. the error message would help to verify there are files but not accessible

what's more, it seems that ls -R would print the filename with question mark in the unaccessible part:

$ ls -l -R
.:
total 4
drwxrwxr-x 4 ubuntu ubuntu 4096 Jul 28 10:41 a

./a:
total 8
dr-------- 2 ubuntu ubuntu 4096 Jul 28 10:41 b
drwxrwxr-x 2 ubuntu ubuntu 4096 Jul 28 10:41 c

./a/b:
ls: cannot access './a/b/one': Permission denied
total 0
-????????? ? ? ? ?            ? one

./a/c:
total 0
-rw-rw-r-- 1 ubuntu ubuntu 0 Jul 28 10:41 two

---

we should create 2 other issues for those 2 discuss

[zarkone]

@zarkone Yeah, you are right. As of now we are not returning any errors. Mostly it is just print the error and continue, but we can change that to return the ExitCode enum.

@meain thanks, sounds good!

@zwpaper, @meain: Thanks for your investigations -- I was under impression that ls prints everything without any caching but It seems like I didn't red ls sources with enough attention.

I'll remove error messages buffering, leaving only status code as an internal state of core then.

I also agree with @zwpaper that those issues are related, but out of scope of this PR -- just wanted to share my findings. I'm happy to jump on it as well after finishing this one -- if you feel it makes sense.

[zarkone]

@zwpaper @meain updated the PR -- kindly ask you to check my yet unfinished code if it is a good direction overall.

I feel that some tests and some code at least in mod.rs is still missing -- but if what I have now looks good than I'll proceed further with it.

[meain]

Closing in favor of https://github.com/Peltoche/lsd/pull/739 . Thanks for starting the work on this PR :D