Multishell/Crossplatform handling of `--`

[39555]

I would like to reopen the discussion around the double-hyphen -- placeholder https://github.com/gokcehan/lf/pull/1606

I found that when I use cygwin bash or any 'posix compliant' shell on windows, arguments to any shell command shift to the command_name $0 because lf uses platform specific functions and doesn't add the -- placeholder on windows:

• https://github.com/gokcehan/lf/blob/master/os_windows.go#L137
• https://github.com/gokcehan/lf/blob/master/os.go#L143

Idea

My idea is to set shell as a template parameter for the entire shell invocation not just the shell path, options and args:

set shell bash -c $c -- $a
set shell bash --norc --noprofile +O eu -c $c -- $a
set shell pwsh -NoLogo -Command $c $a
set shell cmd /k /c $c $a
set shell nu -c $c $a

Than when invoking a script lf would replace placeholders for command $c and args $a inside the template

[joelim-work]

So this is what I found by looking at the history:

• shellopts was introduced because of #102, to allow specifying additional options when invoking the shell (e.g. -f to disable globbing).
• shellflag was introduced because of #597, to override the default -c option (/c for Windows), primarily to allow the use of other shells in Windows.

I agree that such high-level configuration options can be inflexible, and that it would be beneficial to provide a more low-level method of specifying the shell invocation. However, modifying the existing shell configuration option is a breaking change, and given that the shell is a core feature of lf, I am not sure if this is a good idea.

I think it might be better to introduce a new option (e.g. shellcmd) instead, implemented like below (similar changes required for os_windows.go):

Click to expand

```diff diff --git a/eval.go b/eval.go index 23a6be5..ab8f1b0 100644 --- a/eval.go +++ b/eval.go @@ -368,6 +368,12 @@ func (e *setExpr) eval(app *app, args []string) { return } gOpts.shellopts = strings.Split(e.val, ":") + case "shellcmd": + if e.val == "" { + gOpts.shellcmd = nil + return + } + gOpts.shellcmd = tokenize(e.val) case "sortby": method := sortMethod(e.val) if !isValidSortMethod(method) { diff --git a/opts.go b/opts.go index 50db7b1..24d568a 100644 --- a/opts.go +++ b/opts.go @@ -94,6 +94,7 @@ var gOpts struct { rulerfmt string preserve []string shellopts []string + shellcmd []string keys map[string]expr cmdkeys map[string]expr cmds map[string]expr @@ -240,6 +241,7 @@ func init() { gOpts.rulerfmt = " %a| %p| \033[7;31m %m \033[0m| \033[7;33m %c \033[0m| \033[7;35m %s \033[0m| \033[7;34m %f \033[0m| %i/%t" gOpts.preserve = []string{"mode"} gOpts.shellopts = nil + gOpts.shellcmd = nil gOpts.tempmarks = "'" gOpts.numberfmt = "\033[33m" gOpts.tagfmt = "\033[31m" diff --git a/os.go b/os.go index ac2d2f6..95fbe75 100644 --- a/os.go +++ b/os.go @@ -140,6 +140,21 @@ func shellCommand(s string, args []string) *exec.Cmd { s = fmt.Sprintf("IFS='%s'; %s", gOpts.ifs, s) } + if len(gOpts.shellcmd) > 0 { + var words []string + for _, word := range gOpts.shellcmd { + switch word { + case "$a": + words = append(words, args...) + case "$c": + words = append(words, s) + default: + words = append(words, word) + } + } + return exec.Command(words[0], words[1:]...) + } + args = append([]string{gOpts.shellflag, s, "--"}, args...) args = append(gOpts.shellopts, args...) ```

Although I suppose we could just keep shell as a string option and apply the new handling logic based on whether it consists of a single word or not. I could be convinced either way on this.

[39555]

Thanks for the hint! Im going to implement it and we will see how it plays out

[joelim-work]

I experimented a bit with Windows CMD, unfortunately it has its own way of dealing with quoted arguments and requires special handling. You can find some notes about it in the description for #1309.

It might be better to implement the shell option as a string value, something like below:

func shellCommand(s string, args []string) *exec.Cmd {
    // Windows CMD requires special handling to deal with quoted arguments
    if strings.HasPrefix(strings.ToLower(gOpts.shell), "cmd ") {
        var quotedArgs []string
        for _, arg := range args {
            quotedArgs = append(quotedArgs, fmt.Sprintf(`"%s"`), arg)
        }
        cmdline := strings.ReplaceAll(gOpts.shell, "$c", s)
        cmdline = strings.ReplaceAll(cmdline, "$a", strings.Join(quotedArgs, " "))
        cmd := exec.Command("cmd")
        cmd.SysProcAttr = &windows.SysProcAttr{CmdLine: cmdline}
        return cmd
    }

    if strings.Contains(gOpts.shell, " ") {
        var words []string
        for _, word := range tokenize(gOpts.shell) {
            switch word {
            case "$a":
                words = append(words, args...)
            case "$c":
                words = append(words, s)
            default:
                words = append(words, word)
            }
        }
        return exec.Command(words[0], words[1:]...)
    }

    // original legacy configuration which uses shellopts and shellflag
    args = append([]string{gOpts.shellflag, s}, args...)
    args = append(gOpts.shellopts, args...)
    return exec.Command(gOpts.shell, args...)
}

This should work for Windows CMD:

set shell 'cmd /c "$c $a"'

And for PowerShell:

set shell 'pwsh -CommandWithArgs $c $a'

The only other suggestions I have are:

• Whether to implement this as a new command, or just use the existing shell command (must be backwards compatible)
• Whether to use short names for the placeholders ($c and $a) or something longer like $cmd and $args

Anyway I think this is a great idea, and an improvement over the current design. Feel free to submit a PR if you get this working.