unofficial mirror of bug-gnu-emacs@gnu.org 
 help / color / mirror / code / Atom feed
* bug#25157: 26.0.50; whitespace-cleanup does not remove single trailing empty line anymore
@ 2016-12-10 14:41 Mark Karpov
  2016-12-20  4:36 ` npostavs
  0 siblings, 1 reply; 12+ messages in thread
From: Mark Karpov @ 2016-12-10 14:41 UTC (permalink / raw)
  To: 25157


The ‘whitespace-cleanup’ command does not remove single trailing empty
line anymore. To reproduce start emacs with -Q flag, open any file and a
single empty training line, like this:

--- start of file
foo

--- end of file

Then call ‘whitespace-cleanup’. Expected result:

--- start of file
foo
--- end of file

Actual result (nothing happens):

--- start of file
foo

--- end of file

This bug does not manifest itself if there are more than one empty line,
i.e.:

--- start of file
foo


--- end of file

(Executing ‘whitespace-cleanup’):

--- start of file
foo
--- end of file

The result is as expected.

In GNU Emacs 26.0.50.5 (x86_64-unknown-linux-gnu, GTK+ Version 3.22.4)
 of 2016-12-10 built on arch
Repository revision: fc0fd24c105bde4c001ebebe4b8b7e1f96cd2871
Windowing system distributor 'The X.Org Foundation', version 11.0.11804000
Recent messages:
Indentation setup for shell type bash
next-line: End of buffer [5 times]
Saving file /home/mark/.bash_profile...
Wrote /home/mark/.bash_profile
Saving file /home/mark/.bash_profile...
Wrote /home/mark/.bash_profile
Quit
nil
Saving file /home/mark/.bash_profile...
Wrote /home/mark/.bash_profile

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

Important settings:
  value of $LANG: en_US.UTF-8
  locale-coding-system: utf-8-unix

Major mode: iλ

Minor modes in effect:
  shell-dirtrack-mode: t
  diff-auto-refine-mode: t
  aggressive-indent-mode: t
  rainbow-delimiters-mode: t
  whitespace-mode: t
  flycheck-color-mode-line-mode: t
  yas-global-mode: t
  yas-minor-mode: t
  vimish-fold-global-mode: t
  vimish-fold-mode: t
  show-paren-mode: t
  rich-minority-mode: t
  minibuffer-electric-default-mode: t
  ivy-mode: t
  global-auto-revert-mode: t
  display-time-mode: t
  delete-selection-mode: t
  cyphejor-mode: t
  ace-popup-menu-mode: t
  smartparens-global-mode: t
  smartparens-mode: t
  flyspell-mode: t
  flyspell-lazy-mode: t
  mk-highlight-line-mode: t
  modalka-mode: t
  hl-todo-mode: t
  flycheck-mode: t
  tooltip-mode: t
  global-eldoc-mode: t
  eldoc-mode: t
  electric-indent-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
  column-number-mode: t
  line-number-mode: t
  auto-fill-function: do-auto-fill
  transient-mark-mode: t
  auto-fill-mode: 1

Load-path shadows:
None found.

Features:
(shadow sort mail-extr emacsbug counsel esh-util swiper sh-script smie
executable colir haskell-doc inf-haskell haskell-decl-scan imenu shell
pcomplete haskell haskell-completions haskell-load haskell-commands
highlight-uses-mode haskell-modules haskell-sandbox haskell-repl
haskell-debug haskell-interactive-mode haskell-presentation-mode
haskell-collapse haskell-navigate-imports haskell-compile
haskell-process haskell-session url-util haskell-hoogle
smartparens-haskell haskell-mode haskell-font-lock haskell-indentation
haskell-string haskell-sort-imports haskell-lexeme haskell-align-imports
haskell-compat haskell-complete-module haskell-ghc-support noutline
outline flymake etags compile dabbrev haskell-customize vc-git diff-mode
bookmark pp aggressive-indent easy-mmode rainbow-delimiters whitespace
flycheck-haskell haskell-cabal haskell-utils flycheck-color-mode-line
face-remap mk-visual smart-mode-line solarized-dark-theme solarized
color mk-texinfo mk-tex mk-shakespeare mk-prolog mk-org mk-mustache
mk-mu4e mk-markdown mk-man mk-magit mk-lisp mk-js mk-ibuffer mk-html
mk-eshell mk-erc mk-elisp mk-dired mk-clojure kill-or-bury-alive
mk-calendar mk-c mk-minor-modes common-lisp-snippets yasnippet
whole-line-or-region vimish-fold paren rich-minority minibuf-eldef ivy
ivy-overlay ffap autorevert filenotify time delsel cyphejor
ace-popup-menu smartparens-config smartparens flyspell ispell
flyspell-lazy mk-highlight-line mk-global modalka edmacro kmacro
fix-input quail ace-link hl-todo xref project mu4e mu4e-speedbar
speedbar sb-image ezimage dframe mu4e-main mu4e-context mu4e-view
cal-menu calendar cal-loaddefs thingatpt browse-url comint ansi-color
mu4e-headers mu4e-compose mu4e-draft mu4e-actions ido rfc2368 smtpmail
sendmail mu4e-mark mu4e-message html2text mu4e-proc mu4e-utils doc-view
jka-compr image-mode mu4e-lists mu4e-vars message puny dired
dired-loaddefs format-spec rfc822 mml mml-sec epa derived epg gnus-util
rmail rmail-loaddefs mm-decode mm-bodies mm-encode mail-parse rfc2231
rfc2047 rfc2045 mm-util ietf-drums mail-prsvr mailabbrev mail-utils
gmm-utils mailheader hl-line cl mu4e-meta mk-python mk-haskell mk-utils
avy-menu avy ring flycheck json map find-func rx subr-x misc server f
dash s ucs-normalize finder-inf tex-site fix-word advice slime-autoloads
info package epg-config url-handlers url-parse auth-source cl-seq eieio
eieio-core cl-macs eieio-loaddefs password-cache url-vars seq byte-opt
gv bytecomp byte-compile cl-extra help-mode easymenu cconv cl-loaddefs
pcase cl-lib time-date mule-util tooltip eldoc electric uniquify
ediff-hook vc-hooks lisp-float-type mwheel term/x-win x-win
term/common-win x-dnd tool-bar dnd fontset image regexp-opt fringe
tabulated-list replace newcomment text-mode elisp-mode lisp-mode
prog-mode register page menu-bar rfn-eshadow isearch timer select
scroll-bar mouse jit-lock font-lock syntax facemenu font-core
term/tty-colors 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 composite charscript case-table epa-hook jka-cmpr-hook help
simple abbrev obarray 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 470216 19037)
 (symbols 48 40810 0)
 (miscs 40 87 365)
 (strings 32 91124 7084)
 (string-bytes 1 2627337)
 (vectors 16 70269)
 (vector-slots 8 1777314 201698)
 (floats 8 654 304)
 (intervals 56 368 0)
 (buffers 976 16))





^ permalink raw reply	[flat|nested] 12+ messages in thread

* bug#25157: 26.0.50; whitespace-cleanup does not remove single trailing empty line anymore
  2016-12-10 14:41 bug#25157: 26.0.50; whitespace-cleanup does not remove single trailing empty line anymore Mark Karpov
@ 2016-12-20  4:36 ` npostavs
  2016-12-20 18:37   ` Reuben Thomas
  0 siblings, 1 reply; 12+ messages in thread
From: npostavs @ 2016-12-20  4:36 UTC (permalink / raw)
  To: Mark Karpov; +Cc: 25157, Reuben Thomas

tags 25157 confirmed
quit

Mark Karpov <markkarpov@openmailbox.org> writes:

> The ‘whitespace-cleanup’ command does not remove single trailing empty
> line anymore.

This seems to have been caused by the fix for #24745.
https://debbugs.gnu.org/cgi/bugreport.cgi?bug=24745#5 has
 
-(defcustom whitespace-empty-at-eob-regexp "^\\([ \t\n]+\\)"
+(defcustom whitespace-empty-at-eob-regexp "^\\([ \t\n]+\\)\\'"

which is what I would expect, but
https://debbugs.gnu.org/cgi/bugreport.cgi?bug=24745#11 says

   First, I revised my previous patch, which had an error in the new
   version of whitespace-empty-at-eob-regexp.

and has

-(defcustom whitespace-empty-at-eob-regexp "^\\([ \t\n]+\\)"
+(defcustom whitespace-empty-at-eob-regexp "^\\([ \t\n]*\\(\n\\{2,\\}\\|[ \t]+\\)\\)\\'"

I don't quite understand why this more complicated expression is
necessary.  Reuben, can you explain?





^ permalink raw reply	[flat|nested] 12+ messages in thread

* bug#25157: 26.0.50; whitespace-cleanup does not remove single trailing empty line anymore
  2016-12-20  4:36 ` npostavs
