[david.hansen@gmx.net: Re: comint's directory tracking doesn't understand \( or \)]

[david.hansen@gmx.net: Re: comint's directory tracking doesn't understand \( or \)]

[Richard Stallman]

Would someone please study this, install it if it is safe, then ack?

------- Start of forwarded message -------
Mail-Followup-To: emacs-pretest-bug@gnu.org
To: emacs-pretest-bug@gnu.org
From: David Hansen <david.hansen@gmx.net>
Date: Fri, 02 Mar 2007 05:37:27 +0100
Organization: disorganized
Mime-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Mail-Copies-To: nobody
Subject: Re: comint's directory tracking doesn't understand \( or \)
X-Spam-Status: No, score=0.0 required=5.0 tests=none autolearn=failed 
	version=3.0.4

On Thu, 01 Mar 2007 22:28:53 -0500 Richard Stallman wrote:

> Can you write a fix for this?
> We could install the fix, if it is simple and safe.

I already reported that one or two weeks ago.

I can't say if the fix will be safe.  I simply don't know enough
about other comint modes (not a bash shell buffer).

This patch fixes at least one related problem (now arbitrary
characters can be escaped with a backslash).  But this is not the
full fix.  `comint-delim-arg' still breaks at characters in
`comint-delimiter-argument-list' even if they are escaped with a
backslash.

This should be easy to fix but I have no idea if this may break
other comint modes (what about MS-DOS directory delimiters?).

BTW, I have the feeling that how comint splits arguments is overly
complicated.  These regular expressions are just terrible and it
can be easily (and most probably faster) expressed by some loop
across all chars and maintaining a simple state.  But that's for
after the release :)

David

Index: comint.el
===================================================================
RCS file: /sources/emacs/emacs/lisp/comint.el,v
retrieving revision 1.358
diff -c -r1.358 comint.el
*** comint.el	23 Feb 2007 19:21:25 -0000	1.358
- --- comint.el	2 Mar 2007 04:27:30 -0000
***************
*** 1384,1390 ****
    (let* ((first (if (if (fboundp 'w32-shell-dos-semantics)
  			(w32-shell-dos-semantics))
  		    "[^ \n\t\"'`]+\\|"
! 		  "[^ \n\t\"'`\\]+\\|\\\\[\"'`\\ \t]+\\|"))
  	 (argpart (concat first
  			  "\\(\"\\([^\"\\]\\|\\\\.\\)*\"\\|\
  '[^']*'\\|\
- --- 1384,1390 ----
    (let* ((first (if (if (fboundp 'w32-shell-dos-semantics)
  			(w32-shell-dos-semantics))
  		    "[^ \n\t\"'`]+\\|"
!                   "[^ \n\t\"'`\\]+\\|\\(?:\\\\.\\)+\\|"))
  	 (argpart (concat first
  			  "\\(\"\\([^\"\\]\\|\\\\.\\)*\"\\|\
  '[^']*'\\|\



_______________________________________________
emacs-pretest-bug mailing list
emacs-pretest-bug@gnu.org
http://lists.gnu.org/mailman/listinfo/emacs-pretest-bug
------- End of forwarded message -------

Re: [david.hansen@gmx.net: Re: comint's directory tracking doesn't understand \( or \)]

[David Hansen]

On Fri, 02 Mar 2007 12:44:48 -0500 Richard Stallman wrote:

> Would someone please study this, install it if it is safe, then ack?

Alone it's doing not that much.  The attached patch should fix all
problems with shell-modes directory tracking and special characters
in directory names.

I'm sorry that it's a bit longish, but the original regexps are a
bit to much for my small brain ;)

David

*** comint.el	04 Mar 2007 13:04:02 +0100	1.358
--- comint.el	04 Mar 2007 14:07:56 +0100	
***************
*** 105,110 ****
--- 105,112 ----
  ;;; Code:
  
  (require 'ring)
+ (eval-when-compile (require 'cl))
+ 
  \f
  ;; Buffer Local Variables:
  ;;============================================================================
***************
*** 1344,1371 ****
  			(t nth))))
  	(comint-arguments string nth mth)))))
  
- (defun comint-delim-arg (arg)
-   "Return a list of arguments from ARG.
- Break it up at the delimiters in `comint-delimiter-argument-list'.
- Returned list is backwards."
-   (if (null comint-delimiter-argument-list)
-       (list arg)
-     (let ((args nil)
- 	  (pos 0)
- 	  (len (length arg)))
-       (while (< pos len)
- 	(let ((char (aref arg pos))
- 	      (start pos))
- 	  (if (memq char comint-delimiter-argument-list)
- 	      (while (and (< pos len) (eq (aref arg pos) char))
- 		(setq pos (1+ pos)))
- 	    (while (and (< pos len)
- 			(not (memq (aref arg pos)
- 				   comint-delimiter-argument-list)))
- 	      (setq pos (1+ pos))))
- 	  (setq args (cons (substring arg start pos) args))))
-       args)))
- 
  (defun comint-arguments (string nth mth)
    "Return from STRING the NTH to MTH arguments.
  NTH and/or MTH can be nil, which means the last argument.
--- 1346,1351 ----
***************
*** 1375,1423 ****
  Also, a run of one or more of a single character
  in `comint-delimiter-argument-list' is a separate argument.
  Argument 0 is the command name."
!   ;; The first line handles ordinary characters and backslash-sequences
!   ;; (except with w32 msdos-like shells, where backslashes are valid).
!   ;; The second matches "-quoted strings.
!   ;; The third matches '-quoted strings.
!   ;; The fourth matches `-quoted strings.
!   ;; This seems to fit the syntax of BASH 2.0.
!   (let* ((first (if (if (fboundp 'w32-shell-dos-semantics)
! 			(w32-shell-dos-semantics))
! 		    "[^ \n\t\"'`]+\\|"
! 		  "[^ \n\t\"'`\\]+\\|\\\\[\"'`\\ \t]+\\|"))
! 	 (argpart (concat first
! 			  "\\(\"\\([^\"\\]\\|\\\\.\\)*\"\\|\
! '[^']*'\\|\
! `[^`]*`\\)"))
! 	 (args ()) (pos 0)
! 	 (count 0)
! 	 beg str quotes)
!     ;; Build a list of all the args until we have as many as we want.
!     (while (and (or (null mth) (<= count mth))
! 		(string-match argpart string pos))
!       (if (and beg (= pos (match-beginning 0)))
! 	  ;; It's contiguous, part of the same arg.
! 	  (setq pos (match-end 0)
! 		quotes (or quotes (match-beginning 1)))
! 	;; It's a new separate arg.
! 	(if beg
! 	    ;; Put the previous arg, if there was one, onto ARGS.
! 	    (setq str (substring string beg pos)
! 		  args (if quotes (cons str args)
! 			 (nconc (comint-delim-arg str) args))))
! 	(setq count (length args))
! 	(setq quotes (match-beginning 1))
! 	(setq beg (match-beginning 0))
! 	(setq pos (match-end 0))))
!     (if beg
! 	(setq str (substring string beg pos)
! 	      args (if quotes (cons str args)
! 		     (nconc (comint-delim-arg str) args))))
!     (setq count (length args))
!     (let ((n (or nth (1- count)))
! 	  (m (if mth (1- (- count mth)) 0)))
!       (mapconcat
!        (function (lambda (a) a)) (nthcdr n (nreverse (nthcdr m args))) " "))))
  \f
  ;;
  ;; Input processing stuff
