bug#25767: 25.1.91; abbrev-prefix-mark expansion inserts newline

bug#25767: 25.1.91; abbrev-prefix-mark expansion inserts newline

[Allen Li]

Expanding an abbrev while using abbrev-prefix-mark inserts a newline.  Probably
this is best explained through a repro:

1. Start emacs -Q -nw
2. Go to scratch buffer
3. Define an abbrev from f to food: (type food, C-x a l f RET)
4. M-x abbrev-mode
5. Move point so it's at the asterisk:

  ;; This buffer is for text that is not saved, and for Lisp evaluation.
  ;; To create a file, visit it with C-x C-f and enter text in its buffer.

  *

  food

6. Type s M-' f SPC

Expected:

  ;; This buffer is for text that is not saved, and for Lisp evaluation.
  ;; To create a file, visit it with C-x C-f and enter text in its buffer.

  sfood *

  food

Actual:

  ;; This buffer is for text that is not saved, and for Lisp evaluation.
  ;; To create a file, visit it with C-x C-f and enter text in its buffer.

  sfood
   *

  food

In GNU Emacs 25.1.91.1 (x86_64-pc-linux-gnu, GTK+ Version 3.10.8),
modified by Debian
System Description: Ubuntu 14.04 LTS

Configured using:
 'configure --build x86_64-linux-gnu --build x86_64-linux-gnu
 --prefix=/usr --sharedstatedir=/var/lib --libexecdir=/usr/lib
 --localstatedir=/var/lib --infodir=/usr/share/info
 --mandir=/usr/share/man --with-pop=yes
 --enable-locallisppath=/etc/google-emacs:/etc/emacs:/usr/local/share/emacs/25.1.91+gg1+2/site-lisp:/usr/local/share/emacs/site-lisp:/usr/share/emacs/25.1.91+gg1+2/site-lisp:/usr/share/emacs/site-lisp
 --with-crt-dir=/usr/lib/x86_64-linux-gnu --disable-build-details
 --disable-silent-rules --with-modules GOOGLE_VERSION=25.1.91+gg1+2
 --with-x=yes --with-x-toolkit=gtk3 --with-toolkit-scroll-bars
 build_alias=x86_64-linux-gnu 'CFLAGS=-g -O2 -fstack-protector
 --param=ssp-buffer-size=4 -Wformat -Werror=format-security -Wall'
 'LDFLAGS=-Wl,-Bsymbolic-functions -Wl,-z,relro
 -Wl,-fuse-ld=gold,--export-dynamic-symbol=__google_auxv'
 'CPPFLAGS=-D_FORTIFY_SOURCE=2 -DGOOGLE_EMACS_DEFINE_AUXV''

Configured features:
XPM JPEG TIFF GIF PNG RSVG IMAGEMAGICK SOUND GPM DBUS GCONF GSETTINGS
NOTIFY LIBSELINUX GNUTLS LIBXML2 FREETYPE M17N_FLT LIBOTF XFT ZLIB
TOOLKIT_SCROLL_BARS GTK3 X11 MODULES

Important settings:
  value of $LANG: en_US.UTF-8
  value of $XMODIFIERS: @im=ibus
  locale-coding-system: utf-8-unix

Major mode: Lisp Interaction

Minor modes in effect:
  tooltip-mode: t
  global-eldoc-mode: t
  electric-indent-mode: t
  mouse-wheel-mode: t
  tool-bar-mode: t
  menu-bar-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  font-lock-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t
  line-number-mode: t
  transient-mark-mode: t
  abbrev-mode: t

Recent messages:
For information about GNU Emacs and the GNU system, type C-h C-a.
Abbrev mode enabled in current buffer
next-line: End of buffer [3 times]

Load-path shadows:
None found.

Features:
(shadow sort mail-extr emacsbug message dired format-spec rfc822 mml
mml-sec password-cache epg epg-config gnus-util mm-decode mm-bodies
mm-encode mail-parse rfc2231 mailabbrev gmm-utils mailheader sendmail
rfc2047 rfc2045 ietf-drums mm-util help-fns help-mode easymenu
cl-loaddefs pcase cl-lib mail-prsvr mail-utils term/xterm xterm
time-date mule-util tooltip eldoc electric uniquify ediff-hook vc-hooks
lisp-float-type mwheel x-win term/common-win x-dnd tool-bar dnd fontset
image regexp-opt fringe tabulated-list newcomment elisp-mode lisp-mode
prog-mode register page menu-bar rfn-eshadow timer select scroll-bar
mouse jit-lock font-lock syntax facemenu font-core frame cl-generic cham
georgian utf-8-lang misc-lang vietnamese tibetan thai tai-viet lao
korean japanese eucjp-ms cp51932 hebrew greek romanian slovak czech
european ethiopic indian cyrillic chinese charscript case-table epa-hook
jka-cmpr-hook help simple abbrev minibuffer cl-preloaded nadvice
loaddefs button faces cus-face macroexp files text-properties overlay
sha1 md5 base64 format env code-pages mule custom widget
hashtable-print-readable backquote dbusbind inotify dynamic-setting
system-font-setting font-render-setting move-toolbar gtk x-toolkit x
multi-tty make-network-process emacs)

