@ericzolf If you agree with the principle, I will write a unit test to cover the new "--exclude-unreadable" selector.

@ericzolf Sorry for the long list of small commits. At least everything is split into manageable chunks.

The general idea here is to avoid calling os.lstat() on files that are excluded by --exclude ... and not re-included by any other pattern.

1. This reduces the number of I/O calls.
2. When done properly, it prevents permission errors or access denied issues when trying to retrieve metadata from excluded folders.

Your input is appreciated, as always.

Just a heads-up: I’m heading off on vacation and will be back in late May.

Before I review this PR, can you clarify why the PR merges into a branch which isn't master?

Before I review this PR, can you clarify why the PR merges into a branch which isn't master?

I wanted to chain the pull requests because one change depends on the other. However, the changes are unrelated in terms of functionality. I'm not sure how to properly manage this kind of situation in GitHub.

I generally keep it against master, put a note of the "chaining" for the reviewer, and potentially put it in draft mode until the base PR is merged.

I need to take a deeper look at the changes you've made, they're quite extensive, but one thought goes through my mind: why do we need an exclude-unreadable option? If a file is unreadable, we won't anyway be able to backup it (and rdiff-backup wouldn't choke on it, just send a warning). Is it only about avoiding warnings? Then I'd like another option and even approach, but we can talk about once you've clarified the purpose.

I need to take a deeper look at the changes you've made, they're quite extensive, but one thought goes through my mind: why do we need an exclude-unreadable option? If a file is unreadable, we won't anyway be able to backup it (and rdiff-backup wouldn't choke on it, just send a warning). Is it only about avoiding warnings? Then I'd like another option and even approach, but we can talk about once you've clarified the purpose.

Thanks, Eric, for taking the time to review.

To answer your question—yes, the main goal is to avoid raising unnecessary warnings during the backup process. That was the initial motivation for this change.

However, while working on it, I noticed that rdiff-backup calls lstat() even on files that are excluded. This shouldn't be necessary when the exclusion is based on filename patterns. In my scenario, this has a negative impact on performance because lstat() is still called on every file, even those that are excluded.

On avoiding lstat, fine with me on the principle, I just need to find a bit more time to review it properly.

On the exclude option, I don't like it so much:

1. it gives the false impression that something is excluded which isn't normally
2. for avoiding warnings, looking at [ENH] don't return a warning when there are no increments to remove #1071, I was thinking about an option like --ignore-warnings all resp --ignore-warnings unreadable,no-increment and/or --ignore-warnings unreadable --ignore-warnings no-increment, with of course the capability to add further warnings to ignore.

Would it be possible to split both things and add the ignore option in another PR?

I will split the PR in multiple pieces to make everything clear. That I agree.

I like the idea of --ignore-warnings unreadable,no-increment, but the main reason why I wanned to implement an "exclude-unreadable" is to exclude unreadable files from fs_availabilities check. On line 600, fs_availabilities uses a selection.Select to search for a regular file. While doing this on a root filesystem where multiple files are not readable, it raises warnings.

I'm open to suggestions.

@ericzolf Let me recap everything regarding these changes.

The main issue I’m trying to resolve is the following:

On Windows, when configured to back up "C:/", rdiff-backup runs its fs_abilities checks starting from "C:/". This causes several warnings and errors to be logged, especially during the _detect_resource_fork_readonly check.

This particular check attempts to determine if the filesystem supports resource forks. To do so, it needs to find a readable regular file on the filesystem. The current implementation relies on the default selection function to locate such a file, using:

selection.Select(dir_rp).get_select_iter()

However, this traverses the directory tree and, as a side effect, triggers warnings and errors for unreadable or locked files:

WARNING: ListError: 'C:/Users/REDACTED/NTUSER.DAT' [Errno 13] Permission denied: b'C:/Users/REDACTED/NTUSER.DAT'
* Unable to read ACL from path C:/hiberfil.sys due to exception '(32, 'GetNamedSecurityInfo', 'The process cannot access the file because it is being used by another process.')'
* Unable to read ACL from path C:/swapfile.sys due to exception '(32, 'GetNamedSecurityInfo', 'The process cannot access the file because it is being used by another process.')'

This behavior is both annoying and confusing for users reviewing the logs.

To mitigate this, my proposed solution in this pull request was to configure the selection function with the following options: exclude-special-files, exclude-unreadable, and exclude-other-filesystems. This reduces noise and avoids triggering these errors during capability checks.

That said, I acknowledge that this might not be the ideal fix. In hindsight, relying on the selection function inside fs_abilities was probably a design oversight. However, reimplementing a separate mechanism to walk the directory hierarchy just to find a single readable file also seems excessive and redundant. That explain, why I implemented this "exclude-unreadable"...

I hope this make thing more clear.

P.S.: This Pull Request depends on #1074

Could we close this PR and wait for the implementation of #1078 ?

Give me some time to test it end to end in Minarca.

I will mark this PR as draft for now.

This is link to #1064

It all depends how we want to solve the error raised by resource_forks detection.. My initial idea was to create this new exclude and use it in resources forks.