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

[mc-butler]

Important

This issue was migrated from Trac:

Origin	https://midnight-commander.org/ticket/2742
Reporter	kriegaex (Alexander@….name)
Mentions	pahan@….info (@Hubbitus)

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.

Note

Original attachments:

• 030-bash_inputrc.patch (raw) by kriegaex (Alexander@….name) on Mar 8, 2012 at 11:30 UTC
• 040-ash_as_subshell.patch (raw) by kriegaex (Alexander@….name) on Mar 8, 2012 at 11:32 UTC
• 040-ash_as_subshell_additional.patch (raw) by kriegaex (Alexander@….name) on Mar 8, 2012 at 11:33 UTC
• 030-bash_inputrc___mc-4.8.8.patch (raw) by er13 (ernews@….de) on Apr 6, 2013 at 13:57 UTC
• 040-ash_as_subshell___mc-4.8.8.patch (raw) by er13 (ernews@….de) on Apr 6, 2013 at 13:57 UTC
• 050-ash_as_subshell_additional___mc-4.8.8.patch (raw) by er13 (ernews@….de) on Apr 6, 2013 at 13:57 UTC
• 0001-Ticket-2742-detect-csh-as-tcsh-by-name.patch (raw) by zaytsev-work (@zyv) on Mar 16, 2016 at 14:00 UTC

[mc-butler]

Changed by kriegaex (Alexander@….name) on Mar 8, 2012 at 11:30 UTC

• Attachment 030-bash_inputrc.patch (raw) added

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

[mc-butler]

Changed by kriegaex (Alexander@….name) on Mar 8, 2012 at 11:32 UTC

• Attachment 040-ash_as_subshell.patch (raw) added

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

[mc-butler]

Changed by kriegaex (Alexander@….name) on Mar 8, 2012 at 11:33 UTC

• Attachment 040-ash_as_subshell_additional.patch (raw) added

Improved subshell chapter in online help plus some minor code changes

[mc-butler]

Changed by kriegaex (Alexander@….name) on Mar 8, 2012 at 14:30 UTC (comment 1)

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.

[mc-butler]

Changed by slavazanko (@slavaz) on Mar 8, 2012 at 14:40 UTC (comment 2)

• Owner set to slavazanko
• Status changed from new to accepted
• Milestone changed from 4.8.2 to 4.8

[mc-butler]

Changed by er13 (ernews@….de) on Apr 6, 2013 at 13:57 UTC (comment 3)

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

[mc-butler]

Changed by er13 (ernews@….de) on Apr 6, 2013 at 13:57 UTC

• Attachment 030-bash_inputrc___mc-4.8.8.patch (raw) added

[mc-butler]

Changed by er13 (ernews@….de) on Apr 6, 2013 at 13:57 UTC

• Attachment 040-ash_as_subshell___mc-4.8.8.patch (raw) added

[mc-butler]

Changed by er13 (ernews@….de) on Apr 6, 2013 at 13:57 UTC

• Attachment 050-ash_as_subshell_additional___mc-4.8.8.patch (raw) added

[mc-butler]

Changed by andrew_b (@aborodin) on May 23, 2013 at 17:09 UTC

• Blocking set to #3010

[mc-butler]

Changed by slavazanko (@slavaz) on Sep 2, 2013 at 16:23 UTC (comment 5)

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.

[mc-butler]

Changed by kriegaex (Alexander@….name) on Sep 2, 2013 at 17:04 UTC (comment 6)

I do not understand. This is just the prompt for the MC subshell, it does not replace your 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 seems 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. I am answering from memory and from what I see in the patch.

[mc-butler]

Changed by slavazanko (@slavaz) on Sep 2, 2013 at 17:47 UTC (comment 7)

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

[mc-butler]

Changed by slavazanko (@slavaz) on Sep 2, 2013 at 17:47 UTC (comment 8)

• Milestone changed from 4.8 to 4.8.11
• Branch state changed from no branch to on hold

Created branch 2742_ash, initial [f1ceb68592c7f48832b7abc5b5769153c1e8b55e]

Please review.

[mc-butler]

Changed by kriegaex (Alexander@….name) on Sep 2, 2013 at 18:46 UTC (comment 9)

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. :-)

[mc-butler]

Changed by slavazanko (@slavaz) on Sep 2, 2013 at 19:53 UTC (comment 10)

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.

[mc-butler]

Changed by god12 (god12@….hut.fi) on Dec 3, 2013 at 18:17 UTC (comment 11)

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.

[mc-butler]

Changed by kriegaex (Alexander@….name) on Dec 7, 2013 at 12:50 UTC (comment 12)

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.

[mc-butler]

Changed by god12 (god12@….hut.fi) on Aug 7, 2014 at 14:42 UTC (comment 13)

Is this related to recently fixed #3232?

[mc-butler]

Changed by kriegaex (Alexander@….name) on Aug 7, 2014 at 15:34 UTC (comment 14)

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

[mc-butler]

Changed by andrew_b (@aborodin) on Sep 5, 2014 at 9:10 UTC (comment 15)

• Milestone changed from 4.8.11 to Future Releases

[mc-butler]

Changed by Hubbitus (@Hubbitus) on Mar 30, 2015 at 0:42 UTC (comment 16)

• Cc set to pahan@….info

[mc-butler]

Changed by god12 (god12@….hut.fi) on Nov 9, 2015 at 10:32 UTC (comment 17)

• Version changed from 4.8.1 to 4.8.15

Any plans to move this forward?

[mc-butler]

Changed by slavazanko (@slavaz) on Nov 10, 2015 at 10:42 UTC (comment 18)

yep. I'm working with the ticket right now

[mc-butler]

