bug#60730: 29.0.60; Free variable with :buffer keyword in ert-with-temp-file

[J.P.]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: "J.P." <jp@neverwas.me>
>> Cc: 60730@debbugs.gnu.org,  stefankangas@gmail.com
>> Date: Sun, 29 Jan 2023 06:08:01 -0800
>> 
>> > What we need to ensure that both
>> >
>> >   :coding 'raw-text
>> >
>> > and
>> >
>> >   :coding some-coding-variable
>> >
>> > do work as expected, including when coding-system-for-write's value is
>> > a non-nil symbol of a coding-system.
>> 
>> Right, whatever the solution, it should cover those bases. Although, if
>> `some-coding-variable' evaluates to nil, the change I proposed would not
>> fall back on `coding-system-for-write'. (But perhaps it should? [1])
>
> Setting :coding to nil is unusual and I don't expect that to happen.
> Its semantics is tricky and most people aren't aware of that, so they
> (rightfully) don't use it.

If it's unlikely that a user would specify nil explicitly or provide a
variable that might evaluate to nil, then the question of whether to
fall back to `coding-system-for-write' is less important.

>>   diff --git a/lisp/emacs-lisp/ert-x.el b/lisp/emacs-lisp/ert-x.el
>>   index 98a017c8a8e..2605fc22ddf 100644
>>   --- a/lisp/emacs-lisp/ert-x.el
>>   +++ b/lisp/emacs-lisp/ert-x.el
>>   @@ -475,7 +475,7 @@ ert-with-temp-file
>>            (:directory (setq directory (pop body)))
>>            (:text (setq text (pop body)))
>>            (:buffer (setq buffer (pop body)))
>>   -        (:coding (setq coding (pop body)))
>>   +        (:coding (setq coding (list (pop body))))
>>            (_ (push keyw extra-keywords) (pop body))))
>>        (when extra-keywords
>>          (error "Invalid keywords: %s" (mapconcat #'symbol-name 
>> extra-keywords " ")))
>>   @@ -484,7 +484,7 @@ ert-with-temp-file
>>              (suffix (or suffix ert-temp-file-suffix
>>                          (ert--with-temp-file-generate-suffix
>>                           (or (macroexp-file-name) buffer-file-name)))))
>>   -      `(let* ((coding-system-for-write ,(or coding 
>> coding-system-for-write))
>>   +      `(let* (,@(and coding `((coding-system-for-write ,(car coding))))
>>                  (,temp-file (,(if directory 'file-name-as-directory 
>> 'identity)
>>                               (make-temp-file ,prefix ,directory ,suffix 
>> ,text)))
>>                  (,name ,(if directory
>
> I don't think this is right.  coding-system-for-write exists and
> should be heeded because it gives the user control on binding the
> encoding just for this single command via "C-x RET c" prefix.  By
> contrast, the value that comes from :coding is determined by the Lisp
> program, and "C-x RET c" should override it.

Interesting. Makes sense to limit any damage done to that variable to
the extent of the test run. Guess that should have occurred to me.

So, based on what you've said, a couple remaining possibilities are

  diff --git a/lisp/emacs-lisp/ert-x.el b/lisp/emacs-lisp/ert-x.el
  index 98a017c8a8e..3439aacf1ab 100644
  --- a/lisp/emacs-lisp/ert-x.el
  +++ b/lisp/emacs-lisp/ert-x.el
  @@ -484,7 +484,7 @@ ert-with-temp-file
             (suffix (or suffix ert-temp-file-suffix
                         (ert--with-temp-file-generate-suffix
                          (or (macroexp-file-name) buffer-file-name)))))
  -      `(let* ((coding-system-for-write ,(or coding coding-system-for-write))
  +      `(let* ((coding-system-for-write (or ,coding coding-system-for-write))
                 (,temp-file (,(if directory 'file-name-as-directory 'identity)
                              (make-temp-file ,prefix ,directory ,suffix 
,text)))
                 (,name ,(if directory

and

  diff --git a/lisp/emacs-lisp/ert-x.el b/lisp/emacs-lisp/ert-x.el
  index 98a017c8a8e..3439aacf1ab 100644
  --- a/lisp/emacs-lisp/ert-x.el
  +++ b/lisp/emacs-lisp/ert-x.el
  @@ -484,7 +484,7 @@ ert-with-temp-file
             (suffix (or suffix ert-temp-file-suffix
                         (ert--with-temp-file-generate-suffix
                          (or (macroexp-file-name) buffer-file-name)))))
  -      `(let* ((coding-system-for-write ,(or coding coding-system-for-write))
  +      `(let* ((coding-system-for-write ,(or coding 'coding-system-for-write))
                 (,temp-file (,(if directory 'file-name-as-directory 'identity)
                              (make-temp-file ,prefix ,directory ,suffix 
,text)))
                 (,name ,(if directory

The bottom one doesn't fall back if `coding' is a variable that
evaluates to nil. Of course, there are surely other (perhaps smarter)
ways to go about this, in case anyone else wants to take a stab.