workaround for PAT_STATIC bug in zsh #415

docwhat · 2017-02-07T17:21:15Z

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 #407

m0vie · 2017-02-07T20:39:56Z

Isn't is-at-least itself using patterns and therefore causing the exact same bug it tries to prevent?

We should probably determine and remember this beforehand when sourcing z-sy-h, also for performance reasons.

docwhat · 2017-02-07T22:58:53Z

@m0vie as soon as I read your comment, I realized you had to be right. I double checked in my plain-vanilla "fake" config and my work-around didn't work.

I pushed up a revised version that stores the value and uses that. It's also easier to read, which I like.

danielshahaf · 2017-02-08T05:29:09Z

@m0vie Thanks for reviewing.

@docwhat Besides my other comment (just posted), I have only a few minor style tweaks. Will go through them once the functionality is settled.

danielshahaf · 2017-02-08T05:32:10Z

@docwhat Feel free to join us on #zsh-syntax-highlighting on freenode.

danielshahaf

Thanks for putting this together, @docwhat.

Would you be able to revise the PR accordingly? No problem if you can't, just please let me know either way.

danielshahaf · 2017-02-09T13:50:02Z

README.md

-or newer.
+Yes!
+
+If you're using `history-incremental-search-backward` (by default bound to <kbd>Ctrl_R</kbd>


I'm curious what was the reasoning behind the change of Ctrl_R to Ctrl+R. (Not need to revert it — six of one, half a dozen of the other — just curious.)

typo on my part. :-/

danielshahaf · 2017-02-09T13:50:05Z

README.md

