Re: someone broke message-newline-and-reformat

[Vladimir Volovich <vvv@vvv.vsu.ru>]

> This one works for me.  Please could you test and confirm:
> 
> *** message.el	2000/11/04 17:13:56	6.7
> --- message.el	2000/11/10 11:57:50
> ***************
> *** 1711,1724 ****
>   (defun message-newline-and-reformat ()
>     "Insert four newlines, and then reformat if inside quoted text."
>     (interactive)
> !   (let ((prefix "[]>»|:}+ \t]*")
>   	(supercite-thing "[-._a-zA-Z0-9]*[>]+[ \t]*")
>   	quoted point)
>       (unless (bolp)
>         (save-excursion
>   	(beginning-of-line)
> ! 	(when (looking-at (concat prefix "\\|"
> ! 				  supercite-thing))
>   	  (setq quoted (match-string 0))))
>         (insert "\n"))
>       (setq point (point))
> --- 1711,1724 ----
>   (defun message-newline-and-reformat ()
>     "Insert four newlines, and then reformat if inside quoted text."
>     (interactive)
> !   (let ((prefix "[]>»\|:}+ \t]*")
>   	(supercite-thing "[-._a-zA-Z0-9]*[>]+[ \t]*")
>   	quoted point)
>       (unless (bolp)
>         (save-excursion
>   	(beginning-of-line)
> ! 	(when (or (looking-at supercite-thing)
> ! 		  (looking-at prefix))
>   	  (setq quoted (match-string 0))))
>         (insert "\n"))
>       (setq point (point))

i have some questions/suggestions:

1) you changed
  (let ((prefix "[]>»|:}+ \t]*")
to
  (let ((prefix "[]>»\|:}+ \t]*")

But this seems to be not needed (i.e. a backslash before | is not
needed):

(string-equal
  "[]>»|:}+ \t]*"
  "[]>»\|:}+ \t]*")
t

2) (looking-at prefix) will ALWAYS return t because ANY string matches
   a regexp of form [something]*
   so the check for (looking-at prefix) is broken.

   i think that this regexp could be changed to something like
   []>»|:}+]+[ \t]*

   BTW, the broken regexp for prefix was THE reason why your patch
   broke message-newline-and-reformat: the resulting regexp

   <value of prefix>\\|<value of supercite-thing>

   gave a null string match.

3) (supercite-thing "[-._a-zA-Z0-9]*[>]+[ \t]*")
   could be changed (simplified) to
   (supercite-thing "[-._a-zA-Z0-9]*>+[ \t]*")

4) the trick of changing 
  (looking-at (concat prefix "\\|" supercite-thing))
to
  (or (looking-at supercite-thing) (looking-at prefix))
seems to me like a hack.

  of course the original form (the one which you added some time ago)
  should work PROVIDED that prefix is sensibly defined AND
  supercite-thing comes before prefix (because otherwise prefix could
  find a smaller match)

5) the whole concat thing (and assigning prefix and supercite-thing
   variables) is not necessary, and you could just put a ready-to-use
   regexp in looking-at's argument.

6) looking only for latin letters (a-zA-Z) is not necessary (e.g. i
   often need to answer letters written by people with cyrillic
   initials), so it is better to change it to \w.

7) here is a resulting message-newline-and-reformat which seems to
   work much better for me:

(defun message-newline-and-reformat ()
  "Insert four newlines, and then reformat if inside quoted text."
  (interactive)
  (let (quoted point)
    (unless (bolp)
      (save-excursion
        (beginning-of-line)
        (when (looking-at " *\\(\\w\\|[-._]\\)*>+[ \t]*\\|[]>»|:}+ \t]*")
          (setq quoted (match-string 0))))
      (insert "\n"))
    (setq point (point))
    (insert "\n\n\n")
    (delete-region (point) (re-search-forward "[ \t]*"))
    (when quoted
      (insert quoted))
    (fill-paragraph nil)
    (goto-char point)
    (forward-line 1)))

Best,
v.