Add strip option to capture, reinstate stripping by default in the capture methods

[robd]

I am nervous about the decision we made over in #239, to no longer strip the output of capture. Having seen some of the issues that are raised, I think there might be quite a lot of fallout from this. I'm particularly worried that this may not fail in an easy-to-understand way, and that we may waste a lot of everyones time dealing with them. I also think that in many cases, eg the one liners in Capistrano, stripping is a reasonable default.

I do still think that we should make the API consistent between the Local and NetSSH backends. Therefore, I'd like to propose that we make stripping whitespace the default, but add an option to the capture method to turn off stripping.

@townsen Would you be OK with adding the strip: false option in the Local cases where you need full whitespace?

This PR also adds some docs for the basic SSHKit usage, which I believe were sort of missing from the README.

[leehambley]

I am nervous about the decision we made over in #239, to no longer strip the output of capture. Having seen some of the issues that are raised, I think there might be quite a lot of fallout from this.

Welcome to FOSS maintainer-ship of a popular project!

I also think that in many cases, eg the one liners in Capistrano, stripping is a reasonable default.

That's basically why I did it, although it's not a solid reason :) Capistrano could also be tweaked to strip the results of captures() to allow SSHKit to do the right thing, correctly whilst not breaking Cap, but I'd still keep the strip: {bool} option, just maybe leave the default as false and monkeypatch the method in Capistrano to keep the old behaviour, and send a deprecation warning, or something.

I do still think that we should make the API consistent between the Local and NetSSH backends. Therefore, I'd like to propose that we make stripping whitespace the default, but add an option to the capture method to turn off stripping.

Agreed, absolutely even though most people don't capture() locally, in my experience, consistent APIs are better, no doubt.

This PR also adds some docs for the basic SSHKit usage, which I believe were sort of missing from the README.

Cool, good catch.

[robd]

Welcome to FOSS maintainer-ship of a popular project!

Yes - totally! :) I'm trying to focus now on having SSHKit behave in the least surprising way.

That's basically why I did it, although it's not a solid reason

I think this one is finely balanced, but I think your reasoning, to strip, makes sense in most situations, especially the trailing newline.

monkeypatch the method in Capistrano to keep the old behaviour, and send a deprecation warning, or something.

The thing about this is that it might make the documentation a bit confusing - if people read the SSHKit docs, Capistrano then does the opposite. I'm also thinking coding examples which work in Capistrano on Stack overflow etc, might not work in pure SSHKit.

[leehambley]

I think this one is finely balanced, but I think your reasoning, to strip, makes sense in most situations, especially the trailing newline.

Looking back I think it would have been wiser to always document capture being used with capture(:athing) ~= /some string/ rather than in absolutes, but that ship has sailed.

The thing about this is that it might make the documentation a bit confusing

You're right… let's wait for some other feedback from others, and then we might merge this.

[robd]

Looking back I think it would have been wiser to always document capture being used with capture(:athing) ~= /some string/ rather than in absolutes, but that ship has sailed.

Yeah I agree. The thing I'm most worried about is this sort of usage (untested example):

file_path = capture('ls -r1 | head -1')
execute("mv #{file_path} another_path")

It seems like having a newline on the end of file_path seems like it could be a pretty bad thing. I can imagine other much worse examples depending on what people are doing with the output. Especially, it could mean that command line switches aren't evaluated.

I agree would be good to get others' feelings - maybe @townsen, @mattbrictson if you have time?

[ghost]

I agree with most of what has been said regarding the history of how this came to be, it was an understandable 'simplification', but I do feel that it was a mistake to strip any output. I have a few reasons for this:

• Silently removing information that you have no way of getting back is always bad, and
• If you really want it stripped then the fix is very simple - just add .strip to the capture call, and
• I imagine the majority of Capistrano users understand that whitespace is significant - after all it is the command itself that outputs the newline (or whatever), not any framework. It's kind of a core Unix principle so that command pipelines and shell stuff work ok.

But I think the above is easy to agree on, the question is what to do next! My opinion on that is:

• I'm always in favour of making things 'right', so that the future users don't learn 'bad' habits, or have to go through historical hoops, or have inconsistencies.
• Breaking past behaviour is not a problem: People don't have to upgrade the Cap and SSHkit gems, and if they do then the effort of altering their captures strip behaviour is just part of the price.
• Since fixing this 'properly' is a breaking change, we should honour Semantic Versioning and make it part of a major release, perhaps along with any other breaking changes we're collecting.
• But if we are not ready for a major release, then the proposal of a boolean option to enable/disable stripping works well with the default true to preserve backwards compatibility. I would be happy to update my existing code with that.
• Note that I don't think this is a 'clean' way of doing it for version 4 - as it is largely redundant: it takes more characters to use that feature than to add .strip to the end!

I know this is a long reply, I hope it helps!

Nick

[robd]

I think the above is easy to agree on, the question is what to do next!

Yes, I agree with the points above too!

Since fixing this 'properly' is a breaking change, we should honour Semantic Versioning and make it part of a major release, perhaps along with any other breaking changes we're collecting.

I like this idea. Maybe we should add an API cleanup wishlist .md file (or a long running issue?) where we can keep a note of things we want to fix for the next major version API.

But if we are not ready for a major release, then the proposal of a boolean option to enable/disable stripping works well with the default true to preserve backwards compatibility.

I would appreciate that. I have made a number of other changes which should be backwards compatible, and if possible, I'd like to release them as a minor release and fix any problems I've introduced first.

[mattbrictson]

I agree with the proposal to strip by default, with new support for a :strip => false option, and then change the behavior in a future major version.

As an anecdote, I tried doing a cap deploy using SSHKit master and was initially very confused by the resulting error. One of my tasks was doing something like this (similar to @robd's hypothetical example):

set :api_key, capture("grep ... awk ... etc etc")
...
execute "curl ... -F access_token=#{fetch(:api_key)} ..."

The curl failed because somehow a semicolon was mysteriously inserted into the command string. Apparently there is some magic in cap or sshkit that translates newlines into semicolons?

Anyway, it took me a while to finally trace the issue back to the fact that the capture output had changed to include the trailing newline.

Long story short: I think this change will cause confusion. 😉 Better save it for a major version number release.

[robd]

OK, consensus seems to be that it would be good to leave the breaking API change until the next major release. I introduced a new file -BREAKING_API_WISHLIST.md - to keep notes on these type of changes.

[leehambley]

Excellent thought @robd :)

[robd]

Cool. I think we have agreement on this, so would it be possible to merge this?

[leehambley]

Thanks for the teamwork chaps.