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

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

[Vladimir Sedach]

[-- Attachment #1: Type: text/plain, Size: 929 bytes --]


Fix shell-dirtrack-mode showing up as enabled in unrelated buffers

shell-dirtrack-mode always shows up in unrelated buffers.

Steps to reproduce:
1. emacs -q
2. M-x shell
3. describe-mode (C-h m) in *scratch* or *GNU Emacs*

This is because shell-dirtrack-mode is aliased to shell-dirtrackp,
which has default value t.

Changes in this patch:
mark shell-dirtrackp obsolete
replace existing occurrences of shell-dirtrackp with shell-dirtrack-mode
add :interactive (shell-mode) suggestion to shell-dirtrack-mode

shell-dirtrack-mode is still turned on by default. There is an
explicit call to (shell-dirtrack-mode 1) in the definition of
shell-mode.

Definition of shell-dirtrack-mode is moved to quiet warning about
reference to free variable ‘shell-dirtrack-mode’.

Note: I am not sure what to put for WHEN on the obsolete variable.
Does that get filled in when merging?

--
Vladimir Sedach


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-test-lisp-shell-tests.el-added-tests-for-shell-dirtr.patch --]
[-- Type: text/x-diff, Size: 1830 bytes --]

From 4f803f5f2c3e4df48e6509cf238dc420cebc4a0a Mon Sep 17 00:00:00 2001
From: Vladimir Sedach <vas@oneofus.la>
Date: Mon, 26 Jun 2023 22:30:25 -0600
Subject: [PATCH 1/2] ; * test/lisp/shell-tests.el: added tests for
 shell-dirtrack-mode

---
 test/lisp/shell-tests.el | 31 +++++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

