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

[Vladimir Sedach <vas@oneofus.la>]

Eli Zaretskii <eliz@gnu.org> writes:

> The discussion was long because I couldn't connect between the problem
> and the changes you proposed.  The solution I thought about
> immediately was just to change the value of the variable.  The rest
> was getting you to agree that such a change would indeed solve the
> problem (although you disagree it's the right solution).

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.

Where is the bug? Is it in step 1, 2, 3, or all of the above?

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.

So this was far from obvious for Glenn Morris, and it's not obvious
to you, and you are the Emacs maintainer. How is someone who is not
an elisp expert supposed to figure this out? How are people supposed
to avoid more bugs when touching this variable in future shell.el
changes?

> Such confusion can be prevented by adding appropriate comments.

Obviously not in this case, because comments do not affect how
variable bindings change.

> 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. The reason I preferred
to mark it obsolete is that variable aliases cause subtle bugs like
this, and IMO are generally a bad idea.

--
Vladimir Sedach