Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: unbound variables in bash completion #1321

Merged

Conversation

edentsai
Copy link
Contributor

@edentsai edentsai commented Jan 28, 2021

when set -o nounset in Bash,
the warnings of unbound variables will break the bash completion.

use kubectl as example:

$ set -o nounset
$ kubectl <Tab>-bash: BASH_COMP_DEBUG_FILE: unbound variable
$

the warning break bash completion without any completion result,
and cause my cursor move to the newline.

Use ${variable:-} substitution in Bash ,
that assign an empty string as default for unbound variables to fix the warnings.

@CLAassistant
Copy link

CLAassistant commented Jan 28, 2021

CLA assistant check
All committers have signed the CLA.

@edentsai edentsai force-pushed the fix-unbound-variable-in-bash-completion branch from bc9e5d4 to 2bf2b31 Compare Jan 28, 2021
@marckhouzam
Copy link
Collaborator

marckhouzam commented Jan 29, 2021

This seems reasonable.
Why do you need to change ${has_completion_function:-} and ${last_command:-}? Those two variables are declared at the start of the script in __start_kubectl(). Do they still cause unbound variable warnings?

@edentsai
Copy link
Contributor Author

edentsai commented Jan 29, 2021

Do they still cause unbound variable warnings?

Yes, those variables cause unbound variable warnings.

@edentsai
Copy link
Contributor Author

edentsai commented Jan 29, 2021

I just checked the __start_kubectl():

__start_kubectl()
{
    # ...
    local has_completion_function
    local last_command
    # ...
}

__start_kubectl() declared those variables with local scope that cannot share with other functions.

@eparis
Copy link
Collaborator

eparis commented Jan 29, 2021

if you're saying tha last_command can never be bound then this isn't the right fix, is it?

@edentsai
Copy link
Contributor Author

edentsai commented Jan 29, 2021

__start_kubectl() declared those variables with local scope that cannot share with other functions.

Sorry, I verified my guess is not correct.

i just made a small script to test the case,
and those local variables can be shared when other functions are called in the __start_kubectl().

@edentsai
Copy link
Contributor Author

edentsai commented Jan 29, 2021

I didn't dig deep whole flow about the kubectl completion,
just simply fix the unbound variable warnings:

$ set -o nounset
$ kubectl <Tab>-bash: BASH_COMP_DEBUG_FILE: unbound variable
$
# After fixed "$BASH_COMP_DEBUG_FILE" warning:
$ set -o nounset 
$ kubectl <Tab>-bash: last_command: unbound variable
# After fixed "$last_command" warning:
$ set -o nounset 
$ kubectl <Tab>-bash: has_completion_function: unbound variable

maybe we can take a deep look how last_command and has_completion_function
will cause the unbound variable warnings.

@edentsai
Copy link
Contributor Author

edentsai commented Jan 29, 2021

I found the root cause:

  • Declare a variable WITHOUT value will cause unbound variable warning

here is a simple script demo.sh for reproduce the issue:

#!/usr/bin/env bash

__start_demo()
{
  printf "[INFO] START\n"

  # declare a variable WITHOUT value will cause unbound variable warning.
  local last_command
  printf "[INFO] last_command: ${last_command}\n"

  printf "[INFO] END\n"
}

complete -o default -F __start_demo demo

then use demo.sh to reproduce the issue:

$ set -o nounset
$ source demo.sh
$ demo <Tab>[INFO] START
-bash: last_command: unbound variable

just declare the variable with an empty string to fix the issue:

-   local last_command
+   local last_command=""
$ set -o nounset
$ source demo.sh
$ demo <Tab>[INFO] START
[INFO] last_command:
[INFO] END

# finally, the warning is fixed.

@edentsai edentsai force-pushed the fix-unbound-variable-in-bash-completion branch from 2bf2b31 to 034c8aa Compare Jan 29, 2021
@edentsai
Copy link
Contributor Author

edentsai commented Jan 29, 2021

I updated the changes to declare last_command and has_completion_function with an empty string.

@edentsai edentsai closed this Jan 29, 2021
@edentsai edentsai reopened this Jan 29, 2021
Copy link
Collaborator

@marckhouzam marckhouzam left a comment

Thanks for the investigation @edentsai. This make sense to me.

@eparis what do you think?

@scop
Copy link
Contributor

scop commented Feb 15, 2021

I filed #1351 yesterday without being aware of this PR, sorry about that. However I think that one is more complete and subjectively better stylewise than the one here. But I don't really care that deeply how, as long as the issues get fixed. Just pointing that one out in case you wish to update this one with the missing parts (flagvalue, ZSH_VERSION) instead of adopting #1351.

@edentsai
Copy link
Contributor Author

edentsai commented Feb 17, 2021

Hi @scop,

I can help to add the missing parts of #1351 into this PR if you don't mind. :)

@edentsai edentsai force-pushed the fix-unbound-variable-in-bash-completion branch from 034c8aa to a267adb Compare Feb 17, 2021
@edentsai
Copy link
Contributor Author

edentsai commented Feb 17, 2021

I updated this PR:

@scop
Copy link
Contributor

scop commented Feb 18, 2021

* add the missing parts of #1351 (BASH_VERSION, ZSH_VERSION, flagvalue)

Cool. BASH_VERSION was not in #1351 though, and I'm not sure why it is being checked for emptiness in the first place (maybe the intent is to support loading the bash completion in some non-bash shell?), but since the check is there, guarding it for undefinedness doesn't hurt much. It does have a bit of a code smell now that BASH_VERSION is being guarded, but BASH_VERSINFO is accessed unguarded.

