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

Issue error messages (eg permission denied) #311

Closed
HaleTom opened this issue Jul 13, 2018 · 11 comments
Closed

Issue error messages (eg permission denied) #311

HaleTom opened this issue Jul 13, 2018 · 11 comments

Comments

@HaleTom
Copy link

HaleTom commented Jul 13, 2018

If fd can't search a directory, there is no notification:

% mkdir -m000 noperms
% fd -HI moo noperms
%

I'd expect a permission denied by default.

If not default (why not? that's what STDERR is for), perhaps add a switch to enable them.

@HaleTom HaleTom changed the title Issue errors Issue error messages (eg permission denied) Jul 13, 2018
@sharkdp
Copy link
Owner

sharkdp commented Jul 17, 2018

Thank you for the feedback.

I agree. We should definitely make a plan on how to deal with such errors.

I'm not sure if we should always show them by default, but I agree that we should definitely show an error message if one of the explicitly given search directories can not be traversed.

@bernardobelchior
Copy link

What if the errors were shown by default, but there was a flag to suppress them?
This way the user could still be alerted when an error occurred. On the other hand, the flag could be used in an automation perspective, like piping output to other tools.

@HaleTom
Copy link
Author

HaleTom commented Sep 22, 2018

STDOUT (File descriptor 1) gets piped to the next command if there is a pipe.

For errors, there's STDERR (FD 2) which gets connected to the terminal by default, but can be redirected to /dev/null (or anywhere else):

command 2> /dev/null

Similar behaviour in Windows

This is standard Unix design philosophy, where MS got it from. Not having errors enabled by default will raise many eyebrows without a solid reason to deviate from established practice.

Not showing errors by default means that possibly matching files which can't be accessed due to permissions issues will never be highlighted.

@psinghal20
Copy link
Contributor

Hi, I am new to Rust and wanted to gain some experience by contributing to the open source community. Is this issue still there? If yes, could you give me some pointers on how to proceed with the issue? Thanks!

@HaleTom
Copy link
Author

HaleTom commented Sep 26, 2018

Thanks for wanting to help out!

I'm not a rust man, but this does look like a good starting project.

The issue persists.

I suggest you tag some of the others above for guidance - I can't do that this Fasthub app.

@psinghal20
Copy link
Contributor

@sharkdp Could you help me in proceeding with this issue? Any tips would be helpful!

@sharkdp
Copy link
Owner

sharkdp commented Sep 29, 2018

@psinghal20 Thank you for offering to help out!

A quick disclaimer: I did initially put the "good first issue" label on this ticket, but I now realize that it's probably not the most beginner-friendly thing to work on.

That being said, a good place to start would be this match statement:

fd/src/walk.rs

Lines 207 to 210 in 27caa33

let entry = match entry_o {
Ok(e) => e,
Err(_) => return ignore::WalkState::Continue,
};

where we currently silently ignore any errors by skipping to the next entry (return ignore::WalkState::Continue).

A really quick & dirty solution would be to add a print-statement here before returning. However, note that this part of the code runs in a parallelized way inside the "walker" threads (that do the actual work of finding files). Printing unsynchronized from these threads could potentially result in a messed-up terminal output.

A better way to fix this would be to send errors via the mpsc::channel that these worker threads use to communicate with the main thread. Currently, we use the channel to send PathBuf objects for every entry that fd finds. To extend this in such a way that we could alternatively send errors, we would probably write a new enum type along the lines of

enum WorkerResult {
  Entry(PathBuf),
  Error(SomeErrorType)
}

that could then by used as the underlying type for the mpsc::channel.

On the receiving side of the channel, we could then print the errors in a synchronized way from a single thread.

@psinghal20
Copy link
Contributor

@sharkdp Thanks for your guidance. I tried doing it the 2nd way you mentioned, adding the WorkerResult enum which is sent to the main thread through the mpsc::channel. Please take a look at the PR #331 and suggest any changes, if required.

@sharkdp
Copy link
Owner

sharkdp commented Oct 1, 2018

Closed via #331 by @psinghal20. Thanks!

@sharkdp
Copy link
Owner

sharkdp commented Oct 27, 2018

Released in fd-7.2.0.

@HaleTom
Copy link
Author

HaleTom commented Oct 28, 2018

Totally sweet!! Thanks all! :)

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

4 participants