Expose the result message in case of an SSH authentication error

[sschuberth]

Not only expose the error message, but also the result message from
calling SSH which contains details about the cause. At the example of
using the wrong SSH key for authentication, with this the console shows

download check: exit status 255: Permission denied (publickey).

instead of just

download check: exit status 255

and the log shows

trace git-lfs: ssh: download with url failed,
error: exit status 255, message: Permission denied (publickey).

instead of just

trace git-lfs: ssh: download attempted with url.
Error: exit status 255

[ttaylorr]

Looking cool. What do you think about changing the auth.SshAuthenticate method to return the contents of the command's stdout in an error if the command exited non-zero?

This is sort of what I was working on a while ago, but never pushed. Locally, I have a wrapper over *exec.Command (in the subprocess package) that has this behavior.

[sschuberth]

What do you think about changing the auth.SshAuthenticate method to return the contents of the command's stdout in an error if the command exited non-zero?

I was considering this, except that I would have returned the command's stderr, not stdout, but I guess that's what you meant anyway. However, that would only get us the error message, but not the exit code (as a string), which might be useful, too.

Locally, I have a wrapper over *exec.Command (in the subprocess package) that has this behavior.

Unfortunately, commands are not consistent about whether an error message is printed to stderr or stdout, so I went the safe route and only did so for ssh which we know to print the error message to stderr.

Does that make sense?

[ttaylorr]

Makes sense to me. I responded to both of your comments down below with regards to some possible future refactorings, but I don't have a problem merging this in as-is.

I was considering this, except that I would have returned the command's stderr, not stdout, but I guess that's what you meant anyway. However, that would only get us the error message, but not the exit code (as a string), which might be useful, too.

Sorry, I did mean stderr, not stdout. Why not capture both the output and exit code with something like:

msg, _ := ioutil.ReadAll(cmd.Stderr)
if err, ok := cmd.Wait().(*exec.ExitError); err != nil && ok {
        err = errors.Wrapf(err, "cmd exited with: %s", msg)
}

return err

This works fine since *exec.ExitError includes the exit code in its Error() string method.

Unfortunately, commands are not consistent about whether an error message is printed to stderr or stdout, so I went the safe route and only did so for ssh which we know to print the error message to stderr.

If you wanted to go the route of having a command wrapper type, what do you think about giving a parameter to the type specifying which io.Reader to use?

[sschuberth]

I think err = errors.Wrapf(err, "cmd exited with: %s", msg) is not a bad idea, but it would print stderr and exit code in the opposite order. At the example of an SSH authentication failure it would read

cmd exited with: Permission denied (publickey).: exit status 255

but I like my

exit status 255: Permission denied (publickey).

slight better :-)

So I hope you don't mind me merging this and leave the rest as a possible future improvement.