[Date Prev][Date Next] [Thread Prev][Thread Next] [Date Index] [Thread Index]

Bug#289845: xdvizilla: subtle syntax error.



On Tue, Jan 11, 2005 at 03:55:44PM +0100, Frank Küster wrote:
> > According to the bash (and ksh) manual, group commands must be terminated
> > by `;' or newline.  
> 
> Thanks for pointing that out, you are right that there is an error in
> the last patch to xdvizilla.

Yep. My patch was broken, noticed it when doing other patches but forgot to 
report this to this bug. I did say the patch was untested, did I?

> > The following patch restores the correct behaviour:
> >
> > --- xdvizilla.old	2004-12-23 17:39:17.000000000 +0100
> > +++ xdvizilla.new	2005-01-11 11:22:41.000000000 +0100
> > @@ -33,7 +33,10 @@
> >  case "$FILETYPE" in
> >  
> >    *"gzip compressed data"*)
> > -    FILE=`mktemp -t xdvizilla.XXXXXX` || { echo "$0: Cannot create temporary file"; exit 1 }
> > +    FILE=`mktemp -t xdvizilla.XXXXXX` || {
> > +	echo "$0: Cannot create temporary file"
> > +	exit 1
> > +    }
> 
> Is there a specific reason why you chose the reformatting, instead of
> just adding a `;' before the closing parenthesis? Or is it just the
> overlong lines?

I think that (wrapped by mailer, it's just a long line)

  FILE=`mktemp -t xdvizilla.XXXXXX` || { echo "$0: Cannot create temporary file"; exit 1; }

Should work fine. Actually, so would:

FILE=`mktemp -t xdvizilla.XXXXXX || tempfile --prefix=xdvizilla` || { echo "$0: Cannot create temporary file" >2; exit 1; }

Which might be even better if the patch is to be used in some systems that 
don't have mktemp and have tempfile (Debian has both). It will also send 
the error to STDERR, where it belongs. I've tested this (standalone) :-)

I've introduced these changes into the attached patch, I've actually tested 
it this time, and it works fine. It has only one minor issue, the error 
messages related to the temporary file creation will not be shown when 
running xdvizilla through mozilla as there is no tty to send them too 
(maybe xmessage should be used instead?)

I've also fixed two things:

1.- I believe the temporary files (and directory) should always be removed
(regardless of -no-rm being used) and I've introduced a trap to do so. In 
some circunstances (script being aborted before it finishes) the tempfiles 
might lie around

2.- xdvizilla will happily try to work even if the file does not exist. 
I've fixed this with a simple [ ! -e ] check.

Regards

Javier


--- xdvizilla.orig	2005-01-11 16:29:40.000000000 +0100
+++ xdvizilla	2005-01-11 16:34:37.000000000 +0100
@@ -28,12 +28,18 @@
 fi
 
 FILE=$1
+if [ ! -e "$FILE" ] ; then
+  xmessage -nearmouse '$0: $FILE does not exist!'
+  exit 1
+fi
+  
 FILETYPE=`file "$FILE"`
 
 case "$FILETYPE" in
 
   *"gzip compressed data"*)
-    FILE=`mktemp -t xdvizilla.XXXXXX` || { echo "$0: Cannot create temporary file"; exit 1 }
+    FILE=`mktemp -t xdvizilla.XXXXXX || tempfile --prefix=xdvi` || { echo "$0: Cannot create temporary file" >&2; exit 1; }
+    trap "rm -f -- \"$FILE\";" 0 1 2 3 13 15 
     gunzip -c "$1" > $FILE
     [ -n "$NO_RM" ] || rm -f -- "$1"
     NO_RM=
@@ -41,7 +47,8 @@
     ;;
 
   *"compressed data"* | *"compress'd data"*)
-    FILE=`mktemp -t xdvizilla.XXXXXX` || { echo "$0: Cannot create temporary file"; exit 1 }
+    FILE=`mktemp -t xdvizilla.XXXXXX || tempfile --prefix=xdvi` || { echo "$0: Cannot create temporary file" >&2; exit 1; }
+    trap "rm -f -- \"$FILE\";" 0 1 2 3 13 15 
     uncompress -c "$1" > $FILE
     [ -n "$NO_RM" ] || rm -f -- "$1"
     NO_RM=
@@ -60,22 +67,21 @@
 case "$FILETYPE" in
 
   *" tar archive")
-    TARDIR=`mktemp -t -d xdvitar.XXXXXX` || { echo "$0: Cannot create temporary directory"; exit 1 }
-    mkdir $TARDIR
+    TARDIR=`mktemp -t -d xdvitar.XXXXXX` || { echo "$0: Cannot create temporary directory"; >&2 exit 1; }
+    trap "rm -f -- \"$FILE\"; rm -rf -- \"$TARDIR\"; " 0 1 2 3 13 15 
     cat "$FILE" | (cd $TARDIR; tar xf -)
     DVINAME=`tar tf "$FILE" | grep '\.dvi$' | head -1`
-    [ -n "$NO_RM" ] || rm -f -- "$FILE"
     if [ -z "$DVINAME" ]; then
       xmessage -nearmouse "Tar file does not contain a dvi file"
     else
       (cd $TARDIR; "$DIR"xdvi -safer "$DVINAME")
     fi
-    rm -rf $TARDIR
   ;;
 
   *)
     "$DIR"xdvi -safer "$FILE"
-    [ -n "$NO_RM" ] || rm -f -- "$FILE"
   ;;
 
 esac
+
+exit 0

Attachment: signature.asc
Description: Digital signature


Reply to: