Advice is active regardless of whether the mode is active

[phil-s]

(defadvice hack-one-local-variable (before dtrt-indent-advise-hack-one-local-variable activate)

That activate means that the advice is immediately doing things if the library is loaded, regardless of whether the user intends for dtrt-indent to be active.

The version I had installed used a :global minor mode, and therefore I had written a fix to enable/disable the advice in the body of that mode. I see that the current code has switched to a globalized mode, so the situation is more awkward now, as people can enable the buffer-local minor mode independently of the global mode.

As such my intended fix isn't useful as a pull request; but I still think that advice should do as little as possible in cases where dtrt-indent isn't going to act.

And in any case, there should be a defun dtrt-indent-unload-function which disables the advice (refer to (elisp) Unloading).

FWIW the following is what I'd written for the old global mode:

modified   el-get/dtrt-indent/dtrt-indent.el
@@ -159,7 +159,16 @@ mode.
 When dtrt-indent mode is enabled, the proper indentation
 offset will be guessed for newly opened files and adjusted
 transparently."
-  :global t :group 'dtrt-indent)
+  :global t :group 'dtrt-indent
+  (if dtrt-indent-mode
+      (progn
+        (ad-enable-advice 'hack-one-local-variable 'before
+                          'dtrt-indent-advise-hack-one-local-variable)
+        (ad-activate 'hack-one-local-variable))
+    ;; Disable mode.
+    (ad-disable-advice 'hack-one-local-variable 'before
+                       'dtrt-indent-advise-hack-one-local-variable)
+    (ad-activate 'hack-one-local-variable)))
 
 (defvar dtrt-indent-language-syntax-table
   '((c/c++/java ("\""                    0   "\""       nil "\\\\.")
@@ -929,7 +938,7 @@ Note: killed buffer-local value for %s, restoring to default %d"
 (add-hook 'dtrt-indent-unload-hook 'dtrt-indent-unload-hook)
 
 (defadvice hack-one-local-variable
-  (before dtrt-indent-advise-hack-one-local-variable activate)
+  (before dtrt-indent-advise-hack-one-local-variable disable)
   "Adviced by dtrt-indent.
 
 Disable dtrt-indent if offset explicitly set."

[phil-s]

Actually, now that there's a buffer-local minor mode, is there a reason for this advice to exist?

(As opposed to the minor mode taking care of it?)

[phil-s]

Also indent-tab-mode isn't a variable. That bit of the advice never worked??

[rrthomas]

Thanks very much for your issue and analysis.

Excusing myself as merely the maintainer, not the author, of this code, albeit one who relies on it every day, I am nervous of making changes. I would however be happy to review a pull request accompanied by the sort of exegesis you give above.

If I simply removed the advice, wouldn't that mean that the global mode would never be disabled automatically? (I find that really useful.) Also, since the action of the advice is only to disable dtrt-indent settings, that seems fairly safe even if it's not being used.

I agree that indent-tab-mode appears to be a typo for indent-tabs-mode, so I'll fix that; thanks.

[phil-s]

Ah, I see. I suspect you're right about the advice still being useful as-is, although you could possibly still do without it by just checking dir-local-variables-alist and file-local-variables-alist ?

The processing for local variables changed a bit in 26.1, so it would be worth testing in both 25 and 26.

[rrthomas]

Thanks again. I don't really have time to test in multiple Emacsen (unless someone can automate that in Travis‽) so I'm happy if it works in 26 (which I'm using).