ksh93 / ksh

ksh 93u+m: KornShell lives! | Latest release: https://github.com/ksh93/ksh/releases
Eclipse Public License 2.0
192 stars 32 forks source link

History pattern search menu (SHOPT_EDPREDICT) needs work #233

Closed posguy99 closed 2 years ago

posguy99 commented 3 years ago

Would someone point me to some documentation that says when set -o emacs is active, a hash as the first character of a line is not a comment but is instead a search command?

I'm sorry, it sounds elementary and stupid, but ksh93u and ksh93u+m behave exactly the same on the Mac and both behave the same and do NOT do the same thing on a Linux VM with exactly the same rc files.

With no rc file at all they both still do it on the Mac. Two different Mac. Catalina 10.15.7.

[618] mbp13 $ ENV=/dev/null ksh
$ set -o emacs
$ history
51  exit
52  set
53  exit
54  set
55  # test
56  set -o emacs
57  set
58  /bin/ksh
59  set -o
60  set +o emacs
61  set -o emacs
62  #s
63  echo $HISTFILE
64  mg .kshrc
65  set -o emacs
66  history
$ #s
 1) set -o emacs
 2) set +o emacs
 3) set -o
 4) set

I typed #s at the prompt.

If the input is <space>#<some_text> , it's taken as a comment. If you type some text and then use ESC-# , the shell prepends a hash and moves to the next line. It adds that line to the history and if you bring it back with an up-arrow it tries to intrepret the hash as a search command again.

$ #this is a test     
 1) this is a test

It's not Terminal.app, iTerm2 does it too.

hyenias commented 3 years ago

Well, you just taught me all sorts of goodies that I did not know existed. Let me summarize what you are requesting or stated from my current understanding of this issue:

  1. Where is this first character # menu-based history completion documented?
  2. Why is this # menu-based history completion missing on the fore-mentioned Linux VM as compared to the macOS 10.15.7. boxes?
  3. If the first character on an editing line is a <space> then followed with a # then that line is interpreted as a comment.
  4. At the end of an editing line, if you issue an ESC followed by a #; that line becomes a comment and editing starts on a new line.

Additionally, I have found that you can backslash escape the # to avoid the first character # menu-based history completion from activating. This does not create a comment line but allows a command to begin with a #.

$ \#s
-ksh: #s: not found
posguy99 commented 3 years ago

@hyenias Am I understanding that you are seeing this hash-activated search as well? If it's supposed to be doing it, why don't all copies of ksh do it? And what's the control that makes it be there or not? Some iffe test?

It may display a menu, but the menu doesn't work. Further, I would not ever expect that bringing a line back from history, that began with a hash, would immediately trigger the mechanism again.

Edit... I understand now how the menu works, you use the arrow keys and it changes the list without placing the currently selected result on the command line! Least surprise, much?

posguy99 commented 3 years ago

Your # 4 is documented on p107 of the Bolsky.

McDutchie commented 3 years ago

https://github.com/ksh93/ksh/blob/3abbb0dcb5a527fa22f40d31d0ed5737d4dda101/src/cmd/ksh93/RELEASE#L780-L786

hyenias commented 3 years ago

Yes, as you have enlightened me to its existence. I have been playing around with it. The menu works differently than the file completion menu. Not sure why all copies of ksh do it as that is too large of a scope to understand--need to know the version of each ksh plus etc. Control?, meaning option. It appears this # menu-based history completion is an added feature that works both in emacs and vi editing modes.

It may display a menu, but the menu doesn't work.

This # menu-based history completion selection works differently than the file menu one at present. I am able to use my arrows keys to cycle to a choice on the first line aka 1) line then hit TAB to retrieve it. Although, as I up and down arrow through my listing the TAB sometimes stops working and I cannot retrieve what I want. Also, this history completion menu is displayed below the current editing line whereas file menu one is displayed before the editing line.

As I typed this response, @McDutchie has provided the answer on option and its use.

