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.

…king mode This prevents a possible deadlock if the slave flushes the line just before we attempt to read from it. Signed-off-by: Egmont Koblinger <[email protected]>

…e the subshell's prompt Do not override 'PS1' (bash and zsh) and 'prompt' (tcsh). This is unkind behavior (we should respect the user's preference), and due to a bug that is fixed in the next commit (ticket MidnightCommander#4625) it works only for tcsh anyway. Additional comments by Yury regarding tcsh: This override was sneaked in MidnightCommander#2742 without any explanation, presumably for consistency and/or due to confusion with our non-working fallbacks for other shells. In as far as fallbacks are concerned, I still haven't gotten to the bottom of it as to why they were introduced in the first place. By reading some old code, my impression was that the idea was to provide consistent experience across shells. Then probably the code was misinterpreted to mean a fallback if no prompt is defined. And finally other changes broke it anyways and it was carried further as a cargo cult. Even later overrides were removed one by one after the users complained, and finally it seems there is only tcsh one left. So much for a vague backstory on this change. Relevant commit: f596c91 Signed-off-by: Yury V. Zaytsev <[email protected]> Signed-off-by: Egmont Koblinger <[email protected]>

… 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]>

… 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]>

egmontkob · 2025-10-27T22:04:42Z

the reference remains present implicitly by virtue of branches being force-merged.

"implicitly"

If I see a commit and don't quite understand the story or the rationale behind, I look at the issue referenced by the commit log, but it wouldn't occur to me to check the PR / review process of that commit. That's why I made it explicit. But maybe it's just me.

also, commit messages should be self-contained. external references should be supplementary, not essential to understanding the commit. always assume that the hosting provider could vanish in a puff of smoke ...

