mksh as a subshell

[gloomyquazar]

Hello.

On my desktop and notebook I use FreeBSD together with mksh, which I like for it's small size, robustness, security... Recently I got to know that MC supports only bash, (t)csh, zsh and something else as main shells, hence my mksh doesn't work as a subshell, I can't just switch with Ctrl+O and work there, so I had to postpone using MC for now. Is there a way to add mksh for supported shells? Without a subshell working in MC gets very unconvenient.

With regards,
Sergey P.

[gloomyquazar]

It would be nice to also add support for general Bourne /bin/sh as a subshell.

[andrew_b]

Replying to gloomyquazar:

It would be nice to also add support for general Bourne /bin/sh as a subshell.

#3658

[zaytsev]

If mksh has an equivalent of precmd (which, I think, it does) and you can cook up a patch to support it that is not impossible.

[gloomyquazar]

Maybe the function like this will do?

function precmd {

<put something here>

}

PS1="$(precmd) $PS1 "

[zaytsev]

As I said, if you manage to provide a clean working patch, we might eventually get it in.

[gloomyquazar]

Could you please check this patch? It works OK for me.

[woodsb02]

Any progress on this?
The v2 patch look a simple enough.

[zaytsev]

Somebody turned this in a PR:

​https://github.com/MidnightCommander/mc/pull/209

[d3m3t3r]

Replying to zaytsev:

Somebody turned this in a PR:

​https://github.com/MidnightCommander/mc/pull/209

Added support for ksh/oksh/mksh Korn shell variants.

[zaytsev]

I assume you tested it and it works for you?

• I'm surprised that the "precmd" part was so simple. Is that due to static prompt? Is supporting user prompt impossible / problematic?
• I'm not sure that ksh supports HISTCONTROL... Does it really? If yes, good news.

[d3m3t3r]

I need to fix it for mksh which is actually dumber than other pdksh/openbsd based variants. mksh doesn't support HISTCONTROL either \x placeholders in PS1 and recognizes not just ENV but $HOME/.mkshrc too.

Couldn't find any "precmd" like functionality in any variant so I suppose user prompt is not possible.

Since mksh seems to be quite different from other variants, would you recommend using distinct SHELL_KSH and SHELL_MKSH types? Or perhaps use single SHELL_KSH type but specific mc_shell->name?

[zaytsev]

Since mksh seems to be quite different from other variants

I would add SHELL_MKSH if it's different enough, and SHELL_OKSH / SHELL_PDKSH (whichever official names make sense, didn't look into it) for clarity - you can use multiple enum values for the same case.

Couldn't find any "precmd" like functionality in any variant so I suppose user prompt is not possible.

It would be great to have this as a comment. Otherwise our children will wonder...

mksh doesn't support HISTCONTROL

But the others do, or everything is trashed with our cd commands?

[d3m3t3r]

First, thanks much for the feedback. I've split mksh support under its own SHELL_MKSH type since its features differ so much it makes sense to handle it separately.

