within(dir) fails now when dir is an empty string or nil

[mherold]

within('') and within(nil) used to work before #453, but now it fails with "Directory does not exist".

I'm not sure whether the new behaviour is intended? It breaks things in other gems which use sshkit, e.g. capistrano-passenger: https://github.com/capistrano/passenger/blob/6dc408ec690d0cbe8c2e9d86c33ad4f7977dfa73/lib/capistrano/tasks/passenger.cap#L13

Example to reproduce:

> require 'sshkit'
> include SSHKit::DSL
> run_locally { within('') { puts(capture(:ls)) } }
SSHKit::Command::Failed (if test ! -d ''; then echo "Directory does not exist ''''" 1>&2; false; fi exit status: 256)
if test ! -d ''; then echo "Directory does not exist ''''" 1>&2; false; fi stdout: Nothing written
if test ! -d ''; then echo "Directory does not exist ''''" 1>&2; false; fi stderr: Directory does not exist ''''

The reason for the behaviour change seems to be that empty strings will also be shellescape'd:

> ''.shellescape
 => "''" 

(within() in /lib/sshkit/backends/abstract.rb)

[leehambley]

Hi @mherold, I'd say that this is definitely not an intended consequence of #453, but a welcome one.

within('') and within(nil) are definitely ambiguous, and it would be safer if users/maintainers didn't rely on a quirk of how the shell escaping happened to work.

My suggested solution would be to capture a default with capture(:pwd) before entering these within() blocks and incase whatever variable ends up expanding to nil or '', fall-back to a qualified directory name as captured with capture().

Very open to learning more about the use-case for '' and nil as valid directory names, I'm assuming that it's a simple case of "variable happened to be empty, meaning no nesting, rather the pwd" but I'm not certain I've understood fully!

[mherold]

Hi @leehambley, thanks for your response! Yes, the line I linked to seems to be the use case "depending on a condition we do not want to change the directory / nesting".

If you can't come up with a use case where the "old" behaviour is actually required, I can't think of a use case either.

One could argue that it would be nice (and faster) if within() supported a "no-op" (implemented by adding a guard checking whether the argument to_s is empty). But I also see your point that it's less ambiguous if people are forced to be explicit here.

[leehambley]

If you can't come up with a use case where the "old" behaviour is actually required, I can't think of a use case either.

I think that might be the best argument yet to close the issue. Thanks for reporting it. I prefer an obvious API, so implicit things like this feel like they put the burden into the next layer of software.

Ahoj.