fix: support nushell on linux

[klesh]

What it does

Fixes #1605

Why it happend

nushell doesn't accept -- at all, it would throw an exception when it was given

How does the fix work

Remove the -- argument

Will it cause other problems?

I don't think so, the -- for sh is to tell it that the rest of the arguments are meant for the command to be executed, which is not useful in this case because there is only one argument left.
I might be wrong though, 😊

[joelim-work]

This will break other shells because -- here doesn't mean to treat the remaining arguments as positional arguments. It is a placeholder which gets assigned to $0 when executing the script, and everything after will be assigned to $1, $2, etc. as expected. Try running the following command and see what happens:

bash -c 'echo "$@"' foo bar baz

You can also test this by adding the following to your lfrc file:

cmd mkdir %mkdir "$@"

Unfortunately Nushell isn't designed to be a POSIX-compatible shell, and I think the only way is to add shell-specific code similar to what is done for Windows CMD:

lf/os_windows.go

Lines 104 to 117 in cf99626

	// Windows CMD requires special handling to deal with quoted arguments
	if strings.ToLower(gOpts.shell) == "cmd" {
	var builder strings.Builder
	builder.WriteString(s)
	for _, arg := range args {
	fmt.Fprintf(&builder, ` "%s"`, arg)
	}
	shellOpts := strings.Join(gOpts.shellopts, " ")
	cmdline := fmt.Sprintf(`%s %s %s "%s"`, gOpts.shell, shellOpts, gOpts.shellflag, builder.String())
	cmd := exec.Command(gOpts.shell)
	cmd.SysProcAttr = &windows.SysProcAttr{CmdLine: cmdline}
	return cmd
	}

EDIT: Actually I suspect the following change might be OK too. If it works then I would prefer to avoid having custom code just to handle a specific shell.

diff --git a/os.go b/os.go
index 962bb41..ccddb75 100644
--- a/os.go
+++ b/os.go
@@ -139,7 +139,7 @@ func shellCommand(s string, args []string) *exec.Cmd {
 		s = fmt.Sprintf("IFS='%s'; %s", gOpts.ifs, s)
 	}
 
-	args = append([]string{gOpts.shellflag, s, "--"}, args...)
+	args = append([]string{gOpts.shellflag, s, gOpts.shell}, args...)
 
 	args = append(gOpts.shellopts, args...)

[joelim-work]

Also please add something like Fixes #1605 to the PR description to link #1605 properly.

[klesh]

Please correct me if I am wrong: shellCommand could be called by other functions like eval which would pass a list of args that would not work when the shell is POSIX compliant shell on *nix systems.

I don't understand your fix If I understood the problem correctly. It doesn't seem to be working for nushell

[klesh]

Maybe we could add a new option like shellargsep = "--" so Non-Posix shell users could set it to empty?

[jameschensmith]

Sorry for randomly popping in here. I just wanted to chime in here to note that maybe you could take notes from how just handles this situation. just has a setting called positional-arguments, and when set it will support passing positional arguments to the command. The implementation is basically what currently exists in lf (reference), with the difference being just uses the recipe's name rather than "--" or gOpts.shell.

I think what would be desired here is to have a new option for positional arguments (e.g., positionalArguments), and to have it set to true by default (the opposite of just). This would make lf function the same as it currently does, but gives the option to remove it for shells which don't use / support positional arguments.

[joelim-work]

Please correct me if I am wrong: shellCommand could be called by other functions like eval which would pass a list of args that would not work when the shell is POSIX compliant shell on *nix systems.

@klesh I'm not sure if you understood my first comment, but to be more clear: With your changes the command will still run but the arguments will be shifted by one, that is the first argument will now be assigned to $0, the second argument will be assigned to $1, etc.. This is incorrect and will cause existing scripts to fail.

When I suggested to replace -- with the shell name, I was hoping that nu -c would just ignore the extra argument. It seems that there's only an error when passing -- as nu -c would interpret this as an unknown option.

I think what would be desired here is to have a new option for positional arguments (e.g., positionalArguments), and to have it set to true by default (the opposite of just). This would make lf function the same as it currently does, but gives the option to remove it for shells which don't use / support positional arguments.

@jameschensmith I guess we can consider adding an option (or just check the value of gOpts.shell) to determine whether or not to append arguments, although I prefer to avoid it if possible. One thing I'm not sure about is whether it's possible to pass arguments to nu -c <command>, see #1303. If there is a way to pass arguments, then we can look at how to support this in lf, but if not then we could just pass an extra shell name argument and hope that nu -c ignores it.

Correct me if I'm wrong on any of this, I'm not an expert on using Nushell.

[klesh]

@joelim-work Thanks for the clarification, I believe I understood your comment about the argument shifting. However, nu does not ignore the extra arguments and it would throw an error on any unknown option.

