4625 slow zsh macos #4784
4625 slow zsh macos #4784
Conversation
github-actions
bot
added
needs triage
egmontkob
force-pushed
the
4625_slow_zsh_macos
branch
from
152a82e to
11268dd
Compare
zyv
added
area: core
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 :) |
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 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 <yury@shurup.com>
|
Thanks for your followup! Two of your commits I agree with. The one replacing 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
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. |
Unfortunately it doesn't, I tried with commit 2c8ed8d on 4625_slow_zsh_macos branch I've added howto reproduce it here #4071 (comment) |
There was a problem hiding this comment.
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".
src/subshell/common.c
Outdated
| " 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\"';" |
There was a problem hiding this comment.
i'd indent that line to make it clearer that it's a continuation.
There was a problem hiding this comment.
I did that, but then "make indent" reverted that. I'd love to unfold into a single line if folks here don't mind.
There was a problem hiding this comment.
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.
src/subshell/common.c
Outdated
| " 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" |
src/subshell/common.c
Outdated
| // 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", |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.)
There was a problem hiding this comment.
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.
| /** | ||
| * Read in nonblocking mode. | ||
| * | ||
| * On a tty master, waiting for data using a select() and then reading it with a blocking read() |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
i'm not talking about code!
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 :/
| { 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 }, // |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
src/subshell/common.c
Outdated
| // On IBM i, read(1) can return 0 for a non-closed fd | ||
| continue; | ||
| #else | ||
| if (bytes == -1 && (errno == EAGAIN || errno == EWOULDBLOCK)) |
There was a problem hiding this comment.
based on the comment above, this should be outside the #ifdef.
in fact, line 923 can/should be refactored into nested conditionals.
| { | ||
| 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)); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 ...
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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 :)
src/subshell/common.c
Outdated
| " 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", |
There was a problem hiding this comment.
you could add the missing space before { while you're here.
There was a problem hiding this comment.
The space is added later on in 11268dd .
src/subshell/common.c
Outdated
|
|
||
| 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. |
There was a problem hiding this comment.
you should explain why it's ok to discard the data.
There was a problem hiding this comment.
that one really needs addressing.
src/subshell/common.c
Outdated
| "\":\"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" |
There was a problem hiding this comment.
the eval removal really belongs into a separate (prior) commit.
… a function to avoid duplication Signed-off-by: Yury V. Zaytsev <yury@shurup.com>
egmontkob
force-pushed
the
4625_slow_zsh_macos
branch
from
8fc2f3d to
d80b46c
Compare
|
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. |
I don't see how any of them wouldn't stand on their own.
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?
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. |
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".
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. |
|
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 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. |
|
(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.)
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:
and that's great. Often it goes like:
And this is not good at all. The way it really should go instead is:
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.
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:
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). |
|
@zyv Thanks for your comment! Just one reaction on the big argument bits:
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! |
|
@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! |
egmontkob
force-pushed
the
4625_slow_zsh_macos
branch
from
8fbc055 to
58f434e
Compare
… subshell Synchronize first, then print the prompt; this is the proper order. Signed-off-by: Egmont Koblinger <egmont@gmail.com>
…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 <egmont@gmail.com>
egmontkob
force-pushed
the
4625_slow_zsh_macos
branch
from
58f434e to
b8fda45
Compare
egmontkob
force-pushed
the
4625_slow_zsh_macos
branch
from
b8fda45 to
55ad0c1
Compare
|
egmont:
i challenge you to cite a case where this has actually happend.
my compulsion is actually my main driver, and therefore anything but pointless. i'm a package deal. take it or leave it.
i'm actually perfectly willing to do that, but you have to convince me.
sorry, that's just silly.
i'm actually perfectly able and willing to do that. what i'm not willing to accept are bad arguments, esp. when they obviously result from motivated reasoning.
you can also ask questions. yeah, i know - asking good questions takes effort, which you'd rather skip when you don't think that anyone should even care. yury:
i prefer to let others determine their own priorities, esp. when i have no formal authority. when i mention things i consider minor myself, i usually do that from the standpoint that i'd find the effort to fix it trivial. i always use obviously non-committal language for minor things i expect to take significant effort to fix. it's obvious by now that egmont will rather spend an inordinate amount of time on arguing against my position even after declaring that he doesn't care either way, rather than just accepting it (and making a change or just moving on). this is ... unfortunate. |
This is a logical answer, but it still doesn't address my question at all of "what are you going to achieve". I actually find it strange that you choose to skip the part that I find to be most important. Obviously, you are great at constructing logical arguments, and skipping over minor details shouldn't be the issue either. So why is that? I have made my own prognosis above (alienation and no work being done at all), which seems to be confirmed by the subsequent reaction of @egmontkob. My next question, assuming that my prognosis is correct, was whether you see it as a desirable outcome or not, which also went unanswered. And lastly, if this outcome is not desirable, the question was whether we can find a way to stop early. In as far as I'm concerned, I'm not enjoying it, but at least I think I have learned to drop the conversations somewhat better than it used to be the case in the past, instead of keeping pondering on it and coming back with more arguments when I claimed I don't care to argue anymore and my opinion cannot be altered at this point. Also, I see a lot of value in some comments, and as you express it, it's a package deal. But obviously, everybody is different, and maybe Egmont can't or wouldn't want to work with it at all, and if it’s an all-or-nothing package deal, then so be it. In that case, as a maintainer, I would see much more value in hoping to retain him as a contributor. |
i refuse to accept responsibility for other people's feelings over legitimate technical (incl. meta) discourse. i won't let myself be emotionally blackmailed. hence your conclusions/predictions are simply not relevant to me. |
…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 <egmont@gmail.com>
…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 <egmont@gmail.com>
… 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 <egmont@gmail.com>
… 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 <egmont@gmail.com>
…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 <egmont@gmail.com>
egmontkob
force-pushed
the
4625_slow_zsh_macos
branch
from
55ad0c1 to
639d40c
Compare
| /* Force an initial `cd` command, even if the subshell is already in the target directory. | ||
| * Testing the persistent command feature might have read and discarded the prompt. Just get | ||
| * a new one printed. See #4784#issuecomment-3435834623. */ | ||
| vfs_path_t *vfs_subshell_cwd = vfs_path_from_str (subshell_cwd); |
There was a problem hiding this comment.
Please move the variable declaration to the begin of block. Or create a block here.
| pcwd = vfs_path_to_str_flags (subshell_get_cwd (), 0, VPF_RECODE); | ||
|
|
||
| if (!(subshell_state == INACTIVE && strcmp (subshell_cwd, pcwd) != 0)) | ||
| if (!(subshell_state == INACTIVE && strcmp (subshell_cwd, pcwd) != 0) && !force) |
There was a problem hiding this comment.
What about reordering tests: move test of force before call of strcmp() to avoid this call if force == TRUE?
|
Ossi, Over the years, many times we have agreed, and many times (disproportionally way too many times) we have disagreed. But I cannot recall any conversation between the two of us where we managed to get closer to each other in our views.
Yes, those all do count. Because it's not that I don't understand you. I do understand you and I do disagree with you, and no amount of followup discussion could ever change that between the two of us.
No, I absolutely do not have to convince you. We have severely different views on most things, and maybe we're both more stubborn than average. You have to understand that just because I cannot convince you, nor could you convince me, probably a lifetime wouldn't be enough for either of these, it does not mean that you are objectively right and I am objectively wrong.
It's not about legitimate technical discourses. It's about the human side of the story. It's about your behavior. It's about showing respect. It's about acknowledging that your way is not the only way, nor is necessarily a better way. It's about accepting the opinion and decision of the one who puts the hours and days of work into these bugfixes, and not believing that you are in any way superior. It's about being able to back off and let the one who actually does it do it. It's about being able to not respond. It's about not insisting on having the final word. No matter if it's about a minor wording issue, or about you suggesting that I should refactor a much larger area of the code, putting much bigger amount of work, and by the way carefully document it; when I state that I choose not to do this, you simply cannot resist answering that you really think I should. This is an extremely rude and disrespectful behavior. I am the one putting in the work. If we disagree, you have to let it go. You are free to make suggestions, free to tell how you'd do it, free to explain why you think so; but when I tell you why I disagree and why I'm not planning to do that, that has to be the end of it. With rare exceptions if something is truly unclear to you then with one single additional back-and-forth, and you stopping there. You are also free to come up with bugfixes yourself, rather than being so extremely vocal about how we should do it. Recently, in this discussion, you are not only conflicting with me but you're also conflicting with Yury. What truly worries me is that after discussing it so much, you perfectly knowing that your behavior annoys the hell out of someone who contributes much more to the project than you do, you still do not show the slightest willingness to try to adjust your behavior. In fact, you made it explicit that you won't. You refuse to accept responsibility for other people's feelings, triggered by your repeatedly disrespectful behavior. And you're not willing to do anything about your behavior because that would torture yourself. Doesn't sound like team player traits to me. At this point I'll reduce the interactions with you to the absolute bare necessary minimum, let's see if it helps. While at it, would you, very please, also just ever so slightly consider doing something about this ongoing problem? Because if we can't get along, I think Yury made it pretty clear what the solution will be. Not the solution I'm looking for, but if nothing else works then a solution I wouldn't mind. |
|
@mc-worker Thanks for the comments! I'll try to address 4253 / 4789 first, on top of that this PR might become a bit simpler. I'm putting this one on hold until then. |
Thank you for the follow-up! Now I think we are getting somewhere (in as far as the topic that interests me is concerned). I think that irrespective of all other aspects, this position might suit one well under a specific set of circumstances, as in choosing to provide comments and reviews to a project, but not aiming or caring to influence its development any more than that. Unfortunately, even if I were of the same opinion that any other aspects (economic, emotional, etc.) than abstract technical merit in certain mathematical space without any external constraints should be disregarded (which I'm not), it would not suit me, as I have taken upon myself to ensure mc is being maintained (whatever this means). However, even if I were ready to accept that to pursue my beliefs I'd possibly have to work on it alone, I'm not able to make enough time to effectuate anything significant or even "make ends meet" (according to my mind). And again, I don't even want to. In fact, I want quite the opposite. I want to gain more qualified and pleasant contributors to work on the project together, to ensure the longevity of the project, get more things done, and also gain satisfaction from human interactions. It's not like I agree on everything with Andrew, but so far he was always kind enough to eventually accept some sort of compromise, and for my part I tried to accommodate as well... For all these reasons, I'm consciously choosing for "emotional blackmail” to play a role, and I don't see any other way to achieve what I want, but also I don't think it's right to do it differently. So I think it's good that we can fixate on a fundamental disagreement here and move on.
Actually, I find this a bit surprising. I would have assumed that in view of the previous statement, that wouldn't matter to you at all, and the only problem would be the conflict with your chosen principles. Yet, it seems that "mental torture" appears undesirable to you. Anyways, do you have any suggestions from your side as to how we can move on (other than that you can't effectuate any change and others have to deal with that)? As I'm typing this, there is another answer by @egmontkob, and it goes on and on. I don't want to take any "administrative" action because I think that all involved are adult enough, but I simply don't want to see it here anymore. Can you guys, for example, agree in private or in public that in the future, the discussion will cease after one of you uses some code word or something like that? Or is the only solution that one of you stops posting altogether? In this case, I'd please ask you, @ossilator, to leave, even though, as I mentioned before, it would take away many useful and valuable comments and knowledge. By the way, in as far as I'm concerned, we don't have to reach any kind of agreement in public. I'm just assuming that it is somehow important for both of you to have some statements out on the record. |
|
thanks for the excellent writeup, yury!
yes, indeed. 😂 i'll try to answer indirectly by laying out how i see things: i believe in an objective reality. to the degree that particular facts have been established beyond reasonable doubt, we can claim to know it. then there are constraints (usually, available time) and preferences (often as part of a wider culture). their existence is part of reality, not a negation of it. they merely create trade-offs. i feel the urge to determine what is what, and have everyone agree on it. therefore, i just can't let bad arguments stand. to me it seems that egmont has a problem dealing with the cognitive dissonance caused by recognizing the trade-offs. his reaction is motivated reasoning, basically an appeal to consequence fallacy: "this can't be true, as otherwise i'd have to do something about it." he can accuse me of being disrespectful by draining his mental energy. i think that if he sticks to his resolution and manages to resist the urge to become defensive, then the conflict is kind of solved. ideally, this would give him room to sometimes be persuaded to actually adopt my preference. |
5 participants
Proposed changes
Fix slow startup on macOS with zsh, and other related problems also affecting other platforms and shells.
Checklist
👉 Our coding style can be found here: https://midnight-commander.org/coding-style/ 👈
git commit --amend -smake indent && make check)