[Subshell] Support for ash + bugfixes for bash, fish

[kriegaex]

This patch is related to a ​thread on mc-devel. I am copying the main part of my ​previous post to this ticket because I was asked to create one:

I have resolved the remaining issues with differentiating between BusyBox ash and Debian ash (dash). I also refactored the subshell code and documentation a bit for all subshell types. I installed and tested all of them on my Ubuntu 11.10 64-bit desktop system plus Bash and BusyBox ash on my embedded mipsel platform.

I think now I have reached a mature enough code state for you to commit upstream because everything which worked before still does and several things which were buggy or unavailable before have been fixed/added.

The patches currently contain the following changes:

• 030-bash_inputrc.patch
  • Fix non-functional INPUTRC for bash (variable was unset and never used)

• 040-ash_as_subshell.patch
  • Fix chdir for fish: communication with subshell was not working and showing an error message because pwd returns abbreviated home directory "~" instead of full path name. So now we use $PWD and it works. This must have been broken for a while. This is unrelated to ash, but I included it in the refactoring patch for src/subshell.c anyway because it is small and the same code section was also changed for other reasons (set fish prompt).
  • All subshells now have a dynamic, meaningful prompt user@host:cwd. This was tricky for ash/dash and a little easier for the other shells.
  • Shell type recognition for automatic subshell selection has been improved: Now not only the SHELL names are checked, but also their link targets in order to determine the correct type (e.g. if a ksh is really a zsh or if an ash is BusyBox or dash, if sh is whichever type or if bash is symlinked to BusyBox (but BB still is just an ash, never a bash). Even if the type could not be determined or a compatible subshell not found via inspection of SHELL and /etc/passwd, function OS_Setup in src/main.c now searches for known shells by their usual paths. For instance, if you have an exotic login shell called foosh, you can now have subshell support anyway, if MC finds an ash, bash etc. on the system.
  • I have added many more comments to the code sections I changed. Maybe some of you think there are too many now, but I think this tricky part of the code base should be documented rather too much than not enough, so as to make it easier for maintainers to upgrade subshell support or add new subshell types in the future. Sometimes it is not enough to have working code, but helpful to understand why the code was written in a certain way.

• 040-ash_as_subshell_additional.patch
  • ashrc init file added to config file list
  • Subshell documentation page updated with a few corrections, better formatting and some new information (doc/man/mc.1.in), e.g. how to use init files for bash and ash, how to manually call mc with SHELL variable set to another shell type if you want to have a specific subshell type.

BTW, the patches have been created for mc-4.8.1, not for the trunk/master. But they should be easy to merge into the master.

All in all I believe that adding these patches to MC will significantly improve subshell support, fixing two old issues and adding new subshell types often used on servers or embedded systems. That could make MC even more popular. E.g. I have heard from several people that they would like to get rid of feature-rich, but big shells like bash, zsh or tcsh on such systems if they had BusyBox or dash subshell support. I think MC is far from dead and did what I could with my humble means to improve a small part of it.

Open issues (legacy, not created by my patches):

• Still open is the old issue of fish not reliably showing its prompt on the bottom line of mc. With my limited knowledge I could not find out why this happens, but I noticed an interesting fact: If I start mc via strace (I did this because I was debugging/logging something during development), all of a sudden the fish prompt is shown reliably on my test machine. Maybe strace was slowing down something, so this might be a timing issue, possibly a race condition, but this is just a guess. It might be a starting point for further investigation by one of you guys.
• Another (old) open issue which still exists is that if you cd to another directory in full-screen mode (hide panels with Ctrl-O), then show the panels again with Ctrl-O, the new path is not immediately shown on the bottom line, but only after some user interaction such as pressing Tab twice, switching back and forth between left/right panel.

[kriegaex]

Fix bash issue: INPUTRC was never set correctly, thus never used.

[kriegaex]

New subshell types Busybox ash + Debian ash (dash) and some more enhancements plus fish chdir bugfix

[kriegaex]

Improved subshell chapter in online help plus some minor code changes

[kriegaex]

By the way: The BusyBox problem with 4-digit octal codes in printf has been ​fixed upstream yesterday. But because it will be available in version 1.20 earliest and many distributions around will still use older versions for a while, I guess my workaround with echo should remain in the patch for a while.

[er13]

The patches attached to this ticket are successfully used for more than a year in ​freetz.org project. Attaching a version of the patches updated for mc-4.8.8. We would appreciate it if the patches could be applied upstream. Thanks!

Edit: the busybox-printf workaround is not needed anymore, thus commented out but still included in ​this patch

[slavazanko]

Sorry, I disagree with this change:

-                    " PROMPT_COMMAND='pwd>&%d;kill -STOP $$'\n", subshell_pipe[WRITE]);
+            " PROMPT_COMMAND='pwd>&%d; kill -STOP $$'; "
+            "PS1='\\u@\\h:\\w\\$ '\n",
+            subshell_pipe[WRITE]);

