Re: [PATCH] Automatically set `geiser-guile-load-path' from .dir-locals

[Christopher Lemmer Webber]

Miguel Ángel Arruga Vivas writes:

> Hi Christopher,
>
> First of all I want to say sorry: I've tested this and reviewed the
> patch, and this is the second issue that already has caused, so yes, my
> tests weren't enough at all.
>
> Christopher Lemmer Webber <cwebber@dustycloud.org> writes:
>
>> Also, I hope this email isn't interpreted as being dismissive or negative
>> about what looks like it's probably a real usability improvement for
>> hacking on Guix!  I just thought my experience indicated maybe there are
>> additional considerations to address.
>
> At least from my side I see your report as something positive, I cannot
> see how could it be negative at all, and I'd like to thank you for it.
>
>> Christopher Lemmer Webber writes:
>>> I'm a bit unsure what the implications fully are with this change, but
>>> here was my user experience:
>>>
>>>  - Did a pull using magit to guix
>
> I use magit too, so I guess this isn't the source of the error.
>
>>>  - Suddenly every file I open in Guix is prompting me if I want to
>>>    enable these dir-locals, I notice some have "eval" and I don't know
>>>    what it's doing so I say no
>
> Saying no here shouldn't be a problem, as the variables should always be
> optional.
>
>>>  - But it's asking me every file
>
> If every file means "every file on guix project" that should be the
> normal behavior of Emacs for .dir-locals.el since this file was
> introduced long before the patch: you can mark the 'eval' lines to be
> accepted, or to be rejected always, but they're loaded with each file.
> A problem could be that the UI shows the ones you have already accepted
> too, and simply marks with * the new ones.
>
> If it means "every other file too", I'm unable to reproduce that with a
> fresh Emacs session.
>
>>>  - And oh no, it's asking me about ten times for every magit operation!
>>>    (Presumably due to the reload operation.)  Fine okay fine, YES, okay
>>>    cool it's quiet now... I hope that was safe.
>
> The only effects of the new code should be:
>
>   * First eval: Set guix-directory for emacs-guix to the folder where
>     .dir-locals.el is located.  This affects the whole emacs-guix, but
>     AFAIU this isn't your issue.
>
>   * Second eval:
>     - Make geiser-guile-load-path buffer-local, and optionally define it
>       as empty if it was void.
>     - Add the directory where .dir-locals.el is located to
>       geiser-guile-load-path.
>
>>> Later...
>>>
>>>  - I'm hacking on another file in another repo
>>>  - C-x v = (check to see a diff of the work-in-progress changes of the
>>>    file I'm working on)
>
> Thanks, with that I reproduce the problem, but I cannot debug it right
> now.  I'll be available in some hours, as soon as I have anything I'll
> update about this.
>
>>>  - Error in the minibuffer!  "Wrong type argument: stringp, nil"
>>>  - wtf, okay M-x toggle-debug-on-error
>>>
>>> Debugger entered--Lisp error: (wrong-type-argument stringp nil)
>>>   expand-file-name(nil)
>>>   (let* ((root-dir (expand-file-name (locate-dominating-file 
>>> default-directory ".dir-locals.el"))) (root-dir* (directory-file-name 
>>> root-dir))) (unless (boundp 'geiser-guile-load-path) (defvar 
>>> geiser-guile-load-path 'nil)) (make-local-variable 'geiser-guile-load-path) 
>>> (require 'cl-lib) (cl-pushnew root-dir* geiser-guile-load-path :test 
>>> #'string-equal))
>>>   eval((let* ((root-dir (expand-file-name (locate-dominating-file 
>>> default-directory ".dir-locals.el"))) (root-dir* (directory-file-name 
>>> root-dir))) (unless (boundp 'geiser-guile-load-path) (defvar 
>>> geiser-guile-load-path 'nil)) (make-local-variable 'geiser-guile-load-path) 
>>> (require 'cl-lib) (cl-pushnew root-dir* geiser-guile-load-path :test 
>>> #'string-equal)))
>>>   hack-one-local-variable(eval (let* ((root-dir (expand-file-name 
>>> (locate-dominating-file default-directory ".dir-locals.el"))) (root-dir* 
>>> (directory-file-name root-dir))) (unless (boundp 'geiser-guile-load-path) 
>>> (defvar geiser-guile-load-path 'nil)) (make-local-variable 
>>> 'geiser-guile-load-path) (require 'cl-lib) (cl-pushnew root-dir* 
>>> geiser-guile-load-path :test #'string-equal)))
>>>   hack-local-variables-apply()
>>>   hack-dir-local-variables-non-file-buffer()
>>>   diff-mode()
>>>   vc-diff-internal(t (Git 
>>> ("/home/cwebber/devel/scribble/scribble-lib/scribble...")) nil nil t)
>>>   vc-diff(nil t)
>>>   funcall-interactively(vc-diff nil t)
>>>   call-interactively(vc-diff nil nil)
>>>   command-execute(vc-diff)
>>>  
>>>  - Oh... it's whatever thing I enabled earlier in the guix repo.  Well
>>>    now my vc-diff is broken outside of there... :\
>>>
>>> I'm presuming that if I understood whatever this is doing, it's probably
>>> something that gives me a better user experience if I accept it while
>>> working on Guix.  But a) for whatever reason Emacs' dir-locals stuff is
>>> written in such a way that it antagonizes me for not accepting it and I
>>> didn't know what it eval was (maybe this is a lack of understanding in
>>> how to "say no and get it to listen to me"... I didn't resist for too
>>> long) and b) it seems like this change isn't scoped to Guix's checkout
>>> itself somehow...
>
> Sorry again, as soon as I have a bit of time I'll update.
>
> Happy hacking!
> Miguel

I figured out what was happening!  The bug is *technically* in vc-mode.
However, nontheless it manifested here...

Here's what happened.  vc-mode has some various hacks, as you can see
above with "hack-local-variables-apply"... which traverses the dirlocals
stuff.  (Not sure what the purpose is, didn't look too long.)

However for whatever reason, vc-mode also seems to be reusing buffers
such as `*vc-diff*'... and somehow still is left in the directory
context it *first* was used in.

