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

[Eli Zaretskii]

> From: Vladimir Sedach <vas@oneofus.la>
> Cc: 64311@debbugs.gnu.org
> Date: Fri, 30 Jun 2023 10:47:05 -0600
> 
> The problem is not with the variable value, the problem is with the
> variable binding.
> 
> If you look at shell.el right now, there are 3 places where the
> binding changes:
> 
> 1. shell.el:349:  defvaralias 'shell-dirtrack-mode 'shell-dirtrackp
> 2. shell.el:351:  defvar shell-dirtrackp t
> 3. shell.el:1170: define-minor-mode shell-dirtrack-mode
> 
> 1. assigns the alias
> 2. assigns a global binding type to shell-dirtrackp
> 3. assigns a buffer-local binding type to shell-dirtrackp
> 
> If you look at 2, you don't see that shell-dirtrackp becomes
> buffer-local. If you look at 3, you don't see that
> shell-dirtrack-mode gets a default value.

I think the alternative patch below solves all those issues.  If you
disagree, pleased tell what left-over problems you see after applying
that patch.

> Notice how the change in 9c3eeba4db26ddaeead100beea7a96f9fa640918 had
> another unintended effect: before the change, shell-dirtrackp would
> affect every shell-mode buffer; now setting the variable affects only
> the current buffer. Whether you consider that a bug or an "accidental
> improvement" is irrelevant. That commit was to fix compiler warnings,
> not to change global behavior.

I think this change is for the better, and the 5 years since that
change seem to at least tell us it had no negative effects on users.
And your proposed patch AFAICT does nothing to change this aspect of
that old commit, does it?

> > By contrast, removing the variable, or declaring it obsolete, is
> > backward-incompatible change in behavior, which we try to avoid at all
> > costs.  In this case, I see absolutely no justification for such
> > backward incompatibility.  We wouldn't be able to defend such a change
> > if it caused someone annoyance or, worse, breakage of their Emacs
> > setup and usage.
> 
> If you think the patch should do a defvaralias instead of a
> define-obsolete-variable-alias, that's fine.

shell.el already uses defvaralias.

> The reason I preferred to mark it obsolete is that variable aliases
> cause subtle bugs like this, and IMO are generally a bad idea.

It's too late for such considerations, since this alias has been with
us for many years, and it is quite possible some users depend on it in
their setups.  So changes that don't break their setups are
preferable.

Here's the patch I propose:

diff --git a/lisp/shell.el b/lisp/shell.el
index 5cf108b..4bbd295 100644
--- a/lisp/shell.el
+++ b/lisp/shell.el
@@ -348,8 +348,10 @@ shell-dirstack
 
 (defvaralias 'shell-dirtrack-mode 'shell-dirtrackp)
 
-(defvar shell-dirtrackp t
-  "Non-nil in a shell buffer means directory tracking is enabled.")
+(defvar-local shell-dirtrackp nil
+  "Non-nil in a shell buffer means directory tracking is enabled.
+Directory tracking (`shell-dirtrack-mode') is automatically enabled
+when `shell-mode' is activated.")
 
 (defvar shell-last-dir nil
   "Keep track of last directory for ksh `cd -' command.")
@@ -1129,6 +1131,7 @@ shell-extract-num
 
 (define-minor-mode shell-dirtrack-mode
   "Toggle directory tracking in this shell buffer (Shell Dirtrack mode).
+This assigns a buffer-local non-nil value to `shell-dirtrackp'.
 
 The `dirtrack' package provides an alternative implementation of
 this feature; see the function `dirtrack-mode'.  Also see