Don't test the persistent command feature

[egmontkob]

Is there an existing request for this?

• I have searched the existing issues

Is your feature request related to a problem?

Upon startup, mc tests whether the shell supports reporting the pending command line, i.e. our prerequisite for our persistent command line feature.

This resulted in race conditions, some of which were eliminated e.g. as part of #4625 / PR #4934, but due to a Solaris kernel bug #4812 still remains and we don't know how to fix that.

Strictly speaking, these race conditions were/are still there during normal operation of mc, but are many magnitudes less likely to surface because then, unlike at startup, an interactive user action is involved.

At #4625 (comment) it was suggested that we don't need to test for this feature, we can (in some circumstances) be sure that it works.

In turn, at #4625 (comment) it was pointed out that it wouldn't work. But now that I think about it, this was an oversight, this comment was incorrect. The outer terminal, and anything between the outer terminal and mc (e.g. possibly an ssh) isn't involved in this game. It all happens in the inner terminal, i.e. between mc as the tty master and the subshell as the pty slave.

If we know that the shell is new enough to support this feature, there's no need to test it. Now, very slightly maybe, under some extremely heavy and unusual user setups, maybe there's a chance that it won't work, but under such extremely weird setups many of our other solutions (e.g. the sigstop-sigcont dance, or writing to the subshell pipe) could also fail. And, worst case, we'd just hit the timeout then and disable the feature for the rest of the session.

We'd need to know whether the subshell is new enough to support the said feature. There are several ways to do that. We could run the shell with --version first and parse its output. We could construct our shell initialization code in init_subshell_precmd() to report its version via a pipe. Or, if all versions of the shell support the necessary bindkey, but may not yet support querying the command line buffer, then mc could recognize the empty command line and empty (rather than 0) cursor position in the response, with no upfront version reporting whatsoever (actually, in case of bash, we also explicitly report the version).

This approach could allow some code cleanup (e.g. hopefully the removal of the newly introduced gboolean subshell_initialized), some design uniformization (we don't test other features either, we have a timeout just in case they break whenever we use on them), and would work around the aforementioned Solaris bug of #4812.

Describe the solution you'd like

.

Describe alternatives you've considered

No response

Additional context

No response

[ossilator]

it was pointed out that it wouldn't work.

nah, that's not what that comment did. that point was merely an obvious indicator that something is wrong with the argumentation.

anyway, the argument for removing the upfront "live test" seems convincing in principle. but it gives me the feeling that it might make problems harder to debug. regarding races and removing the variable again, what about the scenario of the user entering a command while mc is starting up (on a slow/overloaded machine, obviously)?

[egmontkob]

nah, that's not what that comment did. that point was merely an obvious indicator that something is wrong with the argumentation.

Indeed, the original argument was poor (an irrelevant, although technically correct statement), to which your response seemed to me to aim to disprove the original claim, which I now understand you didn't mean to, you only meant to disprove the argumentation.

but it gives me the feeling that it might make problems harder to debug. regarding races and removing the variable again

If the 1s timer is hit (on an overloaded machine (reminder: it's all about the inner tty, so a possible ssh with slow network would be irrelevant)) then currently the command line is not preserved at that occasion only. I suggested that after refactoring the feature should get turned off for the rest of the session; whereas alternatively we could just not do it, meaning that in the unlikely case that we couldn't configure the feature properly, there'd be a 1s delay every time when bringing back the panels with Ctrl+O.

I understand your worries about "might make problems harder to debug", but I'm not yet convinced that easy debuggability would be more important than simpler, cleaner operation when things go as expected.

what about the scenario of the user entering a command while mc is starting up

I don't see how this is relevant. This is about how mc handles its input from the host terminal, which should be unrelated to what's happening in the inner terminal (assuming that mc doesn't start to forward the outer input to the inner terminal line before completing its initialization stuff, which I sure do hope is the case).

[ossilator]

which I sure do hope is the case

exactly that was the point.

[zyv]

I think that the argument for removing the test is sound in principle and will solve a whole class of problems.

I didn't check what the requirements are for different shells (bash, zsh, and fish), but my feeling is that stuff we need has been supported "forever", so we won't even need a meaningful version test.

I'm not quite sure what the best way to handle the failure modes is though:

I kind of feel it's annoying and unexpected that mc would turn off the feature silently after hitting a 1s timeout once in the middle of the session, because the machine was overloaded (a bit like the "hard to debug" argument).

The alternative, which I think would be fair (showing a red error box that the feature is turned off for the rest of the session), might get very annoying if it is indeed consistently broken and not due to temporary overload.

The next logical step would be to introduce a separate persistent buffer option in addition to -u. Which, in my opinion, is absolutely not worth it, because it means more code, more options, more maintenance effort, and potentially near zero usage (like many other options that we have accumulated over 30 years).

Maybe the first step would be to change arbitrary 1s to arbitrary 2-3s to make the problem even more noticeable (and given the specific value of 3s easily trackable to the source), but also hope that due to the large timeout, it never happens?

[egmontkob]

bash added the necessary READLINE_LINE and READLINE_POINT variables in version 4.0; whereas we have just bumped our required bash version from 1 to 2, and macOS still ships 3. So we do need a version test here.