Bug in vertical-motion vs overlay with display property

[Karl Chen]

Hi Eli/Emacs-devel,

I believe I've discovered a small bug in `vertical-motion' in
indent.c.  It affects movement past blank lines when there is an
overlay with a display/cursor property.

When using `fci-mode' from fill-column-indicator.el,
`previous-line' sometimes goes past two blank lines instead of
one.

Affected Emacs versions:
 - Good: 24.2
 - BAD:  24.3
 - BAD:  24.3.50 as of 2013-04-08

1. One can reproduce the problem using fill-column-indicator.el:

   $ wget 
https://raw.github.com/alpaker/Fill-Column-Indicator/master/fill-column-indicator.el
   $ emacs -Q

   M-x load-file fill-column-indicator.el
   M-x fci-mode
   <enter> <enter> <enter> <enter> <enter>
   <up>
   <up>  ;; BAD: this moves up 2 lines instead of 1 line


2. I minimized the above down to the following self-contained test
   case:

   $ cat > aa.el <<EOF
     (defun fci-redraw-region (start end _ignored)
       (save-match-data
         (save-excursion
           (goto-char start)
           (while
               (search-forward "\n" end t)
             (setq eol-str "X")
             (setq o (make-overlay (match-beginning 0) (match-beginning 0)))
             (overlay-put o 'after-string
                          (propertize eol-str 'display (propertize eol-str 
'cursor t)))
             ))))
     (add-hook 'after-change-functions 'fci-redraw-region)
     (insert "\n\n\n\n")
     (vertical-motion -1)
     (vertical-motion -1)
     (kill-emacs (+ 100 (line-number-at-pos)))
   EOF

   $ emacs-24.2 -nw -Q --load aa.el ; echo $?
   103  # good

   $ emacs-24.3 -nw -Q --load aa.el ; echo $?
   102  # BAD

   Note that `emacs --batch' doesn't reproduce the problem.


3. Using the above I bisected the changes between 24.2 and 24.3
   down to the following commit:

   commit b65a46be5055c338a9f8e7640ad97b8e592d5977
   Refs: HEAD, refs/bisect/bad
   Author:     Eli Zaretskii <address@hidden>
   AuthorDate: 2012-11-21 21:28:14 +0200
   Commit:     Eli Zaretskii <address@hidden>
   CommitDate: 2012-11-21 21:28:14 +0200

       Fix bug #12930 with vertical-motion through a display string.

        src/indent.c (Fvertical_motion): If the starting position is covered
        by a display string, return to one position before that, to avoid
        overshooting it inside move_it_to.
   ---
    src/ChangeLog | 6 ++++++
    src/indent.c  | 8 +++++++-
    2 files changed, 13 insertions(+), 1 deletion(-)

   diff --git a/src/ChangeLog b/src/ChangeLog
   index 020948e..c51f58a 100644
   --- a/src/ChangeLog
   +++ b/src/ChangeLog
   @@ -1,3 +1,9 @@
   +2012-11-21  Eli Zaretskii  <address@hidden>
   +
   +       * indent.c (Fvertical_motion): If the starting position is covered
   +       by a display string, return to one position before that, to avoid
   +       overshooting it inside move_it_to.  (Bug#12930)
   +
    2012-11-20  Daniel Colascione  <address@hidden>

           * w32fns.c (Fx_file_dialog):
   diff --git a/src/indent.c b/src/indent.c
   index bbc944d..3332228 100644
   --- a/src/indent.c
   +++ b/src/indent.c
   @@ -2057,7 +2057,13 @@ whether or not it is currently displayed in some 
window.  */)
              comment said this is "so we don't move too far" (2005-01-19
              checkin by kfs).  But this does nothing useful that I can
              tell, and it causes Bug#2694 .  -- cyd */
   -       move_it_to (&it, PT, -1, -1, -1, MOVE_TO_POS);
   +       /* When the position we started from is covered by a display
   +          string, move_it_to will overshoot it, while vertical-motion
   +          wants to put the cursor _before_ the display string.  So in
   +          that case, we move to buffer position before the display
   +          string, and avoid overshooting.  */
   +       move_it_to (&it, disp_string_at_start_p ? PT - 1 : PT,
   +                   -1, -1, -1, MOVE_TO_POS);

          /* IT may move too far if truncate-lines is on and PT lies
            beyond the right margin.  IT may also move too far if the

Thoughts?

Thanks,
Karl