[jidanni@jidanni.org: shell-script-mode case indenting]

[jidanni@jidanni.org: shell-script-mode case indenting]

[Richard Stallman]

Would someone please DTRT and ack?  This is not a must-fix bug, since
perfect indentation in Shell Script mode is too much to hope for, but
we should fix it if it isn't hard.

------- Start of forwarded message -------
To: emacs-pretest-bug@gnu.org
MIME-version: 1.0
Content-type: text/plain; charset=utf-8
From: Dan Jacobson <jidanni@jidanni.org>
Date: Thu, 21 Sep 2006 18:28:18 +0800
Subject: shell-script-mode case indenting
X-Spam-Status: No, score=0.0 required=5.0 tests=none autolearn=failed 
	version=3.0.4

Pressing TAB on each line in this file.sh produces the first line of
this case expression indented different than the rest:
(sed 's/.$/ &/'|while read route company; do case $company in
	r)set ???? Renyou;;
	    z)set ???? Taizhong;;
	    q)set ???? Quanhang;;
	    j)set ???? Juye;;
	    t)set ???? Tonglian;;
	    *)echo $company: HUH? 1>&2;exit 55;;
	esac; echo "[[????:???? ${1}???? - Operator $2 Bus Co.]]" \
	    >> $route; done)<<EOF
001r
005q
006r
EOF


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

Re: [jidanni@jidanni.org: shell-script-mode case indenting]

[Michael Welsh Duggan]

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

Richard Stallman <rms@gnu.org> writes:

> Would someone please DTRT and ack?  This is not a must-fix bug, since
> perfect indentation in Shell Script mode is too much to hope for, but
> we should fix it if it isn't hard.
>
> From: Dan Jacobson <jidanni@jidanni.org>
> Subject: shell-script-mode case indenting
> To: emacs-pretest-bug@gnu.org
> Date: Thu, 21 Sep 2006 18:28:18 +0800
> X-Spam-Status: No, score=0.0 required=5.0 tests=none autolearn=failed 
> 	version=3.0.4
>
> Pressing TAB on each line in this file.sh produces the first line of
> this case expression indented different than the rest:
> (sed 's/.$/ &/'|while read route company; do case $company in
> 	r)set ???? Renyou;;
> 	    z)set ???? Taizhong;;
> 	    q)set ???? Quanhang;;
> 	    j)set ???? Juye;;
> 	    t)set ???? Tonglian;;
> 	    *)echo $company: HUH? 1>&2;exit 55;;
> 	esac; echo "[[????:???? ${1}???? - Operator $2 Bus Co.]]" \
> 	    >> $route; done)<<EOF
> 001r
> 005q
> 006r
> EOF


