Summary

The asdf function makes use of the local keyword which is not supported by all shells. A simple rewrite of the function allows to eliminate the use of a (local) variable to store the first argument and, as a consequence, removes the need for the local keyword.

Looking at the code, local is used in a lot of other places, but this is the only place where my shell (mksh) generates an error.

Other Information

As per the KSH_VERSION environment variable, I use mksh version @(#)MIRBSD KSH R59 2021/10/15 +Debian. I don't use Docker now, but I'm assuming (untested) the following would create an environment to reproduce the behavior.

docker run --rm -it debian:sid
apt-get update
apt-get -y install git mksh
# install asdf

Is there a bug report for this issue behind this PR? It's not clear to me why this is needed. asdf local ... shouldn't cause local to be recognized by the shell. asdf if doesn't cause any problems for me in zsh and bash (asdf exists with the unknown command error as expected):

bash-3.2$ asdf if
Unknown command: `asdf if`
No plugin named if

Is there a bug report for this issue behind this PR?

I'm reporting the bug and providing a patch at the same time.

It's not clear to me why this is needed. asdf local ... shouldn't cause local to be recognized by the shell. asdf if doesn't cause any problems for me in zsh and bash (asdf exists with the unknown command error as expected):

bash-3.2$ asdf if
Unknown command: `asdf if`
No plugin named if

Yes, bash supports local, obviously; as do zsh; by extension since it tries to be backward-compatible with the former. Maybe educate yourself about the local keyword or, for starters, read this SO post: https://unix.stackexchange.com/questions/493729/list-of-shells-that-support-local-keyword-for-defining-local-variables

Looking at the code, local is used in a lot of other places, but this is the only place where my shell (mksh) generates an error.

Full reproduction example:

docker run --rm -it debian:latest
apt-get update && apt-get -y install git mksh
git clone https://github.com/asdf-vm/asdf.git ~/.asdf --branch v0.8.1
mksh
# mksh defines `local` as an alias, perfect for testing...
# and I undefine all aliases by default
unalias -a

# demonstration
. ~/.asdf/asdf.sh
asdf local
asdf if

# typical use case
asdf plugin add nodejs
asdf install nodejs lts
asdf global nodejs lts

I should note that the "proper" way to support local semantics without a hard-dependency on the local keyword can be found here: https://github.com/canonical/cloud-init/blob/main/tools/Z99-cloud-locale-test.sh#L14

So it seems like out of the box everything does work when invoked from mksh but after removing all aliases it fails. I presume mksh's aliases are on by default for all users, so I wonder how many people this affects.

This could have been argued right at the start, but would only have addressed mksh. I invite you to re-read my original message and the SO link I provided. This PR covers more than mksh and I think that you're entirely missing the underlying point. If you want to focus on mksh, well, yes, in the default configuration it's a non-issue.

local is a keyword that was introduced to control the scope of a variable so that different parts of a script don't clobber each other. The simplest example would be a function that defines a variable v and calls a function that also defines a variable v, the first variable is clobbered.

Most modern shells, in one form or another supports local semantics (keyword/builtin or alias). Again, not a popular issue. My use of mksh in the example is simply because (1) I use it (the factual evidence part) and that (2) bash and its clones (zsh) defines local as a keyword/builtin so I couldn't have demonstrated the issue. Once the issue has been demonstrated, you could disregard that I used mksh and apply the pattern to any other shell that doesn't have local as a builtin or in a non-default configuration. I could also have used another shell.

So there are 3 things here:

1. a fix for asdf()'s use of local;
2. the general mention that local is used throughout the code;
3. a possible solution in the form of a reference.

1 addresses 2 in a very specific way that entirely fixes my usage pattern, but I think it would be nice if 2 was addressed globally. That's up to asdf to decide if 2 is important considering ^^. If it is, 3 is a possible solution. You're welcome.

Now this change is absolutely trivial and localized. It entirely fixes my use case and I would like it to be merged. But if you want to reject the PR entirely based on the fact that it doesn't apply in a default configuration, be my guest. In any case, I think I detailed the issue enough that it should be pretty clear even for someone uneducated on the issue and enough to make a proper decision going forward.

Thank you for your time

@hyperupcall what are your thoughts on this?

@Stratus3D I'm all for these changes in general - I know I originally stated that I want to let local slip even if it's not POSIX, but clearly we have some consumers using mksh. In the same way we want to support Bash entrypoints with various set and shopts configured, I think we should support this mksh setup, which even though it isn't the default, I think it's a reasonable deviation from it. To be clear, I don't think OPs (2) and (3) things are particularly relevant, and I want to use local everywhere else in the code since Bash is a hard dependency - it's just this one place we need to fix. I was going to close this issue when tackling #1397 (which I think is waiting for your approval), but I guess we can fix it independently. My only issue with this particular changeset is that it appears to remove the asdf ignored command comment thingy - seems like it's supposed to be there?

@Stratus3D I think a PR like this should also include some regression testing. In this case, a version of the test/banned_commands.bats which only applies to specific files.

As @hyperupcall mentions, it may be covered by fixing #1397, which requires us to decide whether we want these entrypoint files to be considered as Bash or POSIX when linting. So depending on the outcome of #1397, we may not need the above-mentioned file-specific banned_commands.