1
0
Fork 0
mirror of git://git.code.sf.net/p/cdesktopenv/code synced 2025-02-15 04:32:24 +00:00

emacs: Fix three tab completion bugs

1. The editor accepted literal tabs without escaping in certain
   cases, causing buggy and inconsistent completion behaviour.
   https://github.com/ksh93/ksh/issues/71#issuecomment-656970959
   https://github.com/ksh93/ksh/issues/71#issuecomment-657216472

2. After completing a filename by choosing from a file completion
   menu, the terminal cursor was placed one position too far to the
   right, corrupting command line display. This happened with
   multiline active.
   https://github.com/ksh93/ksh/issues/71#issue-655093805

3. A completion menu was displayed if the file name to be completed
   was at the point where the rest of it started with a number,
   even if that part uniquely identified it so the menu had 1 item.
   https://www.mail-archive.com/ast-users@lists.research.att.com/msg00436.html

src/cmd/ksh93/edit/emacs.c:

- Cosmetic consistency: change two instances of cntl('[') to ESC.

- ed_emacsread(): Fix number 1 by refusing to continue into default
  processing if a tab character was not used for tab completion.
  Instead, beep and continue to the next read loop iteration. This
  behaviour is consistent with most other shells, so I doubt there
  will be objections. To enter a literal tab it's simple enough to
  escape it with ^V (the 'stty lnext' character) or \.

- draw(): Fix number 2 by correcting an off-by-one error in the
  ed_setcursor() call that updates the terminal's cursor display
  in multiline mode. The 'old' and 'new' parameters need to have
  identical values in this particular call to avoid the cursor
  position being off by one to the right. This change makes it
  match the corresponding ed_setcursor() call in vi.c. See below*
  for details. Thanks to Lev Kujawski for the help in analysing.

src/cmd/ksh93/edit/completion.c: ed_expand():

- Fix number 3 by changing from '=' mode (menu-based completion) to
  '\' mode (ordinary filename completion) if the menu would only
  show one option, which was pointless and annoying. This never
  happened in vi mode, so possibly the ed_expand() call in emacs.c
  could have been improved instead. But I'm comfortable with fixing
  it here and not in emacs.c, because this fixes it at a more
  fundamental level, plus it's straightforward and obvious here.

Resolves: https://github.com/ksh93/ksh/issues/71
____
* Further details on bug number 2:

At https://github.com/ksh93/ksh/issues/71#issuecomment-786391565
Martijn Dekker wrote:
> I'm back to my original hypothesis that there is somehow an
> off-by-one error related to the ed_setcursor() call that gets
> executed when in multiline mode. I cannot confirm whether that
> off-by-one error is actually in the call itself, or occurs
> sometime earlier on one of the many possible occasions where
> ep->cursor is changed. But everything else appears to work
> correctly, so it's not unlikely that the problem is in the call
> itself.
>
> For reference, this is the original version of that call in
> emacs.c:
>
> ksh/src/cmd/ksh93/edit/emacs.c
> Lines 1556 to 1557 in df2b9bf
>  if(ep->ed->e_multiline && option == REFRESH)
>  	ed_setcursor(ep->ed, ep->screen, ep->cursor-ep->screen, ep->ed->e_peol, -1);
>
> There is a corresponding call in the vi.c refresh() function
> (which does the same thing as draw() in emacs.c), where the third
> (old) and fourth (new) arguments are actually identical:
>
> ksh/src/cmd/ksh93/edit/vi.c
>
> Lines 2086 to 2087 in df2b9bf
>  if(vp->ed->e_multiline && vp->ofirst_wind==INVALID)
>  	ed_setcursor(vp->ed, physical, last_phys+1, last_phys+1, -1);
>
> The expectation for this particular call is in fact that they
> should be identical, so that a delta of zero is calculated in
> that function. Delta not being zero is what causes the cursor to
> be positioned wrong.
>
> In vi.c, last_phys is a macro that is defined as editb.e_peol,
> and editb is a macro that is defined as (*vp->ed). Which means
> last_phys means vp->ed->e_peol, which is the same as
> ep->ed->e_peol in emacs.c. (These editors were originally
> separate programs by different authors, and I suppose this is how
> it shows. Korn didn't want to change all the variable names to
> integrate them, so made macros instead.)
>
> That leaves the question of why vi.c adds 1 to both last_phys
> a.k.a. e_peol arguments, and emacs.c uses e_peol for new without
> adding anything. Analysing the ed_setcursor() code could answer
> that question.
>
> So, this patch makes emacs.c do it the same way vi.c does. Let's
> make the third argument identical to the fourth. My brief testing
> shows the bug is fixed, and the regression tests yield no
> failures. This fix is also the most specific change possible, so
> there are few opportunities for side effects (I hope).

