Fix shell escaping for single quotes

[brickZA]

Seems the trick is to escape ' as '', i.e.

Frank's socks -> 'Frank''s' socks'

This should fix #118

[bitprophet]

Could you add some tests proving this doesn't break existing behavior? :) The reason we have been avoiding the escaping problem is every change to escaping is likely to break other use cases besides the ones the fixer personally runs into.

[brickZA]

Hi,

I'd be willing to test other cases; you speak as if a regression suite exists, where may I find it ;) In all seriousness, do you have some examples I can use to test?

[bitprophet]

You might want to try looking in the tests/ folder of the repository ;)

git clone <fabric repo>
pip install -r requirements.txt
fab test

Should pass on master last I checked.

There's not a ton of existing tests for contrib because many of them are wrappers around real Unix apps. It should be possible to factor out the escaping code (already is in some cases, albeit mostly for the core run/sudo escaping) to test the functions your patch addresses, though. Then just write unit tests for those escaping functions.

And while "real" tests are great, even just some captured output from a simple fabfile that does a number of basic sed/append/etc operations (esp. with --show=debug on to see the full commands being run) which is run on both master and your branch, would be fine.

The important thing is to test out a number of different types of command strings (doing different types/combinations/nestings of quoting) to make sure that the changes are not too drastic.

[brickZA]

FWIW, I get 6 failures and 13 errors when I do fab test on master. Adding my patches does not seem to change this :) I will add some more tests for escaping shortly.

[bitprophet]

Can you gist/pastebin the output of your test run, and share your exact execution environment (Python interpreter, output of pip freeze, OS/version)?

I ask because master's test suite runs fine for me here on OS X Lion, Python 2.5.6, master of Fabric and the 'ssh' lib installed, and the requirements.txt installed. (So, Fudge 0.9.6, Nose 1.1.2 and those are the big ones off the top of my head.)

[brickZA]

Please find the requested system and test info here: https://gist.github.com/1471928

I could not really come up with a 'unittesty' way of testing my changes, but I did make a test-to-localhost fabfile that demonstrates the working of the patch quite well. The test file as well as the results are here: https://gist.github.com/1472011 It was a good suggestion to add test cases since I found some cases that I wasn't handling before. I must admit that I don't really know exactly why what I did works; let's just say I took an empirical approach :)

In fact it seems that the fabric master branch doesn't successfully complete any of the test examples I chose, whereas they all seem to work with the patches that I have made applied. I hope this passes muster.

[bitprophet]

Thanks a lot -- I'll try to take a look soon. Appreciate your soldiering through on this :)

[bitprophet]

Your test failures look like you're somehow running the test suite without Fabric itself properly installed in your PYTHONPATH. The pip output claims otherwise, though it is saying it's 1.3.2 -- maybe try running pip install -e . when in the root of your Git checkout?

---

A somewhat quick scan of your manual 'test suite' is heartening. I may try something like running the patch with some of my real-world fabfile code and see how that works (though I don't tend to do a LOT of really crazy shell stuff in normal use.)

It should also be noted (mostly for me, if/when this is accepted and documented) that this fixes a large, commonly encountered subset of quoting issues (namely ones found in the contains and append contrib functions) and is not a panacea affecting run/sudo.

[brickZA]

Hi,

Anything I can do to help along this pull request? I'm kinda anxious to stop running a non-upstream branch in production :)

[bitprophet]

I'd still like to get it out with the next bugfix release, though I admit I'm still a little hesitant re: potential for breaking other stuff (despite your tests, which I really appreciate.)

The only other thing that would help (besides me finding the time to focus on it; the last 2-3 weeks have been the holidays where I live :P) is getting other folks to test out your changes too and +1 them as not breaking anything.

[bitprophet]

I think I'll lump this in with 1.4 actually, but take heart, 1.4 is the next thing to work on after 1.3.4 and 1.3.4 should be done in a day or two tops. I just feel that a change like this fits better with a minor release, to better avoid causing backwards compat problems.

[brickZA]

Hart duly taken :)

[bitprophet]

Reviewing this now. A closer look reveals that you pushed the regex escaping into append only, leaving contains requiring a "bare" regex as the input. Was there a reason for this? It feels more sensible to do the opposite and call escape_for_regex inside contains instead.

Otherwise, it's not backwards compatible, and folks who used to do eg contains(path, "I am assuming this auto escapes my quotes") will have their code break. (Put another way, contains is not a subroutine of append, it's its own full fledged API member and must be backwards compatible. At the very least, changing parameter names is a big no-no ;))

I'm going to experiment with mutating your patch in this manner and see how it works.