Do not parse aliases in POSIX mode when passing to shell

Review Request #10149 — Created Sept. 19, 2018 and submitted

Information

RBTools
release-0.7.x
07a2ccb...

Reviewers

We are using shlex.split to parse aliases, which defaults to a POSIX mode,
which strips quotes and evaluates escape sequences. This lead to issues
where an alias of the form !find . -iname '*.pyc' -delete would be
evaluated into find . -iname *.pyc -delete run by the shell, which
would result in the *.pyc being evaluated by the shell instead of by
find.

Now we operate in POSIX mode only when we are not passing the command
to the shell so shell-specific sequences will be untouched. This results
in aliases not needing double escaping when being passed to the shell.

When not passing to the shell, we still parse in POSIX mode to remove
quotes, etc. because otherwise subprocess.call() will pass the quoted
strings as arguments (e.g., ['cmd', '"arg with spaces"'] instead of
['cmd', 'arg with spaces']).

While I was here, I also modernized the docstrings of all the alias
methods to our doc standards. replace_arguments also now returns a
list instead of a generator because we were always immediately wrapping
it in a call to list anyway.

  • Ran unit tests.
  • Ran rbt clear-pycs with .pyc files in the current directory. All
    .pyc files were recursively removed.
Description From Last Updated

E303 too many blank lines (2)

reviewbotreviewbot

E303 too many blank lines (2)

reviewbotreviewbot

Missing a period.

daviddavid

Too many spaces after #

daviddavid

typo: decote

daviddavid

Extra ` at the end

daviddavid

I feel like this would read a little bit more clearly if you swapped them (unicode or list of unicode).

daviddavid

Remove this blank line.

daviddavid

Typo: veresion -> version

daviddavid

This needs a period at the end.

daviddavid

Should this be "Quoted tokens"?

daviddavid
brennie
brennie
Review request changed
Summary:
Fix alias processing to not mangle quotes in aliases
Do not parse aliases in POSIX mode when passing to shell
Description:
   

Previously, we were using shlex.split, which defaults to a POSIX mode,

    which strips quotes and evaluates escape sequences. This lead to issues
    where an alias of the form !find . -iname '*.pyc' -delete would be
    evaluated into find . -iname *.pyc -delete run by the shell, which
    would result in the *.pyc being evaluated by the shell instead of by
    find.

   
~  

Now we operate in non-POSIX mode, which returns all tokens in their

~   quotes and does not process escape sequences. These will be handed off
~   to the shell as-is so that aliases do not need double escaping.

  ~

Now we operate in POSIX mode only when we are not passing the command

  ~ to the shell so shell-specific sequences will be untouched. This results
  ~ in aliases not needing double escaping when being passed to the shell.

  +
  +

When not passing to the shell, we still parse in POSIX mode to remove

  + quotes, etc. because otherwise subprocess.call() will pass the quoted
  + strings as arguments (e.g., ['cmd', '"arg with spaces"'] instead of
  + ['cmd', 'arg with spaces']).

   
   

While I was here, I also modernized the docstrings of all the alias

    methods to our doc standards. replace_arguments also now returns a
    list instead of a generator because we were always immediately wrapping
    it in a call to list anyway.

Commit:
c927aecaed4baaa3e77bfd6c9ec6cea1fd9728eb
ab3aff9e719bc0418150aad51d83309f74a197a6

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

brennie
brennie
david
  1. 
      
  2. rbtools/utils/aliases.py (Diff revision 4)
     
     
    Show all issues

    Missing a period.

  3. rbtools/utils/aliases.py (Diff revision 4)
     
     
    Show all issues

    Too many spaces after #

  4. rbtools/utils/aliases.py (Diff revision 4)
     
     
    Show all issues

    typo: decote

  5. rbtools/utils/aliases.py (Diff revision 4)
     
     
    Show all issues

    Extra ` at the end

  6. rbtools/utils/aliases.py (Diff revision 4)
     
     
     
    Show all issues

    I feel like this would read a little bit more clearly if you swapped them (unicode or list of unicode).

  7. rbtools/utils/aliases.py (Diff revision 4)
     
     
    Show all issues

    Remove this blank line.

  8. 
      
brennie
david
  1. 
      
  2. rbtools/utils/aliases.py (Diff revision 5)
     
     
    Show all issues

    Typo: veresion -> version

  3. rbtools/utils/aliases.py (Diff revision 5)
     
     
    Show all issues

    This needs a period at the end.

  4. rbtools/utils/aliases.py (Diff revision 5)
     
     

    What does this error output look like? Are the exception values good to show to a user?

    Should we list the name or content of the alias that failed to parse?

    1. In the case of a missing quote we get "No closing quotation".

      I've updated the message to include the alias name and its content.

  5. 
      
brennie
brennie
brennie
david
  1. 
      
  2. rbtools/utils/aliases.py (Diff revision 8)
     
     
    Show all issues

    Should this be "Quoted tokens"?

  3. 
      
brennie
david
  1. Ship It!
  2. 
      
brennie
Review request changed
Status:
Completed
Change Summary:
Pushed to release-0.7.x (fb7e8d3)