Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MC clobbers the PROMPT_COMMAND shell variable #2027

Closed
mc-butler opened this issue Feb 14, 2010 · 26 comments
Closed

MC clobbers the PROMPT_COMMAND shell variable #2027

mc-butler opened this issue Feb 14, 2010 · 26 comments
Assignees
Labels
area: core Issues not related to a specific subsystem prio: medium Has the potential to affect progress
Milestone

Comments

@mc-butler
Copy link

Important

This issue was migrated from Trac:

Origin https://midnight-commander.org/ticket/2027
Reporter wjaguar (@wjaguar)
Mentions egmont@….com (@egmontkob), pahan@….info (@Hubbitus)

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.

Note

Original attachments:

@mc-butler
Copy link
Author

Changed by wjaguar (@wjaguar) on Feb 14, 2010 at 13:23 UTC

@mc-butler
Copy link
Author

Changed by storchaka (storchaka@….com) on Feb 15, 2010 at 9:52 UTC (comment 1)

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

@mc-butler
Copy link
Author

Changed by storchaka (storchaka@….com) on Feb 15, 2010 at 10:01 UTC (comment 2)

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

@mc-butler
Copy link
Author

Changed by doktornotor (notordoktor@….com) on Mar 18, 2010 at 11:03 UTC (comment 3)

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.

@mc-butler
Copy link
Author

Changed by doktornotor (notordoktor@….com) on Mar 18, 2010 at 11:03 UTC (comment 4)

  • Cc set to notordoktor@….com

@mc-butler
Copy link
Author

Changed by wjaguar (@wjaguar) on Mar 18, 2010 at 12:05 UTC (comment 3.5)

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

@mc-butler
Copy link
Author

Changed by doktornotor (notordoktor@….com) on Mar 18, 2010 at 12:15 UTC (comment 6)

@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.

@mc-butler
Copy link
Author

Changed by wjaguar (@wjaguar) on Mar 18, 2010 at 14:22 UTC (comment 6.7)

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.

@mc-butler
Copy link
Author

Changed by doktornotor (notordoktor@….com) on Mar 18, 2010 at 14:51 UTC (comment 8)

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.

@mc-butler
Copy link
Author

Changed by wjaguar (@wjaguar) on Mar 18, 2010 at 14:57 UTC (comment 8.9)

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

@mc-butler
Copy link
Author

Changed by doktornotor (notordoktor@….com) on Mar 18, 2010 at 15:19 UTC (comment 9.10)

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.

@mc-butler
Copy link
Author

Changed by wjaguar (@wjaguar) on Mar 18, 2010 at 19:02 UTC (comment 10.11)

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.

@mc-butler
Copy link
Author

Changed by doktornotor (notordoktor@….com) on Mar 18, 2010 at 20:22 UTC (comment 12)

  • Cc notordoktor@….com deleted

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.

@mc-butler
Copy link
Author

Changed by ossi (@ossilator) on Mar 18, 2010 at 21:01 UTC (comment 3.13)

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.

@mc-butler
Copy link
Author

Changed by andrew_b (@aborodin) on Nov 5, 2011 at 6:23 UTC (comment 14)

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

@mc-butler
Copy link
Author

Changed by kecsap (csaba.kertesz@….com) on Nov 17, 2011 at 9:38 UTC (comment 15)

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

@mc-butler
Copy link
Author

Changed by egmont (@egmontkob) on Feb 1, 2015 at 23:38 UTC (comment 16)

  • Cc set to egmont@….com

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)

@mc-butler
Copy link
Author

Changed by andrew_b (@aborodin) on Feb 9, 2015 at 11:02 UTC (comment 17)

  • Owner set to andrew_b
  • Branch state changed from no branch to on review
  • Status changed from new to accepted
  • Milestone changed from Future Releases to 4.8.14

Branch: 2027_bash_prompt
[ff827ad6578cf69c95ebb99aed79194c59848f17]

@mc-butler
Copy link
Author

Changed by egmont (@egmontkob) on Feb 9, 2015 at 15:22 UTC (comment 18)

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.

@mc-butler
Copy link
Author

Changed by egmont (@egmontkob) on Feb 9, 2015 at 15:27 UTC (comment 19)

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.

@mc-butler
Copy link
Author

Changed by pronobis (a.pronobis@….com) on Feb 9, 2015 at 21:55 UTC (comment 20)

That branch works great. Thanks! Btw. Is there a way to get the prompt colors show up also in MC?

@mc-butler
Copy link
Author

Changed by andrew_b (@aborodin) on Feb 10, 2015 at 7:15 UTC (comment 20.21)

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.

@mc-butler
Copy link
Author

Changed by slavazanko (@slavaz) on Feb 13, 2015 at 9:35 UTC (comment 22)

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

@mc-butler
Copy link
Author

Changed by andrew_b (@aborodin) on Feb 16, 2015 at 6:12 UTC (comment 23)

  • Resolution set to fixed
  • Status changed from accepted to testing
  • Votes changed from slavazanko to committed-master

Merged to master: [4361e49].

@mc-butler
Copy link
Author

Changed by andrew_b (@aborodin) on Feb 16, 2015 at 6:15 UTC (comment 24)

  • Status changed from testing to closed

@mc-butler
Copy link
Author

Changed by Hubbitus (@Hubbitus) on Aug 18, 2015 at 15:54 UTC (comment 25)

  • Cc changed from egmont@….com to egmont@….com, pahan@….info

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: core Issues not related to a specific subsystem prio: medium Has the potential to affect progress
Development

No branches or pull requests

2 participants