--with-subshell=optional does not work

[gv]

Starting with mc 4.8.17, the --with-subshell=optional argument passed to configure keep mc subshell enabled.

[zaytsev-work]

Are you sure it started with 4.8.17 ? Grepping for SUBSHELL_OPTIONAL, I can't see it used anywhere, so no wonder it doesn't work...

[gv]

Yes, 100%. I build every version on several distributions of Linux. None works now. All worked up to 4.8.16.

[gv]

Adding "#define SUBSHELL_OPTIONAL 1" in config.h make no difference.
The strange thing, there is no SUBSHELL_OPTIONAL in 4.8.16's config.h. But somehow. it just work.

[zaytsev-work]

Sounds like it's got lost between 4.7.5.6 and 4.8.0 and nobody has noticed it in the last 5 years. I think that we should just throw this option away, or else, re-add it, but only with a test to ensure that it doesn't get lost again, otherwise it's wasted time and effort.

[gv]

It was not lost. It worked on _every_ version on mc I remember. And why throw-it away?

[zaytsev-work]

I can't build test it right now, but from cursory grepping I see no possibility how it could have worked in 4.8.16, because the code in the tarball doesn't contain any relevant references to SUBSHELL_OPTIONAL. I don't believe in magic subshell disabling fairies, you know. If you can bisect it to show which commit exactly between 4.8.16 and 4.8.17 broke it, this will be much appreciated.

And why throw-it away?

If I am right that it was lost 5 years ago, and nobody has complained so far, then it doesn't sound like something that users can't live without. And if there is no test for it, it can vanish anytime without anyone noticing until it's too late.

[gv]

Replying to zaytsev-work:

I can't build test it right now, but from cursory grepping I see no possibility how it could have worked in 4.8.16, because the code in the tarball doesn't contain any relevant references to SUBSHELL_OPTIONAL. I don't believe in magic subshell disabling fairies, you know.

OK, try-it. Your gonna be amazed. Fairies or not.
For me, every version worked fine on AIX and Linux (cento 6 and 7, fedora, openwrt).

If you can bisect it to show which commit exactly between 4.8.16 and 4.8.17 broke it, this will be much appreciated.

Ok. I will try on the next weekend.

If I am right that it was lost 5 years ago, and nobody has complained so far, then it doesn't sound like something that users can't live without.

No one complained because it just worked.

And if there is no test for it, it can vanish anytime without anyone noticing until it's too late.

Agreed.

[gv]

The problem is somewhere in lib/shell.c
Using shell.c from 4.8.16 in 4.8.17 make the the shell optional again. The person who split mc_shell_recognize_and_fill_type() function did not do a very good job.

[zaytsev-work]

Yay, I can now see why I couldn't find any references to SUBSHELL_OPTIONAL, it's because I had lib on my "Enable ignore directories" list! :-(

If what you're saying is correct, then the bad commit must be this attempt to fix a segfault: changeset:83b02196, and specifically this is most probably wrong:

mc_global.tty.use_subshell = mc_shell->type != SHELL_NONE;

and should be something like

mc_global.tty.use_subshell &= mc_shell->type != SHELL_NONE;

[gv]

Yep. Now it's working again. Thx.
Fairies... :-)

[zaytsev-work]

Okay, now the question is how do we make a test for it? I was not the one who broke it, but just fixing it without a test is road to nowhere. This whole subshell crap is so fragile that next time there is any change to add a new feature or to fix a bug, this will get broken again.

Fairies... :-)

Yeah, it seems that the ignore directories thing can easily lead to much embarrassment :-( Imagine my reaction when I search the source code for the use of the flag, and it's simply not used in both allegedly working and non-working versions... Anyways, it turned to be a red herring either way, because the flag got reset much later in a botched attempt to fix a segfault. I hate software.

[gv]

Replying to zaytsev-work:

Okay, now the question is how do we make a test for it?

I cannot help here. I'm not aware with the (automatic) test system for mc (if it is one).
All I can say that I will continue to use --with-subshell=optional and if it's not going to work again I will fill another bug report.

I was not the one who broke it

Does not matter who did-it. We all make mistakes from time to time. But we find them and we correct them.

Yeah, it seems that the ignore directories thing can easily lead to much embarrassment :-(

Relax. :-) It can happen to anyone.

I hate software.

Nah. You're just upset. :-)

[andrew_b]

So, will we fix this (and another) bug(s) without tests?

[zaytsev]

So, will we fix this (and another) bug(s) without tests?

I don't know, what do you think? I find that it's very disappointing that almost every time we try to fix something, a new annoying problem is introduced. Of course, this doesn't mean that we shouldn't keep trying fixing problems. However, I think it's definitively an indicator that we should really start seriously worrying about writing more tests.

We have some simple check unit tests, of which one could make much more use. On top of that, I'm thinking of doing some basic smoke / integration tests with the built-in autotools thing, if Andreas will not beat me to it, e.g. run all of our Perl stuff through perl -w at least to confirm that the syntax is correct.

Of course, it's hard to test C stuff, we have tons of code that isn't covered in any way, and it's not realistic to cover all of it, but I really think we should start doing something about it, and one way to go about it is to try to add a regression test for bugs if it's practical.

Now, the biggest problem that I have with reviewing branches so far, is that it requires an insane amount of effort to do it properly, because there aren't any tests at all that come with it. It's unclear if the stated behavior is really implemented and the only ways to know is to try to do some manual testing, and mentally trace the possible branches by playing compiler in my head. However, I'm not a machine, and I make mistakes and miss errors, which subsequently leads to frustration.

If we would require tests for any non-trivial changes and it was practical to develop them, we would be able to process the contributions waaay faster than we are currently able to.

[zaytsev]

An additional consideration that I have is that now that the release process has been more or less ironed out (for me), I would suggest to start doing the following:

• we merge everything that we think should go into release, including cleanups
• we announce the intention to make a release on the list and cut a tarball from master
• we let whomever is willing to take the time to test the tarball for two weeks
• in the mean time, we work in branches, and only commit translations, etc. to master
• if regressions are found we try to fix them on master
• we aim to make a full official release within two weeks

Of course, this will not solve all problems due to lack of automated tests, but at least it will hopefully help to avoid stupid FTBFS, etc.

[zaytsev]

So, will we fix this (and another) bug(s) without tests?

Regarding this bug, could you please keep it open until I'm back? It doesn't look critical at all, and now that we found what was the problem, it's trivial to fix.

However, I would like to try to write a regression test for it. I have the following idea: the global structure can be initialized with use_subshell set to FALSE, then I can let all initialization magic run, and check that use_subshell is still FALSE. If you can && / || want to help me with that, I will be very happy. If not, then I would like at least to give it a try... If I will fail, we can merge it as is at any time later.

[andrew_b]

Branch: 3642_subshell_optional
changeset:22c3a35e8cc5d1adc50dd1df580e550cd3bfd8a5

[andrew_b]

Merged to master: [1f5e93efc9ebc824b8845b67e8b8b24e60d4e7ad].