Multishell/Crossplatform handling of --

[39555]

I would like to reopen the discussion around the double-hyphen -- placeholder #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 Using commands from lfrc.example is unsafe #102, to allow specifying additional options when invoking the shell (e.g. -f to disable globbing).
• shellflag was introduced because of Cannot use WSL as shell on lf installed on windows; perhaps allow customizing the /c flag added to commands to allow using WSL shell? #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 --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.