Memory information:
((conses 16 91659 6500)
 (symbols 48 20147 0)
 (miscs 40 42 146)
 (strings 32 14851 4772)
 (string-bytes 1 422197)
 (vectors 16 9899)
 (vector-slots 8 393090 9637)
 (floats 8 169 482)
 (intervals 56 254 0)
 (buffers 976 19)
 (heap 1024 8591 1066))

bug#25767: 25.1.91; abbrev-prefix-mark expansion inserts newline

[Allen Li]

The behavior was bugging me so I looked into it.  The root cause is
that `abbrev--before-point' does not adjust `pos' to account for when
it deletes the "-" left by abbrev-prefix-mark.  Therefore, when
`abbrev-before-point' goes to restore point, it moves point one
character too far forward (in the repro example, moving past the
newline).

The fix is really easy, but I haven't contributed to Emacs before, so
I don't know the proper procedure.  (Also, the existing code really
bugs me; I would tear it out and replace it with (with-excursion) if
Emacs was my pet project that millions of users aren't depending on.)

bug#25767: 25.1.91; abbrev-prefix-mark expansion inserts newline

[Eli Zaretskii]

> From: Allen Li <vianchielfaura@gmail.com>
> Date: Fri, 17 Feb 2017 14:15:07 -0800
> 
> The fix is really easy, but I haven't contributed to Emacs before, so
> I don't know the proper procedure.

The simplest way is to post the patch, as "diff -u output", here.  If
it's small enough, that's all you need.  Bonus points for following
the advice in CONTRIBUTE, which you can find in the Emacs repository
and in new enough release tarballs.  Running the relevant tests in the
Emacs test suite (abbrev-tests.el) to make sure the changes cause no
regressions is also a plus.

> (Also, the existing code really bugs me; I would tear it out and
> replace it with (with-excursion) if Emacs was my pet project that
> millions of users aren't depending on.)

You could post 2 alternative patches (assuming the more radical one
passes all the tests in the test suite), and let us judge.

Thanks.

bug#25767: [PATCH] 25.1.91; abbrev-prefix-mark expansion inserts newline

[Allen Li]

Here's the quick fix.  I will opt out of doing more invasive refactoring.

I tried running abbrev-tests.el, but they don't seem to pass, even
without my change.  I am running these tests by: (eval-buffer)
followed by (ert).

I am getting undefined variable errors that I can fix with (require
'obarray).  Am I running these tests incorrectly or should I add a
(require 'obarray) to abbrev-tests.el?

I am also getting a type error for abbrev-table-p-test unrelated to my change.

There appear to be no tests for the actual abbrev expansion.

---
 lisp/abbrev.el | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/lisp/abbrev.el b/lisp/abbrev.el
index cbc604c23d..01ad3d478f 100644
--- a/lisp/abbrev.el
+++ b/lisp/abbrev.el
@@ -720,9 +720,10 @@ then ABBREV is looked up in that table only."
           (setq start abbrev-start-location)
           (setq abbrev-start-location nil)
           ;; Remove the hyphen inserted by `abbrev-prefix-mark'.
