bug#20774: auto-fill doesn't work properly when first-line prefix differs in adaptive-fill-mode

bug#20774: auto-fill doesn't work properly when first-line prefix differs in adaptive-fill-mode

[Samuel Freilich]

[-- Attachment #1.1: Type: text/plain, Size: 1711 bytes --]

The function do-auto-fill doesn't consider that in adaptive-fill-mode, the
first line's prefix might not match the fill-prefix for subsequent lines.

For example, markdown-mode configures adaptive-fill-mode so that when
filling list item paragraphs, there's a hanging indent:

*   List item...
    next line...

Currently, there's a bug where auto-fill-mode will break lines at the start
of a list item, even though the resulting line is just as long, e.g.:

*   [link](http://a-really-long-url.example.com/..
<http://a-really-long-url.example.com/>.)

When you hit enter at the end of the line in markdown-mode with autofill on
and the fill-column set to less than the length of that line, it becomes:

*
   [link](http://a-really-long-url.example.com/..
<http://a-really-long-url.example.com/>.)

That's incorrect behavior because the prefix added after breaking the line
is just as long as the line up to the point before the break.

The attached patch fixes this by ensuring that a line will be broken by
auto-fill only if the position is further along in the line than the length
of the fill prefix. It also ensures that the first-line fill prefix is
skipped when finding a point to break (that is, the line should not be
broken in the middle of the prefix), even if the first-line prefix differs
from the fill-prefix for subsequent lines.


*Detailed Reproduction Steps:*

Get markdown-mode from:
http://jblevins.org/projects/markdown-mode/markdown-mode.el

$ emacs -Q -l markdown-mode.el
M-x markdown-mode RET
M-x auto-fill-mode RET
M-x set-fill-column RET 5 RET

Type the following into the buffer and hit RET:
* Item

Actual result:
*
  Item
  [cursor]

Expected result:
* Item
  [cursor]

Peace,
-Sam

[-- Attachment #1.2: Type: text/html, Size: 3051 bytes --]

[-- Attachment #2: emacs-auto-fill-different-first-line.patch --]
[-- Type: text/x-patch, Size: 2610 bytes --]

*** simple.el.old	2015-06-08 19:19:20.484397387 -0400
--- simple.el.new	2015-06-08 19:18:58.340305301 -0400
***************
*** 6559,6575 ****
  	       (setq fill-prefix prefix))))
  
        (while (and (not give-up) (> (current-column) fc))
! 	;; Determine where to split the line.
! 	(let* (after-prefix
  	       (fill-point
  		(save-excursion
  		  (beginning-of-line)
! 		  (setq after-prefix (point))
! 		  (and fill-prefix
! 		       (looking-at (regexp-quote fill-prefix))
! 		       (setq after-prefix (match-end 0)))
  		  (move-to-column (1+ fc))
! 		  (fill-move-to-break-point after-prefix)
  		  (point))))
  
  	  ;; See whether the place we found is any good.
--- 6559,6581 ----
  	       (setq fill-prefix prefix))))
  
        (while (and (not give-up) (> (current-column) fc))
!         ;; Determine where to split the line.
!         (let* (line-start
!                after-prefix
  	       (fill-point
  		(save-excursion
  		  (beginning-of-line)
! 		  (setq line-start (point))
!       ;; Skip the fill-prefix. Note that we might be on the first line of the
!       ;; paragraph, and the fist line can differ in adaptive-fill-mode.
!       (when (or (and adaptive-fill-mode
!                      adaptive-fill-first-line-regexp
!                      (looking-at adaptive-fill-first-line-regexp))
!                 (and fill-prefix
!                      (looking-at (regex-quote fill-prefix))))
!         (setq after-prefix (match-end 0)))
  		  (move-to-column (1+ fc))
! 		  (fill-move-to-break-point (or after-prefix line-start))
  		  (point))))
  
  	  ;; See whether the place we found is any good.
***************
*** 6578,6586 ****
  		(or (bolp)
  		    ;; There is no use breaking at end of line.
  		    (save-excursion (skip-chars-forward " ") (eolp))
! 		    ;; It is futile to split at the end of the prefix
! 		    ;; since we would just insert the prefix again.
! 		    (and after-prefix (<= (point) after-prefix))
  		    ;; Don't split right after a comment starter
  		    ;; since we would just make another comment starter.
  		    (and comment-start-skip
--- 6584,6592 ----
  		(or (bolp)
  		    ;; There is no use breaking at end of line.
  		    (save-excursion (skip-chars-forward " ") (eolp))
! 		    ;; It is futile to split earlier in the line than the length of the
! 		    ;; prefix, since we would just insert the prefix again.
! 		    (and fill-prefix (<= (point) (+ line-start (length fill-prefix))))
  		    ;; Don't split right after a comment starter
  		    ;; since we would just make another comment starter.
  		    (and comment-start-skip

bug#20774: Typo in Patch

[Samuel Freilich]

[-- Attachment #1.1: Type: text/plain, Size: 113 bytes --]

There was a typo in that previous patch. Should have been regexp-quote, not
regex-quote. Fixed version attached.

[-- Attachment #1.2: Type: text/html, Size: 134 bytes --]

[-- Attachment #2: emacs-auto-fill-different-first-line-fixed.patch --]
[-- Type: text/x-patch, Size: 2611 bytes --]

*** simple.el.old	2015-06-08 19:19:20.484397387 -0400
--- simple.el.new	2015-06-08 19:18:58.340305301 -0400
***************
*** 6559,6575 ****
  	       (setq fill-prefix prefix))))
  
        (while (and (not give-up) (> (current-column) fc))
! 	;; Determine where to split the line.
! 	(let* (after-prefix
  	       (fill-point
  		(save-excursion
  		  (beginning-of-line)
! 		  (setq after-prefix (point))
! 		  (and fill-prefix
! 		       (looking-at (regexp-quote fill-prefix))
! 		       (setq after-prefix (match-end 0)))
  		  (move-to-column (1+ fc))
! 		  (fill-move-to-break-point after-prefix)
  		  (point))))
  
  	  ;; See whether the place we found is any good.
--- 6559,6581 ----
  	       (setq fill-prefix prefix))))
  
        (while (and (not give-up) (> (current-column) fc))
!         ;; Determine where to split the line.
!         (let* (line-start
!                after-prefix
  	       (fill-point
  		(save-excursion
  		  (beginning-of-line)
! 		  (setq line-start (point))
!       ;; Skip the fill-prefix. Note that we might be on the first line of the
!       ;; paragraph, and the fist line can differ in adaptive-fill-mode.
!       (when (or (and adaptive-fill-mode
!                      adaptive-fill-first-line-regexp
!                      (looking-at adaptive-fill-first-line-regexp))
!                 (and fill-prefix
!                      (looking-at (regexp-quote fill-prefix))))
!         (setq after-prefix (match-end 0)))
  		  (move-to-column (1+ fc))
! 		  (fill-move-to-break-point (or after-prefix line-start))
  		  (point))))
  
  	  ;; See whether the place we found is any good.
***************
*** 6578,6586 ****
  		(or (bolp)
  		    ;; There is no use breaking at end of line.
  		    (save-excursion (skip-chars-forward " ") (eolp))
! 		    ;; It is futile to split at the end of the prefix
! 		    ;; since we would just insert the prefix again.
! 		    (and after-prefix (<= (point) after-prefix))
  		    ;; Don't split right after a comment starter
  		    ;; since we would just make another comment starter.
  		    (and comment-start-skip
--- 6584,6592 ----
  		(or (bolp)
  		    ;; There is no use breaking at end of line.
  		    (save-excursion (skip-chars-forward " ") (eolp))
! 		    ;; It is futile to split earlier in the line than the length of the
! 		    ;; prefix, since we would just insert the prefix again.
! 		    (and fill-prefix (<= (point) (+ line-start (length fill-prefix))))
  		    ;; Don't split right after a comment starter
  		    ;; since we would just make another comment starter.
  		    (and comment-start-skip

bug#20774: auto-fill doesn't work properly when first-line prefix differs in adaptive-fill-mode

[npostavs]

Samuel Freilich <sfreilich@google.com> writes:

> $ emacs -Q -l markdown-mode.el
> M-x markdown-mode RET
> M-x auto-fill-mode RET
> M-x set-fill-column RET 5 RET
>
> Type the following into the buffer and hit RET:
> * Item
>
> Actual result:
> *
>   Item
>   [cursor]
>
> Expected result:
> * Item
>   [cursor]

With the patch I get

* Item
[cursor]

i.e, the cursor lands in column 0.  Is it correct?

bug#20774: auto-fill doesn't work properly when first-line prefix differs in adaptive-fill-mode

[Samuel Freilich]

[-- Attachment #1: Type: text/plain, Size: 739 bytes --]

Yes, that's correct. Sorry for getting that wrong in my initial description
of the bug. That's the same as the indentation behavior with auto-fill-mode
disabled. A hanging indent is not assumed with only one line.

On Tue, Aug 22, 2017 at 8:49 AM, <npostavs@users.sourceforge.net> wrote:

> Samuel Freilich <sfreilich@google.com> writes:
>
> > $ emacs -Q -l markdown-mode.el
> > M-x markdown-mode RET
> > M-x auto-fill-mode RET
> > M-x set-fill-column RET 5 RET
> >
> > Type the following into the buffer and hit RET:
> > * Item
> >
> > Actual result:
> > *
> >   Item
> >   [cursor]
> >
> > Expected result:
> > * Item
> >   [cursor]
>
> With the patch I get
>
> * Item
> [cursor]
>
> i.e, the cursor lands in column 0.  Is it correct?
>

[-- Attachment #2: Type: text/html, Size: 1233 bytes --]

bug#20774: auto-fill doesn't work properly when first-line prefix differs in adaptive-fill-mode

[npostavs]

Samuel Freilich <sfreilich@google.com> writes:

> Yes, that's correct. Sorry for getting that wrong in my initial
> description of the bug. That's the same as the indentation behavior
> with auto-fill-mode disabled. A hanging indent is not assumed with
> only one line.

Ah okay.  The patch looks fine (apart from the indentation being off, I
assume because you had tab-width = 4).  Would you mind adding a commit
message?  (See CONTRIBUTE for the format.)

If you're feeling up to it, a test would be nice too.

Since you're sending from a google.com address, I understand the
copyright assignment is covered under Google's blanket agreement, is
that right?

bug#20774: auto-fill doesn't work properly when first-line prefix differs in adaptive-fill-mode

[Samuel Freilich]

[-- Attachment #1.1: Type: text/plain, Size: 1783 bytes --]

tab-width = 4 seems consistent with how the rest of the file is formatted.
I didn't want to change the spacing in lines not touched by my diff.

I added tests and a change description, and formatted the patch according
to the current instructions. I made the patch a bit more minimal. I pruned
the part that dealt with adaptive-fill-first-line-regexp didn't work as
well as expected, since that didn't work as well as expected (it didn't
deal with all the complexity possible with adaptive-fill-function). The
updated version at least handles cases where the fill-prefix isn't shorter
than the first-line prefix. That allowed me to simplify the code quite a
bit, since that makes the previous logic for skipping the exact fill-prefix
redundant, fill-move-to-break-point already handles the logic of trying to
skip at least one word after the start position passed to it. Please take
another look.

You're correct about copyright. I'm doing this for work, copyright is
covered by whatever agreements Google has made.



On Tue, Aug 22, 2017 at 11:56 PM, <npostavs@users.sourceforge.net> wrote:

> Samuel Freilich <sfreilich@google.com> writes:
>
> > Yes, that's correct. Sorry for getting that wrong in my initial
> > description of the bug. That's the same as the indentation behavior
> > with auto-fill-mode disabled. A hanging indent is not assumed with
> > only one line.
>
> Ah okay.  The patch looks fine (apart from the indentation being off, I
> assume because you had tab-width = 4).  Would you mind adding a commit
> message?  (See CONTRIBUTE for the format.)
>
> If you're feeling up to it, a test would be nice too.
>
> Since you're sending from a google.com address, I understand the
> copyright assignment is covered under Google's blanket agreement, is
> that right?
>

[-- Attachment #1.2: Type: text/html, Size: 2616 bytes --]

[-- Attachment #2: 0001-Do-not-split-line-before-length-of-fill-prefix.patch --]
[-- Type: text/x-patch, Size: 3279 bytes --]

From b098b41ca457760a6ddde633cac37e70fbc8aeb9 Mon Sep 17 00:00:00 2001
From: Samuel Freilich <sfreilich@google.com>
Date: Wed, 23 Aug 2017 13:40:45 -0400
Subject: [PATCH] Do not split line before length of fill-prefix

Previous logic already avoided splitting the line immediately after
the exact fill prefix, but in adaptive-fill-mode, the fill-prefix may
not be the same as the prefix of the first line of a paragraph. That
resulted in auto-fill-mode breaking lines in such a way as to make the
subsequent line as long or longer (Bug#20774).
---
 lisp/simple.el            | 26 +++++++++++---------------
 test/lisp/simple-tests.el | 11 +++++++++++
 2 files changed, 22 insertions(+), 15 deletions(-)

diff --git a/lisp/simple.el b/lisp/simple.el
index 072723cd64..a1ba46f586 100644
--- a/lisp/simple.el
+++ b/lisp/simple.el
@@ -7148,18 +7148,17 @@ do-auto-fill
 	       (setq fill-prefix prefix))))
 
       (while (and (not give-up) (> (current-column) fc))
-	;; Determine where to split the line.
-	(let* (after-prefix
-	       (fill-point
-		(save-excursion
-		  (beginning-of-line)
-		  (setq after-prefix (point))
-		  (and fill-prefix
-		       (looking-at (regexp-quote fill-prefix))
-		       (setq after-prefix (match-end 0)))
-		  (move-to-column (1+ fc))
-		  (fill-move-to-break-point after-prefix)
-		  (point))))
+        ;; Determine where to split the line.
+        (let ((fill-point
+               (save-excursion
+                 (beginning-of-line)
+                 (let ((line-start (point)))
+                    (move-to-column (1+ fc))
+                    ;; Don't split earlier in the line than the length of the fill
+                    ;; prefix, since the resulting line would be longer.
+                    (fill-move-to-break-point (min (line-end-position)
+                                                   (+ line-start (length fill-prefix))))
+                    (point)))))
 
 	  ;; See whether the place we found is any good.
 	  (if (save-excursion
@@ -7167,9 +7166,6 @@ do-auto-fill
 		(or (bolp)
 		    ;; There is no use breaking at end of line.
 		    (save-excursion (skip-chars-forward " ") (eolp))
-		    ;; It is futile to split at the end of the prefix
-		    ;; since we would just insert the prefix again.
-		    (and after-prefix (<= (point) after-prefix))
 		    ;; Don't split right after a comment starter
 		    ;; since we would just make another comment starter.
 		    (and comment-start-skip
diff --git a/test/lisp/simple-tests.el b/test/lisp/simple-tests.el
index ad7aee1db1..c7330e4034 100644
--- a/test/lisp/simple-tests.el
+++ b/test/lisp/simple-tests.el
@@ -497,5 +497,16 @@ simple-test-undo-with-switched-buffer
       (should (equal (line-number-at-pos 5) 3))
       (should (equal (line-number-at-pos 7) 4)))))
 
+(ert-deftest auto-fill-mode-no-break-before-length-of-fill-prefix ()
+  (with-temp-buffer
+    (setq-local fill-prefix "   ")
+    (set-fill-column 5)
+    ;; Shouldn't break after 'foo' (3 characters) when the next
+    ;; line is indented >= to that, that woudln't result in shorter
+    ;; lines.
+    (insert "foo bar")
+    (do-auto-fill)
+    (should (string-equal (buffer-string) "foo bar"))))
+
 (provide 'simple-test)
 ;;; simple-test.el ends here
-- 
2.14.1.342.g6490525c54-goog

bug#20774: auto-fill doesn't work properly when first-line prefix differs in adaptive-fill-mode

[npostavs]

Samuel Freilich <sfreilich@google.com> writes:

> tab-width = 4 seems consistent with how the rest of the file is formatted.
> I didn't want to change the spacing in lines not touched by my diff.

Hmm, somehow the code that landed in my buffer looked wrongly indented.
Something got messed up somewhere (possibly by the email system,
possibly during patch application, I've been experimenting with various
forms of automation for that), but since the indentation in your next
patch looks fine there's not really much point in worrying about it.

> I added tests and a change description, and formatted the patch according
> to the current instructions.

Thanks!  

> I made the patch a bit more minimal. I pruned the part that dealt with
> adaptive-fill-first-line-regexp didn't work as well as expected, since
> that didn't work as well as expected (it didn't deal with all the
> complexity possible with adaptive-fill-function). The updated version
> at least handles cases where the fill-prefix isn't shorter than the
> first-line prefix. That allowed me to simplify the code quite a bit,
> since that makes the previous logic for skipping the exact fill-prefix
> redundant, fill-move-to-break-point already handles the logic of
> trying to skip at least one word after the start position passed to
> it. Please take another look.

Your commit message is a bit too focused on the problems of what was
there previously rather than how your new code is correct.  This
description in your email is better, but I'm still struggling a bit (I'm
realizing I was probably a bit too superficial previously, there are a
lot of interacting options which makes things tricky).

If I understand correctly, the previous code would take the fill-prefix
as non-breakable if the current line started with the fill-prefix.  Your
new code takes the first N characters of the line as non-breakable
(where N is the length of fill-prefix, should this be based on
string-width instead?).  Is that correct (for both adaptive-fill-mode
and otherwise)?  If so, please add an explanation of why it's correct to
the commit message.  If not, I hope we can elaborate the commit message
until even I can understand what's happening ;)

(Minor commit message format nitpick: the part explaining the changes to
the function should start with "* lisp/simple.el (do-auto-fill):".)

bug#20774: auto-fill doesn't work properly when first-line prefix differs in adaptive-fill-mode

[Samuel Freilich]

[-- Attachment #1.1: Type: text/plain, Size: 3191 bytes --]

You're right about string-width, since I'm counting number of columns.
Fixed that. (Which required me to deal with the fact that fill-prefix can
be nil.)

Your understanding is correct. My fix avoids do-auto-fill breaking a line
only to create a subsequent line that's as long or longer.

The previous version avoided that in most cases by refusing to break a line
during (or immediately after) the fill-prefix. But that failed to avoid
that problem when:
* It's breaking the first line of a paragraph
* The prefix of that first line doesn't match the fill-prefix for the rest
of the paragraph

I've attached the fixed version of the patch with a hopefully-improved
commit message.



On Wed, Aug 23, 2017 at 10:16 PM, <npostavs@users.sourceforge.net> wrote:

> Samuel Freilich <sfreilich@google.com> writes:
>
> > tab-width = 4 seems consistent with how the rest of the file is
> formatted.
> > I didn't want to change the spacing in lines not touched by my diff.
>
> Hmm, somehow the code that landed in my buffer looked wrongly indented.
> Something got messed up somewhere (possibly by the email system,
> possibly during patch application, I've been experimenting with various
> forms of automation for that), but since the indentation in your next
> patch looks fine there's not really much point in worrying about it.
>
> > I added tests and a change description, and formatted the patch according
> > to the current instructions.
>
> Thanks!
>
> > I made the patch a bit more minimal. I pruned the part that dealt with
> > adaptive-fill-first-line-regexp didn't work as well as expected, since
> > that didn't work as well as expected (it didn't deal with all the
> > complexity possible with adaptive-fill-function). The updated version
> > at least handles cases where the fill-prefix isn't shorter than the
> > first-line prefix. That allowed me to simplify the code quite a bit,
> > since that makes the previous logic for skipping the exact fill-prefix
> > redundant, fill-move-to-break-point already handles the logic of
> > trying to skip at least one word after the start position passed to
> > it. Please take another look.
>
> Your commit message is a bit too focused on the problems of what was
> there previously rather than how your new code is correct.  This
> description in your email is better, but I'm still struggling a bit (I'm
> realizing I was probably a bit too superficial previously, there are a
> lot of interacting options which makes things tricky).
>
> If I understand correctly, the previous code would take the fill-prefix
> as non-breakable if the current line started with the fill-prefix.  Your
> new code takes the first N characters of the line as non-breakable
> (where N is the length of fill-prefix, should this be based on
> string-width instead?).  Is that correct (for both adaptive-fill-mode
> and otherwise)?  If so, please add an explanation of why it's correct to
> the commit message.  If not, I hope we can elaborate the commit message
> until even I can understand what's happening ;)
>
> (Minor commit message format nitpick: the part explaining the changes to
> the function should start with "* lisp/simple.el (do-auto-fill):".)
>

[-- Attachment #1.2: Type: text/html, Size: 3997 bytes --]

[-- Attachment #2: 0002-Do-not-split-line-before-width-of-fill-prefix.patch --]
[-- Type: text/x-patch, Size: 3529 bytes --]

From 42da1d80367b7519b0047328a37d845e78aaa157 Mon Sep 17 00:00:00 2001
From: Samuel Freilich <sfreilich@google.com>
Date: Wed, 23 Aug 2017 13:40:45 -0400
Subject: [PATCH] Do not split line before width of fill-prefix

When auto-filling a paragraph, don't split a line before the width of the
fill-prefix, creating a subsequent line that is as long or longer (Bug#20774).
* lisp/simple.el (do-auto-fill): Only consider break-points that are later in
the line than the width of the fill-prefix. This is a more general solution
than the previous logic, which only skipped over the exact fill-prefix. The
fill-prefix doesn't necessarily match the prefix of the first line of a
paragraph in adaptive-fill-mode.
---
 lisp/simple.el            | 29 ++++++++++++++---------------
 test/lisp/simple-tests.el | 11 +++++++++++
 2 files changed, 25 insertions(+), 15 deletions(-)

diff --git a/lisp/simple.el b/lisp/simple.el
index 072723cd64..15c7f1f935 100644
--- a/lisp/simple.el
+++ b/lisp/simple.el
@@ -7148,18 +7148,20 @@ do-auto-fill
 	       (setq fill-prefix prefix))))
 
       (while (and (not give-up) (> (current-column) fc))
-	;; Determine where to split the line.
-	(let* (after-prefix
-	       (fill-point
-		(save-excursion
-		  (beginning-of-line)
-		  (setq after-prefix (point))
-		  (and fill-prefix
-		       (looking-at (regexp-quote fill-prefix))
-		       (setq after-prefix (match-end 0)))
-		  (move-to-column (1+ fc))
-		  (fill-move-to-break-point after-prefix)
-		  (point))))
+        ;; Determine where to split the line.
+        (let ((fill-point
+               (save-excursion
+                 (beginning-of-line)
+                 (let ((line-start (point)))
+                    (move-to-column (1+ fc))
+                    ;; Don't split earlier in the line than the length of the
+                    ;; fill prefix, since the resulting line would be longer.
+                    (fill-move-to-break-point
+                     (if fill-prefix
+                         (min (line-end-position)
+                              (+ line-start (string-width fill-prefix)))
+                       line-start))
+                    (point)))))
 
 	  ;; See whether the place we found is any good.
 	  (if (save-excursion
@@ -7167,9 +7169,6 @@ do-auto-fill
 		(or (bolp)
 		    ;; There is no use breaking at end of line.
 		    (save-excursion (skip-chars-forward " ") (eolp))
-		    ;; It is futile to split at the end of the prefix
-		    ;; since we would just insert the prefix again.
-		    (and after-prefix (<= (point) after-prefix))
 		    ;; Don't split right after a comment starter
 		    ;; since we would just make another comment starter.
 		    (and comment-start-skip
diff --git a/test/lisp/simple-tests.el b/test/lisp/simple-tests.el
index ad7aee1db1..c7330e4034 100644
--- a/test/lisp/simple-tests.el
+++ b/test/lisp/simple-tests.el
@@ -497,5 +497,16 @@ simple-test-undo-with-switched-buffer
       (should (equal (line-number-at-pos 5) 3))
       (should (equal (line-number-at-pos 7) 4)))))
 
+(ert-deftest auto-fill-mode-no-break-before-length-of-fill-prefix ()
+  (with-temp-buffer
+    (setq-local fill-prefix "   ")
+    (set-fill-column 5)
+    ;; Shouldn't break after 'foo' (3 characters) when the next
+    ;; line is indented >= to that, that woudln't result in shorter
+    ;; lines.
+    (insert "foo bar")
+    (do-auto-fill)
+    (should (string-equal (buffer-string) "foo bar"))))
+
 (provide 'simple-test)
 ;;; simple-test.el ends here
-- 
2.14.1.342.g6490525c54-goog

bug#20774: auto-fill doesn't work properly when first-line prefix differs in adaptive-fill-mode

[npostavs]

Samuel Freilich <sfreilich@google.com> writes:

> You're right about string-width, since I'm counting number of columns.
> Fixed that. (Which required me to deal with the fact that fill-prefix can
> be nil.)

Actually, it's now occuring to me that adding a position to a
string-width isn't quite correct either, since it doesn't take into
account wide characters on the current line.  I guess it should actually
be something like

    (save-excursion
      (if line-prefix
          (move-to-column (string-width line-prefix))
        (beginning-of-line))
      (point))

> I've attached the fixed version of the patch with a hopefully-improved
> commit message.

That is much better, thanks.  (One more minor formatting nitpick, there
should be 2 spaces after the period.)

bug#20774: auto-fill doesn't work properly when first-line prefix differs in adaptive-fill-mode

[npostavs]

[-- Attachment #1: Type: text/plain, Size: 26 bytes --]

[forwarding to bug list]


[-- Attachment #2: Type: message/rfc822, Size: 5302 bytes --]

[-- Attachment #2.1.1.1: Type: text/plain, Size: 517 bytes --]

<npostavs@users.sourceforge.net> wrote:

> Actually, it's now occuring to me that adding a position to a
> string-width isn't quite correct either

Ah right, (point) is in characters, which doesn't correspond directly to
columns at all. I think I can avoid the extra save-excursion and make it a
little cleaner as a result.

> there should be 2 spaces after the period

Heresy! :-P

But done. That does seem to be the most common style in the ChangeLog.

Please take another look. Hopefully this patch can be merged.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2.1.1.2: 0003-Do-not-split-line-before-width-of-fill-prefix.patch --]
[-- Type: text/x-patch; name=0003-Do-not-split-line-before-width-of-fill-prefix.patch, Size: 3440 bytes --]

From 118c8a43510a7d3020a552ce0e87e5a1b9ccec54 Mon Sep 17 00:00:00 2001
From: Samuel Freilich <sfreilich@google.com>
Date: Wed, 23 Aug 2017 13:40:45 -0400
Subject: [PATCH] Do not split line before width of fill-prefix

When auto-filling a paragraph, don't split a line before the width of the
fill-prefix, creating a subsequent line that is as long or longer (Bug#20774).
* lisp/simple.el (do-auto-fill): Only consider break-points that are later in
the line than the width of the fill-prefix.  This is a more general solution
than the previous logic, which only skipped over the exact fill-prefix.  The
fill-prefix doesn't necessarily match the prefix of the first line of a
paragraph in adaptive-fill-mode.
---
 lisp/simple.el            | 27 ++++++++++++---------------
 test/lisp/simple-tests.el | 11 +++++++++++
 2 files changed, 23 insertions(+), 15 deletions(-)

diff --git a/lisp/simple.el b/lisp/simple.el
index 13cfa3487d..27990bb661 100644
--- a/lisp/simple.el
+++ b/lisp/simple.el
@@ -7151,18 +7151,18 @@ do-auto-fill
 	       (setq fill-prefix prefix))))
 
       (while (and (not give-up) (> (current-column) fc))
-	;; Determine where to split the line.
-	(let* (after-prefix
-	       (fill-point
-		(save-excursion
-		  (beginning-of-line)
-		  (setq after-prefix (point))
-		  (and fill-prefix
-		       (looking-at (regexp-quote fill-prefix))
-		       (setq after-prefix (match-end 0)))
-		  (move-to-column (1+ fc))
-		  (fill-move-to-break-point after-prefix)
-		  (point))))
+        ;; Determine where to split the line.
+        (let ((fill-point
+               (save-excursion
+                 (beginning-of-line)
+                 ;; Don't split earlier in the line than the length of the
+                 ;; fill prefix, since the resulting line would be longer.
+                 (when fill-prefix
+                   (move-to-column (string-width fill-prefix)))
+                 (let ((after-prefix (point)))
+                    (move-to-column (1+ fc))
+                    (fill-move-to-break-point after-prefix)
+                    (point)))))
 
 	  ;; See whether the place we found is any good.
 	  (if (save-excursion
@@ -7170,9 +7170,6 @@ do-auto-fill
 		(or (bolp)
 		    ;; There is no use breaking at end of line.
 		    (save-excursion (skip-chars-forward " ") (eolp))
-		    ;; It is futile to split at the end of the prefix
-		    ;; since we would just insert the prefix again.
-		    (and after-prefix (<= (point) after-prefix))
 		    ;; Don't split right after a comment starter
 		    ;; since we would just make another comment starter.
 		    (and comment-start-skip
diff --git a/test/lisp/simple-tests.el b/test/lisp/simple-tests.el
index ad7aee1db1..c7330e4034 100644
--- a/test/lisp/simple-tests.el
+++ b/test/lisp/simple-tests.el
@@ -497,5 +497,16 @@ simple-test-undo-with-switched-buffer
       (should (equal (line-number-at-pos 5) 3))
       (should (equal (line-number-at-pos 7) 4)))))
 
+(ert-deftest auto-fill-mode-no-break-before-length-of-fill-prefix ()
+  (with-temp-buffer
+    (setq-local fill-prefix "   ")
+    (set-fill-column 5)
+    ;; Shouldn't break after 'foo' (3 characters) when the next
+    ;; line is indented >= to that, that woudln't result in shorter
+    ;; lines.
+    (insert "foo bar")
+    (do-auto-fill)
+    (should (string-equal (buffer-string) "foo bar"))))
+
 (provide 'simple-test)
 ;;; simple-test.el ends here
-- 
2.14.1.342.g6490525c54-goog

bug#20774: auto-fill doesn't work properly when first-line prefix differs in adaptive-fill-mode

[npostavs]

> From: Samuel Freilich <sfreilich@google.com>
>
> <npostavs@users.sourceforge.net> wrote:
>
>> Actually, it's now occuring to me that adding a position to a
>> string-width isn't quite correct either
>
> Ah right, (point) is in characters, which doesn't correspond directly to
> columns at all. I think I can avoid the extra save-excursion and make it a
> little cleaner as a result.

Looks good.  I'll wait a couple of days before pushing in case someone
finds we missed something.

>> there should be 2 spaces after the period
>
> Heresy! :-P
>
> But done. That does seem to be the most common style in the ChangeLog.

I think you'll find it's not just most common, it's official policy[1].
However, the Church of Emacs will forgive your impertinence, as long as
you don't bring up the subject of quotation marks ;) [2]

[1]: http://www.gnu.org/prep/standards/html_node/Comments.html
[2]: https://mobile.twitter.com/AMalabarba/status/641598687467163648

bug#20774: auto-fill doesn't work properly when first-line prefix differs in adaptive-fill-mode

[npostavs]

tags 20774 fixed
close 20774 26.1
quit

npostavs@users.sourceforge.net writes:

> Looks good.  I'll wait a couple of days before pushing in case someone
> finds we missed something.

I've pushed to master.

[1: cda26e6462]: 2017-08-30 20:10:36 -0400
  Do not split line before width of fill-prefix
  http://git.savannah.gnu.org/cgit/emacs.git/commit/?id=cda26e64621d71c6a797f694418d844a621998be