@ 2016-12-20 18:37   ` Reuben Thomas
  2016-12-21  0:51     ` Reuben Thomas
  2016-12-21  4:07     ` npostavs
  0 siblings, 2 replies; 12+ messages in thread
From: Reuben Thomas @ 2016-12-20 18:37 UTC (permalink / raw)
  To: Noam Postavsky; +Cc: Mark Karpov, 25157

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

On 20 December 2016 at 04:36, <npostavs@users.sourceforge.net> wrote:

> tags 25157 confirmed
> quit
>
> Mark Karpov <markkarpov@openmailbox.org> writes:
>
> > The ‘whitespace-cleanup’ command does not remove single trailing empty
> > line anymore.
>

​​I can reproduce this; sorry!


> -(defcustom whitespace-empty-at-eob-regexp "^\\([ \t\n]+\\)"
> +(defcustom whitespace-empty-at-eob-regexp "^\\([ \t\n]*\\(\n\\{2,\\}\\|[
> \t]+\\)\\)\\'"
>
> I don't quite understand why this more complicated expression is
> necessary.  Reuben, can you explain?
>

​With the previous regexp, whitespace-cleanup would remove a single newline
at the end of a buffer.

I think I tried to be a bit too clever.

Thinking again, what we require is:

Match at the end of the buffer, either:

a. A mix of spaces and tabs, or

b. Optional whitespace followed by a newline followed by whitespace.

These two categories are not mutually exclusive (which is fine, and avoids
being too clever).

The point is that if there are any newlines, there must be at least two.

Also note that the regexp does not need to be anchored at the start of a
line (I'm not sure why I thought it did).

So, I think a correct regexp, directly translating the above, is: \\([
\t]+\\|\\([ \t\n]*\n[ \t\n]+\\)\\)\\'

However, there's still a problem: while this regexp will not match a single
newline at the end of a buffer, when it does match any number of newlines
(with or without extra space), it will remove all of them, whereas it
should leave a single newline.

I can't see a way around this purely in the regexp, because if for example
the end of the buffer is:

\t\n\t

then the regexp should match (and this one does), but whitespace-cleanup
should leave a newline.

So I think a further change to the code is needed to whitespace-cleanup:
when whitespace-empty-at-eob-regexp is matched, it should check
match-string, and if it contains a newline, it should insert a newline in
the buffer after deleting the matched string.

Given my previous error of reasoning, I'm submitting the above for your
consideration before I prepare a patch!

-- 
http://rrt.sc3d.org

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* bug#25157: 26.0.50; whitespace-cleanup does not remove single trailing empty line anymore
  2016-12-20 18:37   ` Reuben Thomas
