Re: [Quilt-dev] filenames containing whitespace not handled correctly

[Jean Delvare]

Hi Yashi,

Le jeudi 24 septembre 2009, Yasushi SHOJI a écrit :
> Subject: [PATCH] add, files, remove: accept file name with spaces
> 
> http://savannah.nongnu.org/bugs/?19477 and
> http://lists.nongnu.org/archive/html/quilt-dev/2009-09/msg00010.html
> reported that the current quilt does not work with file name with
> spaces. we all know that it is tough to handle it with bash script and
> fix all quilt commands.
> 
> this patch only fixes add, files, and remove command with normal code
> path.  no excessive test has been done but a few test code is also
> added.  hope this starts fixing quilt to handle all possible path
> pattern.

I agree these fixes are good to have, even if they don't fix the
whole problem. If we stack such fixes over time and augment the test
suite accordingly, we might reach the Graal someday.

>  quilt/add.in                    |   16 ++++++++--------
>  quilt/files.in                  |    7 +++++--
>  quilt/remove.in                 |    8 ++++----
>  quilt/scripts/patchfns.in       |   10 ++++------
>  test/add-filename-check.test    |    3 +++
>  test/remove-filename-check.test |   24 ++++++++++++++++++++++++
>  6 files changed, 48 insertions(+), 20 deletions(-)
>  create mode 100644 test/remove-filename-check.test

Comments:

> diff --git a/quilt/add.in b/quilt/add.in
> index 373192f..dfd48f8 100644
> --- a/quilt/add.in
> +++ b/quilt/add.in
> @@ -91,21 +91,21 @@ fi
>  patch=$(find_applied_patch "$opt_patch") || exit 1
>  
>  status=0
> -for file in $*
> +for file in "$@"
>  do
> -     if ! in_valid_dir $SUBDIR$file
> +     if ! in_valid_dir "$SUBDIR$file"
>       then
>               status=1
>               continue
>       fi
> -     if file_in_patch $SUBDIR$file $patch
> +     if file_in_patch "$SUBDIR$file" $patch
>       then
>               printf $"File %s is already in patch %s\n" \
>                      "$SUBDIR$file" "$(print_patch $patch)" >&2
>               [ $status -ne 1 ] && status=2
>               continue
>       fi
> -     next_patch=$(next_patch_for_file $patch $SUBDIR$file)
> +     next_patch=$(next_patch_for_file $patch "$SUBDIR$file")
>       if [ -n "$next_patch" ]
>       then
>               printf $"File %s modified by patch %s\n" \
> @@ -114,24 +114,24 @@ do
>               continue
>       fi
>  
> -     if [ -L $SUBDIR$file ]
> +     if [ -L "$SUBDIR$file" ]
>       then
>               printf $"Cannot add symbolic link %s\n" "$SUBDIR$file" >&2
>               status=1
>               continue
>       fi
>  
> -     if ! $QUILT_LIB/backup-files -b -s -L -B $QUILT_PC/$patch/ $SUBDIR$file
> +     if ! $QUILT_LIB/backup-files -b -s -L -B $QUILT_PC/$patch/ 
> "$SUBDIR$file"
>       then
>               printf $"Failed to back up file %s\n" "$SUBDIR$file" >&2
>               status=1
>               continue
>       fi
>  
> -     if [ -e $SUBDIR$file ]
> +     if [ -e "$SUBDIR$file" ]
>       then
>               # The original tree may be read-only.
> -             chmod u+w $SUBDIR$file
> +             chmod u+w "$SUBDIR$file"
>       fi
>  
>       printf $"File %s added to patch %s\n" \
> diff --git a/quilt/files.in b/quilt/files.in
> index ba8b054..b4a0c51 100644
> --- a/quilt/files.in
> +++ b/quilt/files.in
> @@ -126,7 +126,10 @@ list_files_in_patch()
>               use_status=yes
>       fi
>       # Note: If opt_labels is set, then use_status is not set.
> -     for file in $(files_in_patch $patch | sort)
> +     IFS=

Changing IFS and not restoring it afterwards looks wrong. Look at
annotate.in, there the value of IFS is carefully saved and restored.

