4625 slow zsh macos #4784

egmontkob · 2025-10-11T09:16:06Z

Proposed changes

Fix slow startup on macOS with zsh, and other related problems also affecting other platforms and shells.

Resolves: mc 4.8.32 slow start on MacOS #4625
Resolves: Read from pty master in nonblocking mode (due to unlikely deadlock) #4780
Resolves: Mc start up is slow on FreeBSD with Bash shell when mc is using a subshell #4548
Resolves: mc overrides user subshell prompt for tcsh #3792
Resolves: mc ignores $PS1 on macOS #4723
Resolves: Clean up subshell precmd and PS1 handling #4634

Checklist

👉 Our coding style can be found here: https://midnight-commander.org/coding-style/ 👈

I have referenced the issue(s) resolved by this PR (if any)
I have signed-off my contribution with git commit --amend -s
Lint and unit tests pass locally with my changes (make indent && make check)
I have added tests that prove my fix is effective or that my feature works
I have added the necessary documentation (if appropriate)

zyv · 2025-10-11T10:19:33Z

I wonder if this also fixes #4071... /cc @olfway @krobelus

egmontkob · 2025-10-11T16:29:08Z

I wonder if this also fixes #4071

I have no clue; it's unclear to me based on skimming through that ticket. I could not reproduce that issue yet (without my patches).

Let's leave that investigation for another day :)

zyv · 2025-10-12T07:15:41Z

I wonder if this also fixes #4071

I have no clue; it's unclear to me based on skimming through that ticket. I could not reproduce that issue yet (without my patches).

