Re: [Quilt-dev] [patch 1/5] Exit with an error when diffs retcode=2 (error) on patch refresh

[Jean Delvare]

Hi Martin,

Le Saturday 18 January 2014 à 01:54 +0100, address@hidden a
écrit :
> Description: Exit with an error when diff's retcode=2 (error) on patch refresh
>  This is trigered e.g. when you try to add a binary file to a patch.

Spelling: triggered

>  This is actually creepy to think that we were not checking the
>  retcode of diff :)
> Forwarded: 2014-01-18
> Bug-Debian: http://bugs.debian.org/638313
> Author: Martin Quinson
> 
> ---
>  quilt/diff.in             |    6 ++++++
>  quilt/refresh.in          |    2 +-
>  quilt/scripts/patchfns.in |    6 ++++++
>  test/faildiff.test        |   43 +++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 56 insertions(+), 1 deletion(-)

You didn't test this new version of the patch, did you? It doesn't pass
the test suite...

> Index: b/quilt/diff.in
> ===================================================================
> --- a/quilt/diff.in
> +++ b/quilt/diff.in
> @@ -119,6 +119,12 @@
>               fi
>       else
>               diff_file "$file" "$old_file" "$new_file" | colorize
> +
> +             # Test the return value of diff, and propagate the error if any
> +             if [ ${PIPESTATUS[0]} != 0 ]
> +             then
> +                     die ${PIPESTATUS[0]}
> +             fi

Just like $?, $PIPESTATUS gets a new value with every command bash
executes. So if you need to use the value more than once, you must save
it first and then only use the copy:

                # Test the return value of diff, and propagate the error if any
                status=${PIPESTATUS[0]}
                if [ $status != 0 ]
                then
                        die $status
                fi

>       fi
>  }
>  
> Index: b/quilt/scripts/patchfns.in
> ===================================================================
> --- a/quilt/scripts/patchfns.in
> +++ b/quilt/scripts/patchfns.in
> @@ -763,6 +763,12 @@
>               echo "$line"
>               cat
>       fi
> +
> +     # Test the return value of diff, and propagate the error retcode if any
> +     if [ ${PIPESTATUS[0]} == 2 ]
> +     then
> +             return 1
> +     fi
>  }
>  
>  cat_file()
> Index: b/quilt/refresh.in
> ===================================================================
> --- a/quilt/refresh.in
> +++ b/quilt/refresh.in
> @@ -231,7 +231,7 @@
>       fi
>       if ! diff_file "$file" "$old_file" "$new_file"
>       then
> -             printf $"Diff failed, aborting\n" >&2
> +             printf $"Diff failed on file '%s', aborting.\n" "$new_file" >&2

We normally don't have dots at the end of error messages.

>               die 1
>       fi
>  
> Index: b/test/faildiff.test
> ===================================================================
> --- /dev/null
> +++ b/test/faildiff.test
> @@ -0,0 +1,43 @@
> +     $ mkdir patches
> +
> +     $ cat > test.txt
> +     < This is test.txt.
> +
> +     $ quilt new test.diff
> +     > Patch %{P}test.diff is now on top
> +
> +     $ quilt add test.txt
> +     > File test.txt added to patch %{P}test.diff
> +
> +What happens when diff fails because of a permission error?
> +
> +     $ chmod -r test.txt
> +
> +     $ quilt refresh
> +     > diff: test.txt: Permission denied
> +     > Diff failed on file 'test.txt', aborting.
> +
> +     $ echo %{?}
> +     > 1
> +
> +     $ chmod +r test.txt
> +
> +What happens on binary files?
> +
> +        $ bash -c "printf '\\x02\\x00\\x01'" > test.bin

Indented with spaces instead of a tab.

> +
> +     $ quilt add test.bin
> +     > File test.bin added to patch %{P}test.diff
> +
> +        $ printf "\\x03\\x00\\x01" > test.bin

Indented with spaces instead of a tab, plus it is inconsistent with how
you generated test.bin in the first place. We should use the same syntax
everywhere to limit the portability issues.

As explained in a previous mail, I now believe that:

        $ printf "\\003\\000\\001" > test.bin

is the best approach as it is both portable and fast.

> +     $ quilt diff -pab --no-index
> +     >~ (Files|Binary files) a/test\.bin and b/test\.bin differ
> +
> +     $ echo %{?}
> +     > 1
> +
> +     $ quilt refresh
> +     > Diff failed on file 'test.bin', aborting.
> +
> +     $ echo %{?}
> +     > 1

I've made all the suggested fixes and changes myself and committed the
patch under your name.

-- 
Jean Delvare
Suse L3 Support