I do not posses the Bolsky book. I am so jealous. My book has nothing under its In-Line Editor section for comments.

McDutchie commented 3 years ago

I think we broke it. I can't get the search to function correctly. My history is full of commands that contain make. Upon typing #*make* the first two characters (ma) are matched and then it starts beeping. On 93u+ and ksh2020 it works.

I can't get any version to make ESCncr act as documented, so I think that might be a mistake in the RELEASE entry.

posguy99 commented 3 years ago

Ok, that's just broken. What crazy person thought subverting ksh syntax like that was a good idea? And you can't turn it off at runtime?

Better... why does the same source tree have that compiled in on macOS and not on Linux? It's not like I enabled it on one and not the other... I didn't know it was there.

Whereinh*ll is that defined? RELEASE implies that, assuming it's on, it should be active whether emacs or vi or none are active:

11-09-20  A bug with SHOPT_EDPREDICT when neither vi nor emacs was enabled for
      lines beginning with # when in a multibyte locale has been fixed.

Further, this comment in RELEASE seems relevant:

11-09-20  A bug in emacs edit mode with SHOPT_EDPREDICT that would cause
      history searches matching comments lines to generate predictions
      has been fixed.  Only user typed comment lines generate predictions.
McDutchie commented 3 years ago

I do not posses the Bolsky book. I am so jealous.

Inexpensive second-hand copies are readily available, e.g. here.

McDutchie commented 3 years ago

Better... why does the same source tree have that compiled in on macOS and not on Linux?

On my end, it compiles for Linux the same as it does for macOS. It works in both emacs and vi modes on both systems.

You do of course have to make sure to enable either emacs or vi.

posguy99 commented 3 years ago

On the Linux machine, make.out shows that it's being defined:

arch/linux.i386-64/lib/package/gen/make.out:+ mamake -r '*/*' install KSH_RELFLAGS='-D_AST_git_commit=\"c33b75e5\"' KSH_SHOPTFLAGS='-DSHOPT_2DMATCH=1 -DSHOPT_AUDIT=1 -DSHOPT_AUDITFILE=\"/etc/ksh_audit\" -DSHOPT_BGX=1 -DSHOPT_BRACEPAT=1 -DSHOPT_DYNAMIC=1 -DSHOPT_EDPREDICT=1 -DSHOPT_ESH=1 -DSHOPT_FILESCAN=1 -DSHOPT_FIXEDARRAY=1 -DSHOPT_HISTEXPAND=1 -DSHOPT_MULTIBYTE=1 -DSHOPT_NAMESPACE=1 -DSHOPT_OPTIMIZE=1 -DSHOPT_PFSH=0 -DSHOPT_RAWONLY=1 -DSHOPT_STATS=1 -DSHOPT_SUID_EXEC=1 -DSHOPT_TYPEDEF=1 -DSHOPT_VSH=1'

And emacs mode is on:

mint $ set -o | grep emacs
emacs                    on

But I can type all the comments I want at a prompt and never trigger it:

mint $ 
mint $ # this is a comment
mint $ # this is another comment
mint $ # this is a third comment
mint $

That above does NOT work on the Mac... the moment I hit the <space>, I get:

mac $ 
mac $ # 
 1)  # this is a test

Same version of ksh93u+m.

McDutchie commented 3 years ago

On the Mint machine, you probably don't have a comment containing this in the history (edit: and your bell is probably silent). Try matching against something you know is in the history. Do not use any spaces between the # and the glob pattern.

posguy99 commented 3 years ago

Whyinhll is this on by default? I'm amazed* that I've never tripped over this before.

Not that you can answer that question, I'm ranting.

How do I turn this off...

Edit... obviously I can pass -USHOPT_EDPREDICT (and did) but where is it deciding to include this?

posguy99 commented 3 years ago

Oh, gods...

From https://www.mail-archive.com/ast-users@research.att.com/msg01272.html

If you are just typing comments, you should be able to ignore the predictions
that show up on the screen.

David Korn
d...@research.att.com

