sphuber / aiida-shell

AiiDA plugin that makes running shell commands easy.
MIT License
14 stars 7 forks source link

Improve error message of commands consisting of multiple arguments #99

Closed agoscinski closed 2 months ago

agoscinski commented 3 months ago

Commands like git diff, verdi status or python script.py will fail with different opaque error messages. We add a check that the command consists only out of one argument to give the user a clearer error message.

sphuber commented 2 months ago

Thanks @agoscinski . I guess this makes sense, but I am also wondering if there may be valid use cases where the command would contain a space. In principle it accepts a full path to the executable as well, and having a space in that path should be fine. And there are other checks that could be done, such as checking there are no new lines perhaps?

In https://github.com/sphuber/aiida-shell/issues/100 there is another discussion where validation that we are doing that is supposed to be helping is actually backfiring a bit. I was wondering if a better approach is simply to improve the error reporting instead of preemptively trying to validate. But I just checked and the behavior can be quite different based on the command, e.g.:

$ which git rtghget
/usr/bin/git
$ which git diff
/usr/bin/git
/usr/bin/diff

In the first case, the second argument just gets swallowed as which returns nothing since it cannot find the command. But in the second case, the command is expanded to two lines, since each argument happens to correspond to an existing command.

This leads me to what I think is the actual problem. The code is calling:

status, stdout, stderr = transport.exec_command_wait(f'which {command}')

i.e the command is not quoted. If we quote it then which "git diff" would return a non-zero exit code and report the problem. I think this is the proper solution as it would also still support the use case of an actual command containing a space in the path.

Thoughts?

agoscinski commented 2 months ago

Yes that was the actual problem, I did not think about this solution. I think that works better.