bug#64311: [PATCH] Fix shell-dirtrack-mode showing up as enabled in unrelated buffers

[Vladimir Sedach]

Eli Zaretskii <eliz@gnu.org> writes:

> Why is that a problem.  I understand when it causes irrelevant minor
> mode to be shown by "C-h m", but why should anyone care that some
> global variable is non-nil?

My understanding is that (define-minor-mode X-mode ...) defines a
variable X-mode (the docstring for define-minor-mode calls this a
"control variable") that is supposed to be t when the mode is
enabled, and nil when the mode is not enabled.

Right now the variable shell-dirtrack-mode has a value of t, even
when the mode is not enabled.


> In any case, I don't think a fix (if we need one) should be so
> complicated.  Why do we need all those changes, including making the
> variable obsolete and moving the mode from its place in shell.el to
> another place there?  If all you want is to make this variable
> buffer-local, just making it buffer-local is all that's needed,
> right?

shell-dirtrack-mode is already made buffer-local by
define-minor-mode.

The problem is shell-dirtrackp and its default value.

What is shell-dirtrackp?

Looking at VC-history for shell-dirtrackp, there are 2 commits:

--8<---------------cut here---------------start------------->8---
commit 9c3eeba4db26ddaeead100beea7a96f9fa640918
Author: Glenn Morris <rgm@gnu.org>
Date:   Fri Apr 20 18:34:39 2018 -0400

    The tedious game of whack-a-mole with compiler warnings continues
...

diff --git a/lisp/shell.el b/lisp/shell.el
--- a/lisp/shell.el
+++ b/lisp/shell.el
@@ -317,4 +317,6 @@

+(defvaralias 'shell-dirtrack-mode 'shell-dirtrackp)
+
 (defvar shell-dirtrackp t
   "Non-nil in a shell buffer means directory tracking is enabled.")


commit b493a9b2af805a3097fe53fd472884c268248146
Author: Richard M. Stallman <rms@gnu.org>
Date:   Wed Mar 2 16:55:16 1994 +0000

    (shell-dirtrackp): Variable definition added.

diff --git a/lisp/shell.el b/lisp/shell.el
--- a/lisp/shell.el
+++ b/lisp/shell.el
@@ -226,1 +223,4 @@

+(defvar shell-dirtrackp t
+  "Non-nil in a shell buffer means directory tracking is enabled.")
+
--8<---------------cut here---------------end--------------->8---

So it looks like in 1994 rms introduced the variable shell-dirtrackp,
before define-minor-mode and the X-mode variable convention. Judging
by the documentation string, shell-dirtrackp was intended to do what
the automatically defined X-mode variables do now.

Then in 2018 rgm aliased shell-dirtrackp to shell-dirtrack-mode to
fix a compiler warning. This introduced an incorrect default value
for shell-dirtrack-mode.

The variable shell-dirtrackp should also have been marked obsolete in
rgm's commit.

I moved the definition of shell-dirtrack-mode above the first use of
the variable shell-dirtrack-mode so there would be no compiler
warning (this is noted in the commit message). This also puts the
definition of shell-dirtrack-mode right after the long comment for
the Directory tracking section explaining the mode's purpose, a nice
unintended benefit.

> But first, let's talk about the problem: why is shell-dirtrack-mode
> being t a problem?

If you have a hook that tests if shell-dirtrack-mode is turned on by
looking at the value of the variable shell-dirtrack-mode, that hook
will not work correctly.

--
Vladimir Sedach