--- 1355,1414 ----
  Also, a run of one or more of a single character
  in `comint-delimiter-argument-list' is a separate argument.
  Argument 0 is the command name."
!   (let ((esc (unless (and (fboundp 'w32-shell-dos-semantics)
!                           (w32-shell-dos-semantics))
!                ?\\))
!         (ifs '(?\n ?\t ?\ ))
!         (len (length string))
!         (i 0) (beg 0) state args)
!     (flet ((push-arg (new-beg)
!              ;; with I at the end of an arg push it to `args' and set the
!              ;; beginning of the next arg `beg' to NEW-BEG
!              (push (substring string beg i) args)
!              (and mth (decf mth))
!              (setq beg new-beg)))
!       (while (and (<= i len) (or (not mth) (>= mth 0)))
!         (let ((s (car state))
!               (c (and (< i len) (aref string i))))
!           (cond ((and (integerp s) (not (eq s c)))
!                  (push-arg i)
!                  (pop state)
!                  (decf i))              ; parse this character again
!                 ((eq 'escaped s)
!                  (pop state))
!                 ((eq ?\' c)
!                  (if (eq 'single-quote s)
!                      (pop state)
!                    (or s (push 'single-quote state))))
!                 ((eq ?\" c)
!                  (if (eq 'double-quote s)
!                      (pop state)
!                    (push 'double-quote state)))
!                 ((and esc (eq esc c) (not (eq 'single-quote s)))
!                  (push 'escaped state))
!                 ((and (not s) (member c ifs))
!                  (if (= beg i)
!                      (incf beg)         ; just a second whitespace
!                    (push-arg (1+ i))))
!                 ((and (not s) (member c comint-delimiter-argument-list))
!                  (push c state)
!                  (when (/= beg i)       ; no whitespace before this character
!                    (push-arg i)))
!                 ((not c)                ; end of the string
!                  (unless (= beg len)
!                    (push-arg len))))
!           (incf i)))
!       (if (not nth)
!           ;; if MTH is non nil only return the last argument if there are no
!           ;; non parsed arguments left
!           (or (and (or (not mth)
!                        (>= i len)
!                        (string-match "[\t\n ]*$" string i))
!                    (car args))
!               "")
!         (setf (nthcdr (- (length args) nth) args) nil)
!         (mapconcat #'identity (nreverse args) " ")))))
! 
  \f
  ;;
  ;; Input processing stuff

Re: [david.hansen@gmx.net: Re: comint's directory tracking doesn't understand \( or \)]

[Chong Yidong]

David Hansen <david.hansen@gmx.net> writes:

> On Fri, 02 Mar 2007 12:44:48 -0500 Richard Stallman wrote:
>
>> Would someone please study this, install it if it is safe, then ack?
>
> Alone it's doing not that much.  The attached patch should fix all
> problems with shell-modes directory tracking and special characters
> in directory names.
>
> I'm sorry that it's a bit longish, but the original regexps are a
> bit to much for my small brain ;)

I'm leery of introducing this kind of change into comint.el at this
stage in the release.  It doesn't seem to fit Richard's criterion of
being "simple and safe".

BTW, I can't seem to reproduce the problem referenced in the original
bug report.

M-x shell RET
~ $ cd /tmp/
/tmp $ mkdir /tmp/'(2007)'
/tmp $ cd \(2007\)
/tmp/(2007) $ pwd

 => /tmp/(2007)

This is GNU Emacs 22.0.95.3 (i686-pc-linux-gnu, GTK+ Version 2.10.6)
of 2007-03-04.  Could this be a shell bug on certain systems?

> The *shell* buffer attempts to cd into the current working directory
> of the inferior shell process, but when that directory contains a ( or
> ), it seems to get lost, for example:
>
> M-x shell RET
> $ cd /tmp/
> $ mkdir /tmp/'(2007)'
> $ cd \(2007\)/
>
> The *shell* buffer ends up in /tmp/, not in /tmp/(2007)/ as expected.
>
> Note that if I use:
>
> $ cd '(2007)'
>
> instead, then the tracking works.

Re: [david.hansen@gmx.net: Re: comint's directory tracking doesn't understand \( or \)]

[David Kastrup]

Chong Yidong <cyd@stupidchicken.com> writes:

> David Hansen <david.hansen@gmx.net> writes:
>
>> On Fri, 02 Mar 2007 12:44:48 -0500 Richard Stallman wrote:
>>
>>> Would someone please study this, install it if it is safe, then ack?
>>
>> Alone it's doing not that much.  The attached patch should fix all
>> problems with shell-modes directory tracking and special characters
>> in directory names.
>>
>> I'm sorry that it's a bit longish, but the original regexps are a
>> bit to much for my small brain ;)
>
> I'm leery of introducing this kind of change into comint.el at this
> stage in the release.  It doesn't seem to fit Richard's criterion of
> being "simple and safe".
>
> BTW, I can't seem to reproduce the problem referenced in the original
> bug report.
>
> M-x shell RET
> ~ $ cd /tmp/
> /tmp $ mkdir /tmp/'(2007)'
> /tmp $ cd \(2007\)
> /tmp/(2007) $ pwd
>
>  => /tmp/(2007)

Uh, I guess you should not use 
pwd RET
but rather
M-x pwd RET

for checking the problem.

-- 
David Kastrup, Kriemhildstr. 15, 44793 Bochum

Re: [david.hansen@gmx.net: Re: comint's directory tracking doesn't understand \( or \)]

[Chong Yidong]

David Kastrup <dak@gnu.org> writes:

>> BTW, I can't seem to reproduce the problem referenced in the original
>> bug report.
>>
>> M-x shell RET
>> ~ $ cd /tmp/
>> /tmp $ mkdir /tmp/'(2007)'
>> /tmp $ cd \(2007\)
>> /tmp/(2007) $ pwd
>>
>>  => /tmp/(2007)
>
> Uh, I guess you should not use 
> pwd RET
> but rather
> M-x pwd RET
>
> for checking the problem.

I understand the problem now (the original recipe wasn't clear).

I don't think we should make the proposed change to comint.el.  AFAIK,
the detailed rules for how backslash escape works is, in principle,
different from shell to shell, and even if we choose to obey (e.g.)
bash semantics for backslash escapes, we might still be incompatible
with other shells.  This might also introduce subtle bugs into
non-shell uses of comint mode.

It seems there will always be *some* way of confusing the directory
tracker: that's what `M-x dirs' is for.

Re: [david.hansen@gmx.net: Re: comint's directory tracking doesn't understand \( or \)]

[David Kastrup]

Chong Yidong <cyd@stupidchicken.com> writes:

> David Kastrup <dak@gnu.org> writes:
>
>>> BTW, I can't seem to reproduce the problem referenced in the original
>>> bug report.
>>>
>>> M-x shell RET
>>> ~ $ cd /tmp/
>>> /tmp $ mkdir /tmp/'(2007)'
>>> /tmp $ cd \(2007\)
>>> /tmp/(2007) $ pwd
>>>
>>>  => /tmp/(2007)
>>
>> Uh, I guess you should not use 
>> pwd RET
>> but rather
>> M-x pwd RET
>>
>> for checking the problem.
>
> I understand the problem now (the original recipe wasn't clear).
>
> I don't think we should make the proposed change to comint.el.
> AFAIK, the detailed rules for how backslash escape works is, in
> principle, different from shell to shell, and even if we choose to
> obey (e.g.)  bash semantics for backslash escapes, we might still be
> incompatible with other shells.  This might also introduce subtle
> bugs into non-shell uses of comint mode.
>
> It seems there will always be *some* way of confusing the directory
> tracker: that's what `M-x dirs' is for.

Maybe the directory tracker should check whether the directory it is
thinking of exists, and use `dirs' alias `shell-resync-dirs'
automatically if it finds that it doesn't.  I think it currently
instead just stays where it was previously.  Which rarely is helpful.

-- 
David Kastrup, Kriemhildstr. 15, 44793 Bochum

Re: [david.hansen@gmx.net: Re: comint's directory tracking doesn't understand \( or \)]

[Lennart Borgman (gmail)]

David Kastrup wrote:

> Maybe the directory tracker should check whether the directory it is
> thinking of exists, and use `dirs' alias `shell-resync-dirs'
> automatically if it finds that it doesn't.  I think it currently
> instead just stays where it was previously.  Which rarely is helpful.


But it does not know what program is recieving the input, or? Should it 
not ask the user then?

Re: [david.hansen@gmx.net: Re: comint's directory tracking doesn't understand \( or \)]

[Robert J. Chassell]

   > M-x shell RET
   > ~ $ cd /tmp/
   > /tmp $ mkdir /tmp/'(2007)'
   > /tmp $ cd \(2007\)
   > /tmp/(2007) $ pwd
   >
   >  => /tmp/(2007)

   Uh, I guess you should not use 
   pwd RET
   but rather
   M-x pwd RET

I don't reproduce the problem either.  Emacs works fine.

When I do the above with pwd in the *shell* buffer, I, too, see
/tmp/(2007)

When I do 
       M-x pwd RET
I see the default directory for the buffer, which in my case is  /

After changing the default directory for the buffer with
       M-x cd RET /tmp/(2007) RET
when I do 
       M-x pwd RET
I see  
       /tmp/(2007)/
as expected.

-- 
    Robert J. Chassell                          GnuPG Key ID: 004B4AC8
    bob@rattlesnake.com                         bob@gnu.org
    http://www.rattlesnake.com                  http://www.teak.cc

Re: [david.hansen@gmx.net: Re: comint's directory tracking doesn't understand \( or \)]

[Miles Bader]

David Kastrup <dak@gnu.org> writes:
> Maybe the directory tracker should check whether the directory it is
> thinking of exists, and use `dirs' alias `shell-resync-dirs'
> automatically if it finds that it doesn't.

In general that sort of thing is simply too dangerous -- comint has no
idea when it's safe to use `dirs', because it doesn't know the state of
the child process.

> I think it currently instead just stays where it was previously.
> Which rarely is helpful.

Maybe not, but there doesn't seem much choice.

-Miles

-- 
If you can't beat them, arrange to have them beaten.  [George Carlin]

Re: [david.hansen@gmx.net: Re: comint's directory tracking doesn't understand \( or \)]

[David Kastrup]

"Lennart Borgman (gmail)" <lennart.borgman@gmail.com> writes:

> David Kastrup wrote:
>
>> Maybe the directory tracker should check whether the directory it
>> is thinking of exists, and use `dirs' alias `shell-resync-dirs'
>> automatically if it finds that it doesn't.  I think it currently
>> instead just stays where it was previously.  Which rarely is
>> helpful.
>
>
> But it does not know what program is recieving the input, or? Should
> it not ask the user then?

I don't understand a word of what you are saying.  We are talking
about things like

$ cd ~- RET

in shell-mode.

-- 
David Kastrup, Kriemhildstr. 15, 44793 Bochum

Re: [david.hansen@gmx.net: Re: comint's directory tracking doesn't understand \( or \)]

[David Kastrup]

"Robert J. Chassell" <bob@rattlesnake.com> writes:

>    > M-x shell RET
>    > ~ $ cd /tmp/
>    > /tmp $ mkdir /tmp/'(2007)'
>    > /tmp $ cd \(2007\)
>    > /tmp/(2007) $ pwd
>    >
>    >  => /tmp/(2007)
>
>    Uh, I guess you should not use 
>    pwd RET
>    but rather
>    M-x pwd RET
>
> I don't reproduce the problem either.  Emacs works fine.
>
> When I do the above with pwd in the *shell* buffer, I, too, see
> /tmp/(2007)
>
> When I do 
>        M-x pwd RET
> I see the default directory for the buffer, which in my case is  /
>
> After changing the default directory for the buffer with
>        M-x cd RET /tmp/(2007) RET
> when I do 
>        M-x pwd RET
> I see  
>        /tmp/(2007)/
> as expected.

Emacs usually tracks "cd" commands in shell-mode all by itself,
without any need to do
M-x cd RET

-- 
David Kastrup, Kriemhildstr. 15, 44793 Bochum

Re: [david.hansen@gmx.net: Re: comint's directory tracking doesn't understand \( or \)]

[Lennart Borgman (gmail)]

David Kastrup wrote:
> "Lennart Borgman (gmail)" <lennart.borgman@gmail.com> writes:
> 
>> David Kastrup wrote:
>>
>>> Maybe the directory tracker should check whether the directory it
>>> is thinking of exists, and use `dirs' alias `shell-resync-dirs'
>>> automatically if it finds that it doesn't.  I think it currently
>>> instead just stays where it was previously.  Which rarely is
>>> helpful.
>>
>> But it does not know what program is recieving the input, or? Should
>> it not ask the user then?
> 
> I don't understand a word of what you are saying.  We are talking
> about things like


Just the same thing that Miles just said in his reply. Maybe I should 
take a crash course in expressing myself ;-)

Re: [david.hansen@gmx.net: Re: comint's directory tracking doesn't understand \( or \)]

[Stefan Monnier]

> I don't think we should make the proposed change to comint.el.

Agreed.  It's simply too late for Emacs-22.

> AFAIK, the detailed rules for how backslash escape works is, in principle,
> different from shell to shell, and even if we choose to obey (e.g.)  bash
> semantics for backslash escapes, we might still be incompatible with other
> shells.  This might also introduce subtle bugs into non-shell uses of
> comint mode.

Agreed as well.

> It seems there will always be *some* way of confusing the directory
> tracker: that's what `M-x dirs' is for.

Yes.  Of course we can still improve the code that does it automatically.
E.g. in the example session, the directory is actually available directly in
the prompt, so we could tell shell.el to use it.  I have a proof-of-concept
local hack that does just that (it also works for prompts such as mine which
only include the last 2-3 levels of the full directory name).

Another improvement could be to check that the guessed new working directory
exists and if it doesn't, to try harder to find it.  E.g. by trying to
remove the backslashes ;-)

And if you want the dir-tracking to really work reliably, there's
always eshell.


        Stefan


PS: Repeat: this is just experimental code, not intended for installation.
The regexp in the code is the one that matches my particular prompt-shape.
I might very well be the only one on this planet to use such a prompt.