@edentsai
Copy link
Contributor Author

edentsai commented Feb 19, 2021

maybe the intent is to support loading the bash completion in some non-bash shell?

just treat BASH_VERSION like ZSH_VERSION,
maybe someone make a mistake to load the bash/zsh completion in a wrong shell.
although the case should occur some syntax errors before unbound variable warnings.

It does have a bit of a code smell now that BASH_VERSION is being guarded, but BASH_VERSINFO is accessed unguarded.

humm. I think i should also guard the BASH_VERSINFO variable,
i will update it later.

when `set -o nounset` in Bash,
the warnings of unbound variables will break the bash completion.

use `kubectl` as example:

```sh
$ set -o nounset
$ my-cli <Tab>-bash: BASH_COMP_DEBUG_FILE: unbound variable
$
```

the warning break bash completion without any completion result,
and cause my cursor move to the newline.

Use `${variable:-}` substitution in Bash,
that assign an empty string as default for unbound variables to fix the warnings.
@edentsai edentsai force-pushed the fix-unbound-variable-in-bash-completion branch from a267adb to 72f71f5 Compare Feb 19, 2021
@edentsai
Copy link
Contributor Author

edentsai commented Feb 19, 2021

PR updated.

@github-actions
Copy link

github-actions bot commented Apr 21, 2021

This PR is being marked as stale due to a long period of inactivity

@scop
Copy link
Contributor

scop commented Apr 21, 2021

Bump

@edentsai
Copy link
Contributor Author

edentsai commented May 26, 2021

Bump again.

This MR already got stuck for a few months,
any review or approve please 🙏

@scop
Copy link
Contributor

scop commented Jul 5, 2021

Ping :) More duplicates are starting to crop up, e.g. #1441

@marckhouzam
Copy link
Collaborator

marckhouzam commented Jul 5, 2021

Apologies for missing this PR for the 1.2 release. I'll try to get it added to the list for the next release.

Also, I'll try to add a test for set -o nounset in the repo https://github.com/marckhouzam/cobra-completion-testing to avoid introducing other such issues in the future.

@scop
Copy link
Contributor

scop commented Jul 5, 2021

Cool to hear that such a repo exists 👍

Another similar option worth considering in the tests would be set -o posix, if the limitations are acceptable. But I see at least command substitution (<( ... )) is being used, that'll fail in POSIX mode, so maybe not/JFYI for the time being.

@github-actions
Copy link

github-actions bot commented Sep 6, 2021

This PR is being marked as stale due to a long period of inactivity

@scop
Copy link
Contributor

scop commented Sep 9, 2021

Hi @marckhouzam, anything I can do to help get this moving forward?

@scop
Copy link
Contributor

scop commented Sep 9, 2021

Also, would be good if someone with powers could remove the stale label (unsure if something eventually closes stale bugs), but this isn't really stale anyway.

@marckhouzam
Copy link
Collaborator

marckhouzam commented Sep 9, 2021

Hi @marckhouzam, anything I can do to help get this moving forward?

I expect that once the next release of Cobra is planned, the maintainers will start merging things, so nothing to do right now. Thanks for your patience @scop

umarcor pushed a commit to umarcor/cobra that referenced this pull request Nov 5, 2021
when `set -o nounset` in Bash,
the warnings of unbound variables will break the bash completion.

use `kubectl` as example:

```sh
$ set -o nounset
$ my-cli <Tab>-bash: BASH_COMP_DEBUG_FILE: unbound variable
$
```

the warning break bash completion without any completion result,
and cause my cursor move to the newline.

Use `${variable:-}` substitution in Bash,
that assign an empty string as default for unbound variables to fix the warnings.
Copy link

@stmcginnis stmcginnis left a comment

This looks good to me.

umarcor pushed a commit to umarcor/cobra that referenced this pull request Nov 15, 2021
when `set -o nounset` in Bash,
the warnings of unbound variables will break the bash completion.

use `kubectl` as example:

```sh
$ set -o nounset
$ my-cli <Tab>-bash: BASH_COMP_DEBUG_FILE: unbound variable
$
```

the warning break bash completion without any completion result,
and cause my cursor move to the newline.

Use `${variable:-}` substitution in Bash,
that assign an empty string as default for unbound variables to fix the warnings.
umarcor pushed a commit to umarcor/cobra that referenced this pull request Nov 16, 2021
when `set -o nounset` in Bash,
the warnings of unbound variables will break the bash completion.

use `kubectl` as example:

```sh
$ set -o nounset
$ my-cli <Tab>-bash: BASH_COMP_DEBUG_FILE: unbound variable
$
```

the warning break bash completion without any completion result,
and cause my cursor move to the newline.

Use `${variable:-}` substitution in Bash,
that assign an empty string as default for unbound variables to fix the warnings.
jpmcb
jpmcb approved these changes Dec 7, 2021
Copy link
Collaborator

@jpmcb jpmcb left a comment

Thanks all for the patience here - looks good to me and looks like some good conversation. Let's get this in.

@jpmcb jpmcb merged commit 6f19fa9 into spf13:master Dec 7, 2021
8 checks passed
@jpmcb jpmcb added this to the 1.3.0 milestone Dec 7, 2021
@jpmcb jpmcb removed the kind/stale label Dec 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants