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

History pattern search menu (SHOPT_EDPREDICT) needs work #233

Closed
posguy99 opened this issue Mar 20, 2021 · 34 comments
Closed

History pattern search menu (SHOPT_EDPREDICT) needs work #233

posguy99 opened this issue Mar 20, 2021 · 34 comments
Labels
backburner Low priority (but feel free to fix it and do a PR) enhancement New feature or request help wanted Extra attention is needed

Comments

@posguy99
Copy link

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
Copy link

hyenias commented Mar 20, 2021

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

hyenias added a commit to hyenias/ksh that referenced this issue Mar 20, 2021
While experimenting with ksh93#233, a memory segmentation fault occurred.
A search of other emacs issues found a potential matching issue as
described in att#791. Also, a duplicate
PR of att#1489 was submitted. This
commit backports that fix.

src/cmd/ksh93/edit/history.c: hist_word():
- Switch from using strcpy to memmove as the two strings could overlap.
@posguy99
Copy link
Author

posguy99 commented Mar 20, 2021

@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
Copy link
Author

posguy99 commented Mar 20, 2021

Your # 4 is documented on p107 of the Bolsky.

McDutchie pushed a commit that referenced this issue Mar 20, 2021
While experimenting with #233, a memory segmentation fault occurred.
A search of other emacs issues found a potential matching issue as
described in att#791. Also, a duplicate
PR of att#1489 was submitted. This
commit backports that fix.

src/cmd/ksh93/edit/history.c: hist_word():
- Switch from using strcpy to memmove as the two strings could overlap.
@McDutchie
Copy link

ksh/src/cmd/ksh93/RELEASE

Lines 780 to 786 in 3abbb0d

10-05-20 +The compile option SHOPT_EDPREDICT has been added. When this option
is on, as you type a line beginning with a # the following characters
are treated as a shell pattern and cause matching lines from the
history file to be displayed as a numbered list as you type.
You can scroll up and down this list or you can use <ESC>nTAB
to make this the current line (n defaults to 1 of omitted) or
<ESC>n<cr> to execute.

@hyenias
Copy link

hyenias commented Mar 20, 2021

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
Copy link

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
Copy link
Author

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
Copy link

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

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

@McDutchie
Copy link

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
Copy link
Author

posguy99 commented Mar 20, 2021

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
Copy link

McDutchie commented Mar 20, 2021

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
Copy link
Author

posguy99 commented Mar 20, 2021

Whyinh*ll 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
Copy link
Author

posguy99 commented Mar 20, 2021

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
Copy link
Author

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

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

@posguy99
Copy link
Author

@McDutchie re #233 (comment)

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
Copy link
Author

Side note...

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

@McDutchie
Copy link

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

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

@JohnoKing
Copy link

The following block of code is used by ksh to fill in editor prediction matches after a tab:

#if SHOPT_EDPREDICT
if(ep->ed->hlist)
{
value += ep->ed->hoff;
if(value > ep->ed->nhlist)
beep();
else
{
value = histlines - ep->ed->hlist[value-1]->index;
ed_histlist(ep->ed,0);
ed_ungetchar(ep->ed,cntl('P'));
return(value);
}
}
#endif /* SHOPT_EDPREDICT */

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:

ksh/src/cmd/ksh93/edit/edit.c

Lines 1743 to 1747 in 3abbb0d

for(argv=av=(char**)ep->hlist,mp=ep->hfirst; mp;mp= mp->next)
{
if(n || strmatch(mp->data,cp))
*av++ = (char*)mp;
}

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

@McDutchie McDutchie added bug Something is not working question Further information is requested labels Mar 20, 2021
@posguy99
Copy link
Author

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 #233 (comment) while writing this.

@McDutchie McDutchie changed the title Question re emacs mode Predictive editing (SHOPT_EDPREDICT) is broken Apr 22, 2021
@McDutchie
Copy link

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:

  • It doesn't work reliably (and didn't in 93u+) and I am frankly not motivated to try to fix it.
  • It's virtually undocumented and unknown. Advanced users might happen upon it, but almost none of them will find the one brief explanation of it in the RELEASE file.
  • It's misnamed. It's a regex-based command history search, predictions don't enter into it.
  • It seems redundant. There are other ways to search the history that are, IMO, easier to use.
  • It's beep a beep massive beep annoyance beep if beep you beep want beep to beep enter beep a beep comment beep into beep your beep history. Beep.

So, I propose we remove this experimental SHOPT_EDPREDICT feature.

@JohnoKing, @hyenias, what say you?

@JohnoKing
Copy link

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
Copy link

hyenias commented Apr 22, 2021

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
Copy link
Author

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
Copy link

JohnoKing commented Apr 22, 2021

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
Copy link

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
Copy link

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
Copy link
Author

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

@McDutchie
Copy link

That character could easily be made configurable with a variable.

McDutchie added a commit that referenced this issue Apr 22, 2021
It's experimental, undocumented, at least somewhat broken, and gets
in the way if you type a comment. Should not be enabled by default,
at least not until someone steps up to fix it properly.

This commit also updates the descriptions of the option to clarify
that this provides a pattern-based history search menu. "Predictive
editing" is a misnomer as this does not predict anything.

Note that Solaris already disables it by default:
https://github.com/oracle/solaris-userland/blob/master/components/ksh93/patches/165-CR7186440_ksh93_disable_predictive_editing.patch

Discussion: #233
@McDutchie McDutchie added enhancement New feature or request and removed bug Something is not working question Further information is requested labels Apr 22, 2021
@McDutchie McDutchie changed the title Predictive editing (SHOPT_EDPREDICT) is broken History pattern search menu (SHOPT_EDPREDICT) needs work Apr 22, 2021
@McDutchie McDutchie added the help wanted Extra attention is needed label Apr 22, 2021
@hyenias
Copy link

hyenias commented Apr 22, 2021

@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
Copy link
Author

@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
Copy link

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
Copy link
Author

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

@dannyweldon
Copy link

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 McDutchie added the backburner Low priority (but feel free to fix it and do a PR) label Feb 17, 2022
@McDutchie
Copy link

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backburner Low priority (but feel free to fix it and do a PR) enhancement New feature or request help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

5 participants