There was the same issue with zsh - lockups during directory change (#4331), which I've closed as a duplicate of the fish ticket (#4071) assuming it had the same nature as with fish. It even happened to me a couple of times in the last few years, and the last time I found that resizing the window (sending a SIGWINCH) unlocks the whole thing.

This all sounds very much related to what you were discussing in #4634 regarding the reading of pwd (which again relates to the problem on Solaris caused by too long pwds), so if something has now been done to that effect presumably fixing the issue I would be very much tempted to close it for the time being and let people report brand new complete issues with reproducers if they face any new lockups.

However, I've lost track a bit of what has and hasn't been done, and what you are still planning and not planning to do :( I'll try to read the commits attentively now, but it would help if you could mention which parts of #4634 are in, and which aren't, and if you are still going to do anything next or not.

… a function to avoid duplication Signed-off-by: Yury V. Zaytsev <[email protected]>

egmontkob · 2025-10-12T08:09:37Z

Thanks for your followup!

Two of your commits I agree with.

The one replacing \003 with a variable: I'd defer that for forthcoming #4781 in its separate PR, that's where it logically belongs to, here in this PR it's an irrelevant cleanup.

As for #4634: I did not read through that ticket, I only realized that it claims that overriding PS1 doesn't work and/or we shouldn't do that. The rest I did not work on, and (as for now) not planning to.

zyv · 2025-10-12T09:17:15Z

As for #4634: I did not read through that ticket, I only realized that it claims that overriding PS1 doesn't work and/or we shouldn't do that. The rest I did not work on, and (as for now) not planning to.

Okay, I sadly mistook you discussing the named pipes and so on for the indication that you might do something about it... you know, wishful thinking. Just too much fun working with you and I'm really happy about some obscure and long-standing issues now finally being properly taken care of ;-)

If that's not the case, then I'd just add a few trivial commits in addition to what you already did from #4634 (removing fallbacks, switching from newlines to ;, reformatting commands) and call it a day:

Add missing tests for filenames
Extract more hardcoded filenames as defines
Explain why filenames are the way they are
Add support for lksh in addition to mksh
Remove one last fallback for tcsh fixing mc overrides user subshell prompt for tcsh #3792

The rest (the pipe story) should then be continued in the Solaris ticket where it presents a real and actual problem right now: #4480. Maybe. One day.

zyv

I’d appreciate a sign-off for my changes, thanks!

olfway · 2025-10-12T10:47:43Z

I wonder if this also fixes #4071...

Unfortunately it doesn't, I tried with commit 2c8ed8d on 4625_slow_zsh_macos branch

I've added howto reproduce it here #4071 (comment)

ossilator

this is a somewhat thorough review; i looked at a lot of context. but i wouldn't claim that i have a full understanding.

the commit message of "Fix slow start with zsh on macOS, part 1/2" doesn't fully convince me:

the description of the why the code is there and why it's conditionally disabled should be added as a code comment. the commit message should only mention what was missing, maybe with an explicit reference to the added comment.
the reasoning itself seems suspicious. i can see how it would apply to the flush, but not the interrupt, which obviously does more - so there is more to be documented. "appear" is somewhat unclear; "be fed to the now hidden shell prompt" would follow from the previous.
as a side note, this illustrates how the whole thing is under-documented, here specifically how the init is different from regular operation (the echoed input from the init is simply discarded, i guess?)
i'd split off the dead PS1 overrides to a prior commit, to keep things clean. otoh, zyv's tcsh commit should be squashed with that, because it provides most of the justification. the fact that most of the overrides were ineffective is basically a side note.

"Followup cosmetical changes" is really a misnomer and does too much at once.

zyv's commits all look fine, with the following notes:

"extract command pipe reading code into a function to avoid duplication" should be squashed and then split out again to be put in front of the commit that necessitates the change, to avoid diff churn.
i wouldn't mind the "off-topic" commits being there ... because i linearize feature branches in my projects anyway. but here it's probably a good idea to split them out to separate PRs. an alternative would be to simply widen the scope of the PR, say "overhaul subshell interaction".

ossilator · 2025-10-11T13:28:14Z

src/subshell/common.c

+            " bind -x '\"\\e" SHELL_BUFFER_KEYBINDING "\":\"mc_print_command_buffer\"';"
            " bind -x '\"\\e" SHELL_CURSOR_KEYBINDING
-            "\":\"echo $BASH_VERSINFO:$READLINE_POINT >&%d\"'\n"
+            "\":\"echo $BASH_VERSINFO:$READLINE_POINT >&%d\"';"


i'd indent that line to make it clearer that it's a continuation.

I did that, but then "make indent" reverted that. I'd love to unfold into a single line if folks here don't mind.

I'd rather have it wrapped or otherwise reformatted with a continuation comment than adding a suppression for clang-format. As long as you start out with suppressions, it escalates very quickly and produces unexpected results. Our code was so extremely littered with indent suppressions, and I've put so much work to get rid of them, so that I'm kind of sensitive to the issue.

ossilator · 2025-10-11T13:28:44Z

src/subshell/common.c

-            "   eval \"PROMPT_COMMAND+=( 'pwd>&%d;kill -STOP $$' )\"\n"
-            " else\n"
-            "   PROMPT_COMMAND=${PROMPT_COMMAND:+$PROMPT_COMMAND\n}'pwd>&%d;kill -STOP $$'\n"
+            "/dev/null; then"


ossilator · 2025-10-11T13:29:34Z

src/subshell/common.c

        // pdksh based variants support \x placeholders but not any "precmd" functionality
-        return g_strdup_printf (" PS1='$(pwd>&%d; kill -STOP $$)'"
-                                "\"${PS1:-\\u@\\h:\\w\\$ }\"\n",
+        return g_strdup_printf (" PS1='$(pwd>&%d; kill -STOP $$)'\"${PS1:-\\u@\\h:\\w\\$ }\"\n",


this just re-wraps, so it's actually just a style change, which should be a separate commit.
but i'd actually just discard it, as it's not worth the churn.

A lot in this commit are style change only, so I don't see why it doesn't belong here.

I'd really like to do this reformatting because it's one single like we feed to mc, the line wrap made it much harder to read - IMHO.

it's another place where an indented continuation would be actually the nicest solution.

the core change are the line terminations, which have a functional effect (reducing the number of prompts). where a "mere" style change is directly related to that, it's fine to "sneak in". i just don't think entirely unrelated changes should be mixed with that, even if it's kind of the same type of code being modified.

I agree with @ossilator on principle, but I hope we can find the right degree of pragmatism to get this in rather sooner than later.

In my company, I have been enforcing the separation of changes that might have side effects and those that oughtn't rather rigorously, as well as requiring not so closely unrelated changes to go to different PRs, having commit messages polished closer to the Gerrit standards, and so on, and for a good reason.

But it's one thing to do this with a team of engineers reporting to you getting paid to work full time on a codebase we are responsible for in terms of delivery dates, quality standards, and financial obligations.

It's a different story (to my mind) when we all work on this project unpaid, stealing this time from other activities and without direct reporting structures / obligations. I'm so extremely happy that we (me and Andrew) have just got some help with this insane project and annoying complex issues lingering on for years...

Thus, I just want to get this good work in, so I'm rather relaxed about allowing a few loosely but not directly related changes in the same PR to keep it faster, more pragmatic, and not discourage anyone from contributing further.

Wherever I haven't responded to Ossi yet, I'll get back to them, but after a first quick round I tend to agree with him. In this particular case, I'm happy to separate the functional and the cosmetic changes to separate smaller commits. (Whereas I'm also glad to hear Yury that you're quite loose on these things.)

Feel free to rebase at will. I won't be making any more commits for now. I've added mine as fixups so that they can be autosquashed if needed.

ossilator · 2025-10-11T13:31:47Z

src/subshell/common.c

+/**
+ *  Read in nonblocking mode.
+ *
+ *  On a tty master, waiting for data using a select() and then reading it with a blocking read()


flowed text should be wrapped to 80 columns max.

yeah, i know, this wasn't the policy here so far, but that's not a good reason to perpetuate this typographic mistake.

I followed what seemed to be the style already used in this file. If there's an actual policy that newly written code should follow then I'm happy to. I personally don't like sticking to 80, I find 100-120-ish to be much more usable.

i'm not talking about code!

I didn't want to either – although I indeed say "code" when I meant that both for code & comments :)

Purely in that file, mc adheres to a 99 column margin very-very often, including the separator lines between functions, and a lot of flowed text. At other places, indeed, a smaller (80-ish) margin is used.

If there's a clear, explicit documentation (which I'm not aware of) that newly written code & comments should adhere to then I'm happy to. If there's an implicit style (i.e. let's follow whatever is already there), I'm happy to follow that (but in our case it's already mixed). If there's neither then I guess the author of the commit gets to choose. So I went with my personal preference, which is wider margin, matching the code, leading to less wasted screen real estate.

We have do some formal agreements on style with @aborodin (line width <= 100), but nothing specific regarding line width for comments:

https://midnight-commander.org/coding-style/

My preference is to avoid these types of discussions in the first place if possible in that either the stuff passes the formatter and we don't talk about that, or we change the formatter rules and we don't talk about that ;)

I don't think clang-format supports a separate margin size for reflowing comments, so if we want to have 100-chars lines, we'll have to live with 100-char comments. I don't want to have anything under 100-chars for lines, so I hereby declare 100-char comments as acceptable :/

ossilator · 2025-10-12T11:41:41Z

tests/lib/mcconfig/user_configs_path.c

-    { CONF_CACHE, MC_TREESTORE_FILE },     // 21.
-    { CONF_CACHE, EDIT_HOME_TEMP_FILE },   // 22.
-    { CONF_CACHE, EDIT_HOME_BLOCK_FILE },  // 23.
+    { CONF_MAIN, MC_CONFIG_FILE },      //


if the removal of the enumeration values was intentional, then the empty comment markers should also be removed. though i kind of dislike this being done in the same commit anyway.

Just to clarify, I have no clue why @slavaz added these numbers to all tests that he's written. It could have had something to do with the formatter, or maybe he found it easier to refer to different data points for parameterized tests.

In this case, according to me, the numbers don't add any value and are annoying to maintain in order. Initially, I didn't remove the comments completely because clang-format, which we are now using, sometimes needs them to be convinced to leave the enum values as one item per line. But it doesn't seem to make any difference in this case, so I've removed them altogether now.

ossilator · 2025-10-12T15:29:34Z

src/subshell/common.c

                // On IBM i, read(1) can return 0 for a non-closed fd
                continue;
 #else
+                if (bytes == -1 && (errno == EAGAIN || errno == EWOULDBLOCK))


based on the comment above, this should be outside the #ifdef.
in fact, line 923 can/should be refactored into nested conditionals.

ossilator · 2025-10-12T15:31:31Z

src/subshell/common.c

        {
            const ssize_t bytes =
-                read (mc_global.tty.subshell_pty, pty_buffer, sizeof (pty_buffer));
+                read_nonblock (mc_global.tty.subshell_pty, pty_buffer, sizeof (pty_buffer));


side note: i'd speculate that this will address the fixme three lines up. though then the loop should be a while (TRUE) one in the first place.

another point, here because gh is too damn pathetic to allow adding comments to lines that were unfolded: it would seem that line 979 could also use this treatment, because something else ™ (say, ssh or something) may for whatever reason flush mc's stdin.

You refer to that read (STDIN_FILENO, ...), correct? When mc reads from the outer terminal.

Good point, but it might need more occurrances, all across main mc / mcview / mcedit, all their menu systems and whatnot too. I'll check how much work that would be. Maybe I'll just leave it unfixed for now and file a ticket :) I'll move the helper method to a lower level component if other places will also need it.

if we were to change literally every occurrence, then just setting the fd to non-blocking globally would be a good idea. hmm, that's also the case for our pty master ...

The problem is: while every read() has to be non-blocking, unfortunately every write() has to remain blocking. If the channel is clogged then mc must wait (and blocking is okay), otherwise written data can get lost.

As for the outer terminal, you can't be sure whether stdin and stdout are two different open()s of the slave side by filename (i.e. separate fcntl flags) or dup()s of each other (common set of flags), I think the latter one is more common.

Correcting myself:

Not every read() has to be non-blocking.

If the intent of the code is to block until data arrives then a blocking read() is absolutely fine. We just have to be prepared for a possible false positive -1 EAGAIN if someone flushes (or hijacks) the data.

If the intent of the code is to check whether data is available over one of multiple sources, using select() + FD_ISSET() or equivalent, and then read if there's any, then there's the possibility of a nasty deadlock and we need to read nonblockingly.

blocking writes could be simulated by select()-looping after EAGAIN-ing writes. in cases where reading and writing needs to be interleaved anyway, this wouldn't even add much complexity. and the write() calls are already using a central wrapper anyway.

not having looked at the details, i won't argue that this would be the right solution, only that it's fundamentally feasible.

If you have full control over everything then yes, it could also work this way around, too.

For the inner tty, which I've addressed, I'm not sure if it would simplify or make it more complex, I don't know how many places there are where we write to it (is it indeed only that single write_all()?). I chose one solution, rather than evaluating multiple possible solutions. This one "felt" better to me because blocking operation is typically the default, so I went with having exceptions wherever we had to have exceptions, rather than twisting it inside out. But I guess your approach could equally work. If there's multiple reviewer votes for that then I'll switch to that method.

For the outer tty, I'm afraid output is fully managed by ncurses/slang and I'm afraid we can't hook up there to add such a wrapper, and I don't think they work correctly with a nonblocking fd (although I haven't verified any of these claims, these are just my gut feelings). I think there a read-wrapper is the only way to go. So here, consistency between the two tty lines casts a vote for my current approach :) (even though I don't think I'll address the outer tty now).

Coincidentally, in #4480, I will also have to perform a non-blocking read. Not even on a tty line, but on the pwd pipe. And for completely different reasons than here. Alternatively I could also do a select(timeout=0)+read() there but we all know how cumbersome it is to set up a select().

Anyway, it comes super handy to have a one-liner read_nonblock(). I might even consider moving this method to lib/util.c :)

ossilator · 2025-10-12T16:10:10Z

src/subshell/common.c

            " bindkey '^[" SHELL_CURSOR_KEYBINDING "' mc_print_cursor_position\n"
-            " _mc_precmd(){ pwd>&%d;kill -STOP $$ }; precmd_functions+=(_mc_precmd)\n"
-            "PS1='%%n@%%m:%%~%%# '\n",
+            " _mc_precmd(){ pwd>&%d;kill -STOP $$ }; precmd_functions+=(_mc_precmd)\n",


you could add the missing space before { while you're here.

The space is added later on in 11268dd .

ossilator · 2025-10-12T16:48:21Z

src/subshell/common.c

+
+        if (FD_ISSET (mc_global.tty.subshell_pty, &read_set))
+        {
+            /* Keep reading the pty to avoid possible deadlock with the shell. Throw away the data.


you should explain why it's ok to discard the data.

that one really needs addressing.

ossilator · 2025-10-12T16:53:12Z

src/subshell/common.c

+            "\":\"echo $BASH_VERSINFO:$READLINE_POINT >&%d\"';"
            " if test ${BASH_VERSION%%%%.*} -ge 5 && [[ ${PROMPT_COMMAND@a} == *a* ]] 2> "
-            "/dev/null; then\n"
-            "   eval \"PROMPT_COMMAND+=( 'pwd>&%d;kill -STOP $$' )\"\n"


the eval removal really belongs into a separate (prior) commit.

… a function to avoid duplication Signed-off-by: Yury V. Zaytsev <[email protected]>

egmontkob · 2025-10-19T14:16:41Z

New version pushed, please take another look. I've also sneaked in a few brand new tiny changes.

Ossi, I don't think I've addressed all your concerns, especially around the "1/2" commit message. Could you please be more specific about the changes you'd like to see? Please bear in mind that I don't understand the situation nearly as well as you hope I do, I don't understand why those 4 places where we flush the queue (one explicit flush and three ^Cs) are there exactly, which one is triggered when exactly, or maybe whether some of them can never occur while initializing (therefore my change there is strictly speaking unnecessary, but makes the code more robust and more consistent). So if you're expecting me to document what's going on exactly, unfortunately I can't do that.

zyv

Thank you very much! I understand that the '\003' will come later.

egmontkob · 2025-10-23T08:59:58Z

So, back to the fish problem:

My patch "[...] part 2/2", ever since its first iteration, breaks the initial fish prompt if mc is started up in the home directory.

The obvious low hanging part: in the init string we send to fish, we override the prompt to be

echo "$PWD" >&%d; fish_prompt_mc; kill -STOP $fish_pid

(fish_prompt_mc is the previously saved prompt, i.e. whatever the user really wishes to see as prompt.)

This is clearly wrong, the synchronization is at the wrong place, we cannot distinguish the final bits of a command's output from the prompt. Obviously the second and third commands need to be swapped.

But this is still not enough. There's a race condition (which I don't fully understand yet) competing for reading the prompt (src/filemanager/layout.c do_load_prompt() -> src/subshell/common.c read_subshell_prompt()) vs. reading the persistent cmdline test response. In case of fish, timings just so happen that we receive the prompt while we're waiting for the persistent cmdline test response.

So, while waiting for the persistent cmdline test response, it's not enough for us to either discard or forward the data, we also have to interpret it looking for a prompt -- hell no way.

But the prompt shows up correctly if we start mc from any other directory, not home. This is because then we inject a cd and catch the second prompt which appears as a response to this cd command.

This looks like an easy and robust way. So I guess we could just force that cd even if it stays in the same directory.

… subshell Synchronize first, then print the prompt; this is the proper order. Signed-off-by: Egmont Koblinger <[email protected]>

…t startup Previously we omitted injecting a cd command to the subshell at startup if it was already in the target directory. The first prompt of the subshell was read and displayed in mc's panels. This plays badly with the forthcomming change where testing the persistent buffer code will read and discard the subshell's data to avoid a deadlock. The proper solution would be more complicated than justified. Just get a new prompt printed and we'll catch that. Signed-off-by: Egmont Koblinger <[email protected]>

… a function to avoid duplication Signed-off-by: Yury V. Zaytsev <[email protected]>

egmontkob · 2025-10-23T10:22:17Z

Two new commits added to fix the fish story, titled "Fallout from PR [...]".

I'm not really happy with the one to always inject a cd command, let me know if you see a better place or cleaner way to do this.

zyv · 2025-10-23T10:29:57Z

Two new commits added to fix the fish story, titled "Fallout from PR [...]".

I'm not really happy with the one to always inject a cd command, let me know if you see a better place or cleaner way to do this.

In as far as forcing a cd for fish to get the prompt is concerned, maybe @krobelus has something to say on that...

egmontkob · 2025-10-23T10:31:47Z

In as far as forcing a cd for fish to get the prompt is concerned, maybe @krobelus has something to say on that...

Note: I don't think this problem is specific to fish per se, it's just the one that happened to hit that race condition for me.

ossilator · 2025-10-23T11:14:04Z

this sounds like something that resolving the sync-related issues would address. does it make sense to consider working around it here then?

ossilator · 2025-10-23T11:34:21Z

so, commit message philosophy time. 😁

indeed, a commit message should say what was done, and why. specifically, it should not say how, as that's in the diff.

bullet lists on the "what"-side are a clear indication that a commit should be split, but we agreed that we don't want to waste time on that in this trivial case. that's a reasonable trade-off.

i do however think that listing all the "what"s makes sense, so one can check whether the documented intent matches the implementation, even when it seems trivial and disproportionate - the assessment suddenly changes when a seemingly innocuous change unexpectedly breaks something.

i also don't buy the "it doesn't matter in such a small project" argument. arguably, it's even more important in this case, because personnel continuity is much more spotty, so there might be no-one around to ask later.

egmontkob · 2025-10-23T11:48:50Z

this sounds like something that resolving the sync-related issues would address.

I don't think so. The following happens:

mc sends init string, including setting up precmd
shell executes them
they synchronize via the sigstop-sigcont dance
mc injects the ESC _ and ESC + keypresses to test persistent cmdline
the shell prints its prompt to stdout, and prints the response to ESC _ and ESC + into its pipe

There's no way to synchronize after the shell prints its prompt, to send the keypresses then. (Adam says it's possible to hook up with zsh, but I truly doubt other shells also offer such a hook.)

The prompt in the tty line, and the response to ESC _ ESC + over the pipe, arrive sort of in parallel, and we can't make it serial in this order.

We don't know when to stop reading the prompt and jump to reading the ESC _ ESC + response. Do avoid the deadlock with zsh's draining, we have to at all times read the tty line, and locate the prompt there.

It surely is possible to refactor the code to do that. I.e. while reading the ESC _ ESC + response keep reading the tty, and later see if it contained a prompt. It just requires a way too heavy refactoring of our current business logic of locating the prompt, a part of the code I don't understand at all.

It's just much easier to serialize these two steps; but as seen, we can't do it in the order of prompt first, ESC _ later. What we can easily do is to serialize them in the opposite order (it's reliable in practice, and #4811 can fix the theorethical hole). That is what my proposed patch does. We ignore the first prompt, handle the ESC _ stuff, and once that's done we get the second prompt.

This is what already happens if we start up from any other directory than home. We might read and properly parse the first prompt, but we also get a second one, replacing the first. The simple fix to the drain deadlock makes reading the first prompt unreliable. So always get a second one; serially, after the ESC _ stuff.

Addressing #4811 / #4790 wouldn't automatically fix this problem here.

egmontkob · 2025-10-23T12:07:52Z

bullet lists on the "what"-side are a clear indication that a commit should be split

Let's just agree to disagree.

Following this philosophy, this current pending PR of 16 commits should probably be split to maybe about 100 commits. I can't even imagine how bigger pieces of work should look like.

As a volunteer developer, if I'd have to spend this much overhead on top of the actual work, I'd just leave that project and find another one to improve. (For highly paid jobs, I'm much more willing to follow such given instructions.)

Also, on mc's homepage Andrew and Yury are listed as the two main developers, so I take their instructions over yours.

Ossi, your code reviewing contributions are on one hand absolutely invaluable, you really do find actual mistakes and do ask great questions that make me rething the design, I'm truly grateful for them; but on the other hand, your pedantry around the tiniest insignificant details and your expectations that the code and meta-info should all look like to your personal taste to such incredible extent (let alone that in this very case you expect me to rework zyv's commits) is just ridiculous, and honestly, quite annoying and demotivating.

I'd really appreciate if we could focus our efforts and time on things that really do matter.

ossilator · 2025-10-23T12:32:02Z

It surely is possible to refactor the code to do that. I.e. while reading the ESC _ ESC + response keep reading the tty, and later see if it contained a prompt.

well, that's kind of what i was getting at ...

It just requires a way too heavy refactoring of our current business logic of locating the prompt, a part of the code I don't understand at all.

regarding the "understanding" part, i suspect you'll need to get there anyway to answer the questions i posed elsewhere.

i suspect that the logic of "locating" the prompt is to simply take the last buffered line before the sigstop arrived as the pwd, send the cont, and use the first output that arrives afterwards as the prompt.

my idea would be to put things upside down: shell-side, first do the self-stop, then do the persistent buffer magic, print pwd, and let the shell print the prompt. mc-side, the sigchld (and the sigcont we send) is the start point. then we collect from the pty and the pipe(s) in parallel. these channels would be self-terminating with nulls, and we'd know which data points we expect, so by simply recording what we already received we would know when the "handshake" is over.

this is modular enough that the sigstop dance can be replaced with a sentinel cookie in the prompt, and a tty stream scanner on the mc side. and instead of relying on the shell doing some stuff by itself, all commands (except for the prompt printing itself) can be injected. thus this can even support "dumb" shells while maintaining a uniform architecture.

egmontkob · 2025-10-23T12:52:42Z

regarding the "understanding" part, i suspect you'll need to get there anyway to answer the questions i posed elsewhere.

No, I don't need to understand the code enough to answer each and every question that you have that are tangetially related to something I touch and fix in a different way. I am in the process of fixing a bunch of bugs, but that doesn't mean that I need to become an expert in all neighboring areas of the code.

shell-side, first do the self-stop, then do the persistent buffer magic, print pwd, and let the shell print the prompt

The persistent buffer magic relies on the shell's interactive line editing. The shell can only do that after printing the very first prompt.

So what I do: self-stop, then print a prompt that mc ignores, then persistent buffer magic, then print the second prompt (which mc will see).

then we collect from the pty and the pipe(s) in parallel

Collecting in parallel and later parsing both of them seems to require quite a significant refactoring of the code. Even if the final state would be nicer, I'm not going to put that work into this now when a much simpler solution is already available. Unless someone finds significant funding for the project, allowing me to work significantly more time on it. Alternatively, if you feel like doing refactoring, please go ahead.

a sentinel cookie in the prompt, and a tty stream scanner on the mc side

Same. It's another possible solution to the problem I'm fixing; a noticeably more complex one (and I also have doubts about its feasibility). I have no desire to work on this.

I have a proposed solution to a bunch of located issues; solutions that I believe are a good balance between reliability, maintainability, vs. implementation cost. I don't have unlimited resources to always get to a (potentially) nicer final state if the implementation cost is significantly larger.

ossilator · 2025-10-23T13:28:39Z

The persistent buffer magic relies on the shell's interactive line editing. The shell can only do that after printing the very first prompt.

that's a good point.
on the upside, that kind of makes the thing more self-syncing.

So what I do: self-stop, then print a prompt that mc ignores, then persistent buffer magic,
then print the second prompt (which mc will see).

... given the current architecture.

Even if the final state would be nicer, I'm not going to put that work into this now when a much simpler solution is already available.

that's your prerogative, but the calculation might backfire: with all the issues you're finding and documenting along the way, and evidently also trying to address, it's quite plausible that actually biting the apple might be the overall cheaper approach (potentially even much more so).

egmontkob · 2025-10-23T13:41:22Z

... given the current architecture.

Yes, but I'm not planning to significantly rework the architecture, it's just not worth it for me – while, by the way, I'm addressing tons of bugs that I'm even personally not affected by.

that's your prerogative, but the calculation might backfire

Look, I get to decide what I spend my free time on, not you. If the project's maintainers find my contributions to be potentially harmful in the long run (getting he way of a future refactoring), they can reject my PRs. If I get reasonable requests, either from the two maintainers or from you or from anyone else, I'm happy to adjust my work. But nitpicking on commit splits or log details, or asking for much bigger refactoring, are both unreasonable requests.

If you want to work on that big refactoring, I do encourage you, please go ahead, I'd love to see any big cleanup land in mc, even if that obsoletes and replaces my work.

If you want me to work on that, you somehow have to financially fund that work (and I'm not talking about pocket change).

This project does not have everything. It has certain things, including this PR, and it can decide whether to accept or reject it. But rejecintg it won't make the proper big refactoring magically appear.

… 1/2 Fix data loss while injecting commands to our subshell. This data loss affects all shells, not just zsh, and all platforms, not just macOS. Discarding the keypresses, i.e. removing the ones already placed in the tty line, either explicitly (tcflush()) or implicitly (by sending the interrupt character Ctrl+C) is useful when mc's panels are presented after running a command that the user initiated. Here we don't want letters that the user might have typed ahead to be sent to the subshell. However, when injecting our shell initialization code, these actions are harmful as they can cause parts of these commands to get lost. Signed-off-by: Egmont Koblinger <[email protected]>

… 2/2 Avoid a deadlock arising by zsh draining its output channel (i.e. waiting for mc to read them), while mc is waiting over a different channel for zsh's response to some magic keypresses to test the persistent command line feature. Fix this by making mc behave as it's expected from a terminal master: still consuming incoming data while performing that other task. The bug did not affect Linux because there draining the channel, at least over local pseudoterminals, is a no-op, it does not actually wait for the master side to read that data. Signed-off-by: Egmont Koblinger <[email protected]>

… a function to avoid duplication Signed-off-by: Yury V. Zaytsev <[email protected]>

Code readability, trustworthiness should be more important than supporting 29 year old systems. Signed-off-by: Egmont Koblinger <[email protected]>

We're writing to a pipe, it's pointless to append. Signed-off-by: Egmont Koblinger <[email protected]>

… tcsh command Signed-off-by: Egmont Koblinger <[email protected]>

Whitespace-only changes to our init strings to have consistent formatting across shells. Signed-off-by: Egmont Koblinger <[email protected]>

…ndings in our shell init strings Send one logical line to the shell so that it only executes the pre-prompt function and only prints the prompt once; however, split it into short physical lines for systems where cooked mode's buffer is small. Signed-off-by: Egmont Koblinger <[email protected]>

Signed-off-by: Yury V. Zaytsev <[email protected]>

…ext` Signed-off-by: Yury V. Zaytsev <[email protected]>

ossilator · 2025-10-23T18:01:23Z

Following this philosophy, this current pending PR of 16 commits should probably be split to maybe about 100 commits.

i'm actually quite happy with the current granularity.

your pedantry around the tiniest insignificant details and your expectations that the code and meta-info should all look like to your personal taste to such incredible extent [...] is just ridiculous, and honestly, quite annoying and demotivating.

Look, I get to decide what I spend my free time on, not you.

when you re-read what i actually wrote, you'll find that at no point i implied that i expect anything in particular to happen. i just say what i think, and when you contradict, i argue my point of view. it's between you guys to decide whether you actually heed any particular observation/suggestion/recommendation.

(For highly paid jobs, I'm much more willing to follow such given instructions.)

no, you wouldn't. psychology just doesn't work that way; studies have shown this over and over.

and in most commercial projects, there is even more pressure to cut corners. so the "we're all volunteers" take just isn't convincing.

so let me offer this perspective: what we do here is public. i for one have the ambition that everything i commit to a mainline branch is sufficiently polished that i can proudly point to it. it's really a matter of your personal standards and the surrounding culture what "level of perfection" you deem sufficient.

i don't think that your current work would impede a possible refactoring (or more correctly, a change of the protocol); this isn't something user-visible that would be subject to compatibility constraints, and you're going into the direction of more async operation anyway.
what i did say is that doing the more fundamental change might be cheaper than fixing (and sometimes basically just working around) the issues one by one. of course you can't know that before you actually take a stab at it. but from what i have gathered so far, the task doesn't seem exceedingly large - it's mostly just reshuffling some code and merging some loops into a more state-machine-like thingie.

github-actions bot added this to the Future Releases milestone Oct 11, 2025

github-actions bot added needs triage Needs triage by maintainers prio: medium Has the potential to affect progress labels Oct 11, 2025

egmontkob force-pushed the 4625_slow_zsh_macos branch from 152a82e to 11268dd Compare October 11, 2025 09:18

egmontkob requested review from aborodin and zyv October 11, 2025 09:26

zyv modified the milestones: Future Releases, 4.8.34 Oct 11, 2025

zyv added area: core Issues not related to a specific subsystem and removed needs triage Needs triage by maintainers labels Oct 11, 2025

zyv requested a review from ossilator October 11, 2025 10:20

zyv added a commit to egmontkob/mc that referenced this pull request Oct 12, 2025

Ticket MidnightCommander#4784: extract command pipe reading code into…

1be352f

… a function to avoid duplication Signed-off-by: Yury V. Zaytsev <[email protected]>

This was referenced Oct 12, 2025

mc ignores $PS1 on macOS #4723

Open

Mc start up is slow on FreeBSD with Bash shell when mc is using a subshell #4548

Open

zyv approved these changes Oct 12, 2025

View reviewed changes

zyv mentioned this pull request Oct 12, 2025

Clean up subshell precmd and PS1 handling #4634

Open

olfway mentioned this pull request Oct 12, 2025

mc sometimes hangs on directory change with subshell on macOS #4071

Closed

ossilator suggested changes Oct 12, 2025

View reviewed changes

egmontkob pushed a commit to egmontkob/mc that referenced this pull request Oct 19, 2025

Ticket MidnightCommander#4784: extract command pipe reading code into…

49267c6

… a function to avoid duplication Signed-off-by: Yury V. Zaytsev <[email protected]>

egmontkob force-pushed the 4625_slow_zsh_macos branch from 8fc2f3d to d80b46c Compare October 19, 2025 14:04

zyv approved these changes Oct 19, 2025

View reviewed changes

mc-worker requested review from mc-worker and removed request for aborodin October 19, 2025 18:00

egmontkob added 2 commits October 23, 2025 11:03

Fallout from PR MidnightCommander#4784: Fix synchronization with fish…

4a05c35

… subshell Synchronize first, then print the prompt; this is the proper order. Signed-off-by: Egmont Koblinger <[email protected]>

egmontkob pushed a commit to egmontkob/mc that referenced this pull request Oct 23, 2025

Ticket MidnightCommander#4784: extract command pipe reading code into…

f820d46

… a function to avoid duplication Signed-off-by: Yury V. Zaytsev <[email protected]>

egmontkob force-pushed the 4625_slow_zsh_macos branch from 1bb0e03 to afed302 Compare October 23, 2025 10:20

egmontkob and others added 12 commits October 23, 2025 16:46

Ticket MidnightCommander#4784: extract command pipe reading code into…

054eb4f

… a function to avoid duplication Signed-off-by: Yury V. Zaytsev <[email protected]>

Ticket MidnightCommander#4599: Remove eval hack for bash 1.x

575016e

Code readability, trustworthiness should be more important than supporting 29 year old systems. Signed-off-by: Egmont Koblinger <[email protected]>

Simplify redirection operator in tcsh's init string

789baf4

We're writing to a pipe, it's pointless to append. Signed-off-by: Egmont Koblinger <[email protected]>

Ticket MidnightCommander#4120: Add a brief comment about a mysterious…

b10beee

… tcsh command Signed-off-by: Egmont Koblinger <[email protected]>

Ticket MidnightCommander#4625: Cosmetical changes in shell init strings

659b283

Whitespace-only changes to our init strings to have consistent formatting across shells. Signed-off-by: Egmont Koblinger <[email protected]>

Ticket MidnightCommander#4634: add support for lksh build of mksh

c68e17e

Signed-off-by: Yury V. Zaytsev <[email protected]>

Ticket MidnightCommander#4634: extract init file names defines

c781eb4

Signed-off-by: Yury V. Zaytsev <[email protected]>

Ticket MidnightCommander#4634: add missing tests

a5c3770

Signed-off-by: Yury V. Zaytsev <[email protected]>

tests: fix unneeded symbol duplication in `exec_get_export_variables_…

a23bdb1

…ext` Signed-off-by: Yury V. Zaytsev <[email protected]>

egmontkob force-pushed the 4625_slow_zsh_macos branch from afed302 to a23bdb1 Compare October 23, 2025 14:49

Uh oh!

4625 slow zsh macos #4784

Are you sure you want to change the base?

4625 slow zsh macos #4784

Conversation

egmontkob commented Oct 11, 2025 • edited by zyv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Proposed changes

Checklist

Uh oh!

zyv commented Oct 11, 2025

Uh oh!

egmontkob commented Oct 11, 2025

Uh oh!

zyv commented Oct 12, 2025

Uh oh!

egmontkob commented Oct 12, 2025

Uh oh!

zyv commented Oct 12, 2025

Uh oh!

zyv left a comment

Choose a reason for hiding this comment

Uh oh!

olfway commented Oct 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ossilator left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

egmontkob Oct 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

egmontkob commented Oct 11, 2025 •

edited by zyv

Loading

olfway commented Oct 12, 2025 •

edited

Loading

egmontkob Oct 12, 2025 •

edited

Loading

egmontkob commented Oct 23, 2025 •

edited

Loading

ossilator commented Oct 23, 2025 •

edited

Loading

ossilator commented Oct 23, 2025 •

edited

Loading