Thus if I C-x v = in a guix-oriented buffer first, and then switch to
another completely different project and do the same, it's loading the
dirlocals from Guix(...!!!!) 

This is clearly a bug in vc-mode, I'll try to report it as such.

In the meanwhile, I used this hacky "fix".  Maybe worth applying for the
moment... what do you think of it?

#+BEGIN_SRC diff
diff --git a/.dir-locals.el b/.dir-locals.el
index 8e5d3902e3..2aa446a4f6 100644
--- a/.dir-locals.el
+++ b/.dir-locals.el
@@ -17,17 +17,19 @@
      ;; Geiser
      ;; This allows automatically setting the `geiser-guile-load-path'
      ;; variable when using various Guix checkouts (e.g., via git worktrees).
-     (eval . (let* ((root-dir (expand-file-name
-                               (locate-dominating-file
-                                default-directory ".dir-locals.el")))
-                    ;; Workaround for bug https://issues.guix.gnu.org/43818.
-                    (root-dir* (directory-file-name root-dir)))
-               (unless (boundp 'geiser-guile-load-path)
-                 (defvar geiser-guile-load-path '()))
-               (make-local-variable 'geiser-guile-load-path)
-               (require 'cl-lib)
-               (cl-pushnew root-dir* geiser-guile-load-path
-                           :test #'string-equal)))))
+     (eval . (let ((root-dir-unexpanded (locate-dominating-file
+                                         default-directory ".dir-locals.el")))
+               (when root-dir-unexpanded
+                 (let* ((root-dir (expand-file-name root-dir-unexpanded))
+                        ;; Workaround for bug 
https://issues.guix.gnu.org/43818.
+                        (root-dir* (directory-file-name root-dir)))
+                   
+                   (unless (boundp 'geiser-guile-load-path)
+                     (defvar geiser-guile-load-path '()))
+                   (make-local-variable 'geiser-guile-load-path)
+                   (require 'cl-lib)
+                   (cl-pushnew root-dir* geiser-guile-load-path
+                               :test #'string-equal)))))))
 
  (c-mode          . ((c-file-style . "gnu")))
  (scheme-mode
#+END_SRC

Btw, I'm not very familiar with dirlocals.  I see that setq is used in
the previous definition.  Woudl setq-local be better... or does it do
that by default?