At https://github.com/ksh93/ksh/issues/71#issuecomment-786466652
Lev Kujawski wrote:
> I did a bit of research on this, and I think the fix to have the
> Emacs editing mode do the same as Vi is correct.
>
> From RELEASE:
> 08-05-01 In multiline edit mode, the refresh operation will now clear
> the remaining portion of the last line.
>
> Here's a fragment from the completion.c of the venerable but
> dated CDE DtKsh:
>
>                 else
>                         while (*com)
>                         {
>                                 *out++  = ' ';
>                                 out = strcopy(out,*com++);
>                         }
>                 *cur = (out-outbuff);
>                 /* restore rest of buffer */
>                 out = strcopy(out,stakptr(0));
>                 *eol = (out-outbuff);
>
> Noticeably missing is the code to add a space after file name
> completions. So, it seems plausible that if multiline editing
> mode was added beforehand,the ep->ed->p_eol !=
> ep->cursor-ep->screen case might never have occurred during
> testing.
>
> Setting the 'first' parameter to -1 seems to be a pretty explicit
> indicator that the author(s) intended the line clearing code to
> run, hence the entry in RELASE.
>
> The real issue is that if we update the cursor by calling
> ed_setcursor on line 1554 with old != new, the later call to
> setcursor on line 1583, here:
>
> 	I = (ncursor-nscreen) - ep->offset;
> 	setcursor(ep,i,0);
>
> will use outdated screen information to call setcursor, which,
> coincidentally, calls ed_setcursor.
This commit is contained in:
Martijn Dekker 2021-02-26 11:20:58 +00:00
parent df2b9bf67f
commit d9865ceae1
4 changed files with 29 additions and 5 deletions

20
NEWS
View file

@ -3,6 +3,26 @@ For full details, see the git log at: https://github.com/ksh93/ksh
Any uppercase BUG_* names are modernish shell bug IDs.
2021-02-26:
- Fixed three long-standing bugs with tab completion in the emacs editor:
1. The editor accepted literal tabs without escaping in certain cases,
causing buggy and inconsistent completion behaviour. Details:
https://github.com/ksh93/ksh/issues/71#issuecomment-656970959
https://github.com/ksh93/ksh/issues/71#issuecomment-657216472
To enter a literal tab in emacs, you need to escape it with ^V or \.
2. After completing a filename by choosing from a file completion menu,
the terminal cursor was placed one position too far to the right,
corrupting command line display. This happened with multiline active.
Details: https://github.com/ksh93/ksh/issues/71#issue-655093805
3. A completion menu was displayed if the file name to be completed was
at the point where the rest of it started with a number, even if that
part uniquely identified it so the menu only showed one item. Details:
https://www.mail-archive.com/ast-users@lists.research.att.com/msg00436.html
2021-02-21:
- Fixed: The way that SIGWINCH was handled (i.e. the signal emitted when the

View file

@ -343,6 +343,8 @@ int ed_expand(Edit_t *ep, char outbuff[],int *cur,int *eol,int mode, int count)
}
if(mode=='\\' && out[-1]=='/' && narg>1)
mode = '=';
else if(mode=='=' && narg<2)
mode = '\\'; /* no filename menu if there is only one choice */
if(mode=='=')
{
if (strip && !cmd_completion)

View file

@ -371,6 +371,8 @@ int ed_emacsread(void *context, int fd,char *buff,int scend, int reedit)
}
ep->ed->e_tabcount = 0;
}
beep();
continue;
do_default_processing:
default:
@ -617,7 +619,7 @@ update:
ed_crlf(ep->ed);
draw(ep,REFRESH);
continue;
case cntl('[') :
case ESC :
vt220_save_repeat = oadjust;
do_escape:
adjust = escape(ep,out,oadjust);
@ -984,7 +986,7 @@ static int escape(register Emacs_t* ep,register genchar *out,int count)
ed_ungetchar(ep->ed,'\n');
#endif /* SHOPT_EDPREDICT */
/* file name expansion */
case cntl('[') : /* filename completion */
case ESC :
#if SHOPT_EDPREDICT
if(ep->ed->hlist)
{
@ -1000,7 +1002,7 @@ static int escape(register Emacs_t* ep,register genchar *out,int count)
}
}
#endif /* SHOPT_EDPREDICT */
i = '\\';
i = '\\'; /* filename completion */
case '*': /* filename expansion */
case '=': /* escape = - list all matching file names */
ep->mark = cur;
@ -1554,7 +1556,7 @@ static void draw(register Emacs_t *ep,Draw_t option)
#endif /* SHOPT_MULTIBYTE */
}
if(ep->ed->e_multiline && option == REFRESH)
ed_setcursor(ep->ed, ep->screen, ep->cursor-ep->screen, ep->ed->e_peol, -1);
ed_setcursor(ep->ed, ep->screen, ep->ed->e_peol, ep->ed->e_peol, -1);
/******************

View file

@ -20,7 +20,7 @@
#define SH_RELEASE_FORK "93u+m" /* only change if you develop a new ksh93 fork */
#define SH_RELEASE_SVER "1.0.0-alpha" /* semantic version number: https://semver.org */
#define SH_RELEASE_DATE "2021-02-21" /* must be in this format for $((.sh.version)) */
#define SH_RELEASE_DATE "2021-02-26" /* must be in this format for $((.sh.version)) */
#define SH_RELEASE_CPYR "(c) 2020-2021 Contributors to ksh " SH_RELEASE_FORK
/* Scripts sometimes field-split ${.sh.version}, so don't change amount of whitespace. */