I would recommend adding an option instead of checking the shell just like @jameschensmith (thanks for your input BTW) because there will be an infinite number of shells out there and hardcoding them is not flexible.

[jameschensmith]

One thing I'm not sure about is whether it's possible to pass arguments to nu -c <command>[...]. If there is a way to pass arguments, then we can look at how to support this in lf, but if not then we could just pass an extra shell name argument and hope that nu -c ignores it.

In command mode (-c), Nushell currently (I don't think they ever will for reasons explained ahead, but they could in the future) doesn't support passing arguments in the way that some of the other shells do. What Nushell does have, though, is the flag --stdin, which Nushell uses to accept piped input to the command. I know that's not too applicable here, but I thought I would add that just in case.

Because Nushell doesn't support positional arguments (i.e., $0, $1, $2, ...) in command mode, it will not work if any arguments are passed that are not flags that it knows about. My personal recommendation which avoids shell-specific checking would be what I recommended above (i.e., a new positionalArguments option), but I can understand how adding more options can be a concern. I do personally think that positionalArguments would be more flexible than shellArgSep.

[klesh]

@jameschensmith Aah, I see, you are correct. shellArgSep won't cut it because we still need to pass arguments into the command.
Maybe we could just combine the command and arguments into a single string to solve the problem? Just like the solution for the cmd.exe on Windows.

So, there are a couple of ways to achieve the goal:

1. Change the behavior to always combine s and args without caring the actual shell
2. Adding a new option to activate such a behavior, combineArgs maybe?

[joelim-work]

So I had a look at the fish shell and apparently it doesn't have the concept of $0 either. But it does allow arguments to be passed through via $argv:

fish -c 'mkdir $argv' foo bar

And the corresponding lfrc config would be as below (type :mkdir foo bar on the command line):

set shell fish

cmd mkdir %mkdir $argv

Based on this, I think I agree with adding an option to control this behavior. The only question is, does it make sense to prevent adding -- but still pass additional arguments, or prevent passing both -- and the additional arguments? I was thinking that the former would make more sense, because normally you want to pass through the additional arguments, and in the case of Nushell the user would just not provide any since nu -c doesn't support it anyway.

As for the option name, I haven't really given it much thought, but I think it should start with shell just to be consistent with the other shell-related options (similar to Vim).

[klesh]

How about shellcmdargs = false to indicate that the shell does NOT support passing args on Command mode? 😂
And then lf would combine the command and args into a script to execute with the shell?

[joelim-work]

How about shellcmdargs = false to indicate that the shell does NOT support passing args on Command mode? 😂 And then lf would combine the command and args into a script to execute with the shell?

@klesh Sorry I didn't understand your comment, could you please write some code/pseudocode to explain?

[klesh]

Oh, sorry for the mess. Here is the idea, take the mkdir as an example.

1. Add an option named shellcmdargs with a default value true to indicate if the shell supports -- with positional arguments which is the existing way of handling things, nothing changed.

• define cmd: cmd mkdir %mkdir "$@"
• user typed: :mkdir foo bar
• end up: sh -c 'mkdir "@$"' -- foo bar

2. When shellcmdargs=false, it means the shell won't accept --, and/or positional arguments. So, we would combine them all into a script for the shell to work properly.

• define cmd: cmd mkdir %mkdir
• user typed: :mkdir foo bar
• end up: nu -c 'mkdir foo bar'

[joelim-work]

Thanks for the clarification. In that case I have some questions about shellcmdargs:

1. How would it work for shells do not need -- but still require arguments to be forwarded, like fish?

   • define cmd: cmd mkdir %mkdir $argv
   • user typed: :mkdir foo bar
   • end up: fish -c 'mkdir $argv' foo bar

I think your intended answer here is to not use $argv, but then to just append the arguments to the command before passing it to the shell. But then:

2. What happens if the user wants to run additional commands afterwards? For example:

cmd mkdir %{{
    mkdir ...
    notify-send success
}}

Now you can't add arguments onto the end of the command string. Also you make the assumption the arguments will be used together, when instead the command could theoretically use $1 in one place, and $2 in another, etc. I think it is fine to add this option shellcmdargs, but it has to be done in a way that would be generically useful, not just useful for Nushell users.

[klesh]

@joelim-work Aah, the case is legit, it didn't occured to me because I would write a script for the case which is easier and can be used both in cli and lf.
So, how do you suggest to address the issue? Maybe making it a triple-state variable helps? shellcmdargs=dashed|direct|none?

[klesh]

shellcmdargs=dashed for posix-shells with the existing behavior
shellcmdargs=direct for shells like fish
shellcmdargs=none for nu and one should write a script to handle case you mentioned

[joelim-work]

So, how do you suggest to address the issue? Maybe making it a triple-state variable helps? shellcmdargs=dashed|direct|none?

I think that could work, to allow the following possibilities:

• Pass both -- and the arguments, e.g. sh -c 'mkdir "$@"' -- foo bar
• Pass only the arguments, e.g. fish -c 'mkdir $argv' foo bar
• Do not pass anything, e.g. nu -c 'ls'

The problem with the third scenario is that nu -c just simply doesn't support additional arguments. One workaround is to have lf expand the variables before passing them to the shell, but I'm not a fan of such a solution because it requires extra functionality to be added to lf when really it should be the responsibility of the shell to handle arguments.

---

One other idea I have is, instead of using nu as the shell, you can create a wrapper which will invoke nu as follows:

lfrc:

set shell nuwrapper

cmd mkdir %{{
    def main [...args: string] {
        ^mkdir ...$args
    }
}}

nuwrapper:

#!/bin/sh

# get rid of '-c' flag
shift

# create script file
echo "$1" > /tmp/lfcmd.nu
shift

# get rid of '--' flag
shift

# invoke nu on script file
nu /tmp/lfcmd.nu "$@"

It is a bit hacky, but it does allow arguments to be passed and also doesn't require changes to lf.

[klesh]

I agree with you that nushell should support such a feature.

However, your solution won't work for me because the reason I use nu as the default shell is that it supports Windows, Linux and macOS at the same time, my goal is to make my dotfiles work across these 3.

[joelim-work]

However, your solution won't work for me because the reason I use nu as the default shell is that it supports Windows, Linux and macOS at the same time, my goal is to make my dotfiles work across these 3.

Well theoretically you could add a customized nuwrapper for each OS, and so long as it appears somewhere in PATH it should work. You could also just name the wrapper nu (ensuring it appears earlier in the PATH lookup), and have it invoke the actual shell afterwards.

The problem is that the supporting work has to be done somewhere, whether it is in nushell, lf, or user scripting. And in the case of lf, adding shellcmdargs=none isn't a perfect solution because you can't pass any arguments. In addition, if we add such an option, it becomes public to users, which means modifying/removing the option results in a breaking change.

I am generally more conservative when it comes to adding new features/options, but I think it's worth to ping @gokcehan for an additional opinion. It may be possible that we can add such a shellcmdargs option, but mark it as experimental.

[klesh]

I agree it is not perfect, but I would like to add that it is possible to pass arguments for shellcmdargs=none just like what has already been done for cmd.exe https://github.com/gokcehan/lf/blob/master/os_windows.go#L105 , although it could not handle case like

cmd mkdir %{{
    mkdir ...
    notify-send success
}}

but I don't think it is a big deal, one can write a script file to avoid it.

Is it safe to assume the shell would be POXIS-compliant when CrossPlatform is a main feature of the project? Especially when cmd.exe is apparently no. 🤣

[klesh]

@joelim-work Speaking of which, would you like to share your way of writing this nuwrapper on Windows?

[joelim-work]

I originally had another idea to not include the -c flag in addition to -- when invoking nu from lf. Something like this:

nu mkdir.nu foo bar

But it looks like there's no way to avoid adding shellflag. You can configure its value, but you can't omit it entirely.

Also my Windows enviornment is borked so I don't really have on anymore. But theoretically nuwrapper can be anything (e.g. compiled binary, script), so long as it's executable. You can even write in Nushell, very rough implementation below:

#!/usr/bin/env nu

def --wrapped main [_, script: string, _, ...args: string] {
    let file = '/tmp/lfcmd.nu'
    $script | save --force $file
    nu $file ...$args
}

Anyway I saw you create an issue in the nushell repo, I think it might be worth waiting to see what they have to say on this topic.

[klesh]

@joelim-work Thanks for the information.

removing 'changes requested'

[joelim-work]

Thanks for being understanding - I think it is important to spend time investigating what is the best way to support Nushell integration, instead of committing to a potentially incomplete solution right now.

For now I will remove my Changes requested status and convert the PR to a draft, as this change is still under discussion.

[klesh]

No problem.
Thank you all for participating in the discussion, I have learned a lot.

[msMatix]

I am also interested in this use case. Are there any new results?

[klesh]

@msMatix I ended up writing a wrapper in Golang like @joelim-work suggested.

[msMatix]

ah I see. Thank you!

[joelim-work]

@klesh TBH I don't think nushell will address the issue any time soon, it looks like they have a lot other outstanding issues and can't keep up with all of them. The use of -c is also probably a niche feature and I suspect it would be low priority for them.

In the meantime, it would be good if you can add your wrapper somewhere in the wiki (perhaps as an integration) so that other nushell users can benefit from it.

[klesh]

@joelim-work It was written in Golang and has a couple of files, not sure it is suitable for the wiki.
I think a better approach would be supporting nushell/nushell#11897 , I would like to work on it if it gets approved...