+in zsh's emacs keymap) then it works with _zsh version 5.3 and newer_.
+
+If you're using `history-incremental-pattern-search-backward`, then syntax highlighting works
+in _zsh 5.3.2 and newer_ due to [a bug](http://www.zsh.org/mla/workers//2017/msg00034.html).


Please use the X-Seq links: http://www.zsh.org/cgi-bin/mla/redirect?WORKERNUMBER=40285

Could we qualify the term "bug" to explain it's an upstream (zsh) bug?

Could we reword this to reflect that 5.3.2 is not yet released (and will probably be called 5.4)? I'll update #412 to refer to this.

Right now it describes what the code does...

Will zsh ever release a 5.3.2?

If so, would it just include security patches, or would it include this patch to?

Should I just change the is-at-least to 5.4?

(being discussed in the toplevel comments below)

danielshahaf · 2017-02-09T13:54:08Z

zsh-syntax-highlighting.zsh

+  zsh_highlight__pat_static_bug=f
+else
+  zsh_highlight__pat_static_bug=t
+fi


As much as I like lisp, I think the code would be overall easier to read if it used only a single representation of booleans. We already both x=false/x=true/if $x and x=0/x=1/if (( x )) [my fault for having two]; I'd rather not introduce a third way.

danielshahaf · 2017-02-09T13:55:28Z

zsh-syntax-highlighting.zsh

@@ -67,7 +76,7 @@ _zsh_highlight()

  # Remove all highlighting in isearch, so that only the underlining done by zsh itself remains.
  # For details see FAQ entry 'Why does syntax highlighting not work while searching history?'.
-  if [[ $WIDGET == zle-isearch-update ]] && ! (( $+ISEARCHMATCH_ACTIVE )); then
+  if [[ $WIDGET == zle-isearch-update ]] && ([[ $zsh_highlight__pat_static_bug == t ]] || ! (( $+ISEARCHMATCH_ACTIVE )) ); then


This could use a block { … } rather than a subshell.

Also, I'd like some comment explaining the second conjunct in English: it's hard enough for me to get a mental picture of the behaviour under various zsh versions as the conjunct stands before this patch.

The logic here is "Disabling highlighting during isearch (for reasons explained in README.md) unless zsh is new enough and doesn't have the 5.3.1 bug".

danielshahaf · 2017-02-09T13:57:43Z

zsh-syntax-highlighting.zsh

@@ -53,6 +53,15 @@ fi
 # Core highlighting update system
 # -------------------------------------------------------------------------------------------------

+# Use workaround for bug in ZSH?
+# zsh-users/zsh@48cadf4 http://www.zsh.org/mla/workers//2017/msg00034.html
+autoload -U is-at-least


There's another autoload is-at-least on line 397 which can be removed now.

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 #407

docwhat · 2017-02-09T23:00:53Z

Pulling this out for visibility... The current change has everything you asked for except this...

@danielshahaf said:

Could we reword this to reflect that 5.3.2 is not yet released (and will probably be called 5.4)? I'll update #412 to refer to this.

Right now it describes what the code does...

Will zsh ever release a 5.3.2?
If so, would it just include security patches, or would it include this patch to?
Should I just change the is-at-least to 5.4?

m0vie · 2017-02-09T23:53:08Z

README.md

+in zsh's emacs keymap) then it works with _zsh version 5.3 and newer_.
+
+If you're using `history-incremental-pattern-search-backward`, then syntax highlighting works
+in _zsh 5.3.2 and newer_ due to [a bug in zsh](http://www.zsh.org/cgi-bin/mla/redirect?WORKERNUMBER=40285).


history-incremental-search-backward or history-incremental-pattern-search-backward doesn't matter, we don't check for that. Its 5.3.2 and newer in all cases

The information in the readme is still wrong. See my comment above.

Thanks. Broke this out as #423

m0vie · 2017-02-09T23:55:06Z

zsh-syntax-highlighting.zsh

@@ -67,7 +76,9 @@ _zsh_highlight()

  # Remove all highlighting in isearch, so that only the underlining done by zsh itself remains.
  # For details see FAQ entry 'Why does syntax highlighting not work while searching history?'.
-  if [[ $WIDGET == zle-isearch-update ]] && ! (( $+ISEARCHMATCH_ACTIVE )); then


We should probably add a comment here stating that we must never use any pattern before this line, in order to not break things again.

@m0vie At this point it may be easier to ask pws to release a 5.3.2 with your patch (and other stability fixes) included?

danielshahaf · 2017-02-10T11:06:50Z

@docwhat The next zsh feature release will be 5.4. There may be a 5.3.2 hotfix release before then. In principle, zsh may decide to release a 5.3.1.1, but that's unlikely. And in either of the three cases, we can't be sure whether the release would fix the PAT_STATIC bug: @m0vie's patch might be reverted or the bug accidentally reintroduced before upstream's release.

Now, I won't release z-sy-h with an is-at-least condition that refers to a future zsh release, and people who use zsh from master should use HEAD of master; therefore, I think it doesn't matter what version number we pass to is-at-least, so long as we have a note on #412 to fix it after zsh's release and before z-sy-h's release.

Makes sense?

docwhat · 2017-02-13T17:35:19Z

@danielshahaf:

Just so I understand what my action item is: I should set the is-at-least check to 5.3.1.1 (as you suggested previously?

Can we push zsh to release 5.3.2 sooner rather than later and release it with that version check (as you mentioned above)?

danielshahaf · 2017-02-14T17:39:46Z

@danielshahaf: Just so I understand what my action item is: I should set the `is-at-least` check to `5.3.1.1` (as you suggested previously?

My current idea is that it _doesn't matter_ whether the code says `is-at-least 5.3.1.1` or `is-at-least 5.3.2`, since I will wait for the next zsh release — and update the argument to is-at-least accordingly — before releasing z-sy-h. Makes sense?

Can we push zsh to release 5.3.2 sooner rather than later and release it with that version check (as you mentioned above)?

We certainly can. All it takes is for someone to email zsh-workers@ and ask/convince them to do so. :)

docwhat · 2017-03-29T01:37:23Z

It's been a while, @danielshahaf -- Is there anything else I need to do with this PR? Or are we still good?

danielshahaf · 2017-03-29T09:20:59Z

@movie's patch is ~~workers/40453, zsh-users/zsh@e51c9c1~~ workers/40285.

danielshahaf · 2017-03-29T09:28:12Z

@docwhat Apologies for dropping the ball. Merged this and updated #415 accordingly.

If anyone wants to push upstream for a 5.3.2 (as per my penultimate comment), feel free :-)

docwhat · 2017-04-05T00:46:46Z

@docwhat Apologies for dropping the ball.

No apologies necessary! Thanks!

docwhat mentioned this pull request Feb 7, 2017

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

Closed

danielshahaf reviewed Feb 9, 2017

View reviewed changes

danielshahaf added this to the 0.6.0 milestone Feb 9, 2017

danielshahaf mentioned this pull request Feb 9, 2017

zsh 5.4-to-be features in use #412

Closed

2 tasks

workaround for PAT_STATIC bug in zsh

835fec7

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 #407

m0vie suggested changes Feb 9, 2017

View reviewed changes

danielshahaf merged commit 835fec7 into zsh-users:master Mar 29, 2017

danielshahaf mentioned this pull request Mar 30, 2017

Followup to #415 #423

Closed

docwhat deleted the pr/pat_static branch April 5, 2017 00:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

workaround for PAT_STATIC bug in zsh #415

workaround for PAT_STATIC bug in zsh #415

docwhat commented Feb 7, 2017

m0vie commented Feb 7, 2017

docwhat commented Feb 7, 2017

danielshahaf commented Feb 8, 2017

danielshahaf commented Feb 8, 2017

danielshahaf left a comment

danielshahaf Feb 9, 2017

docwhat Feb 9, 2017

danielshahaf Feb 9, 2017

docwhat Feb 9, 2017

danielshahaf Feb 10, 2017

danielshahaf Feb 9, 2017

docwhat Feb 9, 2017

danielshahaf Feb 9, 2017

docwhat Feb 9, 2017

danielshahaf Feb 9, 2017

docwhat Feb 9, 2017

docwhat commented Feb 9, 2017 •

edited

Loading

m0vie Feb 9, 2017

m0vie Mar 29, 2017

danielshahaf Mar 30, 2017

m0vie Feb 9, 2017

danielshahaf Feb 10, 2017

danielshahaf commented Feb 10, 2017

docwhat commented Feb 13, 2017

danielshahaf commented Feb 14, 2017 via email

docwhat commented Mar 29, 2017

danielshahaf commented Mar 29, 2017 •

edited

Loading

danielshahaf commented Mar 29, 2017

docwhat commented Apr 5, 2017

workaround for PAT_STATIC bug in zsh #415

workaround for PAT_STATIC bug in zsh #415

Conversation

docwhat commented Feb 7, 2017

m0vie commented Feb 7, 2017

docwhat commented Feb 7, 2017

danielshahaf commented Feb 8, 2017

danielshahaf commented Feb 8, 2017

danielshahaf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

docwhat commented Feb 9, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danielshahaf commented Feb 10, 2017

docwhat commented Feb 13, 2017

danielshahaf commented Feb 14, 2017 via email

docwhat commented Mar 29, 2017

danielshahaf commented Mar 29, 2017 • edited Loading

danielshahaf commented Mar 29, 2017

docwhat commented Apr 5, 2017

docwhat commented Feb 9, 2017 •

edited

Loading

danielshahaf commented Mar 29, 2017 •

edited

Loading