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 bug in extracting variable values containing | #81

Merged
merged 5 commits into from
Jan 15, 2021
Merged

Conversation

PatrickF1
Copy link
Owner

@PatrickF1 PatrickF1 commented Jan 15, 2021

The bug happens whenever the variable being previewed contains |. What happens is that the third regex used in __fzf_extract_var_info is greedy and will over-remove text up to the second to last |. For example:

$ set variable "a | b | c | d"
$ __fzf_extract_var_info variable (set --show | psub)
set in global scope, unexported, with 1 elements
[1]  d

By making the third regex lazy, it now works properly:

$ set variable "a | b | c | d"
$ __fzf_extract_var_info variable (set --show | psub)
set in global scope, unexported, with 1 elements
[1] a | b | c | d

Additionally, I've added a Fishtape test to make debugging these tricky regexes more easily in the future, which I expect to need to do as the master branch of Fish has already changed the output of set --show.

@@ -6,17 +6,17 @@ function __fzf_extract_var_info --argument-names variable_name set_show_output -
# $variable_name[
string match --entire --regex "^\\\$$variable_name(?:: set|\[)" <$set_show_output |
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Self-note for later: why are 3 \s necessary to escape the first $? Well, the last one is needed to prevent fish from starting a variable substitution. Then, to prevent the regex library from interpreting the $ as an anchor, we need to prefix it with a \. But to prevent that \ from applying to the now-literal $, we need to escape it as well with another \.


# From the lines of values, keep only the index and value, replacing...
# $variable_name[1]: length=14 value=|variable_value|
# ...with...
# [1] variable_value
string replace --regex "^\\\$$variable_name(\[.+\]).+\|(.+)\|\$" '\$1 \$2'
string replace --regex "^\\\$$variable_name(\[\d+\]).+?\|(.+)\|\$" '\$1 \$2'
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The bug fix is here: make the .+ that comes after the first capturing group non-greedy by appending a ?.

A minor unrelated change is using \d to match inside the [ ] to make it more obvious that it's supposed to be index numbers.

@PatrickF1 PatrickF1 merged commit a734bbc into main Jan 15, 2021
@PatrickF1 PatrickF1 deleted the fix-regex branch January 15, 2021 19:47
@kidonng
Copy link
Contributor

kidonng commented Jan 16, 2021

Just noticed this in the release notes. I remember using non-greedy matching sometime in the original PR, but it seems to be lost. Anyway, thanks for fixing it!

hrshtst pushed a commit to hrshtst/fzf.fish that referenced this pull request Apr 22, 2021
The bug happens whenever the variable being previewed contains a |. What happens is that the third regex used in __fzf_extract_var_info is greedy and will over-remove text up to the second to last |. For example:
$ set variable "a | b | c | d"
$ __fzf_extract_var_info variable (set --show | psub)
set in global scope, unexported, with 1 elements
[1]  d

By making the third regex lazy, it now works properly:
$ set variable "a | b | c | d"
$ __fzf_extract_var_info variable (set --show | psub)
set in global scope, unexported, with 1 elements
[1] a | b | c | d

Additionally, I've added a Fishtape test to make debugging these tricky regexes more easily in the future, which I expect to need to do as the master branch of Fish has already changed the output of set --show.
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.

2 participants