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

fix(_known_hosts_real): exclude directories from the config files #818

Merged
merged 2 commits into from
Sep 12, 2022

Conversation

akinomyoga
Copy link
Collaborator

Fixes #817

Copy link
Owner

@scop scop left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's add some dirs to the mix in the test suite.

bash_completion Outdated Show resolved Hide resolved
@akinomyoga
Copy link
Collaborator Author

akinomyoga commented Sep 12, 2022

Let's add some dirs to the mix in the test suite.

Thanks! I have added an Include directive in config_include for the test of ssh_config inclusions (454bbe3).

However, directories at e.g. ~/.ssh/config and Include <directory> seem to be just misconfigurations. I have checked the man page man ssh_config, but Include directive does not seem to accept a directory name. I have tried it in my actual ssh configuration, but Include <directory> seems to be just ignored. In this sense, maybe we shouldn't fix it but leave the error messages, or we can output a more friendly message that explains the issue. But then, a question is why we have been ignoring unreadable files by testing [[ -r $file ]]. If we take the Include directive as "include-if-exists", we can consider a case an unrelated directory is created at the same path, which should be ignored.

@scop
Copy link
Owner

scop commented Sep 12, 2022

Parts of the discussion about error messages at #807 (comment) applies here too. Silently suppressing isn't nice, but neither is making a mess on the console, producing errors that make no sense to users, or coding and maintaining precise assumptions about undocumented details of external tool behavior into bash-completion. Whether silently suppressing is the best option or not, at least we are quite consistent with it currently. I'm open to discussing the general approach, but that's probably better done elsewhere.

@scop scop merged commit 33df84a into scop:master Sep 12, 2022
@akinomyoga akinomyoga deleted the _known_hosts_real-config branch September 12, 2022 12:55
@akinomyoga
Copy link
Collaborator Author

Thanks!

Parts of the discussion about error messages at #807 (comment) applies here too.

I also remember the discussion at #776 (comment).

Whether silently suppressing is the best option or not, at least we are quite consistent with it currently. I'm open to discussing the general approach, but that's probably better done elsewhere.

OK! Thank you!

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

Successfully merging this pull request may close these issues.

SCP "Is a Directory"
2 participants