Ticket #2027 (new defect)

Opened 5 years ago

Last modified 3 years ago

MC clobbers the PROMPT_COMMAND shell variable

Reported by: wjaguar Owned by:
Priority: major Milestone: Future Releases
Component: mc-core Version: master
Keywords: Cc:
Blocked By: Blocking:
Branch state: no branch Votes for changeset:

Description

When starting a Bash subshell, MC overwrites the PROMPT_COMMAND shell variable, instead of adding to it. Thus user's settings from ~/mc/bashrc are lost.
This breaks use-cases like the one described here:
http://forum.ducea.com/discussion/23/bash-history-for-multiple-session/

Patch is attached.

Attachments

subshell_prompt.patch (785 bytes) - added by wjaguar 5 years ago.

Change History

Changed 5 years ago by wjaguar

comment:1 Changed 5 years ago by storchaka

Use
"PROMPT_COMMAND=\"$PROMPT_COMMAND${PROMPT_COMMAND:+ }\"'pwd>&%d;kill -STOP $$'\n"

comment:2 Changed 5 years ago by storchaka

Sorry, need to use
"PROMPT_COMMAND=\"$PROMPT_COMMAND${PROMPT_COMMAND:+;}\"'pwd>&%d;kill -STOP $$'\n"

comment:3 follow-ups: ↓ 5 ↓ 13 Changed 5 years ago by doktornotor

Well, this patch causes "bad descriptor" and some other weird error stuff occasionally, at least here. Tested with PROMPT_COMMAND='history -a; history -n' from the link above.

comment:4 Changed 5 years ago by doktornotor

  • Cc notordoktor@… added

comment:5 in reply to: ↑ 3 Changed 5 years ago by wjaguar

Replying to doktornotor:

Well, this patch causes "bad descriptor" and some other weird error stuff occasionally, at least here.

Well, here it doesn't (Slackware 12.1, bash 3.1.017, mc 4.7.0.1).
Which is one reason why I run Slackware; a stable system which doesn't throw random weird errors at me, makes it much easier to find and fix real bugs. ;-)

comment:6 follow-up: ↓ 7 Changed 5 years ago by doktornotor

@wjaguar: Well, the reason it's throwing such stuff is not a broken system. Particularly, working with Gentoo portage (emerge and such) which makes heavy and extensive use of bash features seemed to produce the bad descriptor issue quite often with this patch.

Also, wrt the use case described in the issue description, well, that certainly didn't work for me as described, plus caused ignoredups to be essentially ignored. Doesn't look like a very viable use case to me.

comment:7 in reply to: ↑ 6 Changed 5 years ago by wjaguar

Replying to doktornotor:

@wjaguar: Well, the reason it's throwing such stuff is not a broken system.

Reeeally? ;-)
Well, if you can offer _any_ viable mechanism by which this patch could cause something which is NOT broken to misbehave, I'm all ears. Otherwise... well, in my experience, _unexplainable_ glitches are just a sign that one is using too-unstable stuff.

Particularly, working with Gentoo portage

Just what I had in mind, yes. ;-)

(emerge and such) which makes heavy and extensive use of bash features seemed to produce the bad descriptor issue quite often with this patch.

And which bash version exhibits this problem?
BTW - autoconf-generated configure scripts aren't exactly leaving bash idle, either; but nothing untoward does ever happen on my system after the patch. Same with SlackBuild scripts.

Also, wrt the use case described in the issue description, well, that certainly didn't work for me as described,

So it certainly looks like a very broken bash.
Because, see, if it hadn't been working just as advertised on MY system, I would have no reason in the world to patch mc to prevent it interfering, now would I? ;-)

plus caused ignoredups to be essentially ignored.

Again, ignoredups works for me - inside each separate bash instance. And the rare cross-instance dups are taken care of with a Perl one-liner.

Doesn't look like a very viable use case to me.

Myself, I don't care either way. I have made a SlackBuild with the patches I need, and intend to expand it further when another "use case" of some never-fixed mc bug becomes too annoying to me.

But still I think it common courtesy to report the bugs I've found and fixed in others' projects. Because I myself dislike it when users of _my_ software don't report the bugs they've encountered, leaving me no chance to fix them.

comment:8 follow-up: ↓ 9 Changed 5 years ago by doktornotor

Please take this distro-based politics crap outside of this bug. I'm merely reporting that your patch causes problems. Period. And thanks, my bash is just fine.

comment:9 in reply to: ↑ 8 ; follow-up: ↓ 10 Changed 5 years ago by wjaguar

Replying to doktornotor:

Please take this distro-based politics crap outside of this bug. I'm merely reporting that your patch causes problems. Period.

