Ticket #2027 (closed defect: fixed)
MC clobbers the PROMPT_COMMAND shell variable
Reported by: | wjaguar | Owned by: | andrew_b |
---|---|---|---|
Priority: | major | Milestone: | 4.8.14 |
Component: | mc-core | Version: | master |
Keywords: | Cc: | egmont@…, pahan@… | |
Blocked By: | Blocking: | ||
Branch state: | approved | Votes for changeset: | committed-master |
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
Change History
comment:1 Changed 15 years ago by storchaka
Use
"PROMPT_COMMAND=\"$PROMPT_COMMAND${PROMPT_COMMAND:+ }\"'pwd>&%d;kill -STOP $$'\n"
comment:2 Changed 15 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 15 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:5 in reply to: ↑ 3 Changed 15 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 15 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 15 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 15 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 15 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 15 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 15 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 15 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 15 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 13 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 13 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).
comment:16 Changed 10 years ago by egmont
- Cc egmont@… added
Friendly ping, dear developers...
I was about to report this bug now. How come it's been unfixed for 5 years!? :( Obviously overwriting PROMPT_COMMAND is harmful, mc should definitely prepend/append to it, as it does for zsh.
This would probably also make ticket #3088 unnecessary.
(Similar bugreport in another product: https://bugzilla.gnome.org/show_bug.cgi?id=704960)
comment:17 Changed 10 years ago by andrew_b
- Owner set to andrew_b
- Status changed from new to accepted
- Branch state changed from no branch to on review
- Milestone changed from Future Releases to 4.8.14
Branch: 2027_bash_prompt
changeset:ff827ad6578cf69c95ebb99aed79194c59848f17
comment:18 Changed 10 years ago by egmont
Thanks!
Fedora + Gnome-terminal are working on a feature that you get notified if a command in a non-visible terminal tab completes (https://bugzilla.gnome.org/show_bug.cgi?id=711059). This will be technically achieved by emitting a special escape sequence from PROMPT_COMMAND. By appending rather than replacing, this will also work when the command is launched from mc.
comment:19 Changed 10 years ago by egmont
As for #3088 (gnome-terminal opening the new tab in the same dir as the current tab – this is also achieved by emitting an escape sequence from PROMPT_COMMAND telling the cwd to the terminal):
So far it didn't work at all from mc. With this patch, it sometimes works. If you execute a command from a directory, after its completion the new tab will open from that dir (since the prompt is printed and hence PROMPT_COMMAND is executed when the commad finishes). However, it doesn't work after a simple directory change, PROMPT_COMMAND is not executed and hence the relevant sequence is not emitted in this case.
The best would probably be if mc could send a cd command to the subshell each time the directory changes. I'm not familiar with the subshell code to tell if this can be implemented without undesirable side effects.
Other approaches could be manually duplicating executing the shell's pre-prompt hooks (but this sounds fragile in the long run), or independently of this ticket apply 3088's patch.
comment:20 follow-up: ↓ 21 Changed 10 years ago by pronobis
That branch works great. Thanks! Btw. Is there a way to get the prompt colors show up also in MC?
comment:21 in reply to: ↑ 20 Changed 10 years ago by andrew_b
Replying to pronobis:
Is there a way to get the prompt colors show up also in MC?
Unfortunately no. It is not supported, аll control sequences in the prompt are ignored.
comment:22 Changed 10 years ago by slavazanko
- Votes for changeset set to slavazanko
- Branch state changed from on review to approved
comment:23 Changed 10 years ago by andrew_b
- Status changed from accepted to testing
- Votes for changeset changed from slavazanko to committed-master
- Resolution set to fixed
Merged to master: [4361e49a3672041b23a02edfdf022c937720e8c6].