Fix unbound variables in shell completion

[cevhyruz]

when set -o nounset is enabled
completion complains about these variables being unset:

orig_var
__fzf_no_space_commands

[junegunn]

Thanks, but are you setting set -o nounset in an interactive session? Wouldn't it cause a lot of problems with existing scripts? Also, if we apply this patch, bash will successfully load completion.bash file, but the completion still won't work.

$ set -o nounset
$ . shell/completion.bash
$ ssh **-bash: FZF_DEFAULT_OPTS: unbound variable
$ ls **-bash: dir: unbound variable

[cevhyruz]

are you setting set -o nounset in an interactive session?

Yeah, I like it when I'm able to see if a variable is not created yet, rather than printing nothing when echoing.

Wouldn't it cause a lot of problems with existing scripts?

what do you mean by existing scripts?

[junegunn]

what do you mean by existing scripts?

Many scripts in the wild are written with the assumption that set -u is not set in an interactive session, so setting it is going to break them. You're lucky if you haven't run into such a script.

This feature is rather controversial, and has many extremely vocal advocates and opponents
(https://mywiki.wooledge.org/BashFAQ/112)

• consider adding support for "set -o nounset" gentoo/gentoo-functions#1
• stack.sh with set -o nounset and set -o errexit bmc/lib-sh#2
• Fix running in shells with -u/-o nounset option set rupa/z#299
• Does not play well with "set -o nounset" magicmonty/bash-git-prompt#389
• Support bash set -u / nounset (unset noun) checks akesterson/cmdarg#24
• cd hook broken under bash -u rvm/rvm#4342

Making sure that a legacy script works with set -u can be a lot of work and it seems that the maintainers of the scripts are not so sure if it's worth it given that most users historically don't have set -u on interactive sessions.

But anyway, as I said above, the patch alone won't make fuzzy completion work (i.e. ls **<tab>)? Have you tested that?

[cevhyruz]

Making sure that a legacy script works with set -u can be a lot of work and it seems that the maintainers of the scripts are not so sure if it's worth it given that most users historically don't have set -u on interactive sessions

I see the point. Well, It wont hurt to support both cases is it?

The only thing to lookout out for sure is maintaining the way it is which can be solve by adding a little test script.

But anyway, as I said above, the patch alone won't make fuzzy completion work (i.e. ls **)? Have you tested that?

Got it working on my end now there seems a couple of little refactoring needed. Will push later after testing.

Just wanna know what are your thoughts about this?

[junegunn]

Just wanna know what are your thoughts about this?

Not just the fuzzy completion, none of the key bindings (CTRL-T, ALT-C, and CTRL-R) works with set -u and you're the first one to report this compatibility issue says a lot about common users' configuration. Also, zsh implementation has the same problem, but no complaints so far.

Well, It wont hurt to support both cases is it?

The code will have things like ${XXX-} all over the place, they'll likely make the code less readable, and harder to maintain as we always need to put extra care whenever we change the code.

But after all, if we can make things (key bindings and fuzzy completion of bash and zsh) work with set -u with not too many intrusive changes, I'm okay with the direction.

[junegunn]

Please allow me to push more commits to your branch.

(https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/allowing-changes-to-a-pull-request-branch-created-from-a-fork)

[cevhyruz]

looks good to me, what do you think?

[junegunn]

Looks good to me too. Thanks for the work.