Some overview: There seems to be several implementations of ksh around but all but mksh (MirBSD ksh ​https://github.com/MirBSD/mksh) are based on pdksh (Public Domain ksh), in particular ports of OpenBSD ksh, e.g. ​https://github.com/dimkr/loksh and ​https://github.com/ibara/oksh. pdksh supports \x codes, variable and command substitution in PS1, HISTCONTROL (ignorespace & ignoredups) and ENV variable for interactive shell rc-file. mksh, on the other hand, doesn't support \x codes in PS1 either HISTCONTROL. It supports ENV variable for interactive shell rc-file but if it's not set the shell also tries ~/.mkshrc.

[zaytsev]

Sorry, I still don't have time to look into it deeper... but I wonder, can actually the plain POSIX sh be supported with the fallback? Maybe you have the answer since you looked deeper into it.

[d3m3t3r]

but I wonder, can actually the plain POSIX sh be supported with the fallback? Maybe you have the answer since you looked deeper into it.

I don't think the precmd fallback emulation in mc supports the plain POSIX shell because it relies on the command substitution in PS1 which according to the specification (​https://pubs.opengroup.org/onlinepubs/9799919799/utilities/V3_chap02.html#tag_19_05_03) is unspecified.

[d3m3t3r]

There's summary of the changes in my PR:

1. Add support for mksh - uses fallback precmd emulation.
2. Add support for pdksh-based ksh (e.g. OpenBSD's ksh) - supports user prompt.
3. Enhance fallback precmd emulation to support user prompt (affect ash/dash/mksh).

I tested on Alpine Linux with busybox, dash, mksh, oksh (OpenBSD's ksh port) and loksh (another OpenBSD's ksh port) packages.

[zaytsev]

Branch: 3748_ksh_support
Initial changeset: b64512b750351d83691c02e5bf118d3e00dae85f

I have added some fixups, but it me it looks good. Thank you for your high quality contribution!

[ossi]

i haven't looked very closely, but it doesn't seem right to me that a fallback to .profile is forced.

line 385 has a copy&pasto.
line 1199 has incorrect indentation.

[zaytsev]

i haven't looked very closely, but it doesn't seem right to me that a fallback to .profile is forced.

Why? It seems to be the default for ksh according to the man pages.

line 385 has a copy&pasto.
line 1199 has incorrect indentation.

Both are fixed in subsequent commits. I didn't want to squash them because of the inline explanations.

[ossi]

huh, i didn't notice that this is a patch _series_. 🤦🏻‍♂️
(did i already mention this year that i hate trac as a review platform?)

well, if it's the default, then it needs no override. but i suppose it needs some "convincing", because it's not a login shell.
i'm a bit wary about messing with the startup files in general, because i know what an utter mess it is, how some shells aren't even consistent with themselves (bash has build-time switches), and how everybody tries to work around it their own way. (you will find several "treatises" from me on that matter if you google hard enough.) and sourcing .profile in lieu of an rc file doesn't make things better. ah, well.

the merge's diff shows that the indentation mistake persists.

which inline explanations?
i'm an atomicity pedant, so i'd split the fixup patches and squash the hunks where appropriate. shared authorship can be documented in footers.
in the same vein, a commit near the end of the series contains some unrelated comment whitespace changes that i'd split off (or just discard).
but this is a matter of project policy, so whatever.

[zaytsev]

Just tested on OpenIndiana with ksh and ksh93 - the new shell integration is working very nicely and is comfortable to use. The history is clobbered with commands though, because they don't seem to support HISTCONTROL.

I have added a commit to support ksh93 binary, I will squash commits before merge.

[zaytsev]

Merged to master: b683b6388e9249ab986d764763df1cb82fd1d888

[zaytsev]

Now testing the latest master on alpine and ash prompt seems to been affected.

• Before merge the prompt was zaytsev@alpine$ .
• After the merge the prompt is ~ $.

The default PS1 on Alpine should be \h:\w\$ . However, when starting mc as mc it somehow gets lost!

• If I start mc as PS1="\h:\w\$ " mc, then the default prompt is indeed used.
• If I start mc as PS1= mc, then our fallback is used.
• If I start mc as mc, then this special mini-prompt is used.

I think this might have to do with the following ticket:

​https://gitlab.alpinelinux.org/alpine/aports/-/issues/12398

I'm not sure what to think of it:

On one hand, I guess we are now actually doing the "right thing".
On the other hand, we are not doing this for bash and zsh, only for fish to a certain extent.

Then again, I guess the reasoning for bash and zsh is that extra prompt stuff will always break the persistent buffer. For fish... no idea.

[zaytsev]

I'm wrong, actually we are supporting custom PS1 for all other shells (bash, zsh), so, I guess the new way is the right way.

Also noted that there is a mysterious ^H hanging in the background for dash, but it was there before. Added a comment to #3010.

[d3m3t3r]

For ash shell, the subshell would try to use $ENV, then ~/.profile and finally ~/.local/share/mc/ashrc rc files (e.g. shell-specific-rc-env-var, shell-specific-rc-file, mc-custom-shell-specific-rc-file just like with most other supported shells). In the subshell, PS1 (all environment variables in general) set in the selected rc-file would override PS1 inherited from mc itself. However, PS1 is somewhat special as we actually need to adjust it for the precmd-hack needed in some shells. My change allowed this "adjustment" to retain the original PS1 (see MC_PS1_SAVED) in the fallback prompt. Same thing can easily be done for other shells not using the fallback prompt but I considered it too far from the original scope of this ticket ("mksh").

On one hand, I guess we are now actually doing the "right thing".

Exactly. My assumption is the user would like to have his customized PS1 in the subshell.

The default PS1 on Alpine should be \h:\w\$ . However, when starting mc as mc it somehow gets lost!

I believe it's because this PS1 is set in /etc/profile which is not used when ash is executed in the subshell by mc.

[zaytsev]

Same thing can easily be done for other shells not using the fallback prompt but I considered it too far from the original scope of this ticket ("mksh").

But I think this works already. I customized my PS1 for zsh in .zshrc and it's working in mc - same goes for bash, not sure about fish, but I this that it does as well. The only other suspect is tcsh...

Exactly. My assumption is the user would like to have his customized PS1 in the subshell.

Yes, now I get it. But initially I was concerned that the default has become less useful, it doesn't even include pwd :( but then again, probably anyone seriously using these shells is either OK with that, or has customized the prompt already.

I believe it's because this PS1 is set in /etc/profile which is not used when ash is executed in the subshell by mc.

I guess so, I tried to look into ways of telling ash/dash that it will be started as a login shell, but found nothing. Oh well, probably we'll just have to let the sh*t hit the fan and see if there is any backlash.

[d3m3t3r]

Actually, it seems to me the custom prompt in bash works accidentally rather than by design. The shell code sets PS1 at the end but the assignment has no effect for some reason (you need to replace '\n' by ';' there but I don't quite get why).

[zaytsev]

Actually, I can't confirm that it's doing anything at all, with \n or ; or ; \n.

My understanding is that this stuff was added in f596c916a42a0868897b3314e557b0a82df37017 in #2742, because the author apparently wanted a unified prompt for all subshells, but never communicated this properly. This code was committed without testing and review and has already caused quite a bit of trouble.

After that, people started complaining that their custom prompts get overwritten and the change was explicitly reverted for fish (#3944). For other shells the code wasn't touched apparently because it doesn't have any effect.

Maybe we should remove setting PS1? I'm not sure as to why do we need it in the first place, if buffer is processed via hooks. I guess it should just be removed for all shells (including tcsh), the man page should be updated and that's it.

Ossi, maybe you have an opinion as at some point you touched this code?

d3m3t3r, while we are at it, I'm now confused by your comment:

/* pdksh based variants support \x placeholders but not any "precmd" functionality. */

You use fallback for mksh, but for pdksh & co. you use a very simple emulation instead of fallback, and it doesn't use \x (what does that do?).

[d3m3t3r]

Maybe we should remove setting PS1?

I guess the problem lies in the side effect of PROMPT_COMMAND. Code like "PROMPT_COMMAND=...;PS1=...\n" would work as it's fed to bash as a single line while with "PROMPT_COMMAND=...\nPS1=...\n" PROMPT_COMMAND does "its stuff" before the assignment to PS1. Anyway, the fix is to move the assignment of PS1 before the "if test ..." condition and change it to

" PS1=${PS1:-'\\u@\\h:\\w\\$ \'}\n"

to honor the custom PS1.
Can we make a new ticket for this issue? I will prepare a PR for review.
(Note there might be the same problem with zsh just by looking at the shell code. Haven't tried though.)

You use fallback for mksh, but for pdksh & co. you use a very simple emulation instead of fallback, and it doesn't use \x (what does that do?).

By term "\x" I meant all those backslash replacements in general (aka escape sequences, aka fancy prompt). pdksh code can be that much simpler than the fallback because you can use the fancy prompt and $HOME is replaced by "~" automatically.

[zaytsev]

Can we make a new ticket for this issue? I will prepare a PR for review.

I can open a new ticket, but I wanted to clarify what we actually want to achieve first. Is it even possible to have PS1 not set by the shell? Why shouldn't we just remove the fallback PS1 setting from bash, zsh and possibly tcsh code? I'm just trying to understand under what circumstances it would be useful to fix this when it seems to have been broken for a decade... (at least for bash).

pdksh code can be that much simpler than the fallback

Ah, I see, thank you.

[ossi]

i agree that we should get rid of the unnecessary PS1 setting entirely. nothing good can come out of it.

for the fallback path, the current PS1 handling seems weird to me:

• the MC_PS1_SAVED hack seems bogus, as the function doesn't parameter-expand its value. the value should be put directly at the end of PS1 instead. it doesn't need to go through the precmd hack, as it won't cause more problems for sub-subshells than it would if mc wasn't there at all.
• we need to provide some PS1 implementation if it wasn't set, as injecting our precmd suppresses the shell's default. however, what we have is a tad over-engineered, as it tries to be nice rather than providing the bare minimum such a primitive shell would. also, it seems wrong that this implementation is inside the precmd, as it won't be visible from a sub-subshell, so that will end up with no prompt at all again.

btw, i'd call these things _mc_precmd and _MC_PRECMD for clarity. i was a bit confused at first, thinking PRECMD might have some meaning to the shell.

so overall, i think this code should be ok (entirely untested!):

        "_mc_precmd() { "
        "  pwd>&%d; "
        "  kill -STOP $$; "
        "}; "
        "_MC_PRECMD=_mc_precmd; "
        "PS1='$($_MC_PRECMD)'\"${PS1:-'$PWD $ '}\"\n";

[ossi]

to actually answer the question whether PS1 can be unset in the first place: probably not. i mean, it can if you manually unset it, but dash actually shows no prompt at all in this case. so arguably, providing even a minimal PS1 fallback is unnecessary.

[d3m3t3r]

It's definitely quite unlikely an interactive shell not setting PS1. The user would need to mess up something. IMO, the default PS1 could be removed from every 'precmd' code if we check all shells cope with empty PS1 reasonably. For bash, you can test it by adding "unset PS1" into .bashrc. It seems to work fine.

[zaytsev]

I have created a follow-up ticket - #4634. d3m3t3r, if you'd like to make a clean patch, please attach it there, a PR is not necessary.