I have custom prompt which shows me in addition current branch name when I entered in GIT repo tree.
Other folks can also have custom prompt and we will get a lot of bugreports at this point.

[kriegaex]

I do not understand. This is just the prompt for the MC subshell, it does not replace you parent shell's prompt. What I did here was merely to improve the old behaviour to make the prompt nicer and more informative. It is not new functionality, just fixing/improving the old one which also creates a custom prompt for the subshell, totally independent of the parent shell. So your concern is rather irrelevant IMO. Have you even tried?

Sorry, I cannot check/verify anything now because I am on the road, nowhere near a Linux machine. Imwas answering from memory and from what I see in the patch.

[slavazanko]

On master branch I see prompt like:

slavaz@linux-station ~/work/mc-devel/git 2742_ash$ 

this my custom prompt. Within your changes I see like:

slavaz@linux-station ~/work/mc-devel/git$ 

The prompt doesn't contain a branch name, as is defined in my ~/.bashrc file:

export PS1="\[\033[38m\]\u@\h\[\033[01;34m\] \w \[\033[31m\]\`git rev-parse --abbrev-ref HEAD 2>/dev/null\`\[\033[37m\]$\[\033[00m\] "

So looks like your changes rewrites a Bash-prompt from ~/.bashrc file

[slavazanko]

Created branch 2742_ash, initial changeset:f1ceb68592c7f48832b7abc5b5769153c1e8b55e

Please review.

[kriegaex]

I hope I can investigate your remark during the weekend. The ticket is 18 months old, so probably it is not urgent now for you. Thanks for your patience until then. :-)

[slavazanko]

The ticket is 18 months old, so probably it is not urgent now for you. Thanks for your patience until then. :-)

It was our pleasure ;)

Of course, we can await, no problem.

Ticket in 'hold' stage now. Thank you for your time.

[god12]

Just a friendly reminder that there re other tickets blocked by this. There are several patches attached to this ticket - it would be great if some of them could be merged already if they're not affected by prompt issue.

[kriegaex]

Dear makers of MC, dear Slava,

I just tested a self-compiled MC 4.8.11 (fresh download, plain vanilla upstream package) without my patch and as I remembered from older versions, it also overwrites the Bash prompt by something much like I did for Ash. So my patches did not break or change any existing behaviour, they just replicate for Ash what exists for Bash.

Please either extend or rewrite subshell support by yourself or just use my patches to improve the status quo and permit for more shells to be supported and a few little bugs to be fixed.

Thank you and kind regards, Alexander.

[god12]

Is this related to recently fixed #3232?

[kriegaex]

No, it is complementary. If you read the ticket description it becomes clear.

[god12]

Any plans to move this forward?

[slavazanko]

yep. I'm working with the ticket right now

[slavazanko]

merged into master: [b5bfa00130a8373f3dfe93e4df0c1688eda77a76]

git log --pretty=oneline 2e651d0..0d6d1d0

[slavazanko]

​https://www.midnight-commander.org/wiki/NEWS-4.8.16?action=diff&version=2&old_version=1

[kriegaex]

