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.

Sign up for GitHub

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

Jump to bottom

history-incremental-pattern-search-backward is broken after upgrading to zsh 5.3.1 #407

Closed
protist opened this issue Jan 4, 2017 · 28 comments
Closed

history-incremental-pattern-search-backward is broken after upgrading to zsh 5.3.1 #407

protist opened this issue Jan 4, 2017 · 28 comments
Labels
Regression Upstream
Milestone
0.6.0

Comments

@protist
Copy link

protist commented Jan 4, 2017

I recently upgraded zsh from 5.2-2 -> 5.3.1-1, in Arch Linux. After upgrading, history-incremental-pattern-search-backward and history-incremental-pattern-search-forward no longer work. That is, they show the most recent match, then failing bck-i-search: foo, then one other match from eons ago, then it fails to find any more. Reverting to zsh 5.2-2 fixes this problem.

I use zim. When I disable zsh-syntax-highlighting, pattern searching works again. I tried creating a new minimal zsh install, that did not source zim at all. As soon as I directly source zsh-syntax-highlighting, pattern searching breaks again. Unlike issue #387, I'm not using zsh-autocorrect.
@danielshahaf
Copy link
Member

danielshahaf commented Jan 4, 2017 via email

@protist
Copy link
Author

protist commented Jan 5, 2017

Oddly enough, your example fails for me!

$ zsh -f
% bindkey ^R history-incremental-pattern-search-backward
% source ./zsh-syntax-highlighting.zsh 
% foo
zsh: command not found: foo
% bar
zsh: command not found: bar
% baz
zsh: command not found: baz

I type <^R>ba?

% baz
bck-i-search: ba?_

Then again <^R>

% baz
failing bck-i-search: ba?_

And it's stuck on baz, and never gets to bar.

I tested (from the start) searching with ba instead of ba?, which also fails. I then tested ba using the default history-incremental-search-backward, which works as expected.

I wasn't previously familiar with zsh -f, but my understanding is that it skips all initiation files except for the system-wide zshenv. However, I don't have any file on my system named that anyway. I'm not sure how else my system might differ from yours.

@danielshahaf
Copy link
Member

danielshahaf commented Jan 5, 2017 via email

@protist
Copy link
Author

protist commented Jan 5, 2017

So, to clarify, you're saying that a search for just ba with pattern search finds baz but not bar?

Correct.

Distros sometimes change the path of zshenv to /etc/zsh/zshenv or similar.

Sorry, I should have been more specific. locate zshenv comes up with nothing. Also, there's nothing at /etc/zsh/zshenv.

What does zsh -fx print before the first prompt?

Nothing at all.

What versions of everything do you use?

% print -rl -- $ZSH_VERSION $ZSH_PATCHLEVEL $ZSH_HIGHLIGHT_VERSION $ZSH_HIGHLIGHT_REVISION
5.3.1
zsh-5.3.1-0-g06b1b7a
0.6.0-dev
aac4a4423898fccbd1ab72c369ac09995a9139f1

Looks like the zsh version is slightly different to yours, but I am using the latest release. I can try the latest -git release if you think that might be more consistent with yours?

Does your distro patch zsh?

It looks like it does some minimal patching, but I'm not sure if this has any effect.

Do you have more than one zsh binary on your system?

No, just the one.

@danielshahaf
Copy link
Member

danielshahaf commented Jan 5, 2017 via email

@m0vie
Copy link
Contributor

m0vie commented Jan 5, 2017 •
edited
Loading

This is actually an upstream bug.

Minimal example without z-sy-h to reproduce:

% bindkey '^R' history-incremental-pattern-search-backward
% evil_hook() { a=(); : ${a[(r)foo*]}; };
% zle -N zle-isearch-update evil_hook
% : foo
% : bar
% : baz
%

type: <^R>b
% : baz
bck-i-search: b_

type: <^R>
% : foo
bck-i-search: b_

': foo' is found instead of ': bar' because evil_hook modified the
static pattern used in isearch.

The cause is that iseach uses the global PAT_STATIC pattern. But inside the zle-isearch-hook any function that uses patterns, too, will overwrite that pattern.

I sent a patch to workers@.

This bug got introduced in #288, because we deliberately hooked zle-isearch-update to fix another bug.
If a user would have hooked zle-isearch-update himself already, the bug would have already surfaced before.

Question now is: Can we do anything on z-sy-h's side to prevent triggering this?

This (pseudocode) should technically do it,

-  if [[ $WIDGET == zle-isearch-update ]] && ! (( $+ISEARCHMATCH_ACTIVE )); then
+  if [[ $WIDGET == zle-isearch-update ]] && ! zsh_bug_is_fixed; then

since we (luckily) don't use any patterns before it. Of course we'd lose z-sy-h's coloring again until then.

@danielshahaf
Copy link
Member

danielshahaf commented Jan 6, 2017 via email

@m0vie
Copy link
Contributor

m0vie commented Jan 7, 2017

Fixed in upstream with
zsh-users/zsh@48cadf4
http://www.zsh.org/mla/workers//2017/msg00034.html

@danielshahaf
Copy link
Member

danielshahaf commented Jan 7, 2017 via email

@danielshahaf
Copy link
Member

Further discussion on http://www.zsh.org/cgi-bin/mla/redirect?WORKERNUMBER=40328 (same thread, but the web archive doesn't join it)