Please leave emotions aside when discussing technical stuff. And remember - "correlation is not causation".

And thanks, my bash is just fine.

And am I to believe the patch is "causing problems" BY MAGIC, then? (I gather if you had any realistic explanation, you would have told me of it, yes?)

comment:10 in reply to: ↑ 9 ; follow-up: ↓ 11 Changed 5 years ago by doktornotor

Replying to wjaguar:

Please leave emotions aside when discussing technical stuff. And remember - "correlation is not causation".

You are not discussing any technical stuff here, you are just bashing a distro you don't use because obviously it must be "very broken" and "too unstable stuff" just because you don't have problems with your patch but other people have.

And am I to believe the patch is "causing problems" BY MAGIC, then?
(I gather if you had any realistic explanation, you would have told me of it, yes?)

I don't have time to dig into this. A command reads its input from stdin, prints normal output to stdout and error ouput to stderr. If one of the 3 fd's isn't open, then you get "bad file descriptor". It isn't a problem until you start appending stuff you PROMPT_COMMAND like you do in your patch - at least here. Once you do that, you get the problem I've described. Why? Your patch, you take care of explanations.

Your patch is causing problem that wasn't there before until you tried to fix the issue described in this bug. This patch breaks more than it fixes for me, so I'm commenting here b/c I'm obviously not interested in getting a patch committed that breaks more than it fixes (i.e. the illusive history synchronization b/w multiple sessions.

comment:11 in reply to: ↑ 10 Changed 5 years ago by wjaguar

Replying to doktornotor:

You are not discussing any technical stuff here, you are just bashing a distro you don't use because obviously it must be "very broken" and "too unstable stuff" just because you don't have problems with your patch but other people have.

No, I'm just pointing to the obvious fact that trivial modifications of one program triggering unrelated random errors in another, is a thing which very rarely happens with stable software.
Besides, isn't "I don't have problems but other people have" the very definition of the difference between stable system and unstable? ;-)

It isn't a problem until you start appending stuff you PROMPT_COMMAND like you do in your patch - at least here. Once you do that, you get the problem I've described. Why? Your patch, you take care of explanations.

Why should I? The patch is trivial, and it is for mc, it doesn't touch bash's code in any way. PROMPT_COMMAND is a documented variable of bash; mc's clobbering it is an obvious bug (not a documented behaviour, at the very least). I use the variable in a documented way to pass documented commands with documented options to bash. Which, on my system, works beautifully and solves a real problem.

And even if I did pass "rm -rf /" instead, what business is that of yours? Your system is yours to configure, and leaving global PROMPT_COMMAND unset is entirely within your power. As is debugging your copy of bash and/or filing a bugreport in an appropriate place.

comment:12 Changed 5 years ago by doktornotor

  • Cc notordoktor@… removed

Outta here... I'll revert the patch for myself if needed if it gets commited. Other than that - I wanted to comment on a patch causing problems with (frankly, already pretty hackish) subshell stuff in mc, NOT to be preached by some freaking Slackware evangelist.

comment:13 in reply to: ↑ 3 Changed 5 years ago by ossi

Replying to storchaka:

Sorry, need to use
"PROMPT_COMMAND=\"$PROMPT_COMMAND${PROMPT_COMMAND:+;}\"'pwd>&%d;kill -STOP $$'\n"

this seems more elegant :)

"PROMPT_COMMAND=${PROMPT_COMMAND:+$PROMPT_COMMAND; }'pwd>&%d;kill -STOP $$'\n"

Replying to doktornotor:

Well, this patch causes "bad descriptor" and some other weird error stuff [...]

this kinda makes no sense unless any of the built-in commands decide to close the pipe's file descriptor. play some with lsof.

btw, a cleaner solution than inheriting pipe file descriptors would be creating a randomly named fifo and having the prompt command write into it. that way random programs started from the shell would not inherit the handle.

comment:14 Changed 3 years ago by andrew_b

  • Version changed from version not selected to master
  • Branch state set to no branch
  • Milestone changed from 4.7 to Future Releases

comment:15 Changed 3 years ago by kecsap

I just would like to ask to include this one or similar patch to the subshell creation. This problem really breaks the custom prompts which is very useful in development with git. Currently, the prompt variable is overwritten and if I source the prompt modification script in the subshell, the Midnight Commander does not follow the directory changes in the subshell when switching with Ctrl+O back and forth.

I read some of the previous comments, please do not waste energies on flames, but fixing the problem. Even a new command line option/some kind of configuration or shell variable can be fine to have the possibility to override the current behavior of the mc.

Feedback: the proposed patch works fine for me with bash 4.2, mc 4.7.0.9 on Ubuntu 11.04 (Natty).

Note: See TracTickets for help on using tickets.