amoffat / sh

Python process launching
https://sh.readthedocs.io/en/latest/
MIT License
6.98k stars 506 forks source link

Nested contexts duplicate commands #690

Closed johntrimble closed 1 year ago

johntrimble commented 1 year ago

When using nested contexts like the following:

with sh.echo.bake("test1", _with=True):
    with sh.echo.bake("test2", _with=True):
        out = sh.echo("test3")

I'd expect to get the following command:

/usr/bin/echo test1 /usr/bin/echo test2 /usr/bin/echo test3

What I actually get is:

/usr/bin/echo test1 /usr/bin/echo test1 /usr/bin/echo test2 /usr/bin/echo test3

It looks like the source of our problem is here:

https://github.com/amoffat/sh/blob/4941fe03dc05bf3a2414a60999646a5501488edf/sh.py#L1421-L1428

The first with sh.echo.bake("test1", _with=True) pushes /usr/bin/echo test1 onto the stack. The second with sh.echo.bake("test2", _with=True) pushes /usr/bin/echo test1 /usr/bin/echo test2 onto the stack. Then out = sh.echo("test3") comes along and we prepend both /usr/bin/echo test1 /usr/bin/echo test2 and /usr/bin/echo test1.

I think I have solution, but it seems too easy:

# aggregate any 'with' contexts
for prepend in get_prepend_stack():
    pcall_args = prepend.call_args.copy()
    # don't pass the 'with' call arg
    pcall_args.pop("with", None)

    call_args.update(pcall_args)
    if not kwargs.get("_with", False):
        cmd.extend(prepend.cmd)

Essentially, if this command is for a context, don't prepend anything to the command. We still need to update call_args or things like sh.contrib.sudo setting _in will get lost.

I'm happy to provide a PR, but want to make sure this is on the right track first.

amoffat commented 1 year ago

That makes sense. I think you are the first person to use nested _with :) If you can add the correct behavior and some tests, I will merge the PR. lmk if you run into issues.