bug#73005: [REGRESSION, BISECTED]: line numbers disappear when pressing `df` in evil-mode

[David Ponce]

On 04/11/2024 9:08 PM, David Ponce wrote:
> On 04/11/2024 8:53 PM, Konstantin Kharlamov wrote:
> > On Mon, 2024-11-04 at 22:50 +0300, Konstantin Kharlamov wrote:
> > > A more curious way to get line numbers is evaluating just `display-
> > > line-numbers`, i.e. without any modification.
> >
> > s/to get line numbers/to get line numbers back
> >
> > Sorry, I was re-editing this sentence and screwed it up 😅
>
> It seems the problem is the non-nil argument passed to
> `kill-all-local-variables' to also kill permanent local variables:
>
> Here is a minimal recipe:
>
> emacs -Q --eval "(setq-default display-line-numbers 'visual)"
>
> Then evaluate in the *scratch* buffer:
>
> (with-temp-buffer
>    (setq-local display-line-numbers nil)
>    (kill-all-local-variables t))
>
> ==> Line numbers disappear.
>
> emacs -Q --eval "(setq-default display-line-numbers 'visual)"
>
> Then evaluate in the *scratch* buffer:
>
> (with-temp-buffer
>    (setq-local display-line-numbers nil)
>    (kill-all-local-variables))
>
> ==> Line numbers don't change.
>
> No idea why.

I looked at the C code of `kill-all-local-variables' in buffer.c.
This function calls `reset_buffer_local_variables' to do the actual
work of killing local variables.  As far as I can see in the
implementation, when this function is invoked with a true argument to
also kill permanent local variables, it just clears the list of local
variable for the passed buffer.  On the contrary, when called with a
false argument to keep permanent local variables, more things are
done, like triggering variables watchers for example. This might
explain why `(kill-all-local-variables)' and
`(kill-all-local-variables t)' have different side effects on
`display-line-numbers'.

Based on the above analyze, a possible workaround in
`work-buffer--release' could be to call first
(kill-all-local-variables), then (kill-all-local-variables t), like
this:

(defun work-buffer--release (buffer)
  "Release work BUFFER."
  (if (buffer-live-p buffer)
      (with-current-buffer buffer
        ;; Flush BUFFER before making it available again, i.e. clear
        ;; its contents, remove all overlays and buffer-local
        ;; variables.  Is it enough to safely reuse the buffer?
        (let ((inhibit-read-only t)
              ;; Avoid deactivating the region as side effect.
              deactivate-mark)
          (erase-buffer))
        (delete-all-overlays)
        (let (change-major-mode-hook)
          (kill-all-local-variables) ;; Workaround bug#73005
          (kill-all-local-variables t))
        ;; Make the buffer available again.
        (push buffer work-buffer--list)))
  ;; If the maximum number of reusable work buffers is exceeded, kill
  ;; work buffer in excess, taking into account that the limit could
  ;; have been let-bound to temporarily increase its value.
  (when (> (length work-buffer--list) work-buffer-limit)
    (mapc #'kill-buffer (nthcdr work-buffer-limit work-buffer--list))
    (setq work-buffer--list (ntake work-buffer-limit work-buffer--list))))

If my understanding of the C code is correct, the extra cost of the
second call to `kill-all-local-variable' should be negligible.

I did test the above version of `work-buffer--release' which seems to
fix the issue with setting `display-line-numbers' in
`string-pixel-width', using Konstantin recipe.

Depending on what solution is preferred, I could prepare a patch
accordingly.

Thanks!