Skip to content

Conversation

@imposibrus
Copy link

fixes #118

My first experience with Rust. I didn't understand why the InvalidIncludeErrorDetails::HostsInsideHostBlock case is needed, but I can fix the code if you suggest a couple of examples for tests)


if patterns.contains(&"*".to_string()) {
global_host = Host::new(patterns.clone());
hosts.push(global_host.clone());
Copy link
Owner

Choose a reason for hiding this comment

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

This would add an host with patterns but empty entries. I'm not sure if it's needed 🤔 global_host by itself may not be needed as we have current_host now.

let mut global_host = Host::new(Vec::new());
let mut is_in_host_block = false;
let mut hosts = Vec::new();
let mut current_host: Option<Host> = None;
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
let mut current_host: Option<Host> = None;
let mut current_host = Host::new(vec!["*".to_string()]);

current_host is the global host by default. What about this?

@imposibrus
Copy link
Author

@quantumsheep sorry for wasting your time reviewing the previous commit. i think we can simplify this fix to:

if !patterns.contains(&"*".to_string()) {
    is_in_host_block = true;
    hosts.push(Host::new(patterns));
}

after this, everything from Host * will be at global_host. actually i think it's not quite right, but it works for most simple cases.

Offtopic:

While working on the task, I discovered a not quite correct behaviour: according to the specification, the entries below cannot overwrite those already declared above.
I commented out one test case and left the TODO.

Case:

# main.conf
Host *
    User from_main

Include /included.conf
# included.conf
Host included
    User test_included
    HostName included-host

ssh -F main.conf -G included | grep -i user will return user from_main
but Parser.parse() (and Host.apply_patterns() also) uses host.extend_if_not_contained(&global_host); so host fields are not overwritten by the higher priority Host *.
I think we should take into account the order of the entries. I can create a issue for this.

@quantumsheep
Copy link
Owner

after this, everything from Host * will be at global_host. actually i think it's not quite right, but it works for most simple cases.

I think it's not right too. What about *.example.com?

I think we should just set current host when an host in host is found. Include directives should be handled like an extension of the current file's content.

Wildcards are handled by HostVecExt::apply_patterns.

What do you think? Really sorry to make you change your code multiple times.

@quantumsheep
Copy link
Owner

Hey! I took the original idea and simplified it in f04c0b4. Thanks a lot!

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.

Parsing ssh config doesn't work with Include statements in it

2 participants