Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Nested contexts duplicate commands #690

Closed
johntrimble opened this issue Jul 29, 2023 · 1 comment · Fixed by #691
Closed

Nested contexts duplicate commands #690

johntrimble opened this issue Jul 29, 2023 · 1 comment · Fixed by #691

Comments

@johntrimble
Copy link
Contributor

johntrimble commented Jul 29, 2023

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:

sh/sh.py

Lines 1421 to 1428 in 4941fe0

# 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)
cmd.extend(prepend.cmd)

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
Copy link
Owner

amoffat commented Jul 29, 2023

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants