fix(ssh): optimize traversing ssh config includes

[Rogach]

Previously each config file was read with separate sed command to extract Include directives. This can get quite slow if there are a lot of included files (on my machine it was ~0.4s for each TAB keypress).

Instead of reading each individual file, we can put all the files at a current recursion level into a single sed invocation and extract all the directives in one go.

[akinomyoga]

Thank you! These are style and minor changes.

[Rogach]

@akinomyoga Implemented the requested changes.

The option -l (linewise splitting) may be specified to _comp_split

This fails the tests, because single Include may specify multiple included files on the same line (separated by spaces).

[akinomyoga]

This fails the tests, because single Include may specify multiple included files on the same line (separated by spaces).

Thank you for pointing this out. Hmm, so it means that each element of the resulting array of _comp_split does not necessarily correspond to one Include directive. Then, the variable name might better to be something like include_files rather than include_directives, but it's up to you whether to change it to a better one.

[Rogach]

Renamed the variable.

[akinomyoga]

Thank you for quick responses and updating!

These are cosmetic ones. With these addressed, I'd approve it.

[Rogach]

Applied the requested changes.

[scop]

Looks mostly good to me, added a comment about the "16" to make it less magical.

However, the commit message seems to talk about an actual fix in addition to a performance improvement, whereas the PR title here and the commentary seems to refer only to the perf improvement.

If there's a fix in here, could we add a test case showcasing what was broken before this and fixed now?

Or if there's no fix, could we rephrase the commit message some to clarify that? Also, if there's no actual fix here, the conventional commit type would be better set as perf instead of fix.

[Rogach]

However, the commit message seems to talk about an actual fix in addition to a performance improvement, whereas the PR title here and the commentary seems to refer only to the perf improvement.
If there's a fix in here, could we add a test case showcasing what was broken before this and fixed now?

Story about a fix is hidden in the resolved diffs. Basically the problem we found during discussion was that if a user config file included some config in /etc/ssh/, and that config used relative include to include another config, this last Include must be resolved relative to users ~/.ssh. Previous implementation would have resolved it relatively to /etc/ssh/.

I don't know how to add a test for this, since the scenario involves creating an ssh config file somewhere under /etc/ssh/ and that isn't happening.

Or if there's no fix, could we rephrase the commit message some to clarify that? Also, if there's no actual fix here, the conventional commit type would be better set as perf instead of fix.

Original commit message was about perfomance improvement (that's probably the more interesting part anyway, since the failing scenario seems extremely unlikely). However @akinomyoga suggested that it would be changed to talk about the include fix. Should I change it back?

[akinomyoga]

There is a fix. At least the commit changes the behavior, so, in my opinion, we should mark it as fix rather than perf.

Actually, the ideal way is to separate the commit into the fix one and the perf one so that we could later track the relevant changes when necessary.

[Rogach]

Actually, the ideal way is to separate the commit into the fix one and the perf one so that we could later track the relevant changes when necessary.

Ideally yes, but in this particular case it will be difficult. There are two possible commit orderings:

1. "first fix, then perf": in the "fix" commit we'll need to add an additional argument to _included_ssh_config_files, something like top_level_config, to pass around the include base. Then the "perf" commit will be rewriting recurisve version into a loop, removing this additional argument.
2. "first perf, then fix": here I'm not even sure how to keep the old behavior in the non-recursive version. Something like making another array of include bases? I might be okay with doing it in some other language, but in Bash array handling is will be too difficult for me and I might easily make a lot of mistakes.

I support marking commit as "fix". Or maybe we can mark it as "fix,perf"?

[akinomyoga]

For me it's optional to separate the commit; I wouldn't request you to separate it. But if you are willing to separate the commit, either option works. If it were me, I think I'd use option 1.

[Rogach]

I really don't want to separate it :)

[scop]

Ok, merged as is. Thanks!