Never mind that if you're at the bottom of a window, the session scrolls... SMH...

posguy99 commented 3 years ago

@McDutchie You applied this in db5621dbf8abec3ff4f4450845439a014eb6d121 ... what does it actually do?

Ridiculous that bug reports in software that's not theirs would be private...

posguy99 commented 3 years ago

@McDutchie re https://github.com/ksh93/ksh/issues/233#issuecomment-803402801

But I do have such strings in the history... after the first # this is a comment it should find that one, but it doesn't, and it is in the history.

But it finds it on the Mac.

posguy99 commented 3 years ago

Side note...

@hyenias I can recommend ThriftBooks on Amazon, that's the vendor I got both my Bolsky books from.

McDutchie commented 3 years ago

Edit... obviously I can pass -USHOPT_EDPREDICT (and did) but where is it deciding to include this?

As of 6cc2f6a0af3ed1ebef724b3e5b293db716a140ea, you can edit src/cmd/ksh93/SHOPT.sh.

JohnoKing commented 3 years ago

The following block of code is used by ksh to fill in editor prediction matches after a tab: https://github.com/ksh93/ksh/blob/3abbb0dcb5a527fa22f40d31d0ed5737d4dda101/src/cmd/ksh93/edit/emacs.c#L988-L1002

When ksh rejects one of the options from the list (if(value > ep->ed->nhlist)), a crash can occur (although inconsistently) after appending a few characters to the command line. When the crash happens mp->next in ed_histgen() points to an invalid memory address: https://github.com/ksh93/ksh/blob/3abbb0dcb5a527fa22f40d31d0ed5737d4dda101/src/cmd/ksh93/edit/edit.c#L1743-L1747 GDB backtrace from the crash:

