[PATCH] Fix hack-local-variables for find-file-literally with dos encoding

[PATCH] Fix hack-local-variables for find-file-literally with dos encoding

[Tom Gillespie]

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

Hi,
    This patch provides a test and a fix for an edge case when opening
files literally that have dos crlf line endings. The patch was made
against master, but a similar fix could be issued for a potential
Emacs 27.3 release if one is forthcoming. Best!
Tom

[-- Attachment #2: 0001-Fix-hack-local-variables-for-find-file-literally-wit.patch --]
[-- Type: text/x-patch, Size: 2996 bytes --]

From bcbc9af3ddce5e07afc42bddb09e5ff43baf219f Mon Sep 17 00:00:00 2001
From: Tom Gillespie <tgbugs@gmail.com>
Date: Sun, 15 Aug 2021 14:17:42 -0700
Subject: [PATCH] Fix hack-local-variables for find-file-literally with dos
 encoding

* lisp/files.el (hack-local-variables--find-variables): Update
hack-local-variables--find-variables to check whether a buffer has
find-file-literally set before converting ?\^m to ?\n.

* test/lisp/files-tests.el (files-tests-hack-local-literal-dos): Added
test for calling hack-local-variables on files with dos encoding opened
with find-file-literally.

This fixes a bug where hack-local-variables would fail to find a
matching suffix for local variables when files with dos line endings
were opened literally using find-file-literally.

The unless branch is conditioned on buffer-file-coding-system because
the find-file-literally local variable is set after the call to
set-buffer-major-mode in find-file-noselect-1. If find-file-literally
was set before calling set-buffer-major-mode then it could be use
directly.
---
 lisp/files.el            |  6 ++++--
 test/lisp/files-tests.el | 11 +++++++++++
 2 files changed, 15 insertions(+), 2 deletions(-)

diff --git a/lisp/files.el b/lisp/files.el
index 875ac55316..c3518d59b9 100644
--- a/lisp/files.el
+++ b/lisp/files.el
@@ -3931,7 +3931,8 @@ hack-local-variables--find-variables
 	  (forward-line 1)
 	  (let ((startpos (point))
 	        endpos
-	        (thisbuf (current-buffer)))
+	        (thisbuf (current-buffer))
+	        (noconv (eq buffer-file-coding-system 'no-conversion)))
 	    (save-excursion
 	      (unless (let ((case-fold-search t))
 		        (re-search-forward
@@ -3947,7 +3948,8 @@ hack-local-variables--find-variables
 	    (with-temp-buffer
 	      (insert-buffer-substring thisbuf startpos endpos)
 	      (goto-char (point-min))
-	      (subst-char-in-region (point) (point-max) ?\^m ?\n)
+	      (unless noconv
+	        (subst-char-in-region (point) (point-max) ?\^m ?\n))
 	      (while (not (eobp))
 	        ;; Discard the prefix.
 	        (if (looking-at prefix)
diff --git a/test/lisp/files-tests.el b/test/lisp/files-tests.el
index fb24b98595..83659d900e 100644
--- a/test/lisp/files-tests.el
+++ b/test/lisp/files-tests.el
@@ -164,6 +164,17 @@ files-tests-permanent-local-variables
       (hack-local-variables)
       (should (eq lexical-binding nil)))))
 
+(ert-deftest files-tests-hack-local-literal-dos ()
+  (let ((tempfile (make-temp-file "files-tests-test-hack-local-literal-dos" nil ".el")))
+    (unwind-protect
+        (progn
+          (with-temp-buffer
+            (insert ";; -*- mode: Emacs-Lisp -*-\^m\n;; Local Variables:\^m\n;; lol: t\^m\n;; End:\^m\n")
+            (write-file tempfile))
+          (with-current-buffer (find-file-literally tempfile)
+            (hack-local-variables)))
+      (delete-file tempfile))))
+
 (defvar files-test-bug-18141-file
   (ert-resource-file "files-bug18141.el.gz")
   "Test file for bug#18141.")
-- 
2.31.1

Re: [PATCH] Fix hack-local-variables for find-file-literally with dos encoding

[Tom Gillespie]

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

Here is the backport to the emacs-27 branch. Best!
Tom

On Sun, Aug 15, 2021 at 2:23 PM Tom Gillespie <tgbugs@gmail.com> wrote:
>
> Hi,
>     This patch provides a test and a fix for an edge case when opening
> files literally that have dos crlf line endings. The patch was made
> against master, but a similar fix could be issued for a potential
> Emacs 27.3 release if one is forthcoming. Best!
> Tom

[-- Attachment #2: 0001-Backport-hack-local-variables-for-find-file-literall.patch --]
[-- Type: text/x-patch, Size: 2984 bytes --]

From 1c6a76ae9c436fe89bb9ab638696e4509a100a96 Mon Sep 17 00:00:00 2001
From: Tom Gillespie <tgbugs@gmail.com>
Date: Sun, 15 Aug 2021 14:31:43 -0700
Subject: [PATCH] Backport hack-local-variables for find-file-literally with
 dos

* lisp/files.el (hack-local-variables--find-variables): Update
hack-local-variables--find-variables to check whether a buffer has
find-file-literally set before converting ?\^m to ?\n.

* test/lisp/files-tests.el (files-tests-hack-local-literal-dos): Added
test for calling hack-local-variables on files with dos encoding opened
with find-file-literally.

This fixes a bug where hack-local-variables would fail to find a
matching suffix for local variables when files with dos line endings
were opened literally using find-file-literally.

The unless branch is conditioned on buffer-file-coding-system because
the find-file-literally local variable is set after the call to
set-buffer-major-mode in find-file-noselect-1. If find-file-literally
was set before calling set-buffer-major-mode then it could be use
directly.
---
 lisp/files.el            |  6 ++++--
 test/lisp/files-tests.el | 11 +++++++++++
 2 files changed, 15 insertions(+), 2 deletions(-)

diff --git a/lisp/files.el b/lisp/files.el
index 0a00b8b828..86c592e480 100644
--- a/lisp/files.el
+++ b/lisp/files.el
@@ -3742,7 +3742,8 @@ hack-local-variables
 	      (forward-line 1)
 	      (let ((startpos (point))
 		    endpos
-		    (thisbuf (current-buffer)))
+		    (thisbuf (current-buffer))
+		    (noconv (eq buffer-file-coding-system 'no-conversion)))
 		(save-excursion
 		  (unless (let ((case-fold-search t))
 			    (re-search-forward
@@ -3758,7 +3759,8 @@ hack-local-variables
 		(with-temp-buffer
 		  (insert-buffer-substring thisbuf startpos endpos)
 		  (goto-char (point-min))
-		  (subst-char-in-region (point) (point-max) ?\^m ?\n)
+		  (unless noconv
+		    (subst-char-in-region (point) (point-max) ?\^m ?\n))
 		  (while (not (eobp))
 		    ;; Discard the prefix.
 		    (if (looking-at prefix)
diff --git a/test/lisp/files-tests.el b/test/lisp/files-tests.el
index 1fc8007352..d24a4c5a23 100644
--- a/test/lisp/files-tests.el
+++ b/test/lisp/files-tests.el
@@ -150,6 +150,17 @@ files-tests-local-variables
         (dolist (subtest (cdr test))
           (should (file-test--do-local-variables-test str subtest)))))))
 
+(ert-deftest files-tests-hack-local-literal-dos ()
+  (let ((tempfile (make-temp-file "files-tests-test-hack-local-literal-dos" nil ".el")))
+    (unwind-protect
+        (progn
+          (with-temp-buffer
+            (insert ";; -*- mode: Emacs-Lisp -*-\^m\n;; Local Variables:\^m\n;; lol: t\^m\n;; End:\^m\n")
+            (write-file tempfile))
+          (with-current-buffer (find-file-literally tempfile)
+            (hack-local-variables)))
+      (delete-file tempfile))))
+
 (defvar files-test-bug-18141-file
   (expand-file-name "data/files-bug18141.el.gz" (getenv "EMACS_TEST_DIRECTORY"))
   "Test file for bug#18141.")
-- 
2.31.1

Re: [PATCH] Fix hack-local-variables for find-file-literally with dos encoding

[Lars Ingebrigtsen]

Tom Gillespie <tgbugs@gmail.com> writes:

>     This patch provides a test and a fix for an edge case when opening
> files literally that have dos crlf line endings. The patch was made
> against master, but a similar fix could be issued for a potential
> Emacs 27.3 release if one is forthcoming. Best!

There probably won't be a 27.3...

> -	      (subst-char-in-region (point) (point-max) ?\^m ?\n)
> +	      (unless noconv
> +	        (subst-char-in-region (point) (point-max) ?\^m ?\n))

I don't quite understand the logic here -- shouldn't this be a

              (when noconv

?  That is, if we're visiting the file literally, we want to delete the
carriage returns, but not otherwise?  (If we're not visiting the file
literally, the CRLFs will already have been translated into newlines.)

> +          (with-current-buffer (find-file-literally tempfile)
> +            (hack-local-variables)))

Should probably be a `should' in here to make it work as a test, I think.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: [PATCH] Fix hack-local-variables for find-file-literally with dos encoding

[Lars Ingebrigtsen]

Lars Ingebrigtsen <larsi@gnus.org> writes:

>> -	      (subst-char-in-region (point) (point-max) ?\^m ?\n)
>> +	      (unless noconv
>> +	        (subst-char-in-region (point) (point-max) ?\^m ?\n))

Looking at the existing code here, I think that `subst-char-in-region'
is wrong in any case, but following the history of that function is
pretty difficult.  It might have originated here?

commit e0b2a2d9afff14ce5babae84c50f24f1f0e89b02
Author:     Richard M. Stallman <rms@gnu.org>
AuthorDate: Mon Sep 20 16:12:57 2004 +0000

If the intention is to fix up DOS line endings, when it's the wrong
thing -- it should transform \r\n to \n, not transform any \r anywhere
in a line to \n...

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: [PATCH] Fix hack-local-variables for find-file-literally with dos encoding

[Tom Gillespie]

> Looking at the existing code here, I think that `subst-char-in-region'
> is wrong in any case, but following the history of that function is
> pretty difficult.  It might have originated here?

I was equally confused, so I left it alone.

> If the intention is to fix up DOS line endings, when it's the wrong
> thing -- it should transform \r\n to \n, not transform any \r anywhere
> in a line to \n...

I agree, what is strange to me is that as far as I can tell it should
be causing a bug by introducing blank lines in the temporary buffer
but somehow is not? I think the reason it is not might be because
subst-char-in-region doesn't actually do anything if the buffer has
dos encoding.

I think we might be able to remove the line entirely?

Re: [PATCH] Fix hack-local-variables for find-file-literally with dos encoding

[Tom Gillespie]

> There probably won't be a 27.3...

Ok, good to know.

> > -           (subst-char-in-region (point) (point-max) ?\^m ?\n)
> > +           (unless noconv
> > +             (subst-char-in-region (point) (point-max) ?\^m ?\n))
>
> I don't quite understand the logic here -- shouldn't this be a
>
>               (when noconv

No, when we read a file literally we don't want to normalize anything.
The issue is that 'when noconv' is set (i.e. we are in find-file-literally)
then we don't want to convert ?\^m because the value of suffix
contains ?\^m. Does this make sense? I tried other approaches
including modifying the suffix but this one was by far the cleanest.

> ?  That is, if we're visiting the file literally, we want to delete the
> carriage returns, but not otherwise?  (If we're not visiting the file
> literally, the CRLFs will already have been translated into newlines.)
>
> > +          (with-current-buffer (find-file-literally tempfile)
> > +            (hack-local-variables)))
>
> Should probably be a `should' in here to make it work as a test, I think.

I think that is right, it kind of works at the moment because the previous
behavior produces an error.

Re: [PATCH] Fix hack-local-variables for find-file-literally with dos encoding

[Tom Gillespie]

> > > +          (with-current-buffer (find-file-literally tempfile)
> > > +            (hack-local-variables)))
> >
> > Should probably be a `should' in here to make it work as a test, I think.
>
> I think that is right, it kind of works at the moment because the previous
> behavior produces an error.

Correction: hack-local-variables returns nil, so it is working as intended
because the original behavior is an error. Wrapping in should will produce
an incorrect test failure.

Re: [PATCH] Fix hack-local-variables for find-file-literally with dos encoding

[Tom Gillespie]

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

Here is an updated version of the patch that
removes the conversion line and adds a test
for find-file to make sure that nothing unexpected
is happening. Best,
Tom

[-- Attachment #2: 0001-Fix-hack-local-variables-for-find-file-literally-wit.patch --]
[-- Type: text/x-patch, Size: 2655 bytes --]

From 5717ccd1d405f108f3789e6a93912dec39d63e86 Mon Sep 17 00:00:00 2001
From: Tom Gillespie <tgbugs@gmail.com>
Date: Sun, 15 Aug 2021 16:06:29 -0700
Subject: [PATCH] Fix hack-local-variables for find-file-literally with dos
 encoding

* lisp/files.el (hack-local-variables--find-variables): Update
hack-local-variables--find-variables to remove spurious conversion
of ?\^m to ?\n.

* test/lisp/files-tests.el (files-tests-hack-local-literal-dos): Added
test for calling hack-local-variables on files with dos encoding opened
with find-file-literally and find-file.

This fixes a bug where hack-local-variables would fail to find a
matching suffix for local variables when files with dos line endings
were opened literally using find-file-literally.

The test for find-file and find-file-literally both succeed when the
conversion is removed, and it is not clear that it was doing anything
other than causing a bug matching suffix to local variable lines.
---
 lisp/files.el            |  1 -
 test/lisp/files-tests.el | 16 ++++++++++++++++
 2 files changed, 16 insertions(+), 1 deletion(-)

diff --git a/lisp/files.el b/lisp/files.el
index 875ac55316..b2c84cffaa 100644
--- a/lisp/files.el
+++ b/lisp/files.el
@@ -3947,7 +3947,6 @@ hack-local-variables--find-variables
 	    (with-temp-buffer
 	      (insert-buffer-substring thisbuf startpos endpos)
 	      (goto-char (point-min))
-	      (subst-char-in-region (point) (point-max) ?\^m ?\n)
 	      (while (not (eobp))
 	        ;; Discard the prefix.
 	        (if (looking-at prefix)
diff --git a/test/lisp/files-tests.el b/test/lisp/files-tests.el
index fb24b98595..e3c584dd28 100644
--- a/test/lisp/files-tests.el
+++ b/test/lisp/files-tests.el
@@ -164,6 +164,22 @@ files-tests-permanent-local-variables
       (hack-local-variables)
       (should (eq lexical-binding nil)))))
 
+(ert-deftest files-tests-hack-local-literal-dos ()
+  (let ((tempfile (make-temp-file "files-tests-test-hack-local-literal-dos" nil ".el")))
+    (unwind-protect
+        (progn
+          (with-temp-buffer
+            (insert ";; -*- mode: Emacs-Lisp -*-\^m\n;; Local Variables:\^m\n;; lol: t\^m\n;; End:\^m\n")
+            (write-file tempfile))
+          (with-current-buffer (find-file-literally tempfile)
+            (hack-local-variables)
+            (kill-buffer))
+          (with-current-buffer (let (enable-local-variables)
+                                 (find-file tempfile))
+            (hack-local-variables)
+            (kill-buffer)))
+      (delete-file tempfile))))
+
 (defvar files-test-bug-18141-file
   (ert-resource-file "files-bug18141.el.gz")
   "Test file for bug#18141.")
-- 
2.31.1

Re: [PATCH] Fix hack-local-variables for find-file-literally with dos encoding

[Eli Zaretskii]

> From: Tom Gillespie <tgbugs@gmail.com>
> Date: Sun, 15 Aug 2021 16:09:33 -0700
> Cc: Emacs developers <emacs-devel@gnu.org>
> 
> Here is an updated version of the patch that
> removes the conversion line and adds a test
> for find-file to make sure that nothing unexpected
> is happening. Best,

Could we please step back a notch?  I don't think I understand the
problem you are trying to solve, so could you please describe it in
more detail?  Without a sufficiently good understanding of the issue
you are trying to solve, I cannot make up my mind whether your
proposed solution is the best one.

TIA

Re: [PATCH] Fix hack-local-variables for find-file-literally with dos encoding

[Lars Ingebrigtsen]

Tom Gillespie <tgbugs@gmail.com> writes:

> No, when we read a file literally we don't want to normalize anything.
> The issue is that 'when noconv' is set (i.e. we are in find-file-literally)
> then we don't want to convert ?\^m because the value of suffix
> contains ?\^m. Does this make sense? I tried other approaches
> including modifying the suffix but this one was by far the cleanest.

Ah, I see what you mean now.  The code in
`hack-local-variables--find-variables' is clear enough in what it does,
but it's rather obscure in what it intends to do.

The issue is that if you've loaded a DOS file literally, and then you
want to run the local variables anyway, then you have a file like this:

;; -*- mode: Emacs-Lisp -*-^M
;; Local Variables:^M
;; lol: t^M
;; End:^M

Then we have this:

      (when (let ((case-fold-search t))
	      (search-forward "Local Variables:" nil t))
        (skip-chars-forward " \t")
        ;; suffix is what comes after "local variables:" in its line.
        ;; prefix is what comes before "local variables:" in its line.
        (let ((suffix
	       (concat
	        (regexp-quote (buffer-substring (point)
					        (line-end-position)))
	        "$"))

So...  it notes all characters that are after "local variables:", which
in this case is ^M.

Then later it does

	    (with-temp-buffer
	      (insert-buffer-substring thisbuf startpos endpos)
	      (goto-char (point-min))
	      (subst-char-in-region (point) (point-max) ?\^m ?\n)
	      (while (not (eobp))
	        ;; Discard the prefix.
	        (if (looking-at prefix)
		    (delete-region (point) (match-end 0))
		  (error "Local variables entry is missing the prefix"))

and this fails.

I think the intention here is to be able to say:

;; Local Variables:my-identifier
;; lol: t
;; End:my-identifier

although that's not something I can remember seeing in the wild.

So I think your revised patch is correct -- the `subst-char-in-region'
thing is just plain wrong and should be removed.

Does anybody else have any insights into this corner of the
hack-local-variables code?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: [PATCH] Fix hack-local-variables for find-file-literally with dos encoding

[Lars Ingebrigtsen]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> and this fails.

I mean, this is what fails:

	        ;; Discard the suffix.
	        (if (looking-back suffix (line-beginning-position))
		    (delete-region (match-beginning 0) (point))
		  (error "Local variables entry is missing the suffix"))
	        (forward-line 1))


-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: [PATCH] Fix hack-local-variables for find-file-literally with dos encoding

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Date: Mon, 16 Aug 2021 14:01:08 +0200
> Cc: Emacs developers <emacs-devel@gnu.org>
> 
> So I think your revised patch is correct -- the `subst-char-in-region'
> thing is just plain wrong and should be removed.

You only considered the case of DOS EOL format.  What about the Mac
EOL format?

More importantly, what about selective-display, which replaces
newlines with \r's?  I think this replacement is for that use case.

So perhaps the condition for the replacement should be
selective-display?

Re: [PATCH] Fix hack-local-variables for find-file-literally with dos encoding

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

> More importantly, what about selective-display, which replaces
> newlines with \r's?  I think this replacement is for that use case.
>
> So perhaps the condition for the replacement should be
> selective-display?

Aha!  The initial revision of files.el had:

+(defun hack-local-variables (&optional force)
[...]
+	    ;; Look at next local variable spec.
+	    (if selective-display (re-search-forward "[\n\C-m]")
+	      (forward-line 1))

So the ^M stuff that remains in here may have nothing to do with CRLF
DOS stuff, but is something that's left over from code meant to deal
with selective display?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: [PATCH] Fix hack-local-variables for find-file-literally with dos encoding

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: tgbugs@gmail.com,  emacs-devel@gnu.org
> Date: Mon, 16 Aug 2021 14:53:18 +0200
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > More importantly, what about selective-display, which replaces
> > newlines with \r's?  I think this replacement is for that use case.
> >
> > So perhaps the condition for the replacement should be
> > selective-display?
> 
> Aha!  The initial revision of files.el had:
> 
> +(defun hack-local-variables (&optional force)
> [...]
> +	    ;; Look at next local variable spec.
> +	    (if selective-display (re-search-forward "[\n\C-m]")
> +	      (forward-line 1))
> 
> So the ^M stuff that remains in here may have nothing to do with CRLF
> DOS stuff, but is something that's left over from code meant to deal
> with selective display?

Yes, I think so.  It just misfires in the obscure use case that
triggered this discussion.

Re: [PATCH] Fix hack-local-variables for find-file-literally with dos encoding

[Tom Gillespie]

Thank you for the insights. I will update the test to include the old
macos \r line endings and a file where local variables would be hidden
by selective-display and use (when selective-display as the condition
and then report back. Best!
Tom

Re: [PATCH] Fix hack-local-variables for find-file-literally with dos encoding

[Stefan Monnier]

> Thank you for the insights. I will update the test to include the old
> macos \r line endings and a file where local variables would be hidden
> by selective-display and use (when selective-display as the condition
> and then report back. Best!

Note that this use of selective-display is strongly deprecated, so it's
not important to fix it (but it's good to fix it, if it comes for free,
of course).


        Stefan

Re: [PATCH] Fix hack-local-variables for find-file-literally with dos encoding

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: Eli Zaretskii <eliz@gnu.org>,  Lars Ingebrigtsen <larsi@gnus.org>,
>   Emacs developers <emacs-devel@gnu.org>
> Date: Mon, 16 Aug 2021 13:49:21 -0400
> 
> > Thank you for the insights. I will update the test to include the old
> > macos \r line endings and a file where local variables would be hidden
> > by selective-display and use (when selective-display as the condition
> > and then report back. Best!
> 
> Note that this use of selective-display is strongly deprecated, so it's
> not important to fix it (but it's good to fix it, if it comes for free,
> of course).

We need first and foremost to fix the fallout from removing the
selective-display test from that place.