Changed by slavazanko (@slavaz) on Nov 10, 2015 at 12:57 UTC (comment 19)

• Votes set to slavazanko

[mc-butler]

Changed by slavazanko (@slavaz) on Nov 10, 2015 at 13:00 UTC (comment 20)

• Status changed from accepted to testing
• Resolution set to fixed
• Branch state changed from on hold to merged
• Votes changed from slavazanko to committed-master
• Blocking #3010 deleted
• Milestone changed from Future Releases to 4.8.16

merged into master: [b5bfa00]

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

[mc-butler]

Changed by slavazanko (@slavaz) on Nov 10, 2015 at 13:02 UTC (comment 21)

• Status changed from testing to closed

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

[mc-butler]

Changed by kriegaex (Alexander@….name) on Nov 10, 2015 at 21:11 UTC (comment 22)

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

[mc-butler]

Changed by slavazanko (@slavaz) on Nov 11, 2015 at 8:16 UTC (comment 23)

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

[mc-butler]

Changed by mooffie (@mooffie) on Nov 12, 2015 at 13:04 UTC (comment 24)

• Resolution fixed deleted
• Status changed from closed to reopened

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.

[mc-butler]

Changed by mooffie (@mooffie) on Nov 12, 2015 at 15:55 UTC (comment 27)

• Status changed from closed to reopened
• Resolution fixed deleted

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"?

[mc-butler]

Changed by kriegaex (Alexander@….name) on Nov 12, 2015 at 16:11 UTC (comment 28)

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.

[mc-butler]

Changed by mooffie (@mooffie) on Nov 12, 2015 at 21:09 UTC (comment 28.29)

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.

[mc-butler]

Changed by kriegaex (Alexander@….name) on Nov 12, 2015 at 22:09 UTC (comment 30)

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.

[mc-butler]

Changed by zaytsev (@zyv) on Nov 13, 2015 at 20:25 UTC (comment 31)

Just FYI, doc patches can go directly into master.

[mc-butler]

Changed by mooffie (@mooffie) on Nov 14, 2015 at 21:01 UTC (comment 32)

OK, I posted a patch at #3556.

You can close this ticket now. Thanks.

[mc-butler]

Changed by zaytsev (@zyv) on Nov 15, 2015 at 19:00 UTC (comment 33)

• Resolution set to fixed
• Status changed from reopened to closed

[mc-butler]

Changed by Zmiter (zmiters@….ru) on Mar 16, 2016 at 13:40 UTC (comment 34)

• Status changed from closed to reopened
• Resolution fixed deleted

Subshell is gone after upgrading from 4.8.16 to 4.8.17. So this patch (in [b5bfa00]) 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 [296611]: 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.

[mc-butler]

Changed by zaytsev-work (@zyv) on Mar 16, 2016 at 13:49 UTC (comment 35)

Cross-linking with #3619.

[mc-butler]

Changed by zaytsev-work (@zyv) on Mar 16, 2016 at 14:00 UTC

• Attachment 0001-Ticket-2742-detect-csh-as-tcsh-by-name.patch (raw) added

[mc-butler]

Changed by zaytsev-work (@zyv) on Mar 16, 2016 at 14:00 UTC (comment 36)

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?

[mc-butler]

Changed by zaytsev-work (@zyv) on Mar 16, 2016 at 14:01 UTC (comment 37)

• Votes committed-master deleted
• Version changed from 4.8.15 to 4.8.16
• Milestone changed from 4.8.16 to 4.8.17
• Branch state changed from merged to no branch

[mc-butler]

Changed by Zmiter (zmiters@….ru) on Mar 16, 2016 at 14:10 UTC (comment 38)

Yes. It works well now. Thanks a lot!

[mc-butler]

Changed by kriegaex (Alexander@….name) on Mar 16, 2016 at 16:20 UTC (comment 39)

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.

[mc-butler]

Changed by mooffie (@mooffie) on Mar 16, 2016 at 18:17 UTC (comment 40)

@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 :-)

[mc-butler]

Changed by kriegaex (Alexander@….name) on Mar 16, 2016 at 18:34 UTC (comment 41)

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.

[mc-butler]

Changed by zaytsev (@zyv) on Mar 16, 2016 at 19:53 UTC (comment 42)

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.

[mc-butler]

Changed by zaytsev (@zyv) on Mar 16, 2016 at 19:58 UTC (comment 43)

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

[mc-butler]

Changed by zaytsev (@zyv) on Mar 16, 2016 at 20:35 UTC (comment 44)

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 :-/

[mc-butler]

Changed by mooffie (@mooffie) on Mar 17, 2016 at 10:18 UTC (comment 45)

(It's somewhat off-topic, but I mention a test for the subshell #comment:9 here (#3611).)

[mc-butler]

Changed by woodsb02 (woodsb02@….com) on Mar 17, 2016 at 23:21 UTC (comment 46)

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

[mc-butler]

Changed by zaytsev (@zyv) on Apr 24, 2016 at 17:28 UTC (comment 47)

• Owner changed from slavazanko to zaytsev
• Branch state changed from no branch to on review
• Status changed from reopened to accepted

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.

[mc-butler]

Changed by andrew_b (@aborodin) on Apr 25, 2016 at 7:40 UTC (comment 48)

• Votes set to andrew_b
• Branch state changed from on review to approved

[mc-butler]

Changed by zaytsev (@zyv) on Apr 25, 2016 at 19:46 UTC (comment 49)

• Votes changed from andrew_b to committed-master
• Branch state changed from approved to merged
• Resolution set to fixed
• Status changed from accepted to testing

Merged to master: [23c00cf]

[mc-butler]

Changed by zaytsev (@zyv) on Apr 25, 2016 at 19:48 UTC (comment 50)

• Status changed from testing to closed