Check default files for public keys if not set up in ~/.ssh/config.

[carandraug]

Basically, if ~/.ssh/config does not have an IdentityFile field, it will check ~/.ssh/id_rsa and ~/.ssh/id_dsa. At the moment, it was simply failing with "Could not initialize update site".

I took the parsing of the config file out into a separate method because it seems, according to ssh_config(5), that there's a lot more ways to specify where the identity files could be and it seemed that this would keep it cleaner in the future.

For future note, it appears that it might make more sense to collect an array of available identity files, which would be tried later on:

It is possible to have multiple identity files specified in configuration files; all these identities will be tried in sequence. Multiple IdentityFile directives will add to the list of identities tried (this behaviour differs from that of other configuration directives).

[dscho]

Interesting idea. Personally, I do not care all that much about ssh protocol anymore, ever since WebDAV is supported. And I also thought that it would not be too much to ask users to add explicit IdentityFile settings to their ~/.ssh/config.

But since you already put in some work, let's go with it.

However, it is really hard to decipher your changes, for multiple reasons:

• the commit message basically describes the diff in English. The commit message is, however, a better place to put information that cannot be easily deduced from the diff, such as the motivation for the patch.
• instead of refactoring the existing in a separate commit, it is smooshed into the same commit as the one where you add support for additional identity files. That makes it particularly hard to review.

Could you please address both concerns?

[carandraug]

@dscho said:

Interesting idea. Personally, I do not care all that much about ssh protocol anymore, ever since WebDAV is supported. And I also thought that it would not be too much to ask users to add explicit IdentityFile settings to their ~/.ssh/config.

The error message was not really asking the users to do that.

Until last night, I was not even aware I could set IdentityFile in any way other than command line. Most SSH guides (Fiji's would be an exception) won't even mention it (github, Debian, Ubuntu, and Fedora) so I think it's safe to assume this would be more common. Since we have PasswordAuthentication disabled here, there was no way to log in.

I am aware this does not cover all cases ssh identity but I couldn't find a clear and definitive rules to what is used where. Also, such thing would probably be on a class for that specific purpose, outside the imagej project.

By the way, I would probably bump into this later but the reason I bumped on it yesterday was that I could not use it to set an update site locally. It was SSH'ing to localhost instead of... well, nothing.

[...] Could you please address both concerns?

Will do.

[carandraug]

When you have the time, could you please re-review it?

[carandraug]

ping

[dscho]

Okay, here's the deal. I will merge this as soon as:

• the FIXME is removed
• the commit message of Move search for identity files into a ConfigInfo method. is fixed (no dot at the end, just like all the other commit messages have no commit subject ending in a dot)
• the commit Move search for identity files into a ConfigInfo method. does nothing else than refactor the method, i.e. move it verbatim (modulo indentation).
• the commit message of Make use of default IdentityFiles if parsing of ~/.ssh/config brings … …up nothing. is fixed (again, no period at the end of the commit subject, a shorter commit subject, and most importantly: all the relevant information about the commit that cannot be readily inferred from the diff should be put into the commit message's body).
• the coding style is adhering to the rest of the source code (camelCase instead of under_score, spaces in { "id_rsa", "id_dsa" }, etc)
• a note is added to the commit message of the second commit that says that the solution helps in many circumstances, but is not quite correct: it fully ignores the user's IdentityFile setting in $HOME/.ssh/config (which can be even Host-specific). (And no, FIXME is not the correct way to document this, not if you want this to be merged.)

Deal?

[carandraug]

@dscho said:

Okay, here's the deal. I will merge this as soon as:

• the FIXME is removed

Done. As I mentioned on IRC, the usage of FIXME did not meant "come fix this". It is just a convention in many other projects to mark things that are broken. Some text editors will even highlight it in a special manner. Should I open 1 or 3 bug reports for them?

• the commit message of Move search for identity files into a ConfigInfo method. is fixed (no dot at the end, just like all the other commit messages have no commit subject ending in a dot)

Done. I removed the final dot from the end of the commit message.

• the commit Move search for identity files into a ConfigInfo method. does nothing else than refactor the method, i.e. move it verbatim (modulo indentation).

This is just impossible unless you prefer to have a point in history where the package won't even build. Small adjustments were required (as mentioned on the new commit message) but I made them even less than before. Let me know if you really prefer the commit that breaks the build, and I will split this into two.

• the commit message of Make use of default IdentityFiles if parsing of ~/.ssh/config brings ..up nothing. is fixed (again, no period at the end of the commit subject, a shorter commit subject, and most importantly: all the relevant information about the commit that cannot be readily inferred from the diff should be put into the commit message's body).

Again, I removed the dot at the end of the commit message. I expanded the previous comments on the source, and then used them as commit message. Please let me know what is not clear.

• the coding style is adhering to the rest of the source code (camelCase instead of under_score, spaces in { "id_rsa", "id_dsa" }, etc)

Done (I hope).

• a note is added to the commit message of the second commit that says that the solution helps in many circumstances, but is not quite correct: it fully ignores the user's IdentityFile setting in $HOME/.ssh/config (which can be even Host-specific). (And no, FIXME is not the correct way to document this, not if you want this to be merged.)

I moved the comments to the commit message. But it doesn't really ignore the IdentityFile settings. This method is meant to use the default values and is only called if there is no IdentityFile mentioned on the config files. If there is an IdentityFile specified, then it will be caught by the previous code (which is left intact).

Deal?

Deal.

[carandraug]

ping

[dscho]

@carandraug it would be nice if you could address the issues I raised, I would then merge. Thanks!

[ctrueden]

I am happy with this as-is. Thank you @carandraug, and apologies for the incredibly long delay in merge.