The following patch attempts to take `sh-leading-keywords' into account
when calculating (sh-prev-thing).  This patch seems to work in this
and a few other cases I played around with, but I make no guarantees
that I haven't broken something along the way:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Type: text/x-patch, Size: 2322 bytes --]

Index: sh-script.el
===================================================================
RCS file: /cvsroot/emacs/emacs/lisp/progmodes/sh-script.el,v
retrieving revision 1.186
diff -u -p -r1.186 sh-script.el
--- sh-script.el	8 Aug 2006 15:09:25 -0000	1.186
+++ sh-script.el	21 Sep 2006 22:26:28 -0000
@@ -2473,33 +2473,42 @@ we go to the end of the previous line an
 	(skip-chars-backward " \t;")
 	(unless (looking-at "\\s-*;;")
 	(skip-chars-backward "^)}];\"'`({[")
-	(setq c (char-before))))
-      (sh-debug "stopping at %d c is %s  start=%d min-point=%d"
-		(point) c start min-point)
-      (if (< (point) min-point)
-	  (error "point %d < min-point %d" (point) min-point))
-      (cond
-       ((looking-at "\\s-*;;")
-	;; (message "Found ;; !")
-       ";;")
-       ((or (eq c ?\n)
-	    (eq c nil)
-	    (eq c ?\;))
-       (save-excursion
-	 ;; skip forward over white space newline and \ at eol
-	 (skip-chars-forward " \t\n\\\\")
-	 (sh-debug "Now at %d   start=%d" (point) start)
-	 (if (>= (point) start)
-	     (progn
-	       (sh-debug "point: %d >= start: %d" (point) start)
-	       nil)
-	   (sh-get-word))
-	 ))
-       (t
-	;; c	-- return a string
-       (char-to-string c)
-       ))
-      )))
+	(setq c (char-before))
+	(sh-debug "stopping at %d c is %s  start=%d min-point=%d"
+		  (point) c start min-point)
+	(if (< (point) min-point)
+	    (error "point %d < min-point %d" (point) min-point))
+	(cond
+	 ((looking-at "\\s-*;;")
+	  ;; (message "Found ;; !")
+	  ";;")
+	 ((or (eq c ?\n)
+	      (eq c nil)
+	      (eq c ?\;))
+	 (let (done kwd next
+	       (boundary (point)))
+	   (skip-chars-forward " \t\n\\\\")
+	   (while (and (not done) (not (eobp)))
+	     (if next (setq boundary next))
+	     ;; skip forward over white space newline and \ at eol
+	     (sh-debug "Now at %d   start=%d" (point) start)
+	     (if (>= (point) start)
+		 (progn
+		   (sh-debug "point: %d >= start: %d" (point) start)
+		   nil)
+	       (setq kwd (sh-get-word))
+	       (unless (eobp) (forward-char 1))
+	       (if (member kwd (sh-feature sh-leading-keywords))
+		   (setq next (point))
+		 (setq done t)))
+	     (skip-chars-forward " \t\n\\\\"))
+	   (goto-char boundary)
+	   kwd))
+	 (t
+	  ;; c	-- return a string
+	  (char-to-string c)
+	  ))
+	)))))
 
 
 (defun sh-this-is-a-continuation ()

[-- Attachment #3: Type: text/plain, Size: 44 bytes --]


-- 
Michael Welsh Duggan
(md5i@cs.cmu.edu)

[-- Attachment #4: Type: text/plain, Size: 142 bytes --]

_______________________________________________
Emacs-devel mailing list
Emacs-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/emacs-devel

Re: [jidanni@jidanni.org: shell-script-mode case indenting]

[Richard Stallman]

Thanks for working on this.  I hope other people will start trying
your fix now.

Re: [jidanni@jidanni.org: shell-script-mode case indenting]

[Glenn Morris]

Richard Stallman wrote:

> Thanks for working on this.  I hope other people will start trying
> your fix now.

So it seems this was installed. Presumably "no comment" == "assent"

And now indentation of ordinary case blocks is wrong:

case $foo in
    bar)
        echo bar
        ;;

        qux)
        echo qux
        ;;
esac


This is a silly way to run a release, IMO.

Re: [jidanni@jidanni.org: shell-script-mode case indenting]

[Richard Stallman]

    And now indentation of ordinary case blocks is wrong:

Should we take out that change, or what?

Re: [jidanni@jidanni.org: shell-script-mode case indenting]

[Michael Welsh Duggan]

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

Richard Stallman <rms@gnu.org> writes:

>     And now indentation of ordinary case blocks is wrong:
>
> Should we take out that change, or what?

This should fix the problem.  I was originally messed up by the fact
that the `unless' body in the original code was not indented, and due
to that misplaced a parentheses.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Type: text/x-patch, Size: 1312 bytes --]

Index: sh-script.el
===================================================================
RCS file: /cvsroot/emacs/emacs/lisp/progmodes/sh-script.el,v
retrieving revision 1.187
diff -u -p -c -r1.187 sh-script.el
cvs diff: conflicting specifications of output style
*** sh-script.el	30 Sep 2006 09:38:45 -0000	1.187
--- sh-script.el	1 Oct 2006 18:35:31 -0000
*************** we go to the end of the previous line an
*** 2472,2479 ****
  	(point))
  	(skip-chars-backward " \t;")
  	(unless (looking-at "\\s-*;;")
! 	(skip-chars-backward "^)}];\"'`({[")
! 	(setq c (char-before))
  	(sh-debug "stopping at %d c is %s  start=%d min-point=%d"
  		  (point) c start min-point)
  	(if (< (point) min-point)
--- 2472,2479 ----
  	(point))
  	(skip-chars-backward " \t;")
  	(unless (looking-at "\\s-*;;")
! 	  (skip-chars-backward "^)}];\"'`({[")
! 	  (setq c (char-before)))
  	(sh-debug "stopping at %d c is %s  start=%d min-point=%d"
  		  (point) c start min-point)
  	(if (< (point) min-point)
*************** we go to the end of the previous line an
*** 2508,2514 ****
  	  ;; c	-- return a string
  	  (char-to-string c)
  	  ))
! 	)))))
  
  
  (defun sh-this-is-a-continuation ()
--- 2508,2514 ----
  	  ;; c	-- return a string
  	  (char-to-string c)
  	  ))