@danielshahaf danielshahaf added this to the 0.6.0 milestone Jan 11, 2017
@pikeas pikeas mentioned this issue Jan 23, 2017
Open
@protist
Copy link
Author

protist commented Feb 2, 2017

Have you made progress on probing for this bug at runtime, or some other workaround for ≤5.3.1?

Thank you for the quick fix upstream, but is there a way to get it working in the meantime? Thanks in advance.

@danielshahaf
Copy link
Member

danielshahaf commented Feb 2, 2017 via email

@protist
Copy link
Author

protist commented Feb 2, 2017

I'm not sure I have a good suggestion here. Would it work for you to just locally remove the && ! (( $+ISEARCHMATCH_ACTIVE )) clause?

Oh yes, sorry. For some reason I forgot to try that. Yes, that certainly works; thank you. Are there any negative ramifications for removing this?

@danielshahaf
Copy link
Member

danielshahaf commented Feb 2, 2017 via email

@protist
Copy link
Author

protist commented Feb 2, 2017

Thank you again. IMO I'm happy to close this issue now, unless you prefer it open until the fix is released.

@danielshahaf
Copy link
Member

You're welcome. Closing, but if anyone has an idea for a runtime probe, please add it in comments.

@docwhat
Copy link
Contributor

docwhat commented Feb 4, 2017

re: probe

No runtime probe has been devised, and is-at-least wouldn't be right since some distros have backported the upstream fix to 5.3.1.

I would argue that turning this off for versions less than 5.3.2 would be acceptable, since "breaking regex incremental search" is worse than "no highlighting during incremental search".

When the first happens, its hard to figure out why. When highlighting goes away, you know why.

This would work, though I don't know how slow is-at-least is.

  if [[ $WIDGET == zle-isearch-update ]] && (! (( $+ISEARCHMATCH_ACTIVE )) || ! is-at-least 5.3.2); then

@danielshahaf
Copy link
Member

Good point. Reopening.

If is-at-least is slow we could call it once at shell initialization time.

@danielshahaf danielshahaf reopened this Feb 4, 2017
@docwhat
Copy link
Contributor

docwhat commented Feb 5, 2017

@danielshahaf: Should I make a pull request with my simplistic change from above:question:

@danielshahaf
Copy link
Member

@docwhat A PR would be welcome — especially if it updates https://github.com/zsh-users/zsh-syntax-highlighting/#does-syntax-highlighting-work-during-incremental-history-search too ☺.

If you see any way to do a runtime probe, that'd be even better; the relevant code is around https://github.com/zsh-users/zsh/blob/bb6c08b51a0798700b6fdd0b75581dca21dc4e5b/Src/Zle/zle_hist.c#L1220.

Thanks!

P.S. How about checking is-at-least 5.3.1.1, rather than 5.3.2? Just in case. (Not 5.3.1.0 because is-at-least ignores trailing zero components)

@docwhat
Copy link
Contributor

docwhat commented Feb 7, 2017

How about checking is-at-least 5.3.1.1, rather than 5.3.2? Just in case. (Not 5.3.1.0 because is-at-least ignores trailing zero components)

Do distros bump the 4th version number? If so, I don't know what the state of various distros are (e.g. they may have a 5.3.1.1 out already without this patch).

I'd rather err on the side of caution, as I said, since tracking down the regexp-search-is-broken case is so difficult (it's involved weeks of my time over the last 6-8 months).

@docwhat docwhat mentioned this issue Feb 7, 2017
Merged
@docwhat
Copy link
Contributor

docwhat commented Feb 7, 2017

@danielshahaf: PR #415 is created.

@danielshahaf
Copy link
Member

danielshahaf commented Feb 8, 2017 via email

@protist
Copy link
Author

protist commented Feb 8, 2017

I might be completely off target, but are there even zsh x.y.z.a releases? I think zsh just uses x.y.z?

https://sourceforge.net/projects/zsh/files/zsh/

@danielshahaf
Copy link
Member

@protist Correct. The argument for 5.3.1.1 over 5.3.2 is that it would be correct even if zsh started using x.y.z.w. The arguments for 5.3.2 over 5.3.1.1 are that it suffices given upstream's release numbering policy (as you say) and that it's better to be conservative due to the asymmetric failure modes (as pointed out by @docwhat).

@danielshahaf danielshahaf mentioned this issue Feb 9, 2017
Closed
2 tasks
@protist
Copy link
Author

protist commented Mar 21, 2017

Just to update everyone, the zsh patch has hit Arch Linux in 5.3.1-2. I can confirm that history-incremental-pattern-search-backward still works even after reverting the z-s-h hack from this comment. Thanks again!

@danielshahaf
Copy link
Member

danielshahaf commented Mar 21, 2017 via email

@protist
Copy link
Author

protist commented Mar 21, 2017

Thank you for the update.

No worries.

Debian has also backported…

I'm mortified that Debian was quicker than Arch with this! 😧

brandon-fryslie pushed a commit to brandon-fryslie/zsh-syntax-highlighting that referenced this issue Dec 21, 2017
@docwhat @brandon-fryslie
workaround for PAT_STATIC bug in zsh
0cfc34f 
ZSH versions less than 5.3.2 (or 5.4) have a bug that prevents
`history-incremental-pattern-search-backward` for working
correctly (the history stops searching after the first found item).

Closes zsh-users#407
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Regression Upstream
Projects
None yet
Milestone
0.6.0
Development

No branches or pull requests
4 participants
@docwhat @protist @m0vie @danielshahaf