Ticket #4883: Invoke external rm command from mc wrappers

[egmontkob]

Avoid executing a shell alias or function.

Proposed changes

• Resolves: escape rm command in wrappers to avoid using aliases #4883

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)

[ossilator]

looks good, but i wonder how far we should go with this. it is for example entirely plausible that someone messes with cd (though likely not in a detrimental way to us). for test otoh it seems extremely unlikely.

[egmontkob]

... and what if someone aliases if or then... or command... I don't know what would happen in that case :)

I see a high chance of someone aliasing rm, a lower chance that someone aliases cd or cat (so we might want to perhaps address these as well), and an almost zero chance of someone aliasing if, then, elif, else, fi, then, command, unset, eval, or alike... We'll never be 100% foolproof, but that's not the goal either.

It's surely not obvious where to draw the line (e.g. whether to protect cat and cd), it's a somewhat arbitrary decision.

[GfEW]

It's surely not obvious where to draw the line (e.g. whether to protect cat and cd), it's a somewhat arbitrary decision.

From a user's perspective, If the protection is as cheap as command cd or even builtin cd, I'd rather advocate for applying it broadly, so mc "just works" robustly and consistently across shell environments, be they vanilla, somewhat exotic or wildly experimental.

That's particularly true given the non-obvious nature of mc's various internal reliances on external commands - the user shouldn't need to worry about hidden interferences when using mc.

[egmontkob]

If the protection is as cheap as command cd or even builtin cd, I'd rather advocate for applying it broadly

That would mean something like

if command test -n "$MC_TMPDIR"; then
        MC_PWD_FILE="`command mktemp "${MC_TMPDIR}/mc.pwd.XXXXXX"`"
elif command test -n "$TMPDIR"; then
        MC_PWD_FILE="`command mktemp "${TMPDIR}/mc.pwd.XXXXXX"`"
else
        MC_PWD_FILE="`command mktemp "/tmp/mc.pwd.XXXXXX"`"
fi

MC_STATUS=0
@bindir@/mc -P "$MC_PWD_FILE" "$@" || MC_STATUS=$?

if command test -r "$MC_PWD_FILE"; then
        MC_PWD="`command cat "$MC_PWD_FILE"`"
        if command test -n "$MC_PWD" && command test "$MC_PWD" != "$PWD" && command test -d "$MC_PWD"; then
                command cd "$MC_PWD" || command true
        fi
        command unset MC_PWD
fi

command rm -f "$MC_PWD_FILE"
command unset MC_PWD_FILE
command eval "command unset MC_STATUS; command return $MC_STATUS"

That's 18 commands in 22 lines (19 non-empty lines). I'm not happy.

Edit: And mc.sh would also need to have command [, command return and command alias. No-no.

[egmontkob]

The more I think about it, the more I tend to think that the bug should be closed as rejected, the PR dropped.

I'll make a comment about it in the bug.

[GfEW]

The more I think about it, the more I tend to think that the bug should be closed as rejected, the PR dropped.

I'll make a comment about it in the bug.

Style-wise I can only agree, whilst functionality-wise (including reliability, and clarity as to what to expect from certain actions), I can't. It's a dilemma, possibly resolvable by nothing but personal preference.

Could you estimate the overall percentage of mc's codebase that would be affected by that clutter?

[egmontkob]

Could you estimate the overall percentage of mc's codebase that would be affected by that clutter?

It's only these six files. The cost rather comes from having to test these in all sorts of shells; the cost of anyone looking at those files wondering what goes on, the "risk" of a subsequent modification forgetting to add its command etc.

At the same time we should also estimate the number of affected users, which I believe is 1 user over the course of 30 years.

I'd much more work towards converting it to a function (except for csh), as per the comment I made in the bug.

[ossilator]

given that statistically only every 100th user bothers to report problems, it's probably a few more.
but the others probably concluded that it's indeed their own fault and adjusted somehow.

technically, the behavior of the commands is defined by posix. but mc calls them in a context that does not guarantee posix behavior.
overrides to rm are somewhat common, because it's destructive.
overrides to cd are plausible, as one might want some extra cleverness. but it would be kinda stupid to not create a separate ccd command for that.
overrides to the other commands make no sense at all.
so drawing the line at rm is justifiable.

formerly, the sh wrapper was in fact a function. 83cb1dd changed that specifically to avoid use of functions, with no reason given. it might be to support pre-posix shells, or for uniformity between sh and csh. @proski might remember.