! 	))))
  
  
  (defun sh-this-is-a-continuation ()

[-- Attachment #3: Type: text/plain, Size: 44 bytes --]


-- 
Michael Welsh Duggan
(md5i@cs.cmu.edu)

[-- Attachment #4: Type: text/plain, Size: 142 bytes --]

_______________________________________________
Emacs-devel mailing list
Emacs-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/emacs-devel

Re: [jidanni@jidanni.org: shell-script-mode case indenting]

[Glenn Morris]

Michael Welsh Duggan wrote:

> This should fix the problem.

This fixes the issue for me, thanks.

Re: [jidanni@jidanni.org: shell-script-mode case indenting]

[Stefan Monnier]

In your new code, what is the purpose of the following line in the loop:

  (unless (eobp) (forward-char 1))


-- Stefan

Re: [jidanni@jidanni.org: shell-script-mode case indenting]

[Michael Welsh Duggan]

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

> In your new code, what is the purpose of the following line in the loop:
>
>   (unless (eobp) (forward-char 1))

Don't move forward by a character if at the end of a (narrowed)
buffer.  The need to move forward a character was experimentally
determined, I am afraid.

-- 
Michael Welsh Duggan
(md5i@cs.cmu.edu)

Re: [jidanni@jidanni.org: shell-script-mode case indenting]

[Stefan Monnier]

>> In your new code, what is the purpose of the following line in the loop:
>> (unless (eobp) (forward-char 1))
> Don't move forward by a character if at the end of a (narrowed)
> buffer.

Well, that's the result, not the purpose :-(

> The need to move forward a character was experimentally
> determined, I am afraid.

That's the part I'm interested in.  Could you give us some idea of the case(s)
you've encountered where it was found necessary?


        Stefan

Re: [jidanni@jidanni.org: shell-script-mode case indenting]

[Michael Welsh Duggan]

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

>>> In your new code, what is the purpose of the following line in the loop:
>>> (unless (eobp) (forward-char 1))
>> Don't move forward by a character if at the end of a (narrowed)
>> buffer.
>
> Well, that's the result, not the purpose :-(
>
>> The need to move forward a character was experimentally
>> determined, I am afraid.
>
> That's the part I'm interested in.  Could you give us some idea of the case(s)
> you've encountered where it was found necessary?

It actually took me a long time to figure out why.  (I was at work
when I answered you originally, and as such couldn't verify why at the
time.)

Before you restructured `sh-prev-thing' to not use narrowing, the

		(unless (bolp)
		  (forward-char -1)))

on line 2263-4 (in `sh-get-indent-info') would place the cursor before
the last character of the previous token.  When narrowed by the next
call to `sh-prev-thing', the narrowing cut off that character, causing
`sh-get-word' to fail to get the next word.

Your change which removes narrowing in favor of explicit match limits
is a much more elegant solution, and obviates the need for the
questionable line.

-- 
Michael Welsh Duggan
(md5i@cs.cmu.edu)

Re: [jidanni@jidanni.org: shell-script-mode case indenting]

[Stefan Monnier]

>>>> In your new code, what is the purpose of the following line in the loop:
>>>> (unless (eobp) (forward-char 1))
>>> Don't move forward by a character if at the end of a (narrowed)
>>> buffer.
>> 
>> Well, that's the result, not the purpose :-(
>> 
>>> The need to move forward a character was experimentally
>>> determined, I am afraid.
>> 
>> That's the part I'm interested in.  Could you give us some idea of the case(s)
>> you've encountered where it was found necessary?

> It actually took me a long time to figure out why.  (I was at work
> when I answered you originally, and as such couldn't verify why at the
> time.)

> Before you restructured `sh-prev-thing' to not use narrowing, the

> 		(unless (bolp)
> 		  (forward-char -1)))

> on line 2263-4 (in `sh-get-indent-info') would place the cursor before
> the last character of the previous token.  When narrowed by the next
> call to `sh-prev-thing', the narrowing cut off that character, causing
> `sh-get-word' to fail to get the next word.

> Your change which removes narrowing in favor of explicit match limits
> is a much more elegant solution, and obviates the need for the
> questionable line.

Thanks for tracking it down.


        Stefan