I agree with this principle (although it's never a black and white story, and many times even a mile long commit message would only cover a fraction of what happened in the issue tracker -- screenshots, test programs, debugging info, brainstormings, conflicting opinions, various possible solutions evaluated...).

Do you think that these particular commit messages aren't sufficiently detailed?

egmontkob · 2025-10-27T22:24:24Z

The previous push fixes commit "Force a subshell cd command at startup". It caused a 10 second startup delay with mksh. If I avoid sending a Ctrl-C (because we're initializing) then I also have to avoid waiting for the synchronization afterwards.

ossilator · 2025-10-28T09:08:08Z

If I see a commit and don't quite understand the story or the rationale behind, I look at the issue referenced by the commit log, but it wouldn't occur to me to check the PR / review process of that commit.

i'm not sure why it wouldn't occur to you - once you are on the issue page, the link to the PR is right in front of you. i would naturally follow it if i still had questions.

anyway, i was aiming at something else: you can check the context with git log --graph (or f.ex gitk), at least if you are aware of the project's merge policy.

Do you think that these particular commit messages aren't sufficiently detailed?

well, yes - i said so much about the first one.

egmontkob · 2025-10-28T09:41:32Z

i'm not sure why it wouldn't occur to you - once you are on the issue page, the link to the PR is right in front of you. i would naturally follow it if i still had questions.

As I said, "But maybe it's just me.". You do have a point. Next time it'll surely occur to me.

Let's twist it inside out. These bugs were found during review. The PR addresses multiple issues. You say I should jump from the issue page to the PR page when I need more info. So I guess I should mention an issue in the commit log? But which one should I randomly pick? Or if none, because one can always find the PR from the merge commit then what's the point in those "Ticket:" lines at all? Or do I need to open a new ticket?

I came across a scenario to which I wasn't aware of any policy how to handle. I chose a solution myself. If I hear the preference of one of the two official developers then I'm happy to comply.

Between the two of us, it just looks to me yet another case of "you would have done it differently, but it was not you who did it, it was me".

Correct me please if I'm mistaken, but you're not an official developer, you're not "above" me in the review chain; you're on the same level as me, a frequent contributor. Yet you are way more picky than the two main developers, even about things that aren't wrong at all, just different from your preference. (And, as we've seen many-many times, more often than not we have quite different views on things.) This is just very, very bad team dynamics; and no matter how many times I've tried, I can't get you to stop it, can't get to you be more relaxed, can't get you to accept that others are doing things their way, not your way.

well, yes - i said so much about the first one.

All around the code, in the business logic of synchronization, and in all other shells the order is "A, B", in one particular shell it's "B, A". The commit log "only" says it was the wrong order and fixes to the new one. Is this not enough? Do you think I should go into more details? Explain the entire design which is already there in the code which requires the order "A, B"? Explain why "B, A" is functionally incorrect and how can result in incorrect behavior? I really don't think so.

ossilator · 2025-10-28T09:34:08Z

src/subshell/common.c

-                                if (feed_subshell (QUIETLY, TRUE)
-                                    && read_command_line_buffer (FALSE))
-                                    return TRUE;
+                                if (subshell_initialized)


it would be preferable to join the conditionals.

ossilator · 2025-10-28T09:34:54Z

src/subshell/common.c

-        write_all (mc_global.tty.subshell_pty, "\003", 1);
-        subshell_state = RUNNING_COMMAND;
-        if (mc_global.shell->type != SHELL_FISH && !feed_subshell (QUIETLY, TRUE))
+        if (subshell_initialized)


here as well.

ossilator · 2025-10-28T09:35:22Z

src/subshell/common.c

-                write_all (mc_global.tty.subshell_pty, "\003", 1);
-                subshell_state = RUNNING_COMMAND;
-                feed_subshell (QUIETLY, FALSE);
+                if (subshell_initialized)


... and here.

ossilator · 2025-10-28T09:48:02Z

src/subshell/common.c


 static gboolean
-read_command_line_buffer (gboolean test_mode)
+read_command_buffer_with_timeout (const int nfds, int how, void *buffer, const size_t buffer_size,


it's very unclean to pass nfds here, but do the actual fd handling inside.
note that the parameter isn't even documented above, so i guess you might have made the same observation, but then forgot it along the way?

ossilator · 2025-10-28T09:49:38Z

src/subshell/common.c

+
+    const int nfds = MAX (command_buffer_pipe[READ], mc_global.tty.subshell_pty) + 1;
+
+    /* First, flush the command buffer pipe. This pipe shouldn't be written


this could be also de-duplicated, at the cost of adding a flag to the reader function.
not sure it's worth it at this point.

ossilator · 2025-10-28T09:51:09Z

src/subshell/common.c

-    subshell_prompt_timer.tv_sec = 1;
-    subshell_prompt_timer.tv_usec = 0;
+static gboolean
+read_command_line_buffer (gboolean test_mode)


ok, i had a look at the entire file now. it makes sense i think. the diff just became a lot more messy.

ossilator · 2025-10-28T10:11:11Z

These bugs were found during review.

yes, and that's kinda the crux. the commit message seems way to "contextual". it should be able to stand on it own, even if the commit wasn't part of the branch.

Between the two of us, it just looks to me yet another case of "you would have done it differently, but it was not you who did it, it was me".

don't try to make it personal again.
take a step back, and consider why i would have done it differently. it's rarely just a matter of preference.

in all other shells the order is "A, B", in one particular shell it's "B, A".

but your commit message doesn't say so. it just postulates something, without mentioning the inconsistency, or alluding to (specific) adverse consequences.

egmontkob · 2025-10-28T10:47:13Z

yes, and that's kinda the crux. the commit message seems way to "contextual". it should be able to stand on it own, even if the commit wasn't part of the branch.

I don't see how any of them wouldn't stand on their own.

take a step back, and consider why i would have done it differently. it's rarely just a matter of preference.

You see your proposed changes as being objectively better.

I don't see them as such; I see them as similarly good, just different.

This is a reoccurring issue between us that I'm afraid we won't be able to resolve.

Earlier (somewhere, not sure if here; where we discussed how strong the word "should" is) you said that you're only making suggestions. But whenever I explain my thoughts behind my decisions, you counter them claiming that your approach would be better. It cannot go like this. Either you're making suggestions, but then you listen to my counterarguments and (unless there's a really strong reason) it stops there; or you keep on reiterating your view on the story, but then you're not making "should be better"-like suggestions, you really insist on me changing my work to match your ideals.

I am simply not willing to rework each and every single bit of my work according to your preferred (or as you see it: objectively better) approach.

If you need every single bit to look exactly according to your taste, I'd honestly be more than happy to stop working on these issues right here and right now (find a better use of my time) and let you finish them (or restart from scratch) the way you'd prefer. Are you up for this? Or do you let me complete my work, accepting that it's not going to look exacly as you'd prefer?

but your commit message doesn't say so. it just postulates something, without mentioning the inconsistency, or alluding to (specific) adverse consequences.

It says that the old order was wrong and the new one is correct. I honestly don't see the need to further explain it.

ossilator · 2025-10-28T17:38:51Z

Either you're making suggestions, but then you listen to my counterarguments and (unless there's a really strong reason) it stops there; or you keep on reiterating your view on the story, but then you're not making "should be better"-like suggestions, you really insist on me changing my work to match your ideals.

of course i counter arguments i find unconvincing. that's not reiteration.

i think you simply don't like the feeling of finding that i have a point, but really not wanting to do anything about it. you need to learn to deal with it, rather than complaining that i'm being "too thorough".

but your commit message doesn't say so. it just postulates something, without mentioning the inconsistency, or alluding to (specific) adverse consequences.

It says that the old order was wrong and the new one is correct. I honestly don't see the need to further explain it.

when you make an assertion that is not self-evident (without reading the code), you should make an effort to justify it. you understand that concept very well, as evidenced by your other, quite excellent messages.

zyv · 2025-10-28T19:16:41Z

Hey guys, maybe you could please stop bickering now? It's really painful to read all this stuff and not being able to react properly, but I do have to take care of very unfortunate and unpleasant real-life issues right now :( It seems to me that both of you are not being constructive, if the goal is to be defined as getting something done and if not having fun in the process, then at least it not being too unpleasant...

@ossilator irrespective of whether your points are valid or not, you've just recognized yourself that your opponent (which I would actually rather prefer to treat as a teammate) is not comfortable with dealing with a situation when he "has" to work on whatever he sees as "not worth it". So what are you going to achieve by sticking to your principles and just pressing all of your points as having absolutely equal weight, be it genuine logic mistakes, commit structure, or missing commas?

To me, it is quite obvious that the end result will not be the willingness on the part of @egmontkob to accept all of your points as valid and work on them (or at least just ignore whatever he doesn't agree with, as you yourself told him he could do), but rather total alienation, and no work done at all as a result of that.

The next question is, is it a desirable outcome? I don't think so. But trust me, exactly this will happen or is already happening. After all, you've made your points, and in public — isn't that enough? Why not just conclude the discussion of a point after two interactions, especially if the net result for the project on the whole is still positive and nothing like bugs or logic mistakes is being introduced? Maybe you could do us a favor and take this into consideration, whether you think this is right or wrong, because it doesn't even matter.

On the other hand, @egmontkob, why would you necessarily need to get a written statement from @ossilator that he concedes that having considered your arguments, he also thinks it's not worth it? As an intelligent person, you must have made an impression of his personality so far. How likely is it that you'll ever get such a concession? I say it won't happen.

If so, why not just take what is helpful and simply ignore all the rest after two attempts to get your point across? He's even told you that these are his personal opinions and suggestions, and he doesn't imply that you must implement all of them but rather that he sees the need to express them and that he has no formal authority to require you to do anything at all. Basically, you can just do what you want to do, and if me and/or Andrew see a problem with that, you'll certainly hear from us.

And now that I've wasted at least an hour already writing these useless pleas, maybe I'd also get to the part that concerns me specifically. I added two commits to this PRs, one with some insignificant refactoring, and the other one with remaining points I had in mind for #4723. I did it for efficiency reasons, because I thought if this PR is touching the area anyways and parts of #4723 are going to be addressed, what good it is to open another second one with the same circle of participants... Never in my life could I have imagined that it would trigger so much endless argument and dissatisfaction and waste.

If I knew that this is going to happen, I would have never done that. But neither of you came up with the most trivial idea throughout the whole discussion of just running git format-patch HEAD~, attaching them to a comment and asking to take them elsewhere. I absolutely have no problem with that. I would have committed them in a different branch then, or directly to master if this is what I think is the right thing to do, but I would have never insisted on anyone adopting this stuff as their responsibility.

No, hundreds of emails later, no satisfactory consensus is reached, and you end up disabling the editing of the PRs by the maintainer, which makes it extremely annoying to work with PRs in the first place. Now, I can't rebase them before merging with the rebase command I have written, or manually for that matter. All I can do after a PR is accepted is either try to have the submitter rebase it and make sure nothing gets into master in-between, or copy this into my own branch with extra useless and unnecessary work and basically reject the PR. How crazy is that? Can't you just ask to please not to add any commits and I won't do it?

Somehow, this is all really depressing to me. This project seems to only attract a certain type of users I have really no pleasure in dealing with, and the (rare) contributors whom I respect for technical acumen don't seem to be able to work with each other. I don't know whose fault that is and how to change it, and if it's even possible to effectuate any change. What I know is that it's neither fun nor rewarding. And yet I'm still around after several decades, but that definitively doesn't sound healthy. Maybe I have to throw in the towel and learn how to get on with my life without mc.

egmontkob · 2025-10-28T19:18:51Z

(Just when I was about to post this, there's a comment from zyv. Allow me please to post this, not having read that one yet; I have just put quite some time into this.)

think you simply don't like the feeling of finding that i have a point

Sometimes I do see your point, but just don't find it worth it. Sometimes, like with this particular commit message, I don't see your point.

It's not that I don't like the feeling of finding someone else having a point. Quite oppositely, I love when I get to learn, I love seeing new points of view I find great!

This is just not the case with many of your comments in my eyes; and at some other cases I just find your extreme pedantry absolutely counterproductive – taking away time from things that actually matter, and also annoying and demotivating. Many other times I just simply disagree with your preferred approach being superior to mine – and just like you say I don't like if you have a point, the way I see it is that you don't like if I have a point.

Sometimes the discussions between us go like:

I write some code
You comment
I change the code

and that's great.

Often it goes like:

I write some code
You comment
I state why I disagree with you and prefer it my way
You state why you disagree with me
I state why I disagree with you
You state why you disagree with me
I state why I disagree with you
You state why you disagree with me
... and so on and so forth endlessly

And this is not good at all.

The way it really should go instead is:

I write some code
You comment
I state why I disagree with you
end of discussion here

You need to back off because it's me writing the code, you're not in any way above me in the review chain, zyv has on a couple of occasions already stood on my side defending my version, and most importantly: I write the code hence the final decision is mine. If I make a decision and explain it then even if you disagree it's no grounds for continuing the endless conversation where we won't get any closer to each other.

Yet, you are unable to stop any conversation that hasn't reached an agreement between us.

you need to learn to deal with it

Since apparently it has to be me who stops any sub-thread by not responding anymore to your comment, I'm wondering: why not start it sooner? Let me try a new approach: I just won't respond to your comments anymore, it's all an utter waste of time, and more importantly, degrades my motivation every time. I'm hearing what you write; you'll see whether I agree or not based on whether I adjust the code (or commit msg) or not.

If, by any chance, you don't like this, if you want to hear my opinion then you need to learn to accept it and not re-iterate your opposing opinion, because it's me putting the actual work in it. You need to learn to accept that if we disagree then my approach wins – if you don't like it, you can go ahead and fix these bugs yourself, in that case your approach will win. I would be more than happy to hear that the bugs get fixed in an (allegedly) better quality, with no effort required from me: double win for me!

I'm honestly on the verge of just walking away from this project. I just don't want to voluntarily deal with such an attitude that whenever I present my arguments, you counter them by repeating yours. If you want me to contribute to the project and fix some bug, rather than you fixing them yourself, you have to learn not to insist on doing everything your way. Right now I'm in a spree of fixing many-many bugs that don't even affect me personally. If I have to deal with someone (not a project maintainer) not being satisfied until I do every little tiniest insignificant detail exactly to his liking (even despite one of the maintainers having expressed that he's much more relaxed on these issues) then I'll be happy to find a different project to contribute to. If I have to deal with this attitude over and over again, it's just not worth it for me.

The dynamics between the two of us clearly doesn't work, and while I assume you think the problem is me, I do think the problem is your extreme and pointless perfectionism, your lack of willingness to realize that an approach you see as superior is not necessarily objectively superior, your lack of willingness to accept that others see the world differently from you, your lack of ability to respect and accept the author's decision even if you disagree.

By the way, a bit of git magic tells me:

Andrew: 4728 commits, 207216 lines added (surely some of it is reformatting)
Yury: 526 commits, 38873 lines added
me: 79 commits, 7702 lines added
you: 25 commits, 131 lines added

No matter what experience you bring from elsewhere, I'm really not convinced you're in the position to tell us how mc's development should run, what standards should the code, commit logs, tools we use etc. live up to (something I saw you doing elsewhere too with other mc devs where I wasn't involved).

egmontkob · 2025-10-28T19:31:42Z

@zyv Thanks for your comment!

Just one reaction on the big argument bits:

Why not just conclude the discussion of a point after two interactions [...] simply ignore all the rest after two attempts

s/two/one/g

That what I'll try to do.

Re me carrying your commits, disallowing maintainer pushes (which I didn't realize caused inconvenience to you) and whatnot. It's all new workflow to me, I'm in the process of figuring it all out. I'm happy to undo that change of disabling maintainer commits, or do whatever you request.

I'll be pretty much away for the rest of this week, will resume working on my PRs next week. Please don't merge any of them until then, I'd like to do another round of review myself (especially with this one here). Until then, reviews from you & Andrew are welcome if you happen to have time!

egmontkob · 2025-10-28T19:35:17Z

@zyv And yes I realize it was rude for me to disable maintainer pushes, I should have rather asked you about it. I'm sorry for that!

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 and others added 16 commits October 27, 2025 22:26

Ticket MidnightCommander#4780: Read from the tty master fd in nonbloc…

b05c0b9

…king mode This prevents a possible deadlock if the slave flushes the line just before we attempt to read from it. Signed-off-by: Egmont Koblinger <[email protected]>

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

1e32b08

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

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

6abf0b8

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

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

6e133c9

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

b35315e

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…

b1bd2ab

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

Ticket MidnightCommander#4625: Cosmetical changes in shell init strings

bcf0775

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

72dff4d

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

Ticket MidnightCommander#4634: extract init file names defines

6d144fa

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

Ticket MidnightCommander#4634: add missing tests

848c001

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

tests: fix unneeded symbol duplication in `exec_get_export_variables_…

8fbc055

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

egmontkob force-pushed the 4625_slow_zsh_macos branch from a23bdb1 to 8fbc055 Compare October 27, 2025 21:42

ossilator reviewed Oct 28, 2025

View reviewed changes

egmontkob mentioned this pull request Oct 30, 2025

Terminal emulation: missing primary device attribute response (for fish shell) #4837

Open

2 tasks


		const int nfds = MAX (command_buffer_pipe[READ], mc_global.tty.subshell_pty) + 1;

		/* First, flush the command buffer pipe. This pipe shouldn't be written

Uh oh!

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!

egmontkob commented Oct 11, 2025 •

edited by zyv

Loading

olfway commented Oct 12, 2025 •

edited

Loading

egmontkob Oct 12, 2025 •

edited

Loading