--- orig/lisp/shell.el
+++ mod/lisp/shell.el
@@ -472,6 +472,10 @@
       (when (string-equal shell "bash")
         (add-hook 'comint-output-filter-functions
                   'shell-filter-ctrl-a-ctrl-b nil t)))
+    (when shell-dir-cookie-re
+      ;; Watch for magic cookies in the output to track the current dir.
+      (add-hook 'comint-output-filter-functions
+		'shell-dir-cookie-watcher nil t))
     (comint-read-input-ring t)))
 
 (defun shell-filter-ctrl-a-ctrl-b (string)
@@ -609,6 +608,31 @@
 ;; replace it with a process filter that watches for and strips out
 ;; these messages.
 
+(defcustom shell-dir-cookie-re "^[a-z]+-\\(.*/.*\\)-[0-9]+% "
+  "Regexp matching your prompt, including some part of the current directory.
+If your prompt includes the current directory or the last few elements of it,
+set this to a pattern that matches your prompt and whose subgroup 1 matches
+the directory part of it.
+This is used by `shell-dir-cookie-watcher' to try and use this info
+to track your current directory.  It can be used instead of or in addition
+to `dirtrack-mode'."
+  :type '(choice (const nil) regexp))
+
+(defun shell-dir-cookie-watcher (text)
+  ;; This is fragile: the TEXT could be split into several chunks and we'd
+  ;; miss it.  Oh well.  It's a best effort anyway.  I'd expect that it's
+  ;; rather unusual to have the prompt split into several packets, but
+  ;; I'm sure Murphy will prove me wrong.
+  (when (and shell-dir-cookie-re (string-match shell-dir-cookie-re text))
+    (let ((dir (match-string 1 text)))
+      (cond
+       ((file-name-absolute-p dir) (shell-cd dir))
+       ;; Let's try and see if it seems to be up or down from where we were.
+       ((string-match "\\`\\(.*\\)\\(?:/.*\\)?\n\\(.*/\\)\\1\\(?:/.*\\)?\\'"
+		      (setq text (concat dir "\n" default-directory)))
+	(shell-cd (concat (match-string 2 text) dir)))))))
+	
+
 (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.

Re: [david.hansen@gmx.net: Re: comint's directory tracking doesn't understand \( or \)]

[Andreas Seltenreich]

Stefan Monnier writes:

> Yes.  Of course we can still improve the code that does it automatically.
> E.g. in the example session, the directory is actually available directly in
> the prompt, so we could tell shell.el to use it.

I think this feature is already implemented in dirtrack.el.

regards,
andreas

Re: [david.hansen@gmx.net: Re: comint's directory tracking doesn't understand \( or \)]

[David Hansen]

On Sun, 04 Mar 2007 14:26:30 -0500 Chong Yidong wrote:

> I don't think we should make the proposed change to comint.el.  AFAIK,
> the detailed rules for how backslash escape works is, in principle,
> different from shell to shell, and even if we choose to obey (e.g.)
> bash semantics for backslash escapes, we might still be incompatible
> with other shells.

It's an *sh* semantic, not bash.  Do you know of any non sh
compatible interactive shell that is widely used?

> This might also introduce subtle bugs into non-shell uses of comint
> mode.

Valid objection but i can't even imagine a case (where do "command
line arguments" make sense except in a shell?) and a grep on the emacs
sources suggests that it's pretty safe.

> It seems there will always be *some* way of confusing the directory
> tracker: that's what `M-x dirs' is for.

But none that is as easy as typing some non special characters and
hitting TAB.

Anyway, will try to remember that and send it again after Emacs 22 is
out.

David

Re: [david.hansen@gmx.net: Re: comint's directory tracking doesn't understand \( or \)]

[Chris Moore]

On 3/4/07, Chong Yidong <cyd@stupidchicken.com> wrote:

> BTW, I can't seem to reproduce the problem referenced in the original
> bug report.
> /tmp/(2007) $ pwd
>  => /tmp/(2007)

The bug I was reporting is that the buffer's directory doesn't change
correctly.  I realise that the inferior subprocess' working directory
changes correctly, but comint attempts to track the shell's directory.
 That way, you can hit C-x C-f and have it default to the directory
you are currently 'cd'ed to.

For years I used and loved "J Shell"
http://download.mirror.ac.uk/sites/ftp.gnu.org/gnu/elisp-archive/modes/J-Shell-2.1.tar.Z
it almost never gets out of sync with the shell's working directory,
because it redefines the 'cd' (and 'pushd', 'popd', 'rlogin', etc)
commands to echo some special control characters to the shell mode
telling it each time the working directory changes.  It would even
arrange for the shell buffer's working directory to become
/user@host:/path/file when the user logged on to a remote machine
inside the shell buffer.

It's unfortunate that the default Emacs M-x shell mode is so flaky in
this regard.  Even something as simple as "cd $PROJECT" confuses it.

Re: [david.hansen@gmx.net: Re: comint's directory tracking doesn't understand \( or \)]

[Tom Tromey]

>>>>> "Chris" == Chris Moore <dooglus@gmail.com> writes:

Chris> It's unfortunate that the default Emacs M-x shell mode is so flaky in
Chris> this regard.  Even something as simple as "cd $PROJECT" confuses it.

On Linux, Emacs could watch /proc/PID/cwd and not ever get an
incorrect answer, at least not for the shell process.  This is less
handy if the user runs sub-shells or the like.

Tom

Re: [david.hansen@gmx.net: Re: comint's directory tracking doesn't understand \( or \)]

[David Kastrup]

David Hansen <david.hansen@gmx.net> writes:

> On Sun, 04 Mar 2007 14:26:30 -0500 Chong Yidong wrote:
>
>> I don't think we should make the proposed change to comint.el.  AFAIK,
>> the detailed rules for how backslash escape works is, in principle,
>> different from shell to shell, and even if we choose to obey (e.g.)
>> bash semantics for backslash escapes, we might still be incompatible
>> with other shells.
>
> It's an *sh* semantic, not bash.  Do you know of any non sh
> compatible interactive shell that is widely used?

csh and its ilk are actually in non-trivial use.  Their respective
suckage level for interactive use appears tolerable for some people.
One has to be aware that some interactive aids like history
manipulation and command completion appeared first in that family, and
some people were more comfortable with C-like syntax.

-- 
David Kastrup, Kriemhildstr. 15, 44793 Bochum

Re: [david.hansen@gmx.net: Re: comint's directory tracking doesn't understand \( or \)]

[Stefan Monnier]

>> Yes.  Of course we can still improve the code that does it automatically.
>> E.g. in the example session, the directory is actually available directly in
>> the prompt, so we could tell shell.el to use it.

> I think this feature is already implemented in dirtrack.el.

Yes, although it's a bit different.


        Stefan

Re: [david.hansen@gmx.net: Re: comint's directory tracking doesn't understand \( or \)]

[David Hansen]

On Mon, 05 Mar 2007 00:30:09 +0100 David Kastrup wrote:

> David Hansen <david.hansen@gmx.net> writes:
>
>> On Sun, 04 Mar 2007 14:26:30 -0500 Chong Yidong wrote:
>>
>>> I don't think we should make the proposed change to comint.el.  AFAIK,
>>> the detailed rules for how backslash escape works is, in principle,
>>> different from shell to shell, and even if we choose to obey (e.g.)
>>> bash semantics for backslash escapes, we might still be incompatible
>>> with other shells.
>>
>> It's an *sh* semantic, not bash.  Do you know of any non sh
>> compatible interactive shell that is widely used?
>
> csh and its ilk are actually in non-trivial use.  Their respective
> suckage level for interactive use appears tolerable for some people.
> One has to be aware that some interactive aids like history
> manipulation and command completion appeared first in that family, and
> some people were more comfortable with C-like syntax.

My csh man page isn't that clear about it but some simple tests
suggest that it treats the backslash like a sh.

BTW, we would be flooded with bug reports if the way my patch treats
backslashes would be harmful to any widely used shell (from
`shell-quote-argument'):

      ;; Quote everything except POSIX filename characters.
      ;; This should be safe enough even for really weird shells.


David

Re: [david.hansen@gmx.net: Re: comint's directory tracking doesn't understand \( or \)]

[Richard Stallman]

How is it that the problem in directory tracking is fixed by
changing comint-arguments?

Can you add some comments to make your code clearer?
We can't install it unless we can prove to ourselves that
it is correct.  But I think that might be clear once we have
good comments to help show it.

Also, would you please send a change log?

Re: [david.hansen@gmx.net: Re: comint's directory tracking doesn't understand \( or \)]

[David Hansen]

On Sun, 04 Mar 2007 21:55:17 -0500 Richard Stallman wrote:

> How is it that the problem in directory tracking is fixed by
> changing comint-arguments?

shell-mode watches your input for commands that change the directory
(mainly the shells `cd' command but also `popd', `pushd' and maybe
others).

shell-mode then just use the first argument - (comint-arguments 1 1) -
to this command to keep track of the working dir of the shell process.

> Can you add some comments to make your code clearer?

Hope that's enough comments...  I also forgot backticks in the first
version and simplified the code a bit.

> We can't install it unless we can prove to ourselves that
> it is correct.  But I think that might be clear once we have
> good comments to help show it.

I don't think the problem is to prove if it's correct or not.

First of all we can't be 100% correct here (e.g. if backticks or
environment variables are involved).  And even with my patch something
like $((41 + 1)) or other (very) special constructs will not be parsed
in a sensible way (wouldn't be such a big problem to add it but i don't
think it would do any good).

Also it's impossible to support every obscure shell out there.

`comint-arguments' job is to work with all widely used shells for simple
commands.

If you do a grep for `comint-arguments' on the emacs sources you'll see
what I mean.  e.g. telnet.el uses it to extract the hostname.

I think the reason why this patch isn't that popular is that there are
just so many comint based modes out there that it's impossible to say if
it breaks something or not.

I think the risk that it breaks something that ships with emacs is very
low (just do the grep).  Also i don't think there is any widely used
shell out there that treats backslashes very different.

> Also, would you please send a change log?

2007-03-05  David Hansen  <david.hansen@physik.fu-berlin.de>

	* comint.el (comint-delim-arg): Function removed.
        (comint-arguments): Backslash quotes arbitrary characters.
        Moved `comint-delim-arg' functionality here.

*** comint.el	04 Mar 2007 13:04:02 +0100	1.358
--- comint.el	05 Mar 2007 06:30:23 +0100	
***************
*** 105,110 ****
--- 105,112 ----
  ;;; Code:
  
  (require 'ring)
+ (eval-when-compile (require 'cl))
+ 
  \f
  ;; Buffer Local Variables:
  ;;============================================================================
***************
*** 1344,1371 ****
  			(t nth))))
  	(comint-arguments string nth mth)))))
  
- (defun comint-delim-arg (arg)
-   "Return a list of arguments from ARG.
- Break it up at the delimiters in `comint-delimiter-argument-list'.
- Returned list is backwards."
-   (if (null comint-delimiter-argument-list)
-       (list arg)
-     (let ((args nil)
- 	  (pos 0)
- 	  (len (length arg)))
-       (while (< pos len)
- 	(let ((char (aref arg pos))
- 	      (start pos))
- 	  (if (memq char comint-delimiter-argument-list)
- 	      (while (and (< pos len) (eq (aref arg pos) char))
- 		(setq pos (1+ pos)))
- 	    (while (and (< pos len)
- 			(not (memq (aref arg pos)
- 				   comint-delimiter-argument-list)))
- 	      (setq pos (1+ pos))))
- 	  (setq args (cons (substring arg start pos) args))))
-       args)))
- 
  (defun comint-arguments (string nth mth)
    "Return from STRING the NTH to MTH arguments.
  NTH and/or MTH can be nil, which means the last argument.
--- 1346,1351 ----
***************
*** 1373,1423 ****
  We assume whitespace separates arguments, except within quotes
  and except for a space or tab that immediately follows a backslash.
  Also, a run of one or more of a single character
! in `comint-delimiter-argument-list' is a separate argument.
! Argument 0 is the command name."
!   ;; The first line handles ordinary characters and backslash-sequences
!   ;; (except with w32 msdos-like shells, where backslashes are valid).
!   ;; The second matches "-quoted strings.
!   ;; The third matches '-quoted strings.
!   ;; The fourth matches `-quoted strings.
!   ;; This seems to fit the syntax of BASH 2.0.
!   (let* ((first (if (if (fboundp 'w32-shell-dos-semantics)
! 			(w32-shell-dos-semantics))
! 		    "[^ \n\t\"'`]+\\|"
! 		  "[^ \n\t\"'`\\]+\\|\\\\[\"'`\\ \t]+\\|"))
! 	 (argpart (concat first
! 			  "\\(\"\\([^\"\\]\\|\\\\.\\)*\"\\|\
! '[^']*'\\|\
! `[^`]*`\\)"))
! 	 (args ()) (pos 0)
! 	 (count 0)
! 	 beg str quotes)
!     ;; Build a list of all the args until we have as many as we want.
!     (while (and (or (null mth) (<= count mth))
! 		(string-match argpart string pos))
!       (if (and beg (= pos (match-beginning 0)))
! 	  ;; It's contiguous, part of the same arg.
! 	  (setq pos (match-end 0)
! 		quotes (or quotes (match-beginning 1)))
! 	;; It's a new separate arg.
! 	(if beg
! 	    ;; Put the previous arg, if there was one, onto ARGS.
! 	    (setq str (substring string beg pos)
! 		  args (if quotes (cons str args)
! 			 (nconc (comint-delim-arg str) args))))
! 	(setq count (length args))
! 	(setq quotes (match-beginning 1))
! 	(setq beg (match-beginning 0))
! 	(setq pos (match-end 0))))
!     (if beg
! 	(setq str (substring string beg pos)
! 	      args (if quotes (cons str args)
! 		     (nconc (comint-delim-arg str) args))))
!     (setq count (length args))
!     (let ((n (or nth (1- count)))
! 	  (m (if mth (1- (- count mth)) 0)))
!       (mapconcat
!        (function (lambda (a) a)) (nthcdr n (nreverse (nthcdr m args))) " "))))
  \f
  ;;
  ;; Input processing stuff
