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

[Martin Quinson]

Hello Jean,

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.

Bye, Mt.

Description: Exit with an error when diff's retcode=2 (error) on patch
 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 @@
                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.
+
+       $ chmod +r test.txt


On Sun, Dec 22, 2013 at 10:31:06AM +0100, Jean Delvare wrote:
> Hi Martin,
> 
> On Sat, 21 Dec 2013 21:27:58 +0100, address@hidden wrote:
> > Description:
> >  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 patch :)
> 
> You mean diff, not patch, right?
> 
> > Forwarded: 2013-12-21
> > Bug-Debian: http://bugs.debian.org/638313
> > Author: Martin Quinson
> > 
> > ---
> >  quilt/refresh.in          |    2 +-
> >  quilt/scripts/patchfns.in |    6 ++++++
> >  2 files changed, 7 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 @@
> >             echo "$line"
> >             cat
> >     fi
> > +
> > +   # Test the return value of diff, and propagate the error retcode if any
> > +   if [ ${PIPESTATUS[0]} == 2 ] ;
> 
> The semi-colon is not needed.
> 
> > +   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 '$new_file', aborting. Is it a binary 
> > file?\n" >&2
> 
> I don't think it is a good idea to suggest a reason when the problem
> could be completely different. It adds confusion more than it helps.
> 
> If you want to be able to report this specific case then you should
> grep for '^Binary files' in the output of diff, although this could be
> fragile.
> 
> >             die 1
> >     fi
> >  
> 
> I also would like you to add a test case for this in the test suite.
> 
> Thanks,
> -- 
> Jean Delvare
> Suse L3 Support

-- 
Avant d'installer linux, mon ordinateur plantait tout le temps, les
filles me fuyaient, je n'avais pas d'amis ni de vie sociale, et
j'avais des boutons. Maintenant, mon ordinateur ne plante plus.