> +     echo $(files_in_patch "$patch") |
> +     sort |
> +     while read file
>       do
>               if [ -n "$opt_labels" ]
>               then
> @@ -161,7 +164,7 @@ list_files_in_patch()
>  
>  for patch in address@hidden
>  do
> -     list_files_in_patch $patch
> +     list_files_in_patch "$patch"
>  done
>  
>  ### Local Variables:
> diff --git a/quilt/remove.in b/quilt/remove.in
> index a11db0a..96a4e86 100644
> --- a/quilt/remove.in
> +++ b/quilt/remove.in
> @@ -66,9 +66,9 @@ fi
>  patch=$(find_applied_patch "$opt_patch") || exit 1
>  
>  status=0
> -for file in $*
> +for file in "$@"
>  do
> -     if ! file_in_patch $SUBDIR$file $patch
> +     if ! file_in_patch "$SUBDIR$file" $patch
>       then
>               printf $"File %s is not in patch %s\n" \
>                      "$SUBDIR$file" "$(print_patch $patch)" >&2
> @@ -76,7 +76,7 @@ do
>               continue
>       fi
>  
> -     next_patch=$(next_patch_for_file $patch $SUBDIR$file)
> +     next_patch=$(next_patch_for_file $patch "$SUBDIR$file")
>       if [ -n "$next_patch" ]
>       then
>               printf $"File %s modified by patch %s\n" \
> @@ -86,7 +86,7 @@ do
>       fi
>  
>       # Restore file from backup
> -     if ! $QUILT_LIB/backup-files -r -t -s -B $QUILT_PC/$patch/ $SUBDIR$file
> +     if ! $QUILT_LIB/backup-files -r -t -s -B $QUILT_PC/$patch/ 
> "$SUBDIR$file"
>       then
>               printf $"Failed to remove file %s from patch %s\n" \
>                      "$SUBDIR$file" "$(print_patch $patch)" >&2
> diff --git a/quilt/scripts/patchfns.in b/quilt/scripts/patchfns.in
> index 9723685..5707724 100644
> --- a/quilt/scripts/patchfns.in
> +++ b/quilt/scripts/patchfns.in
> @@ -643,16 +643,14 @@ file_in_patch()
>  
>  files_in_patch()
>  {
> -     local patch="$1"
> +     local patch="$*"

This doesn't seem right. There's really only one parameter to this
function. If callers aren't quoting properly then we should fix the
callers.

>       local path="$QUILT_PC/$patch"
>  
>       if [ -d "$path" ]
>       then
> -             local files
> -             files=( $(find "$path" -type f \
> -                            -a ! -path "$path/.timestamp") ) \
> -             || return 1
> -             printf "%s\n" "address@hidden/}"
> +             find "$path" -type f \
> +                            -a ! -path "$path/.timestamp" |
> +             sed -e "s:$path/::"
>       fi
>  }
>  
> diff --git a/test/add-filename-check.test b/test/add-filename-check.test
> index 306af66..aca4b9c 100644
> --- a/test/add-filename-check.test
> +++ b/test/add-filename-check.test
> @@ -14,5 +14,8 @@ $ quilt add patches/bar
>  $ quilt add .pc/baz
>  > File .pc/baz is located below .pc/
>  
> +$ quilt add "foo bar"
> +> File foo bar added to patch patches/test.diff
> +
>  $ cd ..
>  $ rm -rf d
> diff --git a/test/remove-filename-check.test b/test/remove-filename-check.test
> new file mode 100644
> index 0000000..1931a29
> --- /dev/null
> +++ b/test/remove-filename-check.test
> @@ -0,0 +1,24 @@
> +$ rm -rf d
> +$ mkdir -p d/patches
> +$ cd d
> +$ quilt new test.diff
> +>Patch patches/test.diff is now on top
> +
> +$ echo foo > foo
> +$ quilt add foo
> +> File foo added to patch patches/test.diff
> +
> +$ quilt add "foo bar"
> +> File foo bar added to patch patches/test.diff
> +
> +$ quilt add "a b c"
> +> File a b c added to patch patches/test.diff
> +
> +$quilt remove "a b c"
> +> File a b c removed from patch patches/test.diff
> +
> +$quilt remove "foo bar"
> +> File foo bar removed from patch patches/test.diff
> +
> +$ cd ..
> +$ rm -rf d

Thanks,
-- 
Jean Delvare
Suse L3