--- 1353,1459 ----
  We assume whitespace separates arguments, except within quotes
  and except for a space or tab that immediately follows a backslash.
  Also, a run of one or more of a single character
! in `comint-delimiter-argument-list' (unless quoted with a backslash)
! is a separate argument. Argument 0 is the command name."
!   (let ((len (length string))
!         (esc (unless (and (fboundp 'w32-shell-dos-semantics)
!                           (w32-shell-dos-semantics))
!                ?\\))          ; backslash escape character, none on MS-DOS
!         (ifs '(?\n ?\t ?\ ))  ; whitespace word delimiters (the bash default)
!         (quo '(?\" ?\' ?\`))  ; argument quoting characters
!         (i 0)                 ; character index of the string
!         (beg 0)               ; beginning of the currently parsed argument
!         state                 ; stack of parsing states (see below for details)
!         args)                 ; list of arguments parsed so far
!     (flet ((push-arg (new-beg)
!              ;; With the index `i' at the end of an argument push it to the
!              ;; list `args' and set the beginning of the next argument `beg'
!              ;; to NEW-BEG.  Also decrement MTH to keep track of the number of
!              ;; parsed arguments.
!              (push (substring string beg i) args)
!              (and mth (decf mth))
!              (setq beg new-beg)))
!       ;; Loop over the characters of STRING and maintain a stack of "parser
!       ;; states".  Each parser state is a character that describes the state.
!       ;;
!       ;; If it is a member of the list `quo' we are within a quoted string that
!       ;; is delimited by this character.
!       ;;
!       ;; If it is a member of `comint-delimiter-argument-list' it is the value
!       ;; of the prevously scanned character.  We need to keep track of it as a
!       ;; sequence of equal elements of `comint-delimiter-argument-list' are
!       ;; considered as one single argument (like '>>' or '&&').
!       ;;
!       ;; If it is `esc' (a backslash on most systems) the current characte is
!       ;; escaped by a backslash and treated like any ordinary non special
!       ;; character.
!       ;;
!       ;; We stop looping if we reached the end of the string or after the MTH
!       ;; argument is parsed.
!       (while (and (<= i len) (or (not mth) (>= mth 0)))
!         (let ((s (car state))                      ; current state
!               (c (and (< i len) (aref string i)))) ; current character
!           (cond
!             ;; If the current character is backslash escaped update `state'.
!             ((eq ?\\ s)
!              (pop state))
!             ;; If within a sequence of `comint-delimiter-argument-list'
!             ;; characters check for the end of it (some different character).
!             ((and (member s comint-delimiter-argument-list) (not (eq s c)))
!              (push-arg i)
!              (pop state)
!              ;; We need to parse the current character again as it may change
!              ;; the state.  Undo the following `incf' call.
!              (decf i))
!             ;; Check for the beginning or end of quote delimited strings.
!             ((member c quo)
!              (if (eq c s)
!                  ;; We are within a quote delimited string and the current
!                  ;; character is the same as the one that started the string.
!                  ;; We reached the end.  Update `state'.
!                  (pop state)
!                ;; The current character only starts a new quote delimited
!                ;; string if we aren't already in such a construct (which is
!                ;; equivalent to `s' being nil).  Keeping track of nested
!                ;; constructs doesn't make any sense when splitting arguments.
!                (or s (push c state))))
!             ;; If the current character is a backslash it quotes the next
!             ;; character unless we are within a `'' or ``' delimited string.
!             ((and (eq esc c) (not (or (eq ?\' s) (eq ?\` s))))
!              (push c state))
!             ;; Check for space delimiters.
!             ((and (not s) (member c ifs))
!              (if (= beg i)
!                  ;; Some other character befor this space already delimited an
!                  ;; argument.  We just need to adjust the beginning of the next
!                  ;; argument.
!                  (incf beg)
!                ;; We found the end of an argument.
!                (push-arg (1+ i))))
!             ;; Check for special argument delimiting characters.
!             ((and (not s) (member c comint-delimiter-argument-list))
!              (push c state)
!              (when (/= beg i)
!                ;; This character ends the previous argument (there are no
!                ;; spaces before it).
!                (push-arg i)))
!             ;; The end of the string marks the end of an argument but only if
!             ;; the string doesn't end with whitespaces.
!             ((not c)
!              (unless (= beg len)
!                (push-arg len))))
!           (incf i)))
!       (if (not nth)
!           ;; if MTH is non nil only return the last argument if there are no
!           ;; non parsed arguments left
!           (or (and (or (not mth)
!                        (>= i len)
!                        (string-match "^[\t\n ]*$" (substring string i)))
!                    (car args))
!               "")
!         (setf (nthcdr (- (length args) nth) args) nil)
!         (mapconcat #'identity (nreverse args) " ")))))
! 
  \f
  ;;
  ;; Input processing stuff

Re: [david.hansen@gmx.net: Re: comint's directory tracking doesn't understand \( or \)]

[Richard Stallman]

    shell-mode watches your input for commands that change the directory
    (mainly the shells `cd' command but also `popd', `pushd' and maybe
    others).

    shell-mode then just use the first argument - (comint-arguments 1 1) -
    to this command to keep track of the working dir of the shell process.

This patch would be safer if you add a new function comint-dir-argument
which does the new thing, and is used only for dir tracking.
Then you could leave comint-arguments unchanged, and avoid the risk
of breaking something else with some other shell.

With that alteration, I think we could install it now.

Re: [david.hansen@gmx.net: Re: comint's directory tracking doesn't understand \( or \)]

[Stefan Monnier]

>     shell-mode watches your input for commands that change the directory
>     (mainly the shells `cd' command but also `popd', `pushd' and maybe
>     others).

>     shell-mode then just use the first argument - (comint-arguments 1 1) -
>     to this command to keep track of the working dir of the shell process.

> This patch would be safer if you add a new function comint-dir-argument
> which does the new thing, and is used only for dir tracking.
> Then you could leave comint-arguments unchanged, and avoid the risk
> of breaking something else with some other shell.

> With that alteration, I think we could install it now.

Wouldn't it be better to move it to shell.el?
I.e. create a new function shell-argument?


        Stefan

Re: [david.hansen@gmx.net: Re: comint's directory tracking doesn't understand \( or \)]

[David Hansen]

On Mon, 05 Mar 2007 21:23:32 -0500 Stefan Monnier wrote:

>>     shell-mode watches your input for commands that change the directory
>>     (mainly the shells `cd' command but also `popd', `pushd' and maybe
>>     others).
>
>>     shell-mode then just use the first argument - (comint-arguments 1 1) -
>>     to this command to keep track of the working dir of the shell process.
>
>> This patch would be safer if you add a new function comint-dir-argument
>> which does the new thing, and is used only for dir tracking.
>> Then you could leave comint-arguments unchanged, and avoid the risk
>> of breaking something else with some other shell.
>
>> With that alteration, I think we could install it now.
>
> Wouldn't it be better to move it to shell.el?
> I.e. create a new function shell-argument?

Agree with that.  I'll think about a quick and dirty solution that
only solves the directory tracking problem (we don't need to parse
the whole command line here, just the 0th and 1st arg, maybe that
simplifies the problem a bit).

I'll bring up the discussion about a more general solution after
Emacs 22 is out.

David

Re: [david.hansen@gmx.net: Re: comint's directory tracking doesn't understand \( or \)]

[Richard Stallman]

    > This patch would be safer if you add a new function comint-dir-argument
    > which does the new thing, and is used only for dir tracking.
    > Then you could leave comint-arguments unchanged, and avoid the risk
    > of breaking something else with some other shell.

    > With that alteration, I think we could install it now.

    Wouldn't it be better to move it to shell.el?
    I.e. create a new function shell-argument?

If that function would only be used in shell.el,
putting it in shell.el sounds like a good idea.
Since it is specifically for dir tracking,
I would call it shell-dir-argument.

Re: [david.hansen@gmx.net: Re: comint's directory tracking doesn't understand \( or \)]

[Miles Bader]

Stefan Monnier <monnier@iro.umontreal.ca> writes:
> Wouldn't it be better to move it to shell.el?
> I.e. create a new function shell-argument?

I think it would be a nicer interface if it were split into two
functions:  one which which would just parse the arguments and return a
lisp list of them (e.g. `shell-split') and one which would call
shell-split and do the choose-N-through-M-and-apply-mapconcat stuff to
return a string (e.g., `shell-arguments-string').

E.g.:

   (shell-split "this is \"a test\"") => ("this" "is" "a test")

   (shell-arguments-string "this is \"a test\"" 1 2) => "is \"a test\""

Simple uses might use the latter but the former seems a generally
cleaner interface and building block for other uses.

-Miles

-- 
The automobile has not merely taken over the street, it has dissolved the
living tissue of the city.  Its appetite for space is absolutely insatiable;
moving and parked, it devours urban land, leaving the buildings as mere islands
of habitable space in a sea of dangerous and ugly traffic.
[James Marston Fitch, New York Times, 1 May 1960]

Re: [david.hansen@gmx.net: Re: comint's directory tracking doesn't understand \( or \)]

[David Hansen]

On Wed, 07 Mar 2007 09:48:51 +0900 Miles Bader wrote:

> Stefan Monnier <monnier@iro.umontreal.ca> writes:
>> Wouldn't it be better to move it to shell.el?
>> I.e. create a new function shell-argument?
>
> I think it would be a nicer interface if it were split into two
> functions:  one which which would just parse the arguments and return a
> lisp list of them (e.g. `shell-split') and one which would call
> shell-split and do the choose-N-through-M-and-apply-mapconcat stuff to
> return a string (e.g., `shell-arguments-string').
>
> E.g.:
>
>    (shell-split "this is \"a test\"") => ("this" "is" "a test")
>
>    (shell-arguments-string "this is \"a test\"" 1 2) => "is \"a test\""
>
> Simple uses might use the latter but the former seems a generally
> cleaner interface and building block for other uses.

OK, first of all I'm really really sorry, but I just couldn't resist
to allow "&", ";" and "|" in directory names too.  So i dropped what
Stefan called `shell-arguments-string'.  In this case it makes
things just more complicated.

I have named `shell-split' to `shell-split-arguments' and introduced
a little helper function `shell-split-commands'.

`shell-split-commands' replaces the old regular-expression based
guessing of what are different commands in the string (eg. "cd foo;
cd bar && cd baz").  Now where we have the list of arguments from
`shell-split-arguments' this can be done far more reliable.

I feel a bit bad that this became such a long discussion.  I think
this is an improvement to shell-mode but it's not worth to delay the
release of the one true editor.  If these changes go to far lets
just drop it for now and resume the discussion after the release.

David

*** shell.el	05 Mar 2007 01:10:08 +0100	1.149
--- shell.el	07 Mar 2007 15:48:35 +0100	
***************
*** 105,110 ****
--- 105,111 ----
  ;;; Code:
  
  (require 'comint)
+ (eval-when-compile (require 'cl))
  
  ;;; Customization and Buffer Variables
  
***************
*** 569,574 ****
--- 570,681 ----
  ;; Don't do this when shell.el is loaded, only while dumping.
  ;;;###autoload (add-hook 'same-window-buffer-names "*shell*")
  
+ ;;; Argument Splitting
+ 
+ (defun shell-split-arguments (string)
+   "Split STRING into it's arguments and return a list of arguments.
+ We assume members of `comint-delimiter-argument-list' and
+ whitespaces separate arguments, except within quotes or (unless
+ on MS-DOS) if escaped by a backslash character.  A run of more
+ than one character in `comint-delimiter-argument-list' is treated
+ as a single argument."
+   (let ((len (length string))
+         (esc (unless (and (fboundp 'w32-shell-dos-semantics)
+                           (w32-shell-dos-semantics))
+                ?\\))          ; backslash escape character, none on MS-DOS
+         (ifs '(?\n ?\t ?\ ))  ; whitespace word delimiters (the bash default)
+         (quo '(?\" ?\' ?\`))  ; string quoting characters
+         (i 0)                 ; character index of the string
+         (beg 0)               ; beginning of the currently parsed argument
+         state                 ; stack of parsing states (see below for details)
+         args)                 ; list of arguments parsed so far
+     (flet ((push-arg (new-beg)
+              ;; With the index `i' at the end of an argument push it to the
+              ;; list `args' and set the beginning of the next argument `beg'
+              ;; to NEW-BEG.
+              (push (substring string beg i) args)
+              (setq beg new-beg)))
+       ;; Loop over the characters of STRING and maintain a stack of "parser
+       ;; states".  Each parser state is a character.
+       ;;
+       ;; If it is a member of the list `quo' we are within a quoted string that
+       ;; is delimited by this character.
+       ;;
+       ;; If it is a member of `comint-delimiter-argument-list' it is the value
+       ;; of the prevously scanned character.  We need to keep track of it as a
+       ;; sequence of equal elements of `comint-delimiter-argument-list' are
+       ;; considered as one single argument (like '>>' or '&&').
+       ;;
+       ;; If it is `esc' (a backslash on most systems) the current character is
+       ;; escaped by a and treated like any ordinary non special character.
+       (while (<= i len)
+         (let ((s (car state))                      ; current state
+               (c (and (< i len) (aref string i)))) ; current character
+           (cond
+             ((and esc (eq esc s))       ; backslash escaped
+              (pop state))
+             ;; If within a sequence of `comint-delimiter-argument-list'
+             ;; characters check for the end of it (some different character).
+             ((and (member s comint-delimiter-argument-list) (not (eq s c)))
+              (push-arg i)
+              (pop state)
+              (decf i))                  ; parse this character again
+             ((member c quo)             ; quote character
+              (if (eq c s)
+                  ;; We are within a quote delimited string and the current
+                  ;; character is the same as the one that started the string.
+                  ;; We reached the end.  Update `state'.
+                  (pop state)
+                ;; The current character only starts a new quote delimited
+                ;; string if we aren't already in such a construct (which is
+                ;; equivalent to `s' being nil).  Keeping track of nested
+                ;; constructs doesn't make any sense when splitting arguments.
+                (or s (push c state))))
+             ;; If the current character is a backslash it quotes the next
+             ;; character unless we are within a `'' or ``' delimited string.
+             ((and (eq esc c) (not (or (eq ?\' s) (eq ?\` s))))
+              (push c state))
+             ((and (not s) (member c ifs)) ; space delimiters
+              (if (= beg i)
+                  ;; Some other character before this space already delimited an
+                  ;; argument.  Just adjust the beginning of the next argument.
+                  (incf beg)
+                ;; We found the end of an argument.
+                (push-arg (1+ i))))
+             ;; Check for special argument delimiting characters.
+             ((and (not s) (member c comint-delimiter-argument-list))
+              (push c state)
+              (when (/= beg i)
+                ;; This character ends the previous argument (there are no
+                ;; whitespaces before it).
+                (push-arg i)))
+             ((not c)                    ; end of the string
+              (unless (= beg len)        ; no whitespace at the end
+                (push-arg len))))
+           (incf i)))
+       (nreverse args))))
+ 
+ (defun shell-split-commands (string)
+   ;; Split STRING into a list of commands.
+   ;; First STRING is split into its arguments.  Then every argument that does
+   ;; not start with `shell-command-regexp' is assumed to be a seperator of two
+   ;; commands.
+   ;; Return a list of commands where each command itself is a list of all its
+   ;; arguments.
+   (let ((regexp (concat "^" shell-command-regexp))
+         (arguments (shell-split-arguments string))
+         (command-list '())
+         (command '())
+         arg)
+     (while (setq arg (pop arguments))
+       (if (string-match regexp arg)
+           (push arg command)
+         (when command
+           (push (nreverse command) command-list)
+           (setq command nil))))
+     (and command (push (nreverse command) command-list))
+     command-list))
+ 
  ;;; Directory tracking
  ;;
  ;; This code provides the shell mode input sentinel
***************
*** 626,663 ****
    (if shell-dirtrackp
        ;; We fail gracefully if we think the command will fail in the shell.
        (condition-case chdir-failure
! 	  (let ((start (progn (string-match
! 			       (concat "^" shell-command-separator-regexp)
! 			       str) ; skip whitespace
! 			      (match-end 0)))
! 		end cmd arg1)
! 	    (while (string-match shell-command-regexp str start)
! 	      (setq end (match-end 0)
! 		    cmd (comint-arguments (substring str start end) 0 0)
! 		    arg1 (comint-arguments (substring str start end) 1 1))
! 	      (if arg1
! 		  (setq arg1 (shell-unquote-argument arg1)))
! 	      (cond ((string-match (concat "\\`\\(" shell-popd-regexp
! 					   "\\)\\($\\|[ \t]\\)")
! 				   cmd)
! 		     (shell-process-popd (comint-substitute-in-file-name arg1)))
! 		    ((string-match (concat "\\`\\(" shell-pushd-regexp
! 					   "\\)\\($\\|[ \t]\\)")
! 				   cmd)
! 		     (shell-process-pushd (comint-substitute-in-file-name arg1)))
! 		    ((string-match (concat "\\`\\(" shell-cd-regexp
! 					   "\\)\\($\\|[ \t]\\)")
! 				   cmd)
! 		     (shell-process-cd (comint-substitute-in-file-name arg1)))
! 		    ((and shell-chdrive-regexp
! 			  (string-match (concat "\\`\\(" shell-chdrive-regexp
! 						"\\)\\($\\|[ \t]\\)")
! 					cmd))
! 		     (shell-process-cd (comint-substitute-in-file-name cmd))))
! 	      (setq start (progn (string-match shell-command-separator-regexp
! 					       str end)
! 				 ;; skip again
! 				 (match-end 0)))))
  	(error "Couldn't cd"))))
  
  (defun shell-unquote-argument (string)
--- 733,759 ----
    (if shell-dirtrackp
        ;; We fail gracefully if we think the command will fail in the shell.
        (condition-case chdir-failure
!           (loop for args in (shell-split-commands str)
!              as cmd = (pop args)
!              as arg1 = (shell-unquote-argument (or (pop args) ""))
!              do (cond
!                   ((string-match (concat "\\`\\(" shell-popd-regexp
!                                          "\\)\\($\\|[ \t]\\)")
!                                  cmd)
!                    (shell-process-popd (comint-substitute-in-file-name arg1)))
!                   ((string-match (concat "\\`\\(" shell-pushd-regexp
!                                          "\\)\\($\\|[ \t]\\)")
!                                  cmd)
!                    (shell-process-pushd (comint-substitute-in-file-name arg1)))
!                   ((string-match (concat "\\`\\(" shell-cd-regexp
!                                          "\\)\\($\\|[ \t]\\)")
!                                  cmd)
!                    (shell-process-cd (comint-substitute-in-file-name arg1)))
!                   ((and shell-chdrive-regexp
!                         (string-match (concat "\\`\\(" shell-chdrive-regexp
!                                               "\\)\\($\\|[ \t]\\)")
!                                       cmd))
!                    (shell-process-cd (comint-substitute-in-file-name cmd)))))
  	(error "Couldn't cd"))))
  
  (defun shell-unquote-argument (string)

Re: [david.hansen@gmx.net: Re: comint's directory tracking doesn't understand \( or \)]

[Richard Stallman]

It is clear this patch can't break anything except dir tracking.
Would someone please test it, and if it works, then install it?
Then please ack.

Re: [david.hansen@gmx.net: Re: comint's directory tracking doesn't understand \( or \)]

[Chong Yidong]

> OK, first of all I'm really really sorry, but I just couldn't resist
> to allow "&", ";" and "|" in directory names too.  So i dropped what
> Stefan called `shell-arguments-string'.  In this case it makes
> things just more complicated.
>
> [200 line patch]

Good grief.

Re: [david.hansen@gmx.net: Re: comint's directory tracking doesn't understand \( or \)]

[Glenn Morris]

Chong Yidong wrote:

>> [200 line patch]
>
> Good grief.

(wth-hobby-horse It seems that David Hansen has no copyright
assignment for Emacs (only for Gnus), so we couldn't install it even
it were 1/10th the size. )

Re: [david.hansen@gmx.net: Re: comint's directory tracking doesn't understand \( or \)]

[David Hansen]

On Fri, 09 Mar 2007 15:28:40 -0500 Glenn Morris wrote:

> Chong Yidong wrote:
>
>>> [200 line patch]
>>
>> Good grief.
>
> (wth-hobby-horse It seems that David Hansen has no copyright
> assignment for Emacs (only for Gnus), so we couldn't install it even
> it were 1/10th the size. )

That can easily be changed, can't it?

Re: [david.hansen@gmx.net: Re: comint's directory tracking doesn't understand \( or \)]

[David Kastrup]

David Hansen <david.hansen@gmx.net> writes:

> On Fri, 09 Mar 2007 15:28:40 -0500 Glenn Morris wrote:
>
>> Chong Yidong wrote:
>>
>>>> [200 line patch]
>>>
>>> Good grief.
>>
>> (wth-hobby-horse It seems that David Hansen has no copyright
>> assignment for Emacs (only for Gnus), so we couldn't install it even
>> it were 1/10th the size. )
>
> That can easily be changed, can't it?

Well, you likely are familiar with the procedure.  It would appear
that it makes sense for you to have an assignment for Emacs, anyway,
given your willingness to get involved, so I'll send you the request
form (should be the same you got for Gnus, likely).

Just don't expect it to make this code appear in Emacs 22, and even
for later versions you'd likely need to put forward a lot of
persuasion.

-- 
David Kastrup, Kriemhildstr. 15, 44793 Bochum

Re: [david.hansen@gmx.net: Re: comint's directory tracking doesn't understand \( or \)]

[Chong Yidong]

I think the following patch fixes the same bug in a safer way.

*** emacs/lisp/comint.el.~1.358.~	2007-02-27 19:43:48.000000000 -0500
--- emacs/lisp/comint.el	2007-03-09 18:44:15.000000000 -0500
***************
*** 1384,1390 ****
    (let* ((first (if (if (fboundp 'w32-shell-dos-semantics)
  			(w32-shell-dos-semantics))
  		    "[^ \n\t\"'`]+\\|"
! 		  "[^ \n\t\"'`\\]+\\|\\\\[\"'`\\ \t]+\\|"))
  	 (argpart (concat first
  			  "\\(\"\\([^\"\\]\\|\\\\.\\)*\"\\|\
  '[^']*'\\|\
--- 1384,1390 ----
    (let* ((first (if (if (fboundp 'w32-shell-dos-semantics)
  			(w32-shell-dos-semantics))
  		    "[^ \n\t\"'`]+\\|"
!                   "\\([^ \n\t\"'`\\]\\|\\\\.\\)+\\|"))
  	 (argpart (concat first
  			  "\\(\"\\([^\"\\]\\|\\\\.\\)*\"\\|\
  '[^']*'\\|\

Re: [david.hansen@gmx.net: Re: comint's directory tracking doesn't understand \( or \)]

[David Hansen]

On Fri, 09 Mar 2007 19:04:39 -0500 Chong Yidong wrote:

> I think the following patch fixes the same bug in a safer way.
>
> *** emacs/lisp/comint.el.~1.358.~	2007-02-27 19:43:48.000000000 -0500
> --- emacs/lisp/comint.el	2007-03-09 18:44:15.000000000 -0500
> ***************
> *** 1384,1390 ****
>     (let* ((first (if (if (fboundp 'w32-shell-dos-semantics)
>   			(w32-shell-dos-semantics))
>   		    "[^ \n\t\"'`]+\\|"
> ! 		  "[^ \n\t\"'`\\]+\\|\\\\[\"'`\\ \t]+\\|"))
>   	 (argpart (concat first
>   			  "\\(\"\\([^\"\\]\\|\\\\.\\)*\"\\|\
>   '[^']*'\\|\
> --- 1384,1390 ----
>     (let* ((first (if (if (fboundp 'w32-shell-dos-semantics)
>   			(w32-shell-dos-semantics))
>   		    "[^ \n\t\"'`]+\\|"
> !                   "\\([^ \n\t\"'`\\]\\|\\\\.\\)+\\|"))
>   	 (argpart (concat first
>   			  "\\(\"\\([^\"\\]\\|\\\\.\\)*\"\\|\
>   '[^']*'\\|\

No it doesn't. (BTW that was my first suggestion).  The string gets
still splitted at members of `comint-delimiter-argument-list' (note
that it's buffer local) no matter if they are escaped or not.

I have to admit that it probably can be fixed with far less than a
200 line patch but I'm not willing to touch these regexps and i
don't think it will result in anything "safer".

David

Re: [david.hansen@gmx.net: Re: comint's directory tracking doesn't understand \( or \)]

[Chong Yidong]

David Hansen <david.hansen@gmx.net> wrote:

> The string gets still splitted at members of
> `comint-delimiter-argument-list' (note that it's buffer local) no
> matter if they are escaped or not.
>
> I have to admit that it probably can be fixed with far less than a
> 200 line patch but I'm not willing to touch these regexps and i
> don't think it will result in anything "safer".

I think there is an easy way to get around this difficulty: once
backslash-escaped characters have been identified, put a text property
on the string saying that character is literal, then make
comint-delim-arg avoid splitting when such a text property is present.
Here is a patch to implement this.  I can verify that it solves the
original bug.

As far as safety goes, I think it is a little safer than the other
candidate patch, if only because it's simpler to read.

*** /home/cyd/emacs/lisp/comint.el.~1.358.~	2007-02-27 19:43:48.000000000 -0500
--- /home/cyd/emacs/lisp/comint.el	2007-03-10 15:15:40.000000000 -0500
***************
*** 1356,1367 ****
        (while (< pos len)
  	(let ((char (aref arg pos))
  	      (start pos))
! 	  (if (memq char comint-delimiter-argument-list)
  	      (while (and (< pos len) (eq (aref arg pos) char))
  		(setq pos (1+ pos)))
  	    (while (and (< pos len)
! 			(not (memq (aref arg pos)
! 				   comint-delimiter-argument-list)))
  	      (setq pos (1+ pos))))
  	  (setq args (cons (substring arg start pos) args))))
        args)))
--- 1356,1371 ----
        (while (< pos len)
  	(let ((char (aref arg pos))
  	      (start pos))
! 	  (if (and (memq char comint-delimiter-argument-list)
! 		   ;; Ignore backslash-escaped characters.
! 		   (not (get-text-property pos 'literal arg)))
  	      (while (and (< pos len) (eq (aref arg pos) char))
  		(setq pos (1+ pos)))
  	    (while (and (< pos len)
! 			(not (and (memq (aref arg pos)
! 					comint-delimiter-argument-list)
! 				  (not (get-text-property
! 					pos 'literal arg)))))
  	      (setq pos (1+ pos))))
  	  (setq args (cons (substring arg start pos) args))))
        args)))
***************
*** 1381,1404 ****
    ;; The third matches '-quoted strings.
    ;; The fourth matches `-quoted strings.
    ;; This seems to fit the syntax of BASH 2.0.
!   (let* ((first (if (if (fboundp 'w32-shell-dos-semantics)
! 			(w32-shell-dos-semantics))
! 		    "[^ \n\t\"'`]+\\|"
! 		  "[^ \n\t\"'`\\]+\\|\\\\[\"'`\\ \t]+\\|"))
  	 (argpart (concat first
  			  "\\(\"\\([^\"\\]\\|\\\\.\\)*\"\\|\
  '[^']*'\\|\
  `[^`]*`\\)"))
  	 (args ()) (pos 0)
  	 (count 0)
  	 beg str quotes)
      ;; Build a list of all the args until we have as many as we want.
      (while (and (or (null mth) (<= count mth))
  		(string-match argpart string pos))
        (if (and beg (= pos (match-beginning 0)))
  	  ;; It's contiguous, part of the same arg.
  	  (setq pos (match-end 0)
! 		quotes (or quotes (match-beginning 1)))
  	;; It's a new separate arg.
  	(if beg
  	    ;; Put the previous arg, if there was one, onto ARGS.
--- 1385,1414 ----
    ;; The third matches '-quoted strings.
    ;; The fourth matches `-quoted strings.
    ;; This seems to fit the syntax of BASH 2.0.
!   (let* ((backslash-escape (not (and (fboundp 'w32-shell-dos-semantics)
! 				     (w32-shell-dos-semantics))))
! 	 (first (if backslash-escape
! 		    "[^ \n\t\"'`\\]\\|\\(\\\\.\\)\\|"
! 		  "[^ \n\t\"'`]+\\|"))
  	 (argpart (concat first
  			  "\\(\"\\([^\"\\]\\|\\\\.\\)*\"\\|\
  '[^']*'\\|\
  `[^`]*`\\)"))
+ 	 (quote-subexpr (if backslash-escape 2 1))
  	 (args ()) (pos 0)
  	 (count 0)
  	 beg str quotes)
      ;; Build a list of all the args until we have as many as we want.
      (while (and (or (null mth) (<= count mth))
  		(string-match argpart string pos))
+       (and backslash-escape
+ 	   (match-beginning 1)
+ 	   (put-text-property (match-beginning 1) (match-end 1)
+ 			      'literal t string))
        (if (and beg (= pos (match-beginning 0)))
  	  ;; It's contiguous, part of the same arg.
  	  (setq pos (match-end 0)
! 		quotes (or quotes (match-beginning quote-subexpr)))
  	;; It's a new separate arg.
  	(if beg
  	    ;; Put the previous arg, if there was one, onto ARGS.
***************
*** 1406,1412 ****
  		  args (if quotes (cons str args)
  			 (nconc (comint-delim-arg str) args))))
  	(setq count (length args))
! 	(setq quotes (match-beginning 1))
  	(setq beg (match-beginning 0))
  	(setq pos (match-end 0))))
      (if beg
--- 1416,1422 ----
  		  args (if quotes (cons str args)
  			 (nconc (comint-delim-arg str) args))))
  	(setq count (length args))
! 	(setq quotes (match-beginning quote-subexpr))
  	(setq beg (match-beginning 0))
  	(setq pos (match-end 0))))
      (if beg