diff --git a/test/lisp/shell-tests.el b/test/lisp/shell-tests.el
index db9124e2435..ddddfdb2e0f 100644
--- a/test/lisp/shell-tests.el
+++ b/test/lisp/shell-tests.el
@@ -64,4 +64,35 @@ shell-tests-split-string
     (should (equal (split-string-shell-command "ls /tmp/foo\\ bar")
                    '("ls" "/tmp/foo bar")))))
 
+(ert-deftest shell-dirtrack-on-by-default ()
+  (with-temp-buffer
+    (shell-mode)
+    (should shell-dirtrack-mode)))
+
+(ert-deftest shell-dirtrack-should-not-be-on-in-unrelated-modes ()
+  (with-temp-buffer
+    (should (not shell-dirtrack-mode))))
+
+(ert-deftest shell-dirtrack-sets-list-buffers-directory ()
+  (let ((start-dir default-directory))
+    (with-temp-buffer
+      (should-not list-buffers-directory)
+      (shell-mode)
+      (shell-cd "..")
+      (should list-buffers-directory)
+      (should (not (equal start-dir list-buffers-directory)))
+      (should (string-prefix-p list-buffers-directory start-dir)))))
+
+(ert-deftest shell-directory-tracker-cd ()
+  (let ((start-dir default-directory))
+    (with-temp-buffer
+      (should-not list-buffers-directory)
+      (shell-mode)
+      (cl-letf (((symbol-function 'shell-unquote-argument)
+                 (lambda (x) x)))
+        (shell-directory-tracker "cd .."))
+      (should list-buffers-directory)
+      (should (not (equal start-dir list-buffers-directory)))
+      (should (string-prefix-p list-buffers-directory start-dir)))))
+
 ;;; shell-tests.el ends here
-- 
2.20.1


[-- Attachment #3: 0002-Fix-shell-dirtrack-mode-showing-up-as-enabled-in-unr.patch --]
[-- Type: text/x-diff, Size: 4357 bytes --]

From 623704526b081c0d453a84d517f9d02035755504 Mon Sep 17 00:00:00 2001
From: Vladimir Sedach <vas@oneofus.la>
Date: Mon, 26 Jun 2023 22:32:07 -0600
Subject: [PATCH 2/2] Fix shell-dirtrack-mode showing up as enabled in
 unrelated buffers
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

; shell-dirtrack-mode always shows up in unrelated buffers.

; Steps to reproduce:
; 1. emacs -q
; 2. M-x shell
; 3. describe-mode (C-h m) in *scratch* or *GNU Emacs*

; This is because shell-dirtrack-mode is aliased to shell-dirtrackp,
; which has default value t.

; Changes in this patch:
; mark shell-dirtrackp obsolete
; replace existing occurrences of shell-dirtrackp with shell-dirtrack-mode
; add :interactive (shell-mode) suggestion to shell-dirtrack-mode

; shell-dirtrack-mode is still turned on by default. There is an
; explicit call to (shell-dirtrack-mode 1) in the definition of
; shell-mode.

; Definition of shell-dirtrack-mode is moved to quiet warning about
; reference to free variable ‘shell-dirtrack-mode’.
---
 lisp/shell.el | 39 ++++++++++++++++++++-------------------
 1 file changed, 20 insertions(+), 19 deletions(-)

diff --git a/lisp/shell.el b/lisp/shell.el
index b74442f1961..39e12577280 100644
--- a/lisp/shell.el
+++ b/lisp/shell.el
@@ -346,10 +346,10 @@ shell-dirstack
   "List of directories saved by pushd in this buffer's shell.
 Thus, this does not include the shell's current directory.")
 
-(defvaralias 'shell-dirtrack-mode 'shell-dirtrackp)
-
-(defvar shell-dirtrackp t
-  "Non-nil in a shell buffer means directory tracking is enabled.")
+(define-obsolete-variable-alias 'shell-dirtrackp 'shell-dirtrack-mode
+  "???"
+  "Non-nil in a shell buffer means directory tracking is enabled.
+Use the minor mode variable `shell-dirtrack-mode' instead.")
 
 (defvar shell-last-dir nil
   "Keep track of last directory for ksh `cd -' command.")
@@ -997,6 +997,20 @@ shell
 ;; replace it with a process filter that watches for and strips out
 ;; these messages.
 
+(define-minor-mode shell-dirtrack-mode
+  "Toggle directory tracking in this shell buffer (Shell Dirtrack mode).
+
+The `dirtrack' package provides an alternative implementation of
+this feature; see the function `dirtrack-mode'.  Also see
+`comint-osc-directory-tracker' for an escape-sequence based
+solution."
+  :lighter nil
+  :interactive (shell-mode)
+  (setq list-buffers-directory (if shell-dirtrack-mode default-directory))
+  (if shell-dirtrack-mode
+      (add-hook 'comint-input-filter-functions #'shell-directory-tracker nil t)
+    (remove-hook 'comint-input-filter-functions #'shell-directory-tracker t)))
+
 (defun shell-directory-tracker (str)
   "Tracks cd, pushd and popd commands issued to the shell.
 This function is called on each input passed to the shell.
@@ -1013,7 +1027,7 @@ shell-directory-tracker
 and `shell-pushd-dunique' control the behavior of the relevant command.
 
 Environment variables are expanded, see function `substitute-in-file-name'."
-  (if shell-dirtrackp
+  (if shell-dirtrack-mode
       ;; We fail gracefully if we think the command will fail in the shell.
 ;;;      (with-demoted-errors "Directory tracker failure: %s"
       ;; This fails so often that it seems better to just ignore errors (?).
@@ -1167,23 +1181,10 @@ shell-extract-num
   (and (string-match "^\\+[1-9][0-9]*$" str)
        (string-to-number str)))
 
-(define-minor-mode shell-dirtrack-mode
-  "Toggle directory tracking in this shell buffer (Shell Dirtrack mode).
-
-The `dirtrack' package provides an alternative implementation of
-this feature; see the function `dirtrack-mode'.  Also see
-`comint-osc-directory-tracker' for an escape-sequence based
-solution."
-  :lighter nil
-  (setq list-buffers-directory (if shell-dirtrack-mode default-directory))
-  (if shell-dirtrack-mode
-      (add-hook 'comint-input-filter-functions #'shell-directory-tracker nil t)
-    (remove-hook 'comint-input-filter-functions #'shell-directory-tracker t)))
-
 (defun shell-cd (dir)
   "Do normal `cd' to DIR, and set `list-buffers-directory'."
   (cd dir)
-  (if shell-dirtrackp
+  (if shell-dirtrack-mode
       (setq list-buffers-directory default-directory)))
 
 (defun shell-resync-dirs ()
-- 
2.20.1

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

[Eli Zaretskii]

> From: Vladimir Sedach <vas@oneofus.la>
> Date: Mon, 26 Jun 2023 22:39:50 -0600
> 
> Fix shell-dirtrack-mode showing up as enabled in unrelated buffers
> 
> shell-dirtrack-mode always shows up in unrelated buffers.
> 
> Steps to reproduce:
> 1. emacs -q
> 2. M-x shell
> 3. describe-mode (C-h m) in *scratch* or *GNU Emacs*

In what version of Emacs do you see this?  (You didn't include in your
report the information collected by "M-x report-emacs-bug", so I
cannot know what version and on what OS are you using.)  I cannot
reproduce this in Emacs 29: "C-h m" after "M-x shell" shows
shell-dirtrack-mode only in the *shell* buffer.  I do see it in
*scratch* in Emacs 28, so I guess we already fixed this?

> Note: I am not sure what to put for WHEN on the obsolete variable.
> Does that get filled in when merging?

No, you need to specify it explicitly.  It's the first Emacs version
where the variable was declared obsolete, in this case "30.1".  (But
meanwhile I don't think we need to install this.)

Thanks.

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

[Vladimir Sedach]

[-- Attachment #1: Type: text/plain, Size: 1254 bytes --]


Eli Zaretskii <eliz@gnu.org> writes:
> In what version of Emacs do you see this?  (You didn't include in your
> report the information collected by "M-x report-emacs-bug", so I
> cannot know what version and on what OS are you using.)  I cannot
> reproduce this in Emacs 29: "C-h m" after "M-x shell" shows
> shell-dirtrack-mode only in the *shell* buffer.  I do see it in
> *scratch* in Emacs 28, so I guess we already fixed this?

You are right about the steps to reproduce. What changed in Emacs 29
was describe-mode no longer using minor-mode-list, but
(buffer-local-value 'local-minor-modes buffer) instead.

This changed in commit 3c059f269e0182bd19df37871585e0b0bf1d47e5
Redo `C-h m' output
Wed Apr 13 03:50:06 2022 +0200

This only hides the problem, which is: the default value of
shell-dirtrack-mode being t.

Revised steps to reproduce:

1. emacs -q
2. M-x shell
3. M-: shell-dirtrack-mode in *scratch* or *GNU Emacs*

This is also demonstrated in the test
shell-dirtrack-should-not-be-on-in-unrelated-modes
from the first patch, which fails without the fix being applied.

Attached is a revised patch for the fix, with the obsolete variable
version filled in, and updated commit message.

Thank you for looking at this.

--
Vladimir Sedach


[-- Attachment #2: 0002-Fix-shell-dirtrack-mode-showing-up-as-enabled-in-unr.patch --]
[-- Type: text/x-diff, Size: 4360 bytes --]

From 971ac829043c6659acf7b315c0d610de1c7b0d10 Mon Sep 17 00:00:00 2001
From: Vladimir Sedach <vas@oneofus.la>
Date: Mon, 26 Jun 2023 22:32:07 -0600
Subject: [PATCH 2/2] Fix shell-dirtrack-mode showing up as enabled in
 unrelated buffers
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

; shell-dirtrack-mode always shows up in unrelated buffers.

; Steps to reproduce:
; 1. emacs -q
; 2. M-x shell
; 3. M-: shell-dirtrack-mode in *scratch* or *GNU Emacs*

; This is because shell-dirtrack-mode is aliased to shell-dirtrackp,
; which has default value t.

; Changes in this patch:
; mark shell-dirtrackp obsolete
; replace existing occurrences of shell-dirtrackp with shell-dirtrack-mode
; add :interactive (shell-mode) suggestion to shell-dirtrack-mode

; shell-dirtrack-mode is still turned on by default. There is an
; explicit call to (shell-dirtrack-mode 1) in the definition of
; shell-mode.

; Definition of shell-dirtrack-mode is moved to quiet warning about
; reference to free variable ‘shell-dirtrack-mode’.
---
 lisp/shell.el | 39 ++++++++++++++++++++-------------------
 1 file changed, 20 insertions(+), 19 deletions(-)

diff --git a/lisp/shell.el b/lisp/shell.el
index b74442f1961..e6f1c2629f3 100644
--- a/lisp/shell.el
+++ b/lisp/shell.el
@@ -346,10 +346,10 @@ shell-dirstack
   "List of directories saved by pushd in this buffer's shell.
 Thus, this does not include the shell's current directory.")
 
-(defvaralias 'shell-dirtrack-mode 'shell-dirtrackp)
-
-(defvar shell-dirtrackp t
-  "Non-nil in a shell buffer means directory tracking is enabled.")
+(define-obsolete-variable-alias 'shell-dirtrackp 'shell-dirtrack-mode
+  "30.1"
+  "Non-nil in a shell buffer means directory tracking is enabled.
+Use the minor mode variable `shell-dirtrack-mode' instead.")
 
 (defvar shell-last-dir nil
   "Keep track of last directory for ksh `cd -' command.")
@@ -997,6 +997,20 @@ shell
 ;; replace it with a process filter that watches for and strips out
 ;; these messages.
 
+(define-minor-mode shell-dirtrack-mode
+  "Toggle directory tracking in this shell buffer (Shell Dirtrack mode).
+
+The `dirtrack' package provides an alternative implementation of
+this feature; see the function `dirtrack-mode'.  Also see
+`comint-osc-directory-tracker' for an escape-sequence based
+solution."
+  :lighter nil
+  :interactive (shell-mode)
+  (setq list-buffers-directory (if shell-dirtrack-mode default-directory))
+  (if shell-dirtrack-mode
+      (add-hook 'comint-input-filter-functions #'shell-directory-tracker nil t)
+    (remove-hook 'comint-input-filter-functions #'shell-directory-tracker t)))
+
 (defun shell-directory-tracker (str)
   "Tracks cd, pushd and popd commands issued to the shell.
 This function is called on each input passed to the shell.
@@ -1013,7 +1027,7 @@ shell-directory-tracker
 and `shell-pushd-dunique' control the behavior of the relevant command.
 
 Environment variables are expanded, see function `substitute-in-file-name'."
-  (if shell-dirtrackp
+  (if shell-dirtrack-mode
       ;; We fail gracefully if we think the command will fail in the shell.
 ;;;      (with-demoted-errors "Directory tracker failure: %s"
       ;; This fails so often that it seems better to just ignore errors (?).
@@ -1167,23 +1181,10 @@ shell-extract-num
   (and (string-match "^\\+[1-9][0-9]*$" str)
        (string-to-number str)))
 
-(define-minor-mode shell-dirtrack-mode
-  "Toggle directory tracking in this shell buffer (Shell Dirtrack mode).
-
-The `dirtrack' package provides an alternative implementation of
-this feature; see the function `dirtrack-mode'.  Also see
-`comint-osc-directory-tracker' for an escape-sequence based
-solution."
-  :lighter nil
-  (setq list-buffers-directory (if shell-dirtrack-mode default-directory))
-  (if shell-dirtrack-mode
-      (add-hook 'comint-input-filter-functions #'shell-directory-tracker nil t)
-    (remove-hook 'comint-input-filter-functions #'shell-directory-tracker t)))
-
 (defun shell-cd (dir)
   "Do normal `cd' to DIR, and set `list-buffers-directory'."
   (cd dir)
-  (if shell-dirtrackp
+  (if shell-dirtrack-mode
       (setq list-buffers-directory default-directory)))
 
 (defun shell-resync-dirs ()
-- 
2.20.1

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: Tue, 27 Jun 2023 08:09:22 -0600
> 
> This changed in commit 3c059f269e0182bd19df37871585e0b0bf1d47e5
> Redo `C-h m' output
> Wed Apr 13 03:50:06 2022 +0200
> 
> This only hides the problem, which is: the default value of
> shell-dirtrack-mode being t.

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?

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?

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

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

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: Tue, 27 Jun 2023 18:07:27 -0600
> 
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > 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.

So if we make it also buffer-local, the problem is gone, no?

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

[Vladimir Sedach]

Eli Zaretskii <eliz@gnu.org> writes:

> So if we make it also buffer-local, the problem is gone, no?

--8<---------------cut here---------------start------------->8---
(local-variable-if-set-p 'shell-dirtrackp)
t
--8<---------------cut here---------------end--------------->8---

shell-dirtrackp is already buffer-local. Aliased variables share that property.

--
Vladimir Sedach

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: Wed, 28 Jun 2023 10:43:35 -0600
> 
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > So if we make it also buffer-local, the problem is gone, no?
> 
> --8<---------------cut here---------------start------------->8---
> (local-variable-if-set-p 'shell-dirtrackp)
> t
> --8<---------------cut here---------------end--------------->8---
> 
> shell-dirtrackp is already buffer-local. Aliased variables share that property.

So what exactly is the problem now?  You said:

> The problem is shell-dirtrackp and its default value.

But if this is buffer-local, then why is it a problem that it's
non-nil?

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

[Vladimir Sedach]

Eli Zaretskii <eliz@gnu.org> writes:

> So what exactly is the problem now?  You said:
>
>> The problem is shell-dirtrackp and its default value.
>
> But if this is buffer-local, then why is it a problem that it's
> non-nil?

Since shell-dirtrackp is aliased to shell-dirtrack-mode, the default
value of shell-dirtrack-mode gets set to t.

So shell-dirtrack-mode is t in every buffer.

--
Vladimir Sedach

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: Wed, 28 Jun 2023 14:14:44 -0600
> 
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > So what exactly is the problem now?  You said:
> >
> >> The problem is shell-dirtrackp and its default value.
> >
> > But if this is buffer-local, then why is it a problem that it's
> > non-nil?
> 
> Since shell-dirtrackp is aliased to shell-dirtrack-mode, the default
> value of shell-dirtrack-mode gets set to t.
> 
> So shell-dirtrack-mode is t in every buffer.

So making the default value nil will solve the problem?

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

[Vladimir Sedach]

Eli Zaretskii <eliz@gnu.org> writes:

> So making the default value nil will solve the problem?

Yes, but you are asking the wrong question.

When I came upon this problem, the first questions I had were: what
is the variable shell-dirtrackp, and why is it aliased to
shell-dirtrack-mode?

This is why I came up with a fix that follows the X-mode control
variable convention, marks the redundant variable obsolete, replaces
any leftover references to the redundant variable with
shell-dirtrack-mode, and adds unit tests for the control paths of the
affected code.

Instead of a half-assed "let's just flip the value of this boolean
flag" workaround, that doesn't help or explain anything to someone
trying to work with shell.el

--
Vladimir Sedach

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: Thu, 29 Jun 2023 10:26:38 -0600
> 
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > So making the default value nil will solve the problem?
> 
> Yes, but you are asking the wrong question.

Am I?  Asking about the root cause of the problem is not wrong,
because it indicates how best to fix it.

> When I came upon this problem, the first questions I had were: what
> is the variable shell-dirtrackp, and why is it aliased to
> shell-dirtrack-mode?

Why would we bother about that?  With the exception of the default
value, what harm does that variable cause by existing?

> This is why I came up with a fix that follows the X-mode control
> variable convention, marks the redundant variable obsolete, replaces
> any leftover references to the redundant variable with
> shell-dirtrack-mode, and adds unit tests for the control paths of the
> affected code.

Sorry, I'm not interested in making changes unrelated to the problem.
Making a variable obsolete causes Emacs emit annoying messages when
the variable is used, and that can be justified only if the variable
gets in the way.  This one doesn't.

So I think we should just make the default value nil, and be done.

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

[Vladimir Sedach]

Eli Zaretskii <eliz@gnu.org> writes:

> Am I?  Asking about the root cause of the problem is not wrong,
> because it indicates how best to fix it.

The root cause of the problem is the redundant variable
shell-dirtrackp, not its value. It is the variable aliasing in the
2018 commit 9c3eeba4db26ddaeead100beea7a96f9fa640918 that introduced
the bug.

This is why my patch addresses the root cause of the problem, instead
of setting the value of the variable (which commit
9c3eeba4db26ddaeead100beea7a96f9fa640918 did not touch).

> Why would we bother about that?  With the exception of the default
> value, what harm does that variable cause by existing?

It is misleading for someone trying to customize shell-mode, or work
on shell.el. I found it confusing on both counts. If it were not
confusing for you too, we obviously would not be having such a long
back-and-forth conversation about this bug.

--
Vladimir Sedach

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: Thu, 29 Jun 2023 13:24:58 -0600
> 
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > Am I?  Asking about the root cause of the problem is not wrong,
> > because it indicates how best to fix it.
> 
> The root cause of the problem is the redundant variable
> shell-dirtrackp, not its value. It is the variable aliasing in the
> 2018 commit 9c3eeba4db26ddaeead100beea7a96f9fa640918 that introduced
> the bug.
> 
> This is why my patch addresses the root cause of the problem, instead
> of setting the value of the variable (which commit
> 9c3eeba4db26ddaeead100beea7a96f9fa640918 did not touch).

The variable's existence is only the cause of the problem because of
its value.

> > Why would we bother about that?  With the exception of the default
> > value, what harm does that variable cause by existing?
> 
> It is misleading for someone trying to customize shell-mode, or work
> on shell.el. I found it confusing on both counts.

Such confusion can be prevented by adding appropriate comments.

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 it were not confusing for you too, we obviously would not be
> having such a long back-and-forth conversation about this bug.

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).

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

[Vladimir Sedach]

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

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

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

[Vladimir Sedach]

Eli Zaretskii <eliz@gnu.org> writes:
> 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.

How should defvaralias be used?

On the master branch I count 74 defvaralias declarations in lisp/,
excluding lisp/obsolete/ (there are some more generated dynamically
by org-babel). Of these, 73 are used for variable rename refactoring
(the 1 other use is vhdl-last-input-event as a compatibility shim for
xemacs).

There were 2 things that to me looked wrong with the existing
defvaralias declaration in shell.el:

1. As far as I can tell, every declaration aside from shell.el is of
   the form (defvaralias old-variable new-variable). The one in
   shell.el is the other way around.

2. There are only 2 occurrences where the old variable is still
   referenced in Emacs code: erc-join-buffer and shell-dirtrackp

Isn't the point of defvaralias in removing uses of the old variable
name from Emacs code?

--
Vladimir Sedach

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: Mon, 03 Jul 2023 11:03:37 -0600
> 
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> > 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.
> 
> How should defvaralias be used?
> 
> On the master branch I count 74 defvaralias declarations in lisp/,
> excluding lisp/obsolete/ (there are some more generated dynamically
> by org-babel). Of these, 73 are used for variable rename refactoring
> (the 1 other use is vhdl-last-input-event as a compatibility shim for
> xemacs).
> 
> There were 2 things that to me looked wrong with the existing
> defvaralias declaration in shell.el:
> 
> 1. As far as I can tell, every declaration aside from shell.el is of
>    the form (defvaralias old-variable new-variable). The one in
>    shell.el is the other way around.
> 
> 2. There are only 2 occurrences where the old variable is still
>    referenced in Emacs code: erc-join-buffer and shell-dirtrackp
> 
> Isn't the point of defvaralias in removing uses of the old variable
> name from Emacs code?

I'm not sure.  You may be right.  Let's see if Stefan (CC'ed) has any
comments.

But in any case, given how long we have this arrangement, I wouldn't
make any changes in it unless it causes real problems.  And I think
after installing the simple changes I proposed there are no real
problems left to justify such changes.

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

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

FWIW, my take on the root cause is that I made a mistake in commit
05327ca9724287cc3da4c625f180da5ab11be998 where I forgot to remove the
`defvar` of `shell-dirtrackp` (and I swapped the args to `defvaralias`).

It's fundamentally wrong to define a variable at 2 places.

> 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

This aligns the `defvar` of `shell-dirtrackp` with that of
`shell-dirtrack-mode`, which reduces the harm of the duplication, so
it fixes the worst part of my mess, thanks.

I'd prefer the original patch, tho, which should give basically the same
end result but without this weird duplicate definition.

I assume we're discussing this patch for `master`, not `emacs-29`, right?


        Stefan

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

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: Vladimir Sedach <vas@oneofus.la>,  64311@debbugs.gnu.org
> Date: Tue, 04 Jul 2023 10:28:55 -0400
> 
> FWIW, my take on the root cause is that I made a mistake in commit
> 05327ca9724287cc3da4c625f180da5ab11be998 where I forgot to remove the
> `defvar` of `shell-dirtrackp` (and I swapped the args to `defvaralias`).

The problem here for me is that this mistake was made long ago, and by
now there could be people out there who actually rely on this
problematic alias.

> > 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
> 
> This aligns the `defvar` of `shell-dirtrackp` with that of
> `shell-dirtrack-mode`, which reduces the harm of the duplication, so
> it fixes the worst part of my mess, thanks.
> 
> I'd prefer the original patch, tho, which should give basically the same
> end result but without this weird duplicate definition.

The original patch makes a backward-incompatible change, which for me
is a significant disadvantage.

> I assume we're discussing this patch for `master`, not `emacs-29`, right?

Yes.

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

[Vladimir Sedach]

Eli Zaretskii <eliz@gnu.org> writes:

> The original patch makes a backward-incompatible change, which for me
> is a significant disadvantage.

I think we are all in agreement now about keeping the defvaralias,
and not deprecating anything.

--
Vladimir Sedach

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

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

>> FWIW, my take on the root cause is that I made a mistake in commit
>> 05327ca9724287cc3da4c625f180da5ab11be998 where I forgot to remove the
>> `defvar` of `shell-dirtrackp` (and I swapped the args to `defvaralias`).
>
> The problem here for me is that this mistake was made long ago, and by
> now there could be people out there who actually rely on this
> problematic alias.

None of the patches I've seen remove the alias, AFAICT.
Some reverse the direction but I'm hard pressed to imagine a way for
code to be affected by this (unless they call `indirect-variable`,
obviously).

> The original patch makes a backward-incompatible change, which for me
> is a significant disadvantage.

I don't know what backward-incompatible you're referring to.


        Stefan

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

[Vladimir Sedach]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> I don't know what backward-incompatible you're referring to.

I wanted to replace the defvaralias with
define-obsolete-variable-alias, and Eli says that shell-dirtrackp
should not be marked obsolete, which makes sense.

--
Vladimir Sedach

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

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Vladimir Sedach [2023-07-04 16:27:17] wrote:
> Stefan Monnier <monnier@iro.umontreal.ca> writes:
>> I don't know what backward-incompatible you're referring to.
> I wanted to replace the defvaralias with
> define-obsolete-variable-alias,

`define-obsolete-variable-alias` includes `defvaralias`.
It just adds an obsolescence warning, but doesn't introduce any actual
changes in behavior.

> and Eli says that shell-dirtrackp should not be marked obsolete, which
> makes sense.

Not sure if it makes sense, but if that's his preference, it's OK with
me (the name `shell-dirtrackp` is fundamentally wrong since it's
a boolean variable and not a predicate (which is where the "p" suffix
is used), but I can live with it).


        Stefan

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

[Vladimir Sedach]

[-- Attachment #1: Type: text/plain, Size: 98 bytes --]


Attached is a patch that keeps defvaralias, and does not obsolete
anything.

--
Vladimir Sedach


[-- Attachment #2: 0002-Fix-shell-dirtrack-mode-showing-up-as-enabled-in-unr.patch --]
[-- Type: text/x-diff, Size: 4348 bytes --]

From 9edd92e6e2763f5e2d7bf6dc5009ce857f3d36ae Mon Sep 17 00:00:00 2001
From: Vladimir Sedach <vas@oneofus.la>
Date: Mon, 26 Jun 2023 22:32:07 -0600
Subject: [PATCH 2/2] Fix shell-dirtrack-mode showing up as enabled in
 unrelated buffers
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

; shell-dirtrack-mode always shows up in unrelated buffers.

; Steps to reproduce:
; 1. emacs -q
; 2. M-x shell
; 3. M-: shell-dirtrack-mode in *scratch* or *GNU Emacs*

; This is because shell-dirtrack-mode is aliased to shell-dirtrackp,
; which has default value t.

; Changes in this patch:
; remove redundant declaration of shell-dirtrackp
; replace existing occurrences of shell-dirtrackp with shell-dirtrack-mode
; add :interactive (shell-mode) suggestion to shell-dirtrack-mode

; shell-dirtrack-mode is still turned on by default. There is an
; explicit call to (shell-dirtrack-mode 1) in the definition of
; shell-mode.

; Definition of shell-dirtrack-mode is moved to quiet warning about
; reference to free variable ‘shell-dirtrack-mode’.
---
 lisp/shell.el | 38 +++++++++++++++++++-------------------
 1 file changed, 19 insertions(+), 19 deletions(-)

diff --git a/lisp/shell.el b/lisp/shell.el
index b74442f1961..47e7fa4b535 100644
--- a/lisp/shell.el
+++ b/lisp/shell.el
@@ -346,10 +346,9 @@ shell-dirstack
   "List of directories saved by pushd in this buffer's shell.
 Thus, this does not include the shell's current directory.")
 
-(defvaralias 'shell-dirtrack-mode 'shell-dirtrackp)
-
-(defvar shell-dirtrackp t
-  "Non-nil in a shell buffer means directory tracking is enabled.")
+(defvaralias 'shell-dirtrackp 'shell-dirtrack-mode
+  "Non-nil in a shell buffer means directory tracking is enabled.
+Superseded by the minor mode variable `shell-dirtrack-mode'.")
 
 (defvar shell-last-dir nil
   "Keep track of last directory for ksh `cd -' command.")
@@ -997,6 +996,20 @@ shell
 ;; replace it with a process filter that watches for and strips out
 ;; these messages.
 
+(define-minor-mode shell-dirtrack-mode
+  "Toggle directory tracking in this shell buffer (Shell Dirtrack mode).
+
+The `dirtrack' package provides an alternative implementation of
+this feature; see the function `dirtrack-mode'.  Also see
+`comint-osc-directory-tracker' for an escape-sequence based
+solution."
+  :lighter nil
+  :interactive (shell-mode)
+  (setq list-buffers-directory (if shell-dirtrack-mode default-directory))
+  (if shell-dirtrack-mode
+      (add-hook 'comint-input-filter-functions #'shell-directory-tracker nil t)
+    (remove-hook 'comint-input-filter-functions #'shell-directory-tracker t)))
+
 (defun shell-directory-tracker (str)
   "Tracks cd, pushd and popd commands issued to the shell.
 This function is called on each input passed to the shell.
@@ -1013,7 +1026,7 @@ shell-directory-tracker
 and `shell-pushd-dunique' control the behavior of the relevant command.
 
 Environment variables are expanded, see function `substitute-in-file-name'."
-  (if shell-dirtrackp
+  (if shell-dirtrack-mode
       ;; We fail gracefully if we think the command will fail in the shell.
 ;;;      (with-demoted-errors "Directory tracker failure: %s"
       ;; This fails so often that it seems better to just ignore errors (?).
@@ -1167,23 +1180,10 @@ shell-extract-num
   (and (string-match "^\\+[1-9][0-9]*$" str)
        (string-to-number str)))
 
-(define-minor-mode shell-dirtrack-mode
-  "Toggle directory tracking in this shell buffer (Shell Dirtrack mode).
-
-The `dirtrack' package provides an alternative implementation of
-this feature; see the function `dirtrack-mode'.  Also see
-`comint-osc-directory-tracker' for an escape-sequence based
-solution."
-  :lighter nil
-  (setq list-buffers-directory (if shell-dirtrack-mode default-directory))
-  (if shell-dirtrack-mode
-      (add-hook 'comint-input-filter-functions #'shell-directory-tracker nil t)
-    (remove-hook 'comint-input-filter-functions #'shell-directory-tracker t)))
-
 (defun shell-cd (dir)
   "Do normal `cd' to DIR, and set `list-buffers-directory'."
   (cd dir)
-  (if shell-dirtrackp
+  (if shell-dirtrack-mode
       (setq list-buffers-directory default-directory)))
 
 (defun shell-resync-dirs ()
-- 
2.20.1

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

[Eli Zaretskii]

> From: Vladimir Sedach <vas@oneofus.la>
> Cc: Eli Zaretskii <eliz@gnu.org>, 64311@debbugs.gnu.org
> Date: Thu, 06 Jul 2023 14:30:16 -0600
> 
> Attached is a patch that keeps defvaralias, and does not obsolete
> anything.

Thanks, I installed this, with slight changes, on the master branch,
and I'm therefore closing this bug.

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

[Vladimir Sedach]

[-- Attachment #1: Type: text/plain, Size: 286 bytes --]


Eli Zaretskii <eliz@gnu.org> writes:

> Thanks, I installed this, with slight changes, on the master branch,
> and I'm therefore closing this bug.

Thank you. There is also the unit tests patch in the original email
(attached here as well) that should be applied.

--
Vladimir Sedach


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-test-lisp-shell-tests.el-added-tests-for-shell-dirtr.patch --]
[-- Type: text/x-diff, Size: 1830 bytes --]

From ce4b041bd71cd7ee1b025cc14ae962f8d98d9478 Mon Sep 17 00:00:00 2001
From: Vladimir Sedach <vas@oneofus.la>
Date: Mon, 26 Jun 2023 22:30:25 -0600
Subject: [PATCH 1/2] ; * test/lisp/shell-tests.el: added tests for
 shell-dirtrack-mode

---
 test/lisp/shell-tests.el | 31 +++++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

diff --git a/test/lisp/shell-tests.el b/test/lisp/shell-tests.el
index db9124e2435..ddddfdb2e0f 100644
--- a/test/lisp/shell-tests.el
+++ b/test/lisp/shell-tests.el
@@ -64,4 +64,35 @@ shell-tests-split-string
     (should (equal (split-string-shell-command "ls /tmp/foo\\ bar")
                    '("ls" "/tmp/foo bar")))))
 
+(ert-deftest shell-dirtrack-on-by-default ()
+  (with-temp-buffer
+    (shell-mode)
+    (should shell-dirtrack-mode)))
+
+(ert-deftest shell-dirtrack-should-not-be-on-in-unrelated-modes ()
+  (with-temp-buffer
+    (should (not shell-dirtrack-mode))))
+
+(ert-deftest shell-dirtrack-sets-list-buffers-directory ()
+  (let ((start-dir default-directory))
+    (with-temp-buffer
+      (should-not list-buffers-directory)
+      (shell-mode)
+      (shell-cd "..")
+      (should list-buffers-directory)
+      (should (not (equal start-dir list-buffers-directory)))
+      (should (string-prefix-p list-buffers-directory start-dir)))))
+
+(ert-deftest shell-directory-tracker-cd ()
+  (let ((start-dir default-directory))
+    (with-temp-buffer
+      (should-not list-buffers-directory)
+      (shell-mode)
+      (cl-letf (((symbol-function 'shell-unquote-argument)
+                 (lambda (x) x)))
+        (shell-directory-tracker "cd .."))
+      (should list-buffers-directory)
+      (should (not (equal start-dir list-buffers-directory)))
+      (should (string-prefix-p list-buffers-directory start-dir)))))
+
 ;;; shell-tests.el ends here
-- 
2.20.1

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

[Eli Zaretskii]

> From: Vladimir Sedach <vas@oneofus.la>
> Cc: 64311-done@debbugs.gnu.org
> Date: Sat, 08 Jul 2023 10:18:40 -0600
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > Thanks, I installed this, with slight changes, on the master branch,
> > and I'm therefore closing this bug.
> 
> Thank you. There is also the unit tests patch in the original email
> (attached here as well) that should be applied.

Thanks, installed.

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

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

> diff --git a/lisp/shell.el b/lisp/shell.el
> index b74442f1961..39e12577280 100644
> --- a/lisp/shell.el
> +++ b/lisp/shell.el
> @@ -346,10 +346,10 @@ shell-dirstack
>    "List of directories saved by pushd in this buffer's shell.
>  Thus, this does not include the shell's current directory.")
>  
> -(defvaralias 'shell-dirtrack-mode 'shell-dirtrackp)
> -
> -(defvar shell-dirtrackp t
> -  "Non-nil in a shell buffer means directory tracking is enabled.")
> +(define-obsolete-variable-alias 'shell-dirtrackp 'shell-dirtrack-mode
> +  "???"
> +  "Non-nil in a shell buffer means directory tracking is enabled.
> +Use the minor mode variable `shell-dirtrack-mode' instead.")

Indeed `shell-dirtrackp` should not be defvar'd.
The above looks good to me.

> @@ -997,6 +997,20 @@ shell
>  ;; replace it with a process filter that watches for and strips out
>  ;; these messages.
>  
> +(define-minor-mode shell-dirtrack-mode
> +  "Toggle directory tracking in this shell buffer (Shell Dirtrack mode).
> +
> +The `dirtrack' package provides an alternative implementation of
> +this feature; see the function `dirtrack-mode'.  Also see
> +`comint-osc-directory-tracker' for an escape-sequence based
> +solution."
> +  :lighter nil
> +  :interactive (shell-mode)
> +  (setq list-buffers-directory (if shell-dirtrack-mode default-directory))
> +  (if shell-dirtrack-mode
> +      (add-hook 'comint-input-filter-functions #'shell-directory-tracker nil t)
> +    (remove-hook 'comint-input-filter-functions #'shell-directory-tracker t)))

You can make the patch smaller by keeping this definition where it is
and just add a (defvar shell-dirtrack-mode) here instead to silence the
compiler warnings.


        Stefan

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

[Eli Zaretskii]

> Cc: 64311@debbugs.gnu.org
> Date: Mon, 03 Jul 2023 23:32:49 -0400
> From:  Stefan Monnier via "Bug reports for GNU Emacs,
>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
> 
> > diff --git a/lisp/shell.el b/lisp/shell.el
> > index b74442f1961..39e12577280 100644
> > --- a/lisp/shell.el
> > +++ b/lisp/shell.el
> > @@ -346,10 +346,10 @@ shell-dirstack
> >    "List of directories saved by pushd in this buffer's shell.
> >  Thus, this does not include the shell's current directory.")
> >  
> > -(defvaralias 'shell-dirtrack-mode 'shell-dirtrackp)
> > -
> > -(defvar shell-dirtrackp t
> > -  "Non-nil in a shell buffer means directory tracking is enabled.")
> > +(define-obsolete-variable-alias 'shell-dirtrackp 'shell-dirtrack-mode
> > +  "???"
> > +  "Non-nil in a shell buffer means directory tracking is enabled.
> > +Use the minor mode variable `shell-dirtrack-mode' instead.")
> 
> Indeed `shell-dirtrackp` should not be defvar'd.
> The above looks good to me.

It doesn't look good to me, for the reasons I explained in the
discussion.  Do you see anything wrong with the alternative patch I
proposed instead?