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

[Jean Delvare]

Hi Martin,

On Sun, 29 Dec 2013 22:48:01 +0100, Martin Quinson wrote:
> I took all of your suggestions and added a small test case. It does
> not test what happens with binary diffs because I could not think of
> how to generate a binary file in a portable manner, but uses chmod -r.
> 
> The new patch is inline here while the old one is below, quoted in the
> mail for reference.

Looks overall good, although I have a few more comments, see below.

> Bye, Mt.
> 
> Description: Exit with an error when diff's retcode=2 (error) on patch

I am just realizing that the description seems to be truncated. I think
you mean "on patch refresh"?

>  This is trigered e.g. when you try to add a binary file to a patch.
>  This is actually creepy to think that we were not checking the
>  retcode of diff :)
> Forwarded: 2013-12-29
> Bug-Debian: http://bugs.debian.org/638313
> Author: Martin Quinson
> 
> ---
>  quilt/refresh.in          |    2 +-
>  quilt/scripts/patchfns.in |    6 ++++++
>  test/faildiff.test        |   18 ++++++++++++++++++
>  3 files changed, 25 insertions(+), 1 deletion(-)
>  
> pipestatus: 1  
> Index: b/quilt/scripts/patchfns.in
> ===================================================================
> --- a/quilt/scripts/patchfns.in
> +++ b/quilt/scripts/patchfns.in
> @@ -766,6 +766,12 @@

Can you please generate your patches with diff option -p in the future?
I makes reviewing easier.

>               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
>               die 1
>       fi
>  
> Index: b/test/faildiff.test
> ===================================================================
> --- /dev/null
> +++ b/test/faildiff.test
> @@ -0,0 +1,18 @@
> +     $ 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
> +
> +     $ chmod -r test.txt
> +
> +     $ quilt refresh
> +     > diff: test.txt: Permission denied
> +     > Diff failed on file 'test.txt', aborting.

Please check the return value here:

        $ echo %{?}
        > 1

BTW, the return value of "quilt diff" in case a binary file is included
is still 0. I think it needs to be fixed as well,

> +
> +     $ chmod +r test.txt

-- 
Jean Delvare
Suse L3 Support