-          (if (and (< start (point-max))
-                   (eq (char-after start) ?-))
-              (delete-region start (1+ start)))
+          (when (and (< start (point-max))
+                     (eq (char-after start) ?-))
+            (delete-region start (1+ start))
+            (setq pos (1- pos)))
           (skip-syntax-backward " ")
           (setq end (point))
           (when (> end start)
-- 
2.11.1

bug#25767: [PATCH] 25.1.91; abbrev-prefix-mark expansion inserts newline

[Noam Postavsky]

On Fri, Feb 24, 2017 at 12:34 AM, Allen Li <vianchielfaura@gmail.com> wrote:
>
> I tried running abbrev-tests.el, but they don't seem to pass, even
> without my change.  I am running these tests by: (eval-buffer)
> followed by (ert).
>
> I am getting undefined variable errors that I can fix with (require
> 'obarray).  Am I running these tests incorrectly or should I add a
> (require 'obarray) to abbrev-tests.el?

The tests pass for me unchanged, both the "official way": cd into
tests/ and then 'make abbrev-tests', and also M-x eval-buffer M-x ert.

bug#25767: [PATCH] 25.1.91; abbrev-prefix-mark expansion inserts newline

[Allen Li]

On Fri, Feb 24, 2017 at 6:26 AM, Noam Postavsky
<npostavs@users.sourceforge.net> wrote:
> The tests pass for me unchanged, both the "official way": cd into
> tests/ and then 'make abbrev-tests', and also M-x eval-buffer M-x ert.

Thanks, the official way works.  I guess eval-buffer+ert wasn't
working for me because I wasn't running a fresh build.

bug#25767: [PATCH] 25.1.91; abbrev-prefix-mark expansion inserts newline

[npostavs]

Allen Li <vianchielfaura@gmail.com> writes:

> On Fri, Feb 24, 2017 at 6:26 AM, Noam Postavsky
> <npostavs@users.sourceforge.net> wrote:
>> The tests pass for me unchanged, both the "official way": cd into
>> tests/ and then 'make abbrev-tests', and also M-x eval-buffer M-x ert.
>
> Thanks, the official way works.  I guess eval-buffer+ert wasn't
> working for me because I wasn't running a fresh build.

Were you possibly running the tests from master using an Emacs from
emacs-25?  abbrev.el has a dependency on obarray.el that is new in
master.

bug#25767: [PATCH] 25.1.91; abbrev-prefix-mark expansion inserts newline

[Allen Li]

Yep.  I'll be more careful to work on a fresh build in the future.

On Mon, Feb 27, 2017 at 8:47 PM,  <npostavs@users.sourceforge.net> wrote:
> Allen Li <vianchielfaura@gmail.com> writes:
>
>> On Fri, Feb 24, 2017 at 6:26 AM, Noam Postavsky
>> <npostavs@users.sourceforge.net> wrote:
>>> The tests pass for me unchanged, both the "official way": cd into
>>> tests/ and then 'make abbrev-tests', and also M-x eval-buffer M-x ert.
>>
>> Thanks, the official way works.  I guess eval-buffer+ert wasn't
>> working for me because I wasn't running a fresh build.
>
> Were you possibly running the tests from master using an Emacs from
> emacs-25?  abbrev.el has a dependency on obarray.el that is new in
> master.

bug#25767: [PATCH] 25.1.91; abbrev-prefix-mark expansion inserts newline

[npostavs]

tags 25767 fixed
close 25767 26.1
quit

Allen Li <vianchielfaura@gmail.com> writes:

> Here's the quick fix.  I will opt out of doing more invasive refactoring.

Pushed to master [1: c733d9169c].

1: 2017-03-02 19:01:18 -0500 c733d9169ce44f5600d41cf0e67e021371954c8e
  Stop abbrev-prefix-mark from adding extra newline (Bug#25767)

bug#25767: [PATCH] 25.1.91; abbrev-prefix-mark expansion inserts newline

[Eli Zaretskii]

> From: npostavs@users.sourceforge.net
> Cc: Eli Zaretskii <eliz@gnu.org>,  25767@debbugs.gnu.org
> Date: Thu, 02 Mar 2017 19:06:51 -0500
> 
> > Here's the quick fix.  I will opt out of doing more invasive refactoring.
> 
> Pushed to master [1: c733d9169c].
> 
> 1: 2017-03-02 19:01:18 -0500 c733d9169ce44f5600d41cf0e67e021371954c8e
>   Stop abbrev-prefix-mark from adding extra newline (Bug#25767)

This seems to fail my build of master:

  Loading abbrev.el (source)...
  Eager macro-expansion failure: (error "(require cl-lib) while preparing to dump")
  Eager macro-expansion failure: (error "(require cl-lib) while preparing to dump")
  (require cl-lib) while preparing to dump

bug#25767: [PATCH] 25.1.91; abbrev-prefix-mark expansion inserts newline

[Eli Zaretskii]

> Date: Fri, 03 Mar 2017 10:39:43 +0200
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: vianchielfaura@gmail.com, 25767@debbugs.gnu.org
> 
> > 1: 2017-03-02 19:01:18 -0500 c733d9169ce44f5600d41cf0e67e021371954c8e
> >   Stop abbrev-prefix-mark from adding extra newline (Bug#25767)
> 
> This seems to fail my build of master:
> 
>   Loading abbrev.el (source)...
>   Eager macro-expansion failure: (error "(require cl-lib) while preparing to dump")
>   Eager macro-expansion failure: (error "(require cl-lib) while preparing to dump")
>   (require cl-lib) while preparing to dump

False alarm: this was probably caused by changes in pp.el or whatever.
After manually compiling the new Lisp files, the build succeeds.

Sorry for the noise.