Shouldn't the wiki entry relate to this ticket rather than to #3547?

[slavazanko]

yep, you're right. Fixed.
​https://www.midnight-commander.org/wiki/NEWS-4.8.16?action=diff&version=3&old_version=2

[mooffie]

Folks, I don't know if it's a problem or not, but subshell.c now has:

/**
 * Set up `precmd' or equivalent for reading the subshell's CWD.
 *
 * Attention! Never forget that these are *one-liners* even though the concatenated
 * substrings contain line breaks and indentation for better understanding of the
 * shell code. It is vital that each one-liner ends with a line feed character ("\n" ).
 *
 * @return initialized pre-command string
 */

static void
init_subshell_precmd (char *precmd, size_t buff_size)
{

    switch (subshell_type)
    {
    case BASH:
        g_snprintf (precmd, buff_size,
                    " PROMPT_COMMAND='pwd>&%d; kill -STOP $$';\n", subshell_pipe[WRITE]);
        break;
  ...
  ...
  ...
}

void
init_subshell (void)
{
  ...
  ...
  ...

    init_subshell_precmd (precmd, BUF_MEDIUM);

    /* Set up `precmd' or equivalent for reading the subshell's CWD
     *
     * Attention! Never forget that these are *one-liners* even though the concatenated
     * substrings contain line breaks and indentation for better understanding of the
     * shell code. It is vital that each one-liner ends with a line feed character ("\n" ).
     */

    switch (subshell_type)
    {
    case BASH:
        g_snprintf (precmd, sizeof (precmd),
                    " PROMPT_COMMAND=${PROMPT_COMMAND:+$PROMPT_COMMAND\n}'pwd>&%d;kill -STOP $$'\n"
                    "PS1='\\u@\\h:\\w\\$ '\n", subshell_pipe[WRITE]);
        break;

  ...
  ...
  ...
 
}

There are two sets of PROMPT_COMMAND voodoo here and they're not identical. Is this intentional?

Also:

Do we lose the ability to set custom PS1 in our .bashrc? (see comment:5, comment:7) My test shows that we don't. So I was wondering what effect the "PS1=..." in the C code has.

[slavazanko]

Hm... yep, some mistake there. I'll try to fix it in 3547_cleanup branch.

Do we lose the ability to set custom PS1 in our .bashrc?

I suppose that we lose PS1 from ~/.bash_profile (at least the file is present on Fedora and being running once after user logged in). But .bashrc file runs before each Bash instance (even, before each invocation of executables from Bash), so our built-in PS1 definition will be rewrited and will be defined in systems which doesn;t have ~/.bashrc at all (It probably very stricted or broken systems).

[slavazanko]

Duplicates was removed in changeset:542cba0098bb5899832d917d3ec0f5fd4913b447

[mooffie]

Great. Thanks!

Another issue. The documentation's diff is:

 When the subshell code is used, you can suspend applications at any
 time with the sequence C\-o and jump back to the Midnight Commander, if
 you interrupt an application, you will not be able to run other
 external commands until you quit the application you interrupted.
 .PP
-An extra added feature of using the subshell is that the prompt
+A special subshell feature (except Bash shell) is that Midnight Commander displays a dynamic prompt
+like "user@host:current_path> " (with known problems for fish which displays the prompt in
+full-screen mode (Ctrl-o), but not when the MC panels are visible).
+.PP
+An extra added feature for Bash shell of using the subshell is that the prompt
 displayed by the Midnight Commander is the same prompt that you are
 currently using in your shell.

I'm not sure users will understand (nor I understand) what "dynamic prompt" means, and whether "like" means that we can change it. If we can change it, what's the difference between this "dynamic prompt" (which is reported to be missing from Bash) and Bash's "same prompt that you are currently using in your shell"?

[kriegaex]

Dynamic means that it shows the current user, host and path (pwd). Without the patch you have a dumb, static prompt for the subshell if you use a simple shell like Busybox Ash or Dash.

[mooffie]

Replying to kriegaex:

Dynamic means that it shows the current user, host and path (pwd).

OK. I'm just concerned about the clarity of the documentation. Wouldn't the following be a simpler way to describe MC's behavior?

The basic prompt displayed by Midnight Commander is of
the form "user@host:current_path$ ". When using Bash, or
a similarly capable shell, the prompt displayed by Midnight
Commander will be the same prompt that you are currently
using in your shell.

(There's a known problem when using fish: the prompt is
displayed only in full screen mode (Ctrl-o), not when the
panels are visible.)

(If we want to be less Bash-centric, we can replace "When using Bash, or a similarly capable shell," with "When using a capable shell, like Bash,")

OTOH, if you think the documentation is OK as it is, I'm fine: close this ticket; I won't re-open it.

[kriegaex]

Sounds nice, feel free to improve. This is OSS. ;-) Currently Bash is really an explicit exception though, as far as I remember. My patch is really old - look at the ticket creation date - and the first and last C language experiment I ever made.

[zaytsev]

Just FYI, doc patches can go directly into master.

[mooffie]

OK, I posted a patch at ticket:3556.

You can close this ticket now. Thanks.

[Zmiter]

Subshell is gone after upgrading from 4.8.16 to 4.8.17. So this patch (in b5bfa00130a8373f3dfe93e4df0c1688eda77a76) makes subshell disabled in my environment. A couple of days ago port misc/mc was updated to the new version and after upgrading mc subshell have gone. When I press Ctrl-O it shows saved console very well, but without shell prompt. So pressing any key in that mode returns mc to its panel view. No subshell. This happens only in csh, when the user with bash shell logs in, everything works well.

I use FreeBSD: FreeBSD 9.3-RELEASE-p38 #11 r296611: Thu Mar 10 14:24:38 FET 2016
My shell is /bin/csh (as default for FreeBSD).

/root/test# mc -V
GNU Midnight Commander 4.8.16
Built with GLib 2.46.2
Using the S-Lang library with terminfo database
With builtin Editor
With subshell support as default
With support for background operations
With mouse support on xterm
With support for X11 events
With internationalization support
With multiple codepages support
Virtual File Systems: cpiofs, tarfs, sfs, extfs, ftpfs, sftpfs, fish, smbfs
Data types: char: 8; int: 32; long: 64; void *: 64; size_t: 64; off_t: 64;

/root/test# mc -F
Root directory: /root

[System data]
    Config directory: /usr/local/etc/mc/
    Data directory:   /usr/local/share/mc/
    File extension handlers: /usr/local/libexec/mc/ext.d/
    VFS plugins and scripts: /usr/local/libexec/mc/
        extfs.d:        /usr/local/libexec/mc/extfs.d/
        fish:           /usr/local/libexec/mc/fish/

[User data]
    Config directory: /root/.config/mc/
    Data directory:   /root/.local/share/mc/
        skins:          /root/.local/share/mc/skins/
        extfs.d:        /root/.local/share/mc/extfs.d/
        fish:           /root/.local/share/mc/fish/
        mcedit macros:  /root/.local/share/mc/mc.macros
        mcedit external macros: /root/.local/share/mc/mcedit/macros.d/macro.*
    Cache directory:  /root/.cache/mc/

/root/test# mc --configure-options
 '--with-internal-edit' '--enable-charset' '--enable-nls' '--enable-vfs-smb' 
 '--with-smb-configdir=/usr/local/etc' '--with-smb-codepagedir=/usr/local/etc/codepages' 
 '--with-subshell' '--disable-x' '--with-screen=slang' 
 '--with-slang-includes=/usr/local/include' '--prefix=/usr/local' '--localstatedir=/var' 
 '--mandir=/usr/local/man' '--infodir=/usr/local/info/' '--build=amd64-portbld-freebsd9.3' 
 'build_alias=amd64-portbld-freebsd9.3' 'CC=cc' 'CFLAGS=-O2 -pipe -fstack-protector 
 -fno-strict-aliasing' 'LDFLAGS= -L/usr/local/lib -fstack-protector' 'LIBS=' 
 'CPPFLAGS=-I/usr/local/include' 'CPP=cpp' 'PKG_CONFIG=pkgconf'

/root/test# echo $TERM
xterm
/root/test# echo $SHELL
/bin/csh

I make some bisection with git and found exactly this first bad commit. So, before it everything works almost well (I mean the patch in FreeBSD ports for correcting console saving behavior in Ctrl-O mode - mc shows blank console).

Hope, it helps to correct the /bin/csh issue.
Thanks.

[zaytsev-work]

Cross-linking with #3619.

[zaytsev-work]

Apparently, this comment /* Also detects csh symlinked to tcsh */ was a lie. Go trust submitters... How about the attached patch, does this fix the problem for you?

[Zmiter]

Yes. It works well now. Thanks a lot!

[kriegaex]

I cannot help but detect an undercurrent of sarcasm in your voice. All I can say is: I tested it on two systems (one desktop Ubuntu and one embedded Linux) four years ago when I created the patch and it worked. Patches have a tendency to deteriorate or just rot if not applied for a long time. I am sorry this caused pain for someone anyway. I meant to scratch my own itch and it worked nicely for me and still does. Thanks for "trusting a submitter", but I think a code review and retest are fully in order, considering the fact that mc runs on so many systems. No reason to blame a submitter, though. It is just unfair.

[mooffie]

@kriegaex: Don't take it personally. Yury had a rough couple of days: 4.6.16 was released and a few critical bugs were found, requiring some stressed work on his part alone (and having to function as a punchbag). He was just venting steam. We ought to thank the gods for him doing merely this instead of quitting. Ditto for Andrew. So cheer up and continue contributing :-)

[kriegaex]

Understood and accepted. Yury's and Andrew's work is appreciated. I just wanted to make it very clear that I am not a punchbag either.

[zaytsev]

I cannot help but detect an undercurrent of sarcasm in your voice.

Indeed, I found the situation rather ironic, since it is with you specifically that we had a number of conversations on the mailing list and privately with regards to the processing of contributed patches. The last conversation of this sort has triggered Slava to go ahead and commit this particular patch without proper review.

Of course, it's my fault that I didn't object to that, but I didn't want to get into yet another argument. Very unfortunately, we don't have the resources to review the patches quickly as they should be reviewed, so there are two choices out there: (1) do superficial reviews, but commit most likely broken code, or (2) let the patches rot for years until you manage to do a proper review, but piss off the submitters.

In the first case, you get yelled at, because it's your fault that you commit broken stuff, in the second case, you get endless arguments about how slowly the patches are being accepted. So, whatever you do, you have to take the shit and the only way for you to *not* to take the shit is to leave the project.

Hence, I couldn't help myself, but leave a sarcastic comment as I understood what the problem was. If you look at the "fix", you'll see that it doesn't have anything to do with your patch rotting over years. It was a genuine logical mistake to leave out a condition that previously existed, without replacing it with an equivalent one.

Still, I agree that it's the reviewers fault; the submitter by definition isn't supposed to know every dark corner of the code, and can only do limited testing. I didn't have any intention to put the blame on you and I'm sorry that I couldn't keep my sarcasm to myself.

[zaytsev]

Ticket #3619 has been marked as a duplicate of this ticket.

[zaytsev]

Hmmm, it seems that #3606 is more of the same, but here I'm not so sure how exactly the logical error (missing null pointer check) was introduced :-/

[mooffie]

(It's somewhat off-topic, but I mention a test for the subshell ​here.)

[woodsb02]

Please note that FreeBSD bug report 208102 has been raised to include the proposed patch in the FreeBSD mc 4.8.16 packages.
​https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=208102

[zaytsev]

Branch: 2742_csh_as_tcsh_fix
Initial changeset: e8d462b7076e4162cf100bb6b24db9afe79e6a1c

I'm not a huge fan this whole detection business, but the patch seems to work for FreeBSD users, so I think we should include this in the next release.

[zaytsev]

Merged to master: 23c00cf33fbc60263b4805c01c5b34d3229791b4