@ 2016-12-21  0:51     ` Reuben Thomas
  2016-12-21  4:07     ` npostavs
  1 sibling, 0 replies; 12+ messages in thread
From: Reuben Thomas @ 2016-12-21  0:51 UTC (permalink / raw)
  To: Noam Postavsky; +Cc: Mark Karpov, 25157


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

I attach a patch for consideration, along the lines I previously proposed.
It seems to work: at least, it deletes a single trailing extra newline,
while still leaving a newline at the end of the buffer, and doesn't delete
a single newline at the end of the buffer.

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

[-- Attachment #2: 0007-Fix-whitespace-cleanup-to-remove-single-trailing-new.patch --]
[-- Type: text/x-patch, Size: 2323 bytes --]

From fb396e7a732f377747b013f7e589133acb12258f Mon Sep 17 00:00:00 2001
From: Reuben Thomas <rrt@sc3d.org>
Date: Wed, 21 Dec 2016 00:40:11 +0000
Subject: [PATCH 7/7] Fix whitespace-cleanup to remove single trailing newline
 (Bug#25157)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

* lisp/whitespace.el (whitespace-empty-at-eob-regexp): Simplify and
correct the regexp, which I broke when trying to fix Bug#24745.
(whitespace-cleanup): Remove a FIXME: the reason for save-match-data
is the call of re-search-forward.  Detect if we delete at least one
newline at the end of the buffer, and if so, add one back. This is
necessary because there’s no way to construct the the eob-regexp so
that it always leaves a single newline at the end of the buffer if at
least one newline was present after the last non-whitespace character.
---
 lisp/whitespace.el | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/lisp/whitespace.el b/lisp/whitespace.el
index 29d60c9..009511f 100644
--- a/lisp/whitespace.el
+++ b/lisp/whitespace.el
@@ -729,7 +729,7 @@ whitespace-empty-at-bob-regexp
   :group 'whitespace)
 
 
-(defcustom whitespace-empty-at-eob-regexp "^\\([ \t\n]*\\(\n\\{2,\\}\\|[ \t]+\\)\\)\\'"
+(defcustom whitespace-empty-at-eob-regexp "\\([ \t]+\\|\\([ \t\n]*\n[ \t\n]+\\)\\)\\'"
   "Specify regexp for empty lines at end of buffer.
 
 Used when `whitespace-style' includes `empty'."
@@ -1398,7 +1398,7 @@ whitespace-cleanup
    ;; whole buffer
    (t
     (save-excursion
-      (save-match-data                ;FIXME: Why?
+      (save-match-data
 	;; PROBLEM 1: empty lines at bob
 	;; PROBLEM 2: empty lines at eob
 	;; ACTION: remove all empty lines at bob and/or eob
@@ -1408,7 +1408,10 @@ whitespace-cleanup
 	    (when (looking-at whitespace-empty-at-bob-regexp)
 	      (delete-region (match-beginning 1) (match-end 1)))
 	    (when (re-search-forward
-                   whitespace-empty-at-eob-regexp nil t)
+		   whitespace-empty-at-eob-regexp nil t)
+	      ;; If we deleted a trailing newline, reinsert it
+	      (if (string-match-p "\n" (match-string 0))
+		  (insert ?\n))
 	      (delete-region (match-beginning 1) (match-end 1)))))))
     ;; PROBLEM 3: `tab-width' or more SPACEs at bol
     ;; PROBLEM 4: SPACEs before TAB
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* bug#25157: 26.0.50; whitespace-cleanup does not remove single trailing empty line anymore
  2016-12-20 18:37   ` Reuben Thomas
  2016-12-21  0:51     ` Reuben Thomas
@ 2016-12-21  4:07     ` npostavs
  2016-12-21 11:07       ` Reuben Thomas
  1 sibling, 1 reply; 12+ messages in thread
From: npostavs @ 2016-12-21  4:07 UTC (permalink / raw)
  To: Reuben Thomas; +Cc: Mark Karpov, 25157

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

Reuben Thomas <rrt@sc3d.org> writes:

>
> Given my previous error of reasoning, I'm submitting the above for your consideration before I prepare a patch!

Sorry I didn't get around to looking at this before you wrote a patch,
but I think you're overthinking this.  AFAICT, your first fix (in
https://debbugs.gnu.org/cgi/bugreport.cgi?bug=24745#5) was right.
Here's a patch with some tests, let me know if I missed any cases.


[-- Attachment #2: patch --]
[-- Type: text/plain, Size: 3239 bytes --]

From e918857882733460a429f6a37344567a5021efa5 Mon Sep 17 00:00:00 2001
From: Noam Postavsky <npostavs@gmail.com>
Date: Tue, 20 Dec 2016 23:02:48 -0500
Subject: [PATCH v1] Fix whitespace eob cleanup

* lisp/whitespace.el (whitespace-empty-at-eob-regexp): Match any number
of empty lines at end of buffer.
* test/lisp/whitespace-tests.el (whitespace-cleanup-eob): New test.
(whitespace-tests--cleanup-string): New helper function for tests.
---
 lisp/whitespace.el            |  2 +-
 test/lisp/whitespace-tests.el | 52 +++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 53 insertions(+), 1 deletion(-)
 create mode 100644 test/lisp/whitespace-tests.el

diff --git a/lisp/whitespace.el b/lisp/whitespace.el
index 29d60c9..a15308c 100644
--- a/lisp/whitespace.el
+++ b/lisp/whitespace.el
@@ -729,7 +729,7 @@ whitespace-empty-at-bob-regexp
   :group 'whitespace)
 
 
-(defcustom whitespace-empty-at-eob-regexp "^\\([ \t\n]*\\(\n\\{2,\\}\\|[ \t]+\\)\\)\\'"
+(defcustom whitespace-empty-at-eob-regexp "^\\([ \t\n]+\\)\\'"
   "Specify regexp for empty lines at end of buffer.
 
 Used when `whitespace-style' includes `empty'."
diff --git a/test/lisp/whitespace-tests.el b/test/lisp/whitespace-tests.el
new file mode 100644
index 0000000..ffd2e65
--- /dev/null
+++ b/test/lisp/whitespace-tests.el
@@ -0,0 +1,52 @@
+;;; whitespace-tests.el --- Test suite for whitespace -*- lexical-binding: t -*-
+
+;; Copyright (C) 2016 Free Software Foundation, Inc.
+
+;; This file is part of GNU Emacs.
+
+;; GNU Emacs is free software: you can redistribute it and/or modify
+;; it under the terms of the GNU General Public License as published by
+;; the Free Software Foundation, either version 3 of the License, or
+;; (at your option) any later version.
+
+;; GNU Emacs is distributed in the hope that it will be useful,
+;; but WITHOUT ANY WARRANTY; without even the implied warranty of
+;; MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+;; GNU General Public License for more details.
+
+;; You should have received a copy of the GNU General Public License
+;; along with GNU Emacs.  If not, see <http://www.gnu.org/licenses/>.
+
+;;; Code:
+
+(require 'ert)
+(require 'whitespace)
+
+(defun whitespace-tests--cleanup-string (string)
+  (with-temp-buffer
+    (insert string)
+    (whitespace-cleanup)
+    (buffer-string)))
+
+(ert-deftest whitespace-cleanup-eob ()
+  (let ((whitespace-style '(empty)))
+    (should (equal (whitespace-tests--cleanup-string "a\n")
+                   "a\n"))
+    (should (equal (whitespace-tests--cleanup-string "a\n\n")
+                   "a\n"))
+    (should (equal (whitespace-tests--cleanup-string "a\n\t\n")
+                   "a\n"))
+    (should (equal (whitespace-tests--cleanup-string "a\n\t \n")
+                   "a\n"))
+    (should (equal (whitespace-tests--cleanup-string "a\n\t \n\n")
+                   "a\n"))
+    (should (equal (whitespace-tests--cleanup-string "\n\t\n")
+                   ""))
+    ;; Whitespace at end of non-empty line is not covered by the
+    ;; `empty' style.
+    (should (equal (whitespace-tests--cleanup-string "a  \n\t \n\n")
+                   "a  \n"))))
+
+(provide 'whitespace-tests)
+
+;;; whitespace-tests.el ends here
-- 
2.9.3


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* bug#25157: 26.0.50; whitespace-cleanup does not remove single trailing empty line anymore
  2016-12-21  4:07     ` npostavs
@ 2016-12-21 11:07       ` Reuben Thomas
  2016-12-22  2:18         ` npostavs
  0 siblings, 1 reply; 12+ messages in thread
From: Reuben Thomas @ 2016-12-21 11:07 UTC (permalink / raw)
  To: Noam Postavsky; +Cc: Mark Karpov, 25157

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

On 21 December 2016 at 04:07, <npostavs@users.sourceforge.net> wrote:

>
> Sorry I didn't get around to looking at this before you wrote a patch,
> but I think you're overthinking this.


I confirm that your patch seems to work, thanks very much. Adding some
tests is particularly helpful: if I'd done that, I wouldn't have gotten in
this mess in the first place!

How can one overthink a logical problem? It's possible to underthink, but
in this case, I believe I misunderstood what the
whitespace-empty-at-eob-regexp was supposed to match: only empty lines, not
trailing space at the end of non-empty lines (as one of your comments
mentions). (I was trying to make it match ALL the whitespace at the end of
the buffer, as you probably realised.)

The only thing I'd change in your match is, as in mine, remove the FIXME
after save-match-data.​

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* bug#25157: 26.0.50; whitespace-cleanup does not remove single trailing empty line anymore
  2016-12-21 11:07       ` Reuben Thomas
@ 2016-12-22  2:18         ` npostavs
  2016-12-22 12:53           ` Reuben Thomas
  0 siblings, 1 reply; 12+ messages in thread
From: npostavs @ 2016-12-22  2:18 UTC (permalink / raw)
  To: Reuben Thomas; +Cc: Mark Karpov, 25157

Reuben Thomas <rrt@sc3d.org> writes:

>(I was trying to make it match ALL the whitespace at the end of the
>buffer, as you probably realised.)

Oh, I thought that might have been the case.  It explains why you were
going through all that trouble with the regexp.

>
> The only thing I'd change in your match is, as in mine, remove the FIXME after save-match-data.​

    (whitespace-cleanup): Remove a FIXME: the reason for save-match-data
    is the call of re-search-forward.

I'm not sure this is a sufficient explanation of why save-match-data is
needed.  Usually in Emacs the caller is responsible for saving the match
data, if it needs to.  I don't really see the need here.






^ permalink raw reply	[flat|nested] 12+ messages in thread

* bug#25157: 26.0.50; whitespace-cleanup does not remove single trailing empty line anymore
  2016-12-22  2:18         ` npostavs
@ 2016-12-22 12:53           ` Reuben Thomas
  2016-12-23  0:03             ` npostavs
  0 siblings, 1 reply; 12+ messages in thread
From: Reuben Thomas @ 2016-12-22 12:53 UTC (permalink / raw)
  To: Noam Postavsky; +Cc: Mark Karpov, 25157

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

On Dec 22, 2016 2:17 AM, <npostavs@users.sourceforge.net> wrote:
>
> Reuben Thomas <rrt@sc3d.org> writes:
>
> >(I was trying to make it match ALL the whitespace at the end of the
> >buffer, as you probably realised.)
>
> Oh, I thought that might have been the case.  It explains why you were
> going through all that trouble with the regexp.
>
> >
> > The only thing I'd change in your match is, as in mine, remove the
FIXME after save-match-data.​
>
>     (whitespace-cleanup): Remove a FIXME: the reason for save-match-data
>     is the call of re-search-forward.
>
> I'm not sure this is a sufficient explanation of why save-match-data is
> needed.  Usually in Emacs the caller is responsible for saving the match
> data, if it needs to.  I don't really see the need here.

Can it be removed then? The only perhaps unexpected thing I can think of is
that whitespace-cleanup can be called from a hook, such as save-file-hook,
but that doesn't seem to matter.

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* bug#25157: 26.0.50; whitespace-cleanup does not remove single trailing empty line anymore
  2016-12-22 12:53           ` Reuben Thomas
@ 2016-12-23  0:03             ` npostavs
  2016-12-23 10:44               ` Reuben Thomas
  0 siblings, 1 reply; 12+ messages in thread
From: npostavs @ 2016-12-23  0:03 UTC (permalink / raw)
  To: Reuben Thomas; +Cc: Mark Karpov, 25157

Reuben Thomas <rrt@sc3d.org> writes:

> On Dec 22, 2016 2:17 AM, <npostavs@users.sourceforge.net> wrote:
>> >
>> > The only thing I'd change in your match is, as in mine, remove the FIXME after save-match-data.​
>>
>> (whitespace-cleanup): Remove a FIXME: the reason for save-match-data
>> is the call of re-search-forward.
>>
>> I'm not sure this is a sufficient explanation of why save-match-data is
>> needed. Usually in Emacs the caller is responsible for saving the match
>> data, if it needs to. I don't really see the need here.
>
> Can it be removed then?

Probably yes.

> The only perhaps unexpected thing I can think of is that whitespace-cleanup can be called from a hook, such as save-file-hook, but that
> doesn't seem to matter.







^ permalink raw reply	[flat|nested] 12+ messages in thread

* bug#25157: 26.0.50; whitespace-cleanup does not remove single trailing empty line anymore
  2016-12-23  0:03             ` npostavs
@ 2016-12-23 10:44               ` Reuben Thomas
  2016-12-24 15:34                 ` npostavs
  0 siblings, 1 reply; 12+ messages in thread
From: Reuben Thomas @ 2016-12-23 10:44 UTC (permalink / raw)
  To: Noam Postavsky; +Cc: Mark Karpov, 25157

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

On 23 December 2016 at 00:03, <npostavs@users.sourceforge.net> wrote:

> Reuben Thomas <rrt@sc3d.org> writes:
>
> > On Dec 22, 2016 2:17 AM, <npostavs@users.sourceforge.net> wrote:
> >> >
> >> > The only thing I'd change in your match is, as in mine, remove the
> FIXME after save-match-data.​
> >>
> >> (whitespace-cleanup): Remove a FIXME: the reason for save-match-data
> >> is the call of re-search-forward.
> >>
> >> I'm not sure this is a sufficient explanation of why save-match-data is
> >> needed. Usually in Emacs the caller is responsible for saving the match
> >> data, if it needs to. I don't really see the need here.
> >
> > Can it be removed then?
>
> Probably yes.


​Great; can you then install a suitable patch, please?

-- 
http://rrt.sc3d.org

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* bug#25157: 26.0.50; whitespace-cleanup does not remove single trailing empty line anymore
  2016-12-23 10:44               ` Reuben Thomas
@ 2016-12-24 15:34                 ` npostavs
  2016-12-26 22:37                   ` Reuben Thomas
  0 siblings, 1 reply; 12+ messages in thread
From: npostavs @ 2016-12-24 15:34 UTC (permalink / raw)
  To: Reuben Thomas; +Cc: Mark Karpov, 25157

tags 25157 fixed
close 25157 26.1
quit

Reuben Thomas <rrt@sc3d.org> writes:

>  >>
>  >> (whitespace-cleanup): Remove a FIXME: the reason for save-match-data
>  >> is the call of re-search-forward.
>  >>
>  >> I'm not sure this is a sufficient explanation of why save-match-data is
>  >> needed. Usually in Emacs the caller is responsible for saving the match
>  >> data, if it needs to. I don't really see the need here.
>  >
>  > Can it be removed then?
>
>  Probably yes.
>
> ​Great; can you then install a suitable patch, please?

I've pushed the first patch as cf5417f02887 "Fix whitespace eob
cleanup", removed save-match-data in da52e939aa26 "Remove redundant
`save-match-data' in whitespace.el".





^ permalink raw reply	[flat|nested] 12+ messages in thread

* bug#25157: 26.0.50; whitespace-cleanup does not remove single trailing empty line anymore
  2016-12-24 15:34                 ` npostavs
@ 2016-12-26 22:37                   ` Reuben Thomas
  0 siblings, 0 replies; 12+ messages in thread
From: Reuben Thomas @ 2016-12-26 22:37 UTC (permalink / raw)
  To: Noam Postavsky; +Cc: Mark Karpov, 25157

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

On 24 December 2016 at 15:34, <npostavs@users.sourceforge.net> wrote:

> I've pushed the first patch as cf5417f02887 "Fix whitespace eob
> cleanup", removed save-match-data in da52e939aa26 "Remove redundant
> `save-match-data' in whitespace.el".
>

​Thanks very much for this, in particular digging my mis-analysis out of a
hole.

-- 
http://rrt.sc3d.org

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2016-12-26 22:37 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-12-10 14:41 bug#25157: 26.0.50; whitespace-cleanup does not remove single trailing empty line anymore Mark Karpov
2016-12-20  4:36 ` npostavs
2016-12-20 18:37   ` Reuben Thomas
2016-12-21  0:51     ` Reuben Thomas
2016-12-21  4:07     ` npostavs
2016-12-21 11:07       ` Reuben Thomas
2016-12-22  2:18         ` npostavs
2016-12-22 12:53           ` Reuben Thomas
2016-12-23  0:03             ` npostavs
2016-12-23 10:44               ` Reuben Thomas
2016-12-24 15:34                 ` npostavs
2016-12-26 22:37                   ` Reuben Thomas

Code repositories for project(s) associated with this public inbox

	https://git.savannah.gnu.org/cgit/emacs.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).