bug#48137: 27.2; `package-install-file' fails when loading a package file with DOS line endings

[Eli Zaretskii]

> From: Ioannis Kappas <ioannis.kappas@gmail.com>
> Date: Sat, 1 May 2021 12:48:06 +0100
> 
> The error appears to be a side effect of `package-install-file'
> loading the file literally with `insert-file-contents-literally' and
> thus setting the coding system to 'no-conversion (as reported by the
> `buffer-file-coding-system' variable ). This means that for a DOS
> encoded file the ?\r (Carriage Return) in the DOS line ending ?\r?\n
> will be treated as an independent control character rather than
> consumed as part of the line ending as far as Emacs is concerned.
> 
> This extra control character in our case appears in the version number
> extracted with `lm-header' (.e.g. "0.1^M"), failing the
> `version-to-list' conversion in
> `package-buffer-info'->`package-strip-rcs-id', resulting to the error
> that it lacks a version header.
> 
> It is not obvious why `package-install-file' loads package files
> literally at all times, `package--with-response-buffer-1' also does
> the same. If the reason turns out not to be important, then a possible
> solution could be to load package files metaphorically:
> 
> diff --git a/lisp/emacs-lisp/package.el b/lisp/emacs-lisp/package.el
> index ecb2573cab..98d63f1aff 100644
> --- a/lisp/emacs-lisp/package.el
> +++ b/lisp/emacs-lisp/package.el
> @@ -2147,8 +2147,11 @@ package-install-file
>          (progn
>            (setq default-directory file)
>            (dired-mode))
> -      (insert-file-contents-literally file)
> -      (when (string-match "\\.tar\\'" file) (tar-mode)))
> +      (if (string-match "\\.tar\\'" file)
> +          (progn
> +            (insert-file-contents-literally file)
> +            (tar-mode))
> +        (insert-file-contents file)))

I don't think this is TRT, because insert-file-contents also decodes
the character encoding, not just the EOL encoding.

> Another solution could be to upgrade the 'lisp-mnt package to ignore
> ?\r characters. Looking at the `lm-header' fn invoked by
> `package-buffer-info', it does have a list of characters to stop at
> when looking for a header, we can thus add the carriage return to the
> list:
> 
> diff --git a/lisp/emacs-lisp/lisp-mnt.el b/lisp/emacs-lisp/lisp-mnt.el
> index 9cba232e16..3eb493d286 100644
> --- a/lisp/emacs-lisp/lisp-mnt.el
> +++ b/lisp/emacs-lisp/lisp-mnt.el
> @@ -267,7 +267,7 @@ lm-header
>                 (if (save-excursion
>                       (skip-chars-backward "^$" (match-beginning 0))
>                       (= (point) (match-beginning 0)))
> -                   "[^\n]+" "[^$\n]+")))
> +                   "[^\n\r]+" "[^$\n\r]+")))

This is better, but IMO the code should be rewritten not to allow a
lone CR character, only either a lone LF or the CRLF pair.

> Or is it perhaps that packages should only be written with Unix line
> endings and thus the solution is to update documentation and create
> more targeted error messages?

This is probably the best, IMO.  Stefan?

Thanks.