(gdb) bt
#0  0x00007ffff7ddc505 in __strlen_avx2 () from /usr/lib/libc.so.6
#1  0x0000555555636cf8 in regexec_20120528 (p=0x7ffff7944a18, s=0x20 <error: Cannot access memory at address 0x20>, nmatch=0, match=0x7ffff7948bc0, flags=49) at /home/johno/GitRepos/KornShell/ksh/src/lib/libast/regex/regexec.c:53
#2  0x00005555555f6415 in strgrpmatch_20120528 (b=0x20 <error: Cannot access memory at address 0x20>, p=0x7ffff7c5d7a0 "@(make)*", sub=0x0, n=0, flags=7) at /home/johno/GitRepos/KornShell/ksh/src/lib/libast/string/strmatch.c:143
#3  0x00005555555f659b in strmatch (s=0x20 <error: Cannot access memory at address 0x20>, p=0x7ffff7c5d7a0 "@(make)*") at /home/johno/GitRepos/KornShell/ksh/src/lib/libast/string/strmatch.c:179
#4  0x000055555557d80e in ed_histgen (ep=0x7ffff7fbe480, pattern=0x7ffff7fa6051 "make") at /home/johno/GitRepos/KornShell/ksh/src/cmd/ksh93/edit/edit.c:1745
#5  0x00005555555d47c2 in draw (ep=0x7ffff794f550, option=APPEND) at /home/johno/GitRepos/KornShell/ksh/src/cmd/ksh93/edit/emacs.c:1465
#6  0x00005555555d1ed8 in ed_emacsread (context=0x7ffff7fbe480, fd=0, buff=0x7ffff7fa6050 "#make", scend=1016, reedit=0) at /home/johno/GitRepos/KornShell/ksh/src/cmd/ksh93/edit/emacs.c:386
#7  0x000055555558e26b in slowread (iop=0x5555556f6a40 <_Sfstdin>, buff=0x7ffff7fa6050, size=65536, handle=0x7ffff7fbec20) at /home/johno/GitRepos/KornShell/ksh/src/cmd/ksh93/sh/io.c:1953
#8  0x000055555564c2bd in sfrd (f=0x5555556f6a40 <_Sfstdin>, buf=0x7ffff7fa6050, n=65536, disc=0x7ffff7fbec20) at /home/johno/GitRepos/KornShell/ksh/src/lib/libast/sfio/sfrd.c:253
#9  0x0000555555645ee8 in _sffilbuf (f=0x5555556f6a40 <_Sfstdin>, n=-1) at /home/johno/GitRepos/KornShell/ksh/src/lib/libast/sfio/sffilbuf.c:105
#10 0x000055555564d1b0 in sfreserve (f=0x5555556f6a40 <_Sfstdin>, size=0, type=0) at /home/johno/GitRepos/KornShell/ksh/src/lib/libast/sfio/sfreserve.c:148
#11 0x00005555555681f8 in exfile (shp=0x20, iop=0x20, fno=32) at /home/johno/GitRepos/KornShell/ksh/src/cmd/ksh93/sh/main.c:528
#12 0x00005555555679c8 in sh_main (ac=1, av=0x7fffffffe6b8, userinit=0x0) at /home/johno/GitRepos/KornShell/ksh/src/cmd/ksh93/sh/main.c:351
#13 0x0000555555566d6e in main (argc=1, argv=0x7fffffffe6b8) at /home/johno/GitRepos/KornShell/ksh/src/cmd/ksh93/sh/pmain.c:45
(gdb) frame 1
#1  0x0000555555636cf8 in regexec_20120528 (p=0x7ffff7944a18, s=0x20 <error: Cannot access memory at address 0x20>, nmatch=0, match=0x7ffff7948bc0, flags=49) at /home/johno/GitRepos/KornShell/ksh/src/lib/libast/regex/regexec.c:53
53      return regnexec(p, s, s ? strlen(s) : 0, nmatch, match, flags);
(gdb) p s
$1 = 0x20 <error: Cannot access memory at address 0x20>
(gdb) p p
$2 = (const regex_t *) 0x7ffff7944a18
(gdb) p nmatch
$3 = 0
(gdb) p s
$4 = 0x20 <error: Cannot access memory at address 0x20>
(gdb) frame 3
#3  0x00005555555f659b in strmatch (s=0x20 <error: Cannot access memory at address 0x20>, p=0x7ffff7c5d7a0 "@(make)*") at /home/johno/GitRepos/KornShell/ksh/src/lib/libast/string/strmatch.c:179
179     return strgrpmatch(s, p, NiL, 0, STR_MAXIMAL|STR_LEFT|STR_RIGHT);
(gdb) frame 4
#4  0x000055555557d80e in ed_histgen (ep=0x7ffff7fbe480, pattern=0x7ffff7fa6051 "make") at /home/johno/GitRepos/KornShell/ksh/src/cmd/ksh93/edit/edit.c:1745
1745                    if(n || strmatch(mp->data,cp))
(gdb) p mp->data
Cannot access memory at address 0x20
(gdb) p cp
$5 = 0x7ffff7c5d7a0 "@(make)*"
(gdb) p n
$6 = 0
posguy99 commented 3 years ago

Here's a related bug for you in kshu93+ itself.

If you trigger the search, type some text that generates matches, then ^U to erase the input line... the result menu isn't removed.

mac $ ver
ksh: Version AJM 93u+ 2012-08-01
mac $ #config add
 1) config add .ksh/rc/pager.ksh
 2) config add .ksh/rc/aliases.ksh
 3) config add -a
 4) config add .ksh/fun/ver
 5) config add .kshrc

Type ^U

mac $ ver
ksh: Version AJM 93u+ 2012-08-01
mac $            
 1) config add .ksh/rc/pager.ksh
 2) config add .ksh/rc/aliases.ksh
 3) config add -a
 4) config add .ksh/fun/ver
 5) config add .kshrc

The menu is still active and you can still make selections from it, even though there really should be no search active.

@JohnoKing I managed to trigger the fault from https://github.com/ksh93/ksh/issues/233#issuecomment-803440569 while writing this.

McDutchie commented 3 years ago

I think we broke it. I can't get the search to function correctly. My history is full of commands that contain make. Upon typing #*make* the first two characters (ma) are matched and then it starts beeping. On 93u+ and ksh2020 it works.

Funny. Now it (sort of) works again on my system, for no apparent reason.

In any case, I tend to agree with @posguy99 about this feature. In summary:

So, I propose we remove this experimental SHOPT_EDPREDICT feature.

@JohnoKing, @hyenias, what say you?

JohnoKing commented 3 years ago

It seems redundant. There are other ways to search the history that are, IMO, easier to use.

I agree, especially with this point. I always find myself using reverse search instead (usually with the up arrow, sometimes with Ctrl+R).

hyenias commented 3 years ago

My view is to simply set the default compile option to off instead of on and thus deprecate the code and phase it out from the global ksh user community instead of deleting it . I assume that using -DSHOPT_EDPREDICT=0 will kept it from being compiled into the code. @JohnoKing has already done some research to where the code is causing memory faults. There might be someone in the ksh community that eventually corrects the code. Meanwhile, we can find out if others are using it and want it.

All-in-all, if we become aware of the usefulness this # menu-based history search selection, then it could be altered or improved to a point of overall acceptance. I believe in a phase out approach instead of immediate deletion aka deprecate code with user notification along with timeline.

posguy99 commented 3 years ago

Myself, I run with it turned off right now. Once you see it, it's like a gnat, you can't unsee it. But delete it? Not so sure.

re @hyenias question... Do those who ship ksh93u+ have it enabled or disabled? I know Apple has it enabled, I know Linux Mint has it enabled which means Ubuntu has it enabled which means Debian has it enabled. Of course, do they know it's enabled?

JohnoKing commented 3 years ago

To add onto the discussion, I'll note that predictive editing is disabled on Solaris (possibly because of this bug): https://github.com/oracle/solaris-userland/blob/master/components/ksh93/patches/165-CR7186440_ksh93_disable_predictive_editing.patch

McDutchie commented 3 years ago

Right, looks like there is a consensus for turning it off by default. I don't want to keep shipping broken stuff forever (even turned off), but I'm okay with that for the first few releases (perhaps versions 1.*). It gives the community a chance to take an interest in it and possibly fix it. I doubt that will actually happen but I've been wrong before.

McDutchie commented 3 years ago

If it is fixed, another thing that needs doing is a shell option that allows users to turn this on or off at runtime.

posguy99 commented 3 years ago

Having another character as the trigger would go a long way, too.

McDutchie commented 3 years ago

That character could easily be made configurable with a variable.

hyenias commented 3 years ago

@posguy99 exactly! I was thinking along the lines of using a non-printable key such as a function key (F1-F10) and allow the menu to be launched at any point not just the first character of the line.

posguy99 commented 3 years ago

@hyenias That wouldn't work. Function keys are multi-byte ANSI sequences. Yes, if the running KSH had intimate knowledge of where it was running, it could capture actual low-level key codes, but it'd never work through, say, an SSH session.

It really has to be a printable character. The hash sign was just a singularly bad choice.

McDutchie commented 3 years ago

ksh already interprets multi-byte ANSI sequences such as your up-arrow key, so there's no real reason it couldn't be done, although it might be nontrivial.

posguy99 commented 3 years ago

Yeah, I know @JohnoKing fixed it, but to me up arrow is ^P .

dannyweldon commented 3 years ago

My thoughts are that this menu that is activated by a leading hash should instead be activated by Ctrl-R as a more interactive version. I think bash is doing something similar for Ctrl-R now instead of it's old incremental search. That's what it does for me on Cygwin, which I don't have in front of me right now. Perhaps this could be made into a runtime option for Ctrl-R, although it would have to be activated by <esc>/ in vi mode I suppose.

McDutchie commented 2 years ago

Interest seems to have died out here. If no one takes this up, I'll remove SHOPT_EDPREDICT after the 1.0 release to avoid bit rot.