search

mwilliamson / spur.py

Run commands and manipulate files locally or over SSH using the same interface
BSD 2-Clause "Simplified" License
267 stars 37 forks source link

Check the result of the cwd in SshShell #48

Closed beenje closed 8 years ago

beenje commented 8 years ago

Using the cwd argument used to run something like that: $ cd '/some/path'; command -v 'ls' > /dev/null 2>&1 || which 'ls' > /dev/null 2>&1; echo $?; exec 'ls'

Even if the directory doesn't exist, the command is run. I first tried to change it to: $ cd '/some/path' && { command -v 'ls' > /dev/null 2>&1 || which 'ls' > /dev/null 2>&1; echo $?; exec 'ls'; }

But we can't check the which/command result if the cd fails. To minimize the impact, I changed it to: $ cd '/some/path' 2>/dev/null; command -v 'ls' > /dev/null 2>&1 || which 'ls' > /dev/null 2>&1; echo $?; cd '/some/path' && exec 'ls'

We have to run cd before to check if the command exists because we might run a script in the cwd directory. So we don't catch if this cd fails and run it again just before the exec where we catch it. The only drawback is that if a command is not found because the cwd is incorrect, we'll raise NoSuchCommandError instead of RunProcessError.

Fixes #46

mwilliamson commented 8 years ago

Thanks for the pull request. I think it can be made a little simpler, so I've left some comments to that effect.

beenje commented 8 years ago

Added new exception NoSuchDirectoryError. The behavior is consistent between local and ssh shell.

mwilliamson commented 8 years ago

Thanks, and apologies for the delay!

beenje commented 8 years ago

No problem. Thanks!

mwilliamson commented 8 years ago

While updating the changelog, I just realised that there's now inconsistent behaviour if there are other reasons why setting the current directory fails, such as permissions. While checking the documentation for that, I also discovered that Windows and Unix have inconsistent behaviour in one of the new test cases (see https://github.com/mwilliamson/spur.py/commit/c088d129eff48e96440a36a417da77b14af7e7fb)

A solution to the latter problem hasn't hit me immediately, but the former can probably be solved by raising a more general exception -- something more like CouldNotChangeDirectoryError, and updating the handling of exceptions accordingly. I was going to take a look, but give me a shout if you're interested in looking yourself.

beenje commented 8 years ago

Sorry for the inconsistencies. I'm on vacations and I won